Thanks for the thorough review! I really appreciate it, Philippe :)
Philippe Blain <levraiphilippeblain@xxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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.
