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

[Glen Choo <chooglen@google.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Mon, Nov 22 2021, Glen Choo wrote:
>
>>  	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.
>>  	addIgnoredFile::
>>  		Advice shown if a user attempts to add an ignored file to
>>  		the index.
>
> Does it need to be submodule*s*NotUpdated? I.e. the existing error is
> submodule.. (non-plural), and we surely error on this per-submodule?

From the user's perspective, failing on a per-submodule basis looks like
an implementation detail. As a user, I am looking for advice on errors
that could be avoided by running "git submodule update"; I'd want the
advice option to describe what "git submodule update" does, which is
update submodule*s*.

>> [...]
>> +	for (i = 0; i < submodule_entry_list->entry_nr; i++) {
>> +		submodules[i] = *submodule_from_path(
>> +			r, &super_oid,
>> +			submodule_entry_list->name_entries[i].path);
>> +
>> +		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);
>> +		}
>
> Uh, a call to advise() after die()? :) That code isn't reachable.
>
> It would be good to add test for what the output is in the next
> iteration, which would be a forcing function for making sure this code
> works.

Whoops. Good catch. Yes I should test that.

> One thing I find quite annoying about submodules currently is the
> verbosity of the output when we do N operations. E.g. I've got a repo
> with 15-20 small submodules, cloning it prints out the usual "git clone"
> verbosity, which isn't so much when cloning one repo, but with 15-20 it
> fills up your screen.
>
> Operations like these should also behave more like "git fetch --all",
> surely? I.e. let's try to run the operation on all, but if some failed
> along the way let's have our exit code reflect that, and ideally print
> out an error summary, not N number of error() calls.

That's a valid criticism, and one we're concerned about as well. I'm
not 100% satisfied with how I've structured the output either, but as a
practical matter, figuring out an *ideal* output format and
standardizing it takes up too much valuable iteration time that could be
spent on improving the UX instead.

So the approach here is to have output that is 'good enough' for now,
and to create better and standardized output when we've nailed down more
of the UX.

> That would also justify the plural "submodulesNotUpdated", if such an
> advise() printed out a summary of the N that failed at the end.

Fair. We could treat an uninitialized submodule the same as any other
failure and summarize all failures.