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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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? The
plural would make senes if the advice aggregates them to the end, let's
look at the implementation...

> [...]
> +	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.

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 would also justify the plural "submodulesNotUpdated", if such an
advise() printed out a summary of the N that failed at the end.