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

[Philippe Blain <levraiphilippeblain@gmail.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.