Re: [PATCH 4/4] branch: add --recurse-submodules option for branch creation

[Glen Choo <chooglen@google.com>]

Thanks! The feedback is really useful.

Jonathan Tan <jonathantanmy@google.com> writes:

>> +static int submodule_create_branch(struct repository *r, const char *name,
>> +				   const char *start_oid,
>> +				   const char *start_name, int force,
>> +				   int reflog, int quiet,
>> +				   enum branch_track track, char **err_msg)
>> +{
>> +	int ret = 0;
>> +	struct child_process child = CHILD_PROCESS_INIT;
>> +	struct strbuf child_err = STRBUF_INIT;
>> +	child.git_cmd = 1;
>> +	child.err = -1;
>> +
>> +	prepare_other_repo_env(&child.env_array, r->gitdir);
>> +	strvec_pushl(&child.args, "submodule--helper", "create-branch", NULL);
>
> Before this function is a function that calls "git branch" directly -
> couldn't this function do the same? (And then you can make both of them
> into one function.) The functionality should be exactly the same except
> that one has "--dry-run" and the other doesn't.

I see two somewhat valid interpretations, so I will address both.

If you are suggesting that I should call "git branch" instead of a new
"git submodule--helper create-branch", that unfortunately does not work.
Because of how we've defined the semantics, we want the submodule to
branch off the commit in the superproject tree (which is a bare object
id), but we want to set up tracking based off the ref that the user gave
(evaluating it in the context of the submodule). This is why
submodule--helper.c:module_create_branch() makes two calls like so:

	create_branch(the_repository, "<branch name>", "<object id>", force, 0, reflog, quiet,
		      BRANCH_TRACK_NEVER);
	setup_tracking("<branch name>", "<tracking name>", track, quiet, 0);

On the other hand, you might be suggesting that I should just add
--dry-run to "git submodule--helper create-branch". That way, the
dry-run form of the command is validating the non dry-run form (instead
of using "git branch --dry-run" to validate "git submodule--helper
create-branch"). That's a reasonable suggestion that avoids
bikeshedding around "git branch --dry-run".

>> @@ -874,6 +894,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>  			FREE_AND_NULL(unused_full_ref);
>>  			return 0;
>>  		}
>> +		if (recurse_submodules) {
>> +			create_submodule_branches(the_repository, branch_name,
>> +						  start_name, force, reflog,
>> +						  quiet, track);
>> +			return 0;
>> +		}
>>  		create_branch(the_repository, branch_name, start_name, force, 0,
>>  			      reflog, quiet, track);
>>  	} else
>
> create_submodule_branches() here is a bit misleading since it also
> creates branches in the_repository. Might be better to write explicitly
> check in submodules -> create in main repository -> create in
> submodules. Or, if you want to combine all the submodule code in one
> function, (check in submodules -> create in submodules) -> create in
> main repository.

Philippe had a similar comment, I will rename it.

>> +test_expect_success 'setup superproject and submodule' '
>> +	git init super &&
>> +	test_commit foo &&
>> +	git init sub-upstream &&
>> +	test_commit -C sub-upstream foo &&
>> +	git -C super submodule add ../sub-upstream sub &&
>> +	git -C super commit -m "add submodule" &&
>> +	git -C super config submodule.propagateBranches true
>> +'
>
> If making each test independent is important (which seems like a good
> goal to me, although I know that the Git tests are inconsistent on
> that), we could make this into a bash function (with test_when_finished)
> that gets called in every test. It doesn't violate the t/README request
> to put all test code inside test_expect_success assertions (since the
> function is still being run inside an assertion).

That's an interesting idea and it's more likely to be correct than my
approach. I think it lines up better with testing best practices.
However...

> In the general case, it will make test code slower to run, but if you're
> going to have to cleanup branches, I think it's better to just recreate
> the repo. In any case, for the general case, I can start a separate
> email thread for this discussion.

I'm concerned about the same thing and I suspect that recreating the
repo won't be kindly received by some reviewers, even if they might be
able to stomach cleanup_branches(). I think your suggestion is a better
long-term direction, but I'd like to see discussion on the general case
before changing the tests. A separate thread sounds good.

