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

[Glen Choo <chooglen@google.com>]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> Hi Glen,
>
> Le 2021-12-09 à 13:49, Glen Choo a écrit :
>>   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/

Ah, I missed Documentation/git-branch.txt. Thanks.

I avoided updating 'submodule.recurse' because it seemed a bit redundant
when 'submodule.propagateBranches' is in the next paragraph. But I can
imagine that this helps users with tunnel vision (like me...), so I'll
do the update.

>> 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 ?

Sounds good.

>> +	/*
>> +	 * 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" ?

I don't think that adding the word 'repository' makes the problem
clearer to the user. It might even be misleading e.g. a hypotethical
user might think "Unable to find the repository? The remote repository
is giving a 404 Not Found?".

>> +			    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 ?

This is valid feedback, but I'm not sure how to make this clearer
(besides adding comments). In every case, we really do want the
hard-coded value, i.e. we'd always want dry_run = 1 (validation) at the
beginning, and always dry_run = 0 (actually create branches) at the end.

As such, a named variable like the following:

  int dry_run_off = 0;
  setup_tracking(name, tracking_name, track, quiet, dry_run_off);

seems rather artificial. Perhaps you had something like an enum in mind,
like:

  enum dry_run {
    DRY_RUN_OFF,
    DRY_RUN_ON
  };
  setup_tracking(name, tracking_name, track, quiet, DRY_RUN_OFF);

which is better, but still feels artificial for a single function with
an boolean parameter.

Passing literal 0s and 1s don't seem to contradict
Documentation/CodingGuidelines or the style of code I have touched, e.g.

>> -		create_branch(the_repository,
>> -			      argv[0], (argc == 2) ? argv[1] : head,
>> -			      force, 0, reflog, quiet, track);

hard-codes 0 to clobber_head_ok.

Perhaps you had something else in mind?

>> @@ -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.

I agree that the previous example of create_branches_recursively() is
hard to follow, but for cmd_branch() I think there is very little reason
to name a variable for dry_run when cmd_branch() only uses 0.

>>   	} 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.

Thanks!

>> +
>> +test_expect_success 'should create branch when submodule is not in HEAD .gitmodules' '
>
> micro-nit: maybe write: HEAD:.gitmodules as this is revision synatx.

Sounds good.