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

From: Glen Choo <chooglen@google.com>
To: Philippe Blain <levraiphilippeblain@gmail.com>, git@vger.kernel.org
Cc: "Jonathan Tan" <jonathantanmy@google.com>,
	"Josh Steadmon" <steadmon@google.com>,
	"Emily Shaffer" <emilyshaffer@google.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH 4/4] branch: add --recurse-submodules option for branch creation
Date: Tue, 23 Nov 2021 15:43:28 -0800	[thread overview]
Message-ID: <kl6lwnkytp3z.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <3ad3941c-de18-41bf-2e44-4238ae868d79@gmail.com>

Thanks for the thorough review! I really appreciate it, Philippe :)

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> Hi Glen,
>
> Le 2021-11-22 à 17:32, Glen Choo a écrit :
>> Add a --recurse-submodules option when creating branches so that `git
>> branch --recurse-submodules topic` will create the "topic" branch in the
>> superproject and all submodules. Guard this (and future submodule
>> branching) behavior behind a new configuration value
>> 'submodule.propagateBranches'.
>> 
>> Signed-off-by: Glen Choo <chooglen@google.com>
>> ---
>>   Documentation/config/advice.txt    |   3 +
>>   Documentation/config/submodule.txt |   9 ++
>
> We would need to add the new flag to Documentation/git-branch.txt,
> and also probably update the documentation of 'submodule.recurse'
> in 'Documentation/config/submodule.txt'.

Yes, thanks for the catch.

>> diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
>> index ee454f8126..c318b849aa 100644
>> --- a/Documentation/config/submodule.txt
>> +++ b/Documentation/config/submodule.txt
>> @@ -72,6 +72,15 @@ submodule.recurse::
>>   	For these commands a workaround is to temporarily change the
>>   	configuration value by using `git -c submodule.recurse=0`.
>>   
>> +submodule.propagateBranches::
>> +	[EXPERIMENTAL] A boolean that enables branching support with
>> +	submodules. This allows certain commands to accept
>> +	`--recurse-submodules` (`git branch --recurse-submodules` will
>> +	create branches recursively), and certain commands that already
>> +	accept `--recurse-submodules` will now consider branches (`git
>> +	switch --recurse-submodules` will switch to the correct branch
>> +	in all submodules).
>
> Looking at the rest of the patch, this just implements 'branch --recurse-submodules', right ?
> i.e. 'git switch' and 'git checkout' are left alone for
> now, so I think this addition to the doc should only mention 'git
> branch'.

That sounds reasonable. I can move this description into the commit
message instead.