>> +test_expect_success '--recurse-submodules should be ignored if submodule.propagateBranches is false' '
>> +	test_when_finished "cleanup_branches super branch-a" &&
>> +	(
>> +		cd super &&
>> +		git -c submodule.propagateBranches=false branch --recurse-submodules branch-a &&
>> +		git rev-parse branch-a &&
>> +		test_must_fail git -C sub rev-parse branch-a
>> +	)
>> +'
>
> This doesn't sound like the right behavior to me - I think it's fine if
> it was the config "submodule.recurse" instead of "--recurse-submodules",
> but if the argument is given on CLI, it should be a fatal error.

Philippe mentioned the same thing, which sounds right to me.

>> +test_expect_success 'should create branch when submodule is in .git/modules but not .gitmodules' '
>> +	test_when_finished "cleanup_branches super branch-a branch-b branch-c" &&
>> +	(
>> +		cd super &&
>> +		git branch branch-a &&
>> +		git checkout -b branch-b &&
>> +		git submodule add ../sub-upstream sub2 &&
>> +		# branch-b now has a committed submodule not in branch-a
>> +		git commit -m "add second submodule" &&
>> +		git checkout branch-a &&
>> +		git branch --recurse-submodules branch-c branch-b &&
>> +		git rev-parse branch-c &&
>> +		git -C sub rev-parse branch-c &&
>> +		git checkout --recurse-submodules branch-c &&
>> +		git -C sub2 rev-parse branch-c
>> +	)
>> +'
>
> Hmm...how is this submodule in .git/modules but not .gitmodules? It
> looks like a normal submodule to me.

The test title is probably too terse - the submodule is not in the
working tree's .gitmodules, but it is in branch-b's .gitmodules. I'll
reword the title.

>> +test_expect_success 'should not create branch when submodule is not in .git/modules' '
>
> The title of this test contradicts the title of the test that I quoted
> previously.

I'm not sure how this is a contradiction, from before..

  should create branch when submodule is in .git/modules but not
  [the working tree's] .gitmodules

meaning "we should create the branch if we can find the submodule in
.git/modules", i.e. the implication is: presence in .git/modules => can
create branch.

Whereas

  should not create branch when submodule is not in .git/modules

meaning "we should not create the branch if we cannot find the submodule
in .git/modules", i.e. the implication is: absence in .git/modules =>
cannot create branch.

Taken together, they assert that presence in .git/modules is a necessary
condition for the subodule branch to be created.

>> +test_expect_success 'should not fail when unable to set up tracking in submodule' '
>> +	test_when_finished "cleanup_branches super-clone branch-b" &&
>> +	(
>> +		cd super-clone &&
>> +		git branch --recurse-submodules branch-b origin/branch-b
>> +	)
>> +'
>
> Is there a warning printed that we can check?

"git branch" does not warn if tracking is not set up when it is not
explicitly required, so this does not warn. However, I can imagine that
if the superproject branch has tracking set up, a user might expect that
all submodules would also have tracking set up, and thus a warning might
be useful. I don't think it will be _that_ useful for most users, but at
least some users would probably appreciate it.

For slightly unrelated reasons, I tried to get the tracking info of a
newly created branch and it is tedious. For this reason and the fact
that I'm not sure if the benefit is that great, I'm tempted *not* to add
the warning, but perhaps you feel more strongly than I do?

> Also, this patch set doesn't discuss the case in which the branch in a
> submodule already exists, but it points to the exact commit that we
> want. What is the functionality in that case?

The behavior is identical to the general case where the branch already
exists - branch validation (git branch --dry-run) fails.

> I would say that the user should be able to recursively create the
> branch in this case, but am open to other opinions.

I'm inclined to disagree. To illustrate this in the real world, say a
user wants to create a 'new-feature' branch recursively, but there is
already a 'new-feature in a submodule. Here are two possible sets of
events that could lead to this situation:

1) the 'new-feature' branch was intended for the same work as the new
   branch but the user just happened to create the 'new-feature'
   subomdule branch first
2) the existing 'new-feature' branch doesn't actually contain the same
   work (maybe it's an overloaded term, or maybe the user used a branch
   to bookmark a commit before starting work on it)

What would happen if we allowed the branch to be created? In case 1,
the user would be more than happy (because we read their mind! woohoo!)
But in case 2, the user might not realize that their bookmark is getting
clobbered by the new recursive branch - they might try revisiting their
bookmark only to realize that they've accidentally committed on top of
it.

I think the incidence of case 2 is far lower than case 1's, but I think
it's reasonable to be defensive by default. In any case, we can change
the default after more user testing.

> In any case, that case should be tested.

Testing this case would make the intended behavior clearer, so I will
add this test.