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

[Glen Choo <chooglen@google.com>]

Jonathan Tan <jonathantanmy@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.

Ok, I'll replace "incompatible with" with "does not interact well with".

>> + */
>> +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.

Makes sense. Also the comment on create_branches_recursively() doesn't
do a great job of explaining why we need the additional piece of
information (tracking name), so I'll tidy them up together.

> 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.

Yes, I considered this, but I can't think of a good reason to expose
this functionality to users - users can set their upstream with the more
intuitive "git branch --set-upstream-to".

>> +/*
>> + * 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.

I used start_name to mirror create_branches(), but start_name makes less
sense here because it's ambiguous.

Since the start_name is always an oid or NULL, this makes sense. I'll do
this.

>> +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".)

Ah good catch. I'll make the test helper.

>
>> +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.

Thanks for the catch and double thanks for the recommended fix.

Will reroll soon.