>> diff --git a/advice.h b/advice.h
>> index 601265fd10..a7521d6087 100644
>> --- a/advice.h
>> +
>> +		if (repo_submodule_init(
>> +			    &subrepos[i], r,
>> +			    submodule_entry_list->name_entries[i].path,
>> +			    &super_oid)) {
>> +			die(_("submodule %s: unable to find submodule"),
>> +			    submodules[i].name);
>> +			if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
>> +				advise(_("You may try initializing the submodules using 'git checkout %s && git submodule update'"),
>> +				       start_name);
>
> Apart from what Ævar pointed out about advise() being called after die(),
> I'm not sure this is the right advice, because if repo_submodule_init fails
> it means there is no .git/modules/<name> directory corresponding to the submodule's
> Git repository, i.e. the submodule was never cloned. So it's not guaranteed
> that 'git checkout $start_name && git submodule update' would initialize (and clone) it,
> not without '--init'.

After further testing, it seems that --init might be required for
recursive submodules, but as you note later on, it's not needed for the
test case I have created. Using --init is still good advice though, so I
will add that.

>> +	for (i = 0; i < submodule_entry_list->entry_nr; i++) {
>
> Here we loop over all submodules, so branches are created even in
> inactive submodules... this might not be wanted.

Yes, we should ignore inactive submodules. This is a bug.

>> @@ -713,9 +726,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>   	if (create < 0)
>>   		usage_with_options(builtin_branch_usage, options);
>>   
>> +	if (recurse_submodules_explicit && submodule_propagate_branches &&
>> +	    !create)
>> +		die(_("--recurse-submodules can only be used to create branches"));
>>   	if (dry_run && !create)
>>   		die(_("--dry-run can only be used when creating branches"));
>>   
>> +	recurse_submodules =
>> +		(recurse_submodules || recurse_submodules_explicit) &&
>> +		submodule_propagate_branches;
>> +
>
> OK, so we get the new behaviour if either --recurse-submodules was used, or 'submodule.recurse' is true,
> and in both case we also need the new submodule.propagateBranches config set.
>
> Why not adding 'branch.recurseSubmodules' instead, with a higher priority than submodule.recurse ?
> Is it because then it would be mildly confusing for 'git checkout / git switch' to also honor
> a setting named 'branch.*' when they learn the new behaviour ? (I don't think this would be the
> first time that 'git foo' honors 'bar.*', so it might be worth mentioning).

I am avoiding the prefix 'branch.' because that might suggest that the
functionality is centered around the 'git branch' command. I chose the
'submodule.' prefix because what we are feature flagging is an entirely
redesigned UX for _submodules_ that uses branches; we also have work
planned for other commands like push/merge/rebase/reset.

> Also, why do we quietly ignore '--recurse-submodules' if submodule.propagateBranches is unset ?
> Wouldn't it be better to warn the user "hey, if you want this new behaviour you need to
> set that config !" ?

Ah, yes this is an oversight on my part.

> I don't have a strong opinion about the fact that you need to set the config in the first
> place, but I think it should be mentioned in the commit message why you chose to implement
> it that way (meaning, why do we need a config set, instead of adding the config but defaulting it
> to true, so that you get the new behaviour by default, but you still can disable it if you do not
> want it)...

It seems self-evident to me that experimental features should not be
shipped to users by default.

>> +test_expect_success 'should not set up unnecessary tracking of local branches' '
>> +	test_when_finished "cleanup_branches super branch-a" &&
>> +	(
>> +		cd super &&
>> +		git 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)" = ""
>> +	)
>
> don't we have a "config is empty" test helper or something similar ?

Hm, I couldn't find one, but there is test_cmp_config(). That's probably
better than calling test() directly.

>> +test_expect_success 'should not create branch when submodule is not in .git/modules' '
>> +	# The cleanup needs to delete sub2:branch-b in particular because main does not have sub2
>> +	test_when_finished "git -C super-clone/sub2 branch -D branch-b && \
>> +		cleanup_branches super-clone branch-a branch-b" &&
>> +	(
>> +		cd super-clone &&
>> +		# This should succeed because super-clone has sub.
>> +		git branch --recurse-submodules branch-a origin/branch-a &&
>> +		# This should fail because super-clone does not have sub2.
>> +		test_must_fail git branch --recurse-submodules branch-b origin/branch-b 2>actual &&
>> +		cat >expected <<-EOF &&
>> +		fatal: submodule sub: unable to find submodule
>> +		You may reinitialize the submodules using ${SQ}git checkout origin/branch-b && git submodule update${SQ}
>> +		EOF
>> +		test_must_fail git rev-parse branch-b &&
>> +		test_must_fail git -C sub rev-parse branch-b &&
>> +		# User can fix themselves by initializing the submodule
>> +		git checkout origin/branch-b &&
>> +		git submodule update &&
>> +		git branch --recurse-submodules branch-b origin/branch-b
>> +	)
>
> Considering what has been pointed out above, I'm not sure why this test passes...
> Unless I'm missing something.

As I understand it, --init is used to set values in .git/config. My best
guess is that 'git submodule update' doesn't use .git/config at all - it
looks for submodules in the index and .gitmodules and clones the
submodules as expected.

I still think that we should promote --init, but I still find this
situation very strange and inconsistent.