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

On Mon, Nov 22 2021, Glen Choo wrote:

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.