From: Philippe Blain Date: Sat, 11 Dec 2021 18:08:11 GMT Subject: Re: [PATCH v3 4/5] branch: add --recurse-submodules option for branch creation Message-ID: 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 > --- > 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 ?")); > @@ -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.