Re: [PATCHv9 4/6] git submodule update: have a dedicated helper for cloning

Stefan Beller [off-list ref] writes:

+	for (; pp->count < pp->list.nr; pp->count++) {
+		const struct submodule *sub = NULL;
+		const struct cache_entry *ce = pp->list.entries[pp->count];
+		struct strbuf displaypath_sb = STRBUF_INIT;
+		struct strbuf sb = STRBUF_INIT;
+		const char *displaypath = NULL;
+		char *url = NULL;
+		int needs_cloning = 0;
+
+		if (ce_stage(ce)) {
+			if (pp->recursive_prefix)
+				strbuf_addf(err,
+					"Skipping unmerged submodule %s/%s\n",
+					pp->recursive_prefix, ce->name);

The funny indentation of the string is a workaround for overly deep
nesting, but is the overly deep nesting telling us that perhaps one
iteration of this loop can be an invocation of a helper function, I
wonder?

+			else
+				strbuf_addf(err,
+					"Skipping unmerged submodule %s\n",
+					ce->name);
+			goto cleanup_and_continue;
+		}

+
+		sub = submodule_from_path(null_sha1, ce->name);
+
+		if (pp->recursive_prefix)
+			displaypath = relative_path(pp->recursive_prefix,
+						    ce->name, &displaypath_sb);
+		else
+			displaypath = ce->name;
+
+		if ((pp->update && !strcmp(pp->update, "none")) ||
+		    (!pp->update && sub->update == SM_UPDATE_NONE)) {

This looks a bit strange.  I wonder pp->update should also become
enum for the same reason why sub->update has become enum.  That way,
we need to be worried about parsing these tokens in one place where
a textual string "none" is translated to SM_UPDATE_NONE.  If we
started allowing "None" in the sub->update parse_config() in
submodule-config.c, we would want that new parsing rule propagated
to pp->update, right?

+			strbuf_addf(err, "Skipping submodule '%s'\n",
+				    displaypath);
+			goto cleanup_and_continue;
+		}