Re: [PATCH v7 5/6] branch: add --recurse-submodules option for branch creation

[Jonathan Tan <jonathantanmy@google.com>]

Patches up to this one look good, and the implementation looks good too.
I just have minor comments on some of the text and tests.

Glen Choo <chooglen@google.com> writes:
> Although this commit does not introduce breaking changes, it is
> incompatible with existing --recurse-submodules commands because "git
> branch --recurse-submodules" writes to the submodule ref store, but most
> commands only consider the superproject gitlink and ignore the submodule
> ref store. For example, "git checkout --recurse-submodules" will check
> out the commits in the superproject gitlinks (and put the submodules in
> detached HEAD) instead of checking out the submodule branches.

The usual meaning of "incompatible" is "cannot be used together", I
think, which is not what's happening here - it's just that the user
would expect them to work together in a specific way but they don't do
that.

> To Jonathan: When describing the differences between "git branch" and "git
> checkout", I opted to emphasize the fact that "git checkout" checks out the
> gitlink in detached HEAD and not the branches themselves (vs your suggestion of
> mentioning that checkout would check out the wrong version if the branch is
> subsequently updated).

OK - makes sense.

> +/**
> + * Creates a branch in a submodule by calling
> + * create_branches_recursively() in a child process. The child process
> + * is necessary because install_branch_config_multiple_remotes() (which
> + * is called by setup_tracking()) does not support writing configs to
> + * submodules.

OK, good explanation for why a child process is needed.

> + */
> +static int submodule_create_branch(struct repository *r,
> +				   const struct submodule *submodule,
> +				   const char *name, const char *start_oid,
> +				   const char *start_name, int force,
> +				   int reflog, int quiet,
> +				   enum branch_track track, int dry_run)
> +{
> +	int ret = 0;
> +	struct child_process child = CHILD_PROCESS_INIT;
> +	struct strbuf child_err = STRBUF_INIT;
> +	struct strbuf out_buf = STRBUF_INIT;
> +	char *out_prefix = xstrfmt("submodule '%s': ", submodule->name);
> +	child.git_cmd = 1;
> +	child.err = -1;
> +	child.stdout_to_stderr = 1;
> +
> +	prepare_other_repo_env(&child.env_array, r->gitdir);
> +	/*
> +	 * submodule_create_branch() is indirectly invoked by "git
> +	 * branch", but we cannot invoke "git branch" in the child
> +	 * process because it does not let us set start_name and
> +	 * start_oid separately (see create_branches_recursively()).

Probably clearer to enumerate the 3 different pieces of information
needed: the name of the branch to be created, the OID, and the name of
the branch that would be used for tracking purposes.

An argument could be made that "git branch" should be extended to be
able to take in these 3 different pieces of information, but it also
makes sense to put this functionality in submodule--helper for now,
since the whole thing is marked as experimental.

> +/*
> + * Creates a new branch in repository and its submodules (and its
> + * submodules, recursively). Besides these exceptions, the parameters
> + * function identically to create_branch():
> + *
> + * - start_name is the name of the ref, in repository r, that the new
> + *   branch should start from. In submodules, branches will start from
> + *   the respective gitlink commit ids in start_name's tree.
> + *
> + * - tracking_name is the name used of the ref that will be used to set
> + *   up tracking, e.g. origin/main. This is propagated to submodules so
> + *   that tracking information will appear as if the branch branched off
> + *   tracking_name instead of start_name (which is a plain commit id for
> + *   submodules). If omitted, start_name is used for tracking (just like
> + *   create_branch()).
> + *
> + */
> +void create_branches_recursively(struct repository *r, const char *name,
> +				 const char *start_name,
> +				 const char *tracking_name, int force,
> +				 int reflog, int quiet, enum branch_track track,
> +				 int dry_run);

Instead of taking in "name", "start_name", and "tracking_name", could we
take in "name", "oid", and "tracking_name"? That way, it's clearer what
each parameter is used for.

> +test_expect_success 'should not create branches in inactive submodules' '
> +	test_when_finished "reset_test" &&
> +	test_config -C super submodule.sub.active false &&
> +	(
> +		cd super &&
> +		git branch --recurse-submodules branch-a &&
> +		git rev-parse branch-a &&
> +		test_must_fail git -C sub branch-a
> +	)
> +'

The "test_must_fail" line doesn't make sense - there is no such command
branch-a. To avoid errors like this, maybe make sure that all
test_must_fail invocations are accompanied by assertions on the error
message. (And for "test_must_fail git rev-parse", we could have a helper
function here that asserts the "object not found".)

> +test_expect_success 'should set up tracking of local branches with track=always' '
> +	test_when_finished "reset_test" &&
> +	(
> +		cd super &&
> +		git -c branch.autoSetupMerge=always branch --recurse-submodules branch-a main &&
> +		git -C sub rev-parse main &&
> +		test "$(git -C sub config branch.branch-a.remote)" = . &&
> +		test "$(git -C sub config branch.branch-a.merge)" = refs/heads/main
> +	)
> +'

As described in t/README line 671, this means that the inner command
could silently fail. You can do `REMOTE=$(...) &&` <newline> `test ...`.
Same comment throughout this file.