From: Philippe Blain <levraiphilippeblain@gmail.com>
To: Glen Choo <chooglen@google.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>,
"Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH v3 4/5] branch: add --recurse-submodules option for branch creation
Date: Sat, 11 Dec 2021 13:08:11 -0500 [thread overview]
Message-ID: <a54c6015-6afb-b25f-d2d2-392bf77e93f0@gmail.com> (raw)
In-Reply-To: <20211209184928.71413-5-chooglen@google.com>
Hi Glen,
Le 2021-12-09 à 13:49, Glen Choo a écrit :
> To improve the submodules UX, we would like to teach Git to handle
> branches in submodules. Start this process by teaching `git branch` the
> --recurse-submodules option so that `git branch --recurse-submodules
> topic` will create the "topic" branch in the superproject and its
> submodules.
>
> Although this commit does not introduce breaking changes, it is
> incompatible with existing --recurse-submodules semantics e.g. `git
> checkout` does not recursively checkout the expected branches created by
> `git branch` yet. To ensure that the correct set of semantics is used,
> this commit introduces a new configuration value,
> `submodule.propagateBranches`, which enables submodule branching when
> true (defaults to false).
>
> This commit includes changes that allow Git to work with submodules
> that are in trees (and not just the index):
>
> * add a submodules_of_tree() helper that gives the relevant
> information of an in-tree submodule (e.g. path and oid) and
> initializes the repository
> * add is_tree_submodule_active() by adding a treeish_name parameter to
> is_submodule_active()
> * add the "submoduleNotUpdated" advice to advise users to update the
> submodules in their trees
>
> Other changes
>
> * add a "dry_run" parameter to create_branch() in order to support
> `git submodule--helper create-branch --dry-run`
>
> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
> Documentation/config/advice.txt | 3 +
> Documentation/config/submodule.txt | 8 +
Same comment as I remarked in v1 [1]:
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'.
[1] https://lore.kernel.org/git/3ad3941c-de18-41bf-2e44-4238ae868d79@gmail.com/
> advice.c | 1 +
> advice.h | 1 +
> branch.c | 129 ++++++++++++-
> branch.h | 31 +++-
> builtin/branch.c | 40 +++-
> builtin/checkout.c | 3 +-
> builtin/submodule--helper.c | 38 ++++
> submodule-config.c | 35 ++++
> submodule-config.h | 35 ++++
> submodule.c | 11 +-
> submodule.h | 3 +
> t/t3207-branch-submodule.sh | 284 +++++++++++++++++++++++++++++
> 14 files changed, 609 insertions(+), 13 deletions(-)
> create mode 100755 t/t3207-branch-submodule.sh
>
> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
> index 063eec2511..e52262dc69 100644
> --- a/Documentation/config/advice.txt
> +++ b/Documentation/config/advice.txt
> @@ -116,6 +116,9 @@ advice.*::
> submoduleAlternateErrorStrategyDie::
> Advice shown when a submodule.alternateErrorStrategy option
> configured to "die" causes a fatal error.
> + submodulesNotUpdated::
> + Advice shown when a user runs a submodule command that fails
> + because `git submodule update` was not run.
I see you added '--init' in the actual message below, but maybe it would be more accurate
to also add it here ?
> addIgnoredFile::
> Advice shown if a user attempts to add an ignored file to
> the index.
> diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
> index ee454f8126..52b35964c0 100644
> --- a/Documentation/config/submodule.txt
> +++ b/Documentation/config/submodule.txt
> @@ -72,6 +72,14 @@ 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 when
> + using `--recurse-submodules` or `submodule.recurse=true`.
> + Enabling this will allow certain commands to accept
> + `--recurse-submodules` and certain commands that already accept
> + `--recurse-submodules` will now consider branches.
> + Defaults to false.
> +
Thanks, that updated description seems simpler and more accurate with what
this series is doing.
> submodule.fetchJobs::
> Specifies how many submodules are fetched/cloned at the same time.
> A positive integer allows up to that number of submodules fetched
> diff --git a/branch.c b/branch.c
> index 6b9d64cdf9..305154de0b 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -8,6 +8,8 @@
> +void create_branches_recursively(struct repository *r, const char *name,
> + const char *start_name,
> + const char *tracking_name, int force,
> + int reflog, int quiet, enum branch_track track,
> + int dry_run)
> +{
> + int i = 0;
> + char *branch_point = NULL;
> + struct object_id super_oid;
> + struct submodule_entry_list submodule_entry_list;
> +
> + /* Perform dwim on start_name to get super_oid and branch_point. */
> + validate_branch_start(r, start_name, BRANCH_TRACK_NEVER, &super_oid,
> + &branch_point);
> +
> + /*
> + * If we were not given an explicit name to track, then assume we are at
> + * the top level and, just like the non-recursive case, the tracking
> + * name is the branch point.
> + */
> + if (!tracking_name)
> + tracking_name = branch_point;
> +
> + submodules_of_tree(r, &super_oid, &submodule_entry_list);
> + /*
> + * Before creating any branches, first check that the branch can
> + * be created in every submodule.
> + */
> + for (i = 0; i < submodule_entry_list.entry_nr; i++) {
> + if (submodule_entry_list.entries[i].repo == NULL) {
> + if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
> + advise(_("You may try updating the submodules using 'git checkout %s && git submodule update --init'"),
> + start_name);
> + die(_("submodule '%s': unable to find submodule"),
small nit, maybe write: "unable to find submodule repository" ?
> + submodule_entry_list.entries[i].submodule->name);
> + }
> +
> + if (submodule_create_branch(
> + submodule_entry_list.entries[i].repo,
> + submodule_entry_list.entries[i].submodule, name,
> + oid_to_hex(&submodule_entry_list.entries[i]
> + .name_entry->oid),
> + tracking_name, force, reflog, quiet, track, 1))
Here, we do not actually use the dry_run argument, since we *always* want to
do a dry run for the submodules...
> + die(_("submodule '%s': cannot create branch '%s'"),
> + submodule_entry_list.entries[i].submodule->name,
> + name);
> + }
> +
> + create_branch(the_repository, name, start_name, force, 0, reflog, quiet,
> + BRANCH_TRACK_NEVER, dry_run);
... whereas for the superproject branch, we use the given dry_run argument...
> + if (dry_run)
> + return;
> + /*
> + * NEEDSWORK If tracking was set up in the superproject but not the
> + * submodule, users might expect "git branch --recurse-submodules" to
> + * fail or give a warning, but this is not yet implemented because it is
> + * tedious to determine whether or not tracking was set up in the
> + * superproject.
> + */
> + setup_tracking(name, tracking_name, track, quiet, 0);
> +
> + for (i = 0; i < submodule_entry_list.entry_nr; i++) {
> + if (submodule_create_branch(
> + submodule_entry_list.entries[i].repo,
> + submodule_entry_list.entries[i].submodule, name,
> + oid_to_hex(&submodule_entry_list.entries[i]
> + .name_entry->oid),
> + tracking_name, force, reflog, quiet, track, 0))
... and if !dry_run, then we do create the branches in the submodules.
That is a little bit hard to follow if you are not careful. Maybe it's just me,
but as I was first reading it I wondered why '0' and '1' were hard-coded as the dry-run
arguments to submodule_create_branch... It would maybe be clearer to use a named
variable ?
> @@ -851,6 +874,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
> strbuf_release(&buf);
> } else if (!noncreate_actions && argc > 0 && argc <= 2) {
> + const char *branch_name = argv[0];
> + const char *start_name = argc == 2 ? argv[1] : head;
> +
> if (filter.kind != FILTER_REFS_BRANCHES)
> die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
> "Did you mean to use: -a|-r --list <pattern>?"));
> @@ -858,10 +884,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> if (track == BRANCH_TRACK_OVERRIDE)
> die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
>
> - create_branch(the_repository,
> - argv[0], (argc == 2) ? argv[1] : head,
> - force, 0, reflog, quiet, track);
> -
> + if (recurse_submodules) {
> + create_branches_recursively(the_repository, branch_name,
> + start_name, NULL, force,
> + reflog, quiet, track, 0);
Here again, maybe it would be clearer to use a named variable instead of hard-coding '0' ?
> + return 0;
> + }
> + create_branch(the_repository, branch_name, start_name, force, 0,
> + reflog, quiet, track, 0);
Same here.
> } else
> usage_with_options(builtin_branch_usage, options);
>
> diff --git a/t/t3207-branch-submodule.sh b/t/t3207-branch-submodule.sh
> new file mode 100755
> index 0000000000..2dd0e2b01f
> --- /dev/null
> +++ b/t/t3207-branch-submodule.sh
The tests look pretty thourough.
> +
> +test_expect_success 'should create branch when submodule is not in HEAD .gitmodules' '
micro-nit: maybe write: HEAD:.gitmodules as this is revision synatx.
Cheers,
Philippe.
next prev parent reply other threads:[~2021-12-11 18:08 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
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 [this message]
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=a54c6015-6afb-b25f-d2d2-392bf77e93f0@gmail.com \
--to=levraiphilippeblain@gmail.com \
--cc=avarab@gmail.com \
--cc=chooglen@google.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.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).