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

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

Philippe Blain [off-list ref] writes:

Hi Glen,

Le 2021-11-22 à 17:32, Glen Choo a écrit :

quoted

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

quoted

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.

quoted

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.

quoted

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

quoted

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

quoted

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

quoted

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