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

Ævar Arnfjörð Bjarmason [off-list ref] writes:

On Mon, Nov 22 2021, Glen Choo wrote:

quoted

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

quoted

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