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.
next prev parent reply other threads:[~2021-11-23 23:43 UTC|newest]
Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-22 22:32 [PATCH 0/4] implement branch --recurse-submodules Glen Choo
2021-11-22 22:32 ` [PATCH 1/4] submodule-config: add submodules_of_tree() helper Glen Choo
2021-11-23 2:12 ` Jonathan Tan
2021-11-23 19:48 ` Glen Choo
2021-11-23 10:53 ` Ævar Arnfjörð Bjarmason
2021-11-23 18:35 ` Glen Choo
2021-11-23 22:46 ` Junio C Hamano
2021-11-22 22:32 ` [PATCH 2/4] branch: refactor out branch validation from create_branch() Glen Choo
2021-11-22 22:32 ` [PATCH 3/4] branch: add --dry-run option to branch Glen Choo
2021-11-23 10:42 ` Ævar Arnfjörð Bjarmason
2021-11-23 18:42 ` Glen Choo
2021-11-23 23:10 ` Jonathan Tan
2021-11-24 0:52 ` Glen Choo
2021-11-22 22:32 ` [PATCH 4/4] branch: add --recurse-submodules option for branch creation Glen Choo
2021-11-23 10:45 ` Ævar Arnfjörð Bjarmason
2021-11-23 18:56 ` Glen Choo
2021-11-23 19:41 ` Philippe Blain
2021-11-23 23:43 ` Glen Choo [this message]
2021-11-24 1:31 ` Jonathan Tan
2021-11-24 18:18 ` Glen Choo
2021-11-29 21:01 ` Jonathan Tan
2021-12-06 21:55 ` [PATCH v2 0/3] implement branch --recurse-submodules Glen Choo
2021-12-06 21:55 ` [PATCH v2 1/3] branch: move --set-upstream-to behavior to setup_tracking() Glen Choo
2021-12-06 22:48 ` Junio C Hamano
2021-12-08 18:48 ` Glen Choo
2021-12-06 23:28 ` Junio C Hamano
2021-12-08 17:09 ` Glen Choo
2021-12-06 21:55 ` [PATCH v2 2/3] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-06 21:55 ` [PATCH v2 3/3] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-09 18:49 ` [PATCH v3 0/5] implement branch --recurse-submodules Glen Choo
2021-12-09 18:49 ` [PATCH v3 1/5] branch: move --set-upstream-to behavior to setup_tracking() Glen Choo
2021-12-09 21:19 ` Jonathan Tan
2021-12-09 22:16 ` Glen Choo
2021-12-09 18:49 ` [PATCH v3 2/5] branch: remove forward declaration of validate_branch_start() Glen Choo
2021-12-09 18:49 ` [PATCH v3 3/5] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-09 21:23 ` Jonathan Tan
2021-12-09 21:57 ` Glen Choo
2021-12-09 18:49 ` [PATCH v3 4/5] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-11 18:08 ` Philippe Blain
2021-12-14 20:08 ` Glen Choo
2021-12-09 18:49 ` [PATCH v3 5/5] branch.c: replace questionable exit() codes Glen Choo
2021-12-10 2:21 ` Ævar Arnfjörð Bjarmason
2021-12-10 17:43 ` Glen Choo
2021-12-13 9:02 ` Junio C Hamano
2021-12-13 9:19 ` Ævar Arnfjörð Bjarmason
2021-12-13 19:26 ` Junio C Hamano
2021-12-09 21:59 ` [PATCH v3 0/5] implement branch --recurse-submodules Jonathan Tan
2021-12-09 22:21 ` Glen Choo
2021-12-13 23:20 ` Jonathan Tan
2021-12-14 18:47 ` Glen Choo
2021-12-16 0:32 ` [PATCH v4 " Glen Choo
2021-12-16 0:32 ` [PATCH v4 1/5] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2021-12-16 0:32 ` [PATCH v4 2/5] branch: make create_branch() always create a branch Glen Choo
2021-12-16 0:32 ` [PATCH v4 3/5] branch: add a dry_run parameter to create_branch() Glen Choo
2021-12-16 0:32 ` [PATCH v4 4/5] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-16 0:32 ` [PATCH v4 5/5] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-16 23:33 ` [PATCH v5 0/5] implement branch --recurse-submodules Glen Choo
2021-12-16 23:33 ` [PATCH v5 1/5] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2021-12-16 23:33 ` [PATCH v5 2/5] branch: make create_branch() always create a branch Glen Choo
2021-12-16 23:33 ` [PATCH v5 3/5] branch: add a dry_run parameter to create_branch() Glen Choo
2021-12-16 23:33 ` [PATCH v5 4/5] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-16 23:33 ` [PATCH v5 5/5] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-17 0:34 ` [PATCH v5 0/5] implement branch --recurse-submodules Junio C Hamano
2021-12-17 0:45 ` Junio C Hamano
2021-12-20 19:09 ` Glen Choo
2021-12-20 19:50 ` Junio C Hamano
2021-12-20 20:25 ` Glen Choo
2021-12-20 23:34 ` [PATCH v6 " Glen Choo
2021-12-20 23:34 ` [PATCH v6 1/5] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2022-01-11 2:09 ` Jonathan Tan
2022-01-11 17:29 ` Glen Choo
2022-01-11 20:03 ` Jonathan Tan
2021-12-20 23:34 ` [PATCH v6 2/5] branch: make create_branch() always create a branch Glen Choo
2022-01-11 2:19 ` Jonathan Tan
2022-01-11 17:51 ` Glen Choo
2021-12-20 23:34 ` [PATCH v6 3/5] branch: add a dry_run parameter to create_branch() Glen Choo
2021-12-20 23:34 ` [PATCH v6 4/5] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-20 23:34 ` [PATCH v6 5/5] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-26 4:09 ` Junio C Hamano
2022-01-11 3:28 ` Jonathan Tan
2022-01-11 18:11 ` Glen Choo
2022-01-11 20:15 ` Jonathan Tan
2022-01-11 23:22 ` Glen Choo
2021-12-20 23:36 ` [PATCH v6 0/5] implement branch --recurse-submodules Glen Choo
2021-12-21 1:07 ` Junio C Hamano
2021-12-21 17:51 ` Glen Choo
2022-01-24 20:44 ` [PATCH v7 0/6] " Glen Choo
2022-01-24 20:44 ` [PATCH v7 1/6] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2022-01-24 20:44 ` [PATCH v7 2/6] branch: make create_branch() always create a branch Glen Choo
2022-01-24 20:44 ` [PATCH v7 3/6] branch: add a dry_run parameter to create_branch() Glen Choo
2022-01-24 20:44 ` [PATCH v7 4/6] builtin/branch: consolidate action-picking logic in cmd_branch() Glen Choo
2022-01-24 20:44 ` [PATCH v7 5/6] branch: add --recurse-submodules option for branch creation Glen Choo
2022-01-27 20:29 ` Jonathan Tan
2022-01-27 21:32 ` Glen Choo
2022-01-27 22:42 ` Glen Choo
2022-01-24 20:44 ` [PATCH v7 6/6] branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaks Glen Choo
2022-01-27 22:15 ` Junio C Hamano
2022-01-28 19:44 ` Glen Choo
2022-01-29 0:04 ` [PATCH v8 0/6] implement branch --recurse-submodules Glen Choo
2022-01-29 0:04 ` [PATCH v8 1/6] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2022-01-29 0:04 ` [PATCH v8 2/6] branch: make create_branch() always create a branch Glen Choo
2022-02-01 22:20 ` Junio C Hamano
2022-01-29 0:04 ` [PATCH v8 3/6] branch: add a dry_run parameter to create_branch() Glen Choo
2022-01-29 0:04 ` [PATCH v8 4/6] builtin/branch: consolidate action-picking logic in cmd_branch() Glen Choo
2022-01-29 0:04 ` [PATCH v8 5/6] branch: add --recurse-submodules option for branch creation Glen Choo
2022-02-04 1:10 ` Glen Choo
2022-02-04 16:15 ` Junio C Hamano
2022-02-04 18:10 ` Glen Choo
2022-01-29 0:04 ` [PATCH v8 6/6] branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaks Glen Choo
2022-02-01 17:43 ` [PATCH v8 0/6] implement branch --recurse-submodules Jonathan Tan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=kl6lwnkytp3z.fsf@chooglen-macbookpro.roam.corp.google.com \
--to=chooglen@google.com \
--cc=avarab@gmail.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.com \
--cc=levraiphilippeblain@gmail.com \
--cc=steadmon@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).