submodule: remove "--recursive-prefix"

= Description

This series is a refactor of "git submodule--helper update" that replaces "--recursive-prefix" with "--super-prefix" (see Background). This was initially motivated by:

* Junio's suggestion to simplify the code [1] (in response to a memory leak found by Johannes Schindelin [2]).
* A desire to use the module_list API + get_submodule_displaypath() outside of builtin/submodule--helper.c (I expect to use this to check out branches in each submodule).

But it also happens to remove some overly complicated/duplicated code that was literally converted from shell :)

= Background

When recursing into nested submodules, Git commands keep track of the path from the superproject to the submodule in order to print a "display path" to the user, e.g.

Submodule path '../super/sub/nested-sub': checked out 'abcdef'

For historical reasons, "git submodule--helper update" uses "--recursive-prefix" for this purpose, but it should use "--super-prefix" instead because:

* That's what every other command uses (not just submodule--helper subcommands).
* Using the "--super-prefix" helper functions makes the "git submodule--helper update" code simpler

= Patch organization

* Patch 1/5 makes sure that display paths are only computed using display path helper functions ([do_]get_submodule_displaypath()) and fixes some display paths that we never realized were broken.
* Patches 2-3/5 simplify and deduplicate some display path computations.
* Patch 4/5 replaces "--recursive-prefix" with "--super-prefix", making do_get_submodule_displaypath() obsolete.
* Patch 5/5 removes do_get_submodule_displaypath().

= Interactions with other series

This would have been rebased onto ab/submodule-cleanup, but it's not yet clear to me if that series will be merged first. That series is almost done, but Ævar is still doing some digging on SUPPORT_SUPER_PREFIX [3] and may come back with findings that affect this series.

Fortunately, this series is only tangentially related to ab/submodule-cleanup, and the conflicts are quite simple:

| this | ab/submodule-cleanup | resolution |
|-----------------------------+-------------------------------+---------------------------|
| push --super-prefix arg | add new "ud" var | keep both |
|-----------------------------+-------------------------------+---------------------------|
| remove "--recursive-prefix" | add "--checkout", "--rebase", | keep both |
| | "--merge" | |
|-----------------------------+-------------------------------+---------------------------|
| add SUPPORT_SUPER_PREFIX | remove SUPPORT_SUPER_PREFIX | keep
ab/submodule-cleanup | | to "git submodule--helper | from "git
submodule--helper" | | | update" | | |

= Future work

At the end of this series, get_submodule_displaypath() can be moved to submodule.h, which would make submodule.c:get_super_prefix_or_empty() obsolete. I have a patch that demonstrates this (CI run: [4]), but I decided to keep this series more focused on "git submodule--helper update" so that it's easier to review.

[1] https://lore.kernel.org/git/[email protected] [2]
https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com
[3]
https://lore.kernel.org/git/[email protected]
[4] https://github.com/chooglen/git/actions/runs/2572557584

Glen Choo (5):
submodule--helper update: use display path helper
submodule--helper: don't recreate recursive prefix
submodule--helper: use correct display path helper submodule--helper update: use --super-prefix
submodule--helper: remove display path helper

 builtin/submodule--helper.c | 82 +++++++++----------------------------
 t/t7406-submodule-update.sh | 59 ++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 62 deletions(-)

base-commit: 39c15e485575089eb77c769f6da02f98a55905e0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1282%2Fchooglen%2Fsubmodule%2Fremove-recursive-prefix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1282/chooglen/submodule/remove-recursive-prefix-v1
Pull-Request: https://github.com/git/git/pull/1282

gitgitgadget

From: Glen Choo <[email protected]>

There are two locations in prepare_to_clone_next_submodule() that manually calculate the submodule display path, but should just use do_get_submodule_displaypath() for consistency.

Do this replacement and reorder the code slightly to avoid computing the display path twice.

This code was never tested, and adding tests shows that both these sites have been computing the display path incorrectly ever since they were introduced in 48308681b0 (git submodule update: have a dedicated helper for cloning, 2016-02-29) [1]:

- The first hunk puts a "/" between recursive_prefix and ce->name, but recursive_prefix already ends with "/".
- The second hunk calls relative_path() on recursive_prefix and ce->name, but relative_path() only makes sense when both paths share the same base directory. This is never the case here:
- recursive_prefix is the path from the topmost superproject to the current submodule
- ce->name is the path from the root of the current submodule to its submodule.
so, e.g. recursive_prefix="super" and ce->name="submodule" produces displayname="../super" instead of "super/submodule".

While we're fixing the display names, also fix inconsistent quoting of the submodule name.

[1] I verified this by applying the tests to 48308681b0.

---
 builtin/submodule--helper.c | 22 +++++---------
 t/t7406-submodule-update.sh | 59 +++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 15 deletions(-)

index c597df7528e..63c661b26a6 100644
@@ -1949,30 +1949,22 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	const char *update_string;
 	enum submodule_update_type update_type;
 	char *key;
-	struct strbuf displaypath_sb = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
-	const char *displaypath = NULL;
+	char *displaypath = NULL;
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
+	displaypath = do_get_submodule_displaypath(ce->name,
+						   suc->update_data->prefix,
+						   suc->update_data->recursive_prefix);
+
 	if (ce_stage(ce)) {
-		if (suc->update_data->recursive_prefix)
-			strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name);
-		else
-			strbuf_addstr(&sb, ce->name);
-		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
-		strbuf_addch(out, '\n');
+		strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);
 		goto cleanup;
 	}
 
 	sub = submodule_from_path(the_repository, null_oid(), ce->name);
 
-	if (suc->update_data->recursive_prefix)
-		displaypath = relative_path(suc->update_data->recursive_prefix,
-					    ce->name, &displaypath_sb);
-	else
-		displaypath = ce->name;
-
 	if (!sub) {
 		next_submodule_warn_missing(suc, out, displaypath);
 		goto cleanup;

@@ -2062,7 +2054,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 					      "--no-single-branch");
 
 cleanup:
-	strbuf_release(&displaypath_sb);
+	free(displaypath);
 	strbuf_release(&sb);
 	if (need_free_url)
 		free((void*)url);

gitgitgadget

I think the tests should really be split up as their own preceding commit, so we're assured that this "git rebase -x 'make test'"'s cleanly.

Thanks! Everything not mentioned here seems like an obviously good suggestion. I especially appreciate the translation/compiler flow analysis pieces (I hadn't considered either).

Hm, I'm not sure how this changes the result of "git rebase -x 'make test'", since these tests will fail prior to this patch. I could modify the tests to demonstrate the broken behavior though, which would save me some effort in describing it in the commit message.

Yeah admittedly this is quite odd. I was hoping that this would be more readable than big chunks of "-C", since I personally find those quite difficult to follow when the directory changes from line to line. But, maybe it's just better to be consistent in just using "-C", and I'll find some way of organizing the lines that keeps them readable.

I suppose so, though I think there isn't enough repetition here to justify the readability/correctness trade-off.

From: Glen Choo <[email protected]>

update_submodule() uses duplicated code to compute update_data->displaypath and next.recursive_prefix. The latter is just the former with "/" appended to it, and since update_data->displaypath not changed outside of this statement, we can just reuse the already computed result.

We can go one step further and remove the reference to next.recursive_prefix altogether. Since it is only used in update_data_to_args() (to compute the "--recursive-prefix" flag for the recursive update child process) we can just use the already computed .displaypath value of there.

Delete the duplicated code, and remove the unnecessary reference to next.recursive_prefix. As a bonus, this fixes a memory leak where prefixed_path was never freed (this leak was first reported in [1]).

[1] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com

---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

index 63c661b26a6..7a963a967b8 100644
@@ -2401,9 +2401,9 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 {
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
-	if (update_data->recursive_prefix)
-		strvec_pushl(args, "--recursive-prefix",
-			     update_data->recursive_prefix, NULL);
+	if (update_data->displaypath)
+		strvec_pushf(args, "--recursive-prefix=%s/",
+			     update_data->displaypath);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)

@@ -2497,14 +2497,6 @@ static int update_submodule(struct update_data *update_data)
 		struct update_data next = *update_data;
 		int res;
 
-		if (update_data->recursive_prefix)
-			prefixed_path = xstrfmt("%s%s/", update_data->recursive_prefix,
-						update_data->sm_path);
-		else
-			prefixed_path = xstrfmt("%s/", update_data->sm_path);
-
-		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
-								  update_data->prefix);
 		next.prefix = NULL;
 		oidcpy(&next.oid, null_oid());
 		oidcpy(&next.suboid, null_oid());

gitgitgadget

From: Glen Choo <[email protected]>

Replace a chunk of code in update_submodule() with an equivalent do_get_submodule_displaypath() invocation. This is already tested by t/t7406-submodule-update.sh:'submodule update --init --recursive from subdirectory', so no tests are added.

The two are equivalent because:

- Exactly one of recursive_prefix|prefix is non-NULL at a time; prefix is set at the superproject level, and recursive_prefix is set when recursing into submodules. There is also a BUG() statement in get_submodule_displaypath() that asserts that both cannot be non-NULL.

- In get_submodule_displaypath(), get_super_prefix() always returns NULL because "--super-prefix" is never passed. Thus calling it is equivalent to calling do_get_submodule_displaypath() with super_prefix = NULL.

Therefore:

- When recursive_prefix is non-NULL, prefix is NULL, and thus get_submodule_displaypath() just returns prefixed_path. This is identical to calling do_get_submodule_displaypath() with super_prefix = recursive_prefix because the return value is still the concatenation of recursive_prefix + update_data->sm_path.

- When prefix is non-NULL, prefixed_path = update_data->sm_path. Thus calling get_submodule_displaypath() with prefixed_path is equivalent to calling do_get_submodule_displaypath() with update_data->sm_path

---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

index 7a963a967b8..fa8256526e9 100644
@@ -2445,19 +2445,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 
 static int update_submodule(struct update_data *update_data)
 {
-	char *prefixed_path;
-
 	ensure_core_worktree(update_data->sm_path);
 
-	if (update_data->recursive_prefix)
-		prefixed_path = xstrfmt("%s%s", update_data->recursive_prefix,
-					update_data->sm_path);
-	else
-		prefixed_path = xstrdup(update_data->sm_path);
-
-	update_data->displaypath = get_submodule_displaypath(prefixed_path,
-							     update_data->prefix);
-	free(prefixed_path);
+	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
+								update_data->prefix,
+								update_data->recursive_prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,

gitgitgadget

From: Glen Choo <[email protected]>

Unlike the other subcommands, "git submodule--helper update" uses the "--recursive-prefix" flag instead of "--super-prefix". The two flags are otherwise identical (they only serve to compute the 'display path' of a submodule), except that there is a dedicated helper function to get the value of "--super-prefix".

This inconsistency exists because "git submodule update" used to pass "--recursive-prefix" between shell and C (introduced in [1]) before "--super-prefix" was introduced (in [2]), and for simplicity, we kept this name when "git submodule--helper update" was created.

Remove "--recursive-prefix" and its associated code from "git submodule--helper update", replacing it with "--super-prefix".

To use "--super-prefix", module_update is marked with SUPPORT_SUPER_PREFIX. Note that module_clone must also be marked with SUPPORT_SUPER_PREFIX, otherwise the "git submodule--helper clone" subprocess will fail check because "--super-prefix" is propagated via the environment.

[1] 48308681b0 (git submodule update: have a dedicated helper for cloning, 2016-02-29)
[2] 74866d7579 (git: make super-prefix option, 2016-10-07)

---
 builtin/submodule--helper.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

index fa8256526e9..81ea4669aab 100644
@@ -477,22 +477,18 @@ static int starts_with_dot_dot_slash(const char *const path)
 
 struct init_cb {
 	const char *prefix;
-	const char *superprefix;
 	unsigned int flags;
 };
 #define INIT_CB_INIT { 0 }
 
 static void init_submodule(const char *path, const char *prefix,
-			   const char *superprefix, unsigned int flags)
+			   unsigned int flags)
 {
 	const struct submodule *sub;
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	/* try superprefix from the environment, if it is not passed explicitly */
-	if (!superprefix)
-		superprefix = get_super_prefix();
-	displaypath = do_get_submodule_displaypath(path, prefix, superprefix);
+	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 

@@ -566,7 +562,7 @@ static void init_submodule(const char *path, const char *prefix,
 static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)
 {
 	struct init_cb *info = cb_data;
-	init_submodule(list_item->name, info->prefix, info->superprefix, info->flags);
+	init_submodule(list_item->name, info->prefix, info->flags);
 }
 
 static int module_init(int argc, const char **argv, const char *prefix)

@@ -1880,7 +1876,6 @@ struct submodule_update_clone {
 
 struct update_data {
 	const char *prefix;
-	const char *recursive_prefix;
 	const char *displaypath;
 	const char *update_default;
 	struct object_id suboid;

@@ -1956,7 +1951,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 
 	displaypath = do_get_submodule_displaypath(ce->name,
 						   suc->update_data->prefix,
-						   suc->update_data->recursive_prefix);
+						   get_super_prefix());
 
 	if (ce_stage(ce)) {
 		strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);

@@ -2399,11 +2394,11 @@ static void ensure_core_worktree(const char *path)
 
 static void update_data_to_args(struct update_data *update_data, struct strvec *args)
 {
-	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
-	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->displaypath)
-		strvec_pushf(args, "--recursive-prefix=%s/",
+		strvec_pushf(args, "--super-prefix=%s/",
 			     update_data->displaypath);
+	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
+	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)

@@ -2449,7 +2444,7 @@ static int update_submodule(struct update_data *update_data)
 
 	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
 								update_data->prefix,
-								update_data->recursive_prefix);
+								get_super_prefix());
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,

@@ -2573,10 +2568,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "prefix", &opt.prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
-			   N_("path"),
-			   N_("path into the working tree, across nested "
-			      "submodule boundaries")),
 		OPT_STRING(0, "update", &opt.update_default,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),

@@ -2655,7 +2646,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			module_list_active(&list);
 
 		info.prefix = opt.prefix;
-		info.superprefix = opt.recursive_prefix;
 		if (opt.quiet)
 			info.flags |= OPT_QUIET;
 

@@ -3352,9 +3342,9 @@ struct cmd_struct {
 static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
-	{"clone", module_clone, 0},
+	{"clone", module_clone, SUPPORT_SUPER_PREFIX},
 	{"add", module_add, SUPPORT_SUPER_PREFIX},
-	{"update", module_update, 0},
+	{"update", module_update, SUPPORT_SUPER_PREFIX},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},

gitgitgadget

This is a good change, it was slightly confusing that --recursive-prefix is left in git-submodule.sh after this, but then I remembered that I removed it in my ab/submodule-cleanup, and you were presumably trying to avoid the conflict.

Still, I think it's probably better to either base this on my series (re-roll incoming), or take make this truly stand-alone, and have Junio sort out the minor conflict.

	f445c57490d (submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags, 2022-06-27)
	bac3def78e9 (submodule--helper.c: remove unnecessary ", 0" in init, 2022-06-27)
	af03aa2ad40 (submodule--helper.c: create a command dispatch helper, 2022-06-27)
	952fdec4cc0 (submodule--helper.c: make "support super prefix" a bitfield, not a flag, 2022-06-09)
	2d30186e633 (cocci: don't use strvec_pushl() if strvec_push() will do, 2022-06-27)
	8aa7e049360 (git.c: die earlier on bad "--super-prefix" combined with "-h", 2022-06-27)
	b0d324e9ad2 (git: make --super-prefix truly internal-only, BUG() on misuse, 2022-06-27)

Ah good point. This was an oversight actually; I initially based this off ab/submodule-cleanup, but decided to make it standalone. Thanks for spotting the mistake.

At any rate, I'm quite sure that ab/submodule-cleanup v5 is ready for 'next', so I'll rebase this.

Yes. Sigh..

It looks like "--histogram" produces the diff I'd want:

          enum submodule_update_type update_type = update_data->update_default;

+ if (update_data->displaypath)
+ strvec_pushf(args, "--super-prefix=%s/",

  +                            update_data->displaypath);
          strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
          strvec_pushf(args, "--jobs=%d", update_data->max_jobs);

- if (update_data->displaypath)
- strvec_pushf(args, "--recursive-prefix=%s/",
- update_data->displaypath);

but that probably means I'd need to give up on GGG :/ (which I've grown to like despite its shortcomings).

Very interesting, thanks for sharing :) I'll take your suggestion to cherry-pick f445c57490d.

From: Glen Choo <[email protected]>

All invocations of do_get_submodule_displaypath() pass get_super_prefix() as the super_prefix arg, which is exactly the same as get_submodule_displaypath().

Replace all calls to do_get_submodule_displaypath() with get_submodule_displaypath(), and since it has no more callers, remove it.

---
 builtin/submodule--helper.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

index 81ea4669aab..73b36f47600 100644
@@ -118,10 +118,11 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
-static char *do_get_submodule_displaypath(const char *path,
-					  const char *prefix,
-					  const char *super_prefix)
+/* the result should be freed by the caller. */
+static char *get_submodule_displaypath(const char *path, const char *prefix)
 {
+	const char *super_prefix = get_super_prefix();
+
 	if (prefix && super_prefix) {
 		BUG("cannot have prefix '%s' and superprefix '%s'",
 		    prefix, super_prefix);

@@ -137,13 +138,6 @@ static char *do_get_submodule_displaypath(const char *path,
 	}
 }
 
-/* the result should be freed by the caller. */
-static char *get_submodule_displaypath(const char *path, const char *prefix)
-{
-	const char *super_prefix = get_super_prefix();
-	return do_get_submodule_displaypath(path, prefix, super_prefix);
-}
-
 static char *compute_rev_name(const char *sub_path, const char* object_id)
 {
 	struct strbuf sb = STRBUF_INIT;

@@ -488,7 +482,7 @@ static void init_submodule(const char *path, const char *prefix,
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
+	displaypath = get_submodule_displaypath(path, prefix);
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 

@@ -1949,9 +1943,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
-	displaypath = do_get_submodule_displaypath(ce->name,
-						   suc->update_data->prefix,
-						   get_super_prefix());
+	displaypath =
+		get_submodule_displaypath(ce->name, suc->update_data->prefix);
 
 	if (ce_stage(ce)) {
 		strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);

@@ -2442,9 +2435,8 @@ static int update_submodule(struct update_data *update_data)
 {
 	ensure_core_worktree(update_data->sm_path);
 
-	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
-								update_data->prefix,
-								get_super_prefix());
+	update_data->displaypath = get_submodule_displaypath(
+		update_data->sm_path, update_data->prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,

gitgitgadget

Hm, maybe I'm not using GGG correctly, but these whitespace issues seem pretty common (Markdown collapsing the whitespace maybe?). Maybe I need to blockquote the monospaced stuff.

Here's the original table:

| this                        | ab/submodule-cleanup          | resolution                |
|-----------------------------+-------------------------------+---------------------------|
| push --super-prefix arg     | add new "ud" var              | keep both                 |
|-----------------------------+-------------------------------+---------------------------|
| remove "--recursive-prefix" | add "--checkout", "--rebase", | keep both                 |
|                             | "--merge"                     |                           |
|-----------------------------+-------------------------------+---------------------------|
| add SUPPORT_SUPER_PREFIX    | remove SUPPORT_SUPER_PREFIX   | keep ab/submodule-cleanup |
| to "git submodule--helper   | from "git submodule--helper"  |                           |
| update"                     |                               |                           |

(For the org-mode users: org-table-align fixes everything except the last row, presumably because of the rogue line breaks.)

Thanks Ævar for the feedback! I've incorporated all of the suggestions, except breaking up the tests in 1/6 into their own patch - I wasn't convinced of the value of having prescriptive tests (with test_expect_failure), and I found it difficult to discuss descriptive tests (with test_expect_success) without also having the code in the same diff. FWIW, this version (and the previous one) "git rebase -x 'make test'" cleanly :)

Note for Junio: this version is based on ab/submodule-cleanup (and so will future versions).

= Description

This series is a refactor of "git submodule--helper update" that replaces "--recursive-prefix" with "--super-prefix" (see Background). This was initially motivated by:

* Junio's suggestion to simplify the code [1] (in response to a memory leak found by Johannes Schindelin [2]).
* A desire to use the module_list API + get_submodule_displaypath() outside of builtin/submodule--helper.c (I expect to use this to check out branches in each submodule).

But it also happens to remove some overly complicated/duplicated code that was literally converted from shell :)

= Background

When recursing into nested submodules, Git commands keep track of the path from the superproject to the submodule in order to print a "display path" to the user, e.g.

Submodule path '../super/sub/nested-sub': checked out 'abcdef'

For historical reasons, "git submodule--helper update" uses "--recursive-prefix" for this purpose, but it should use "--super-prefix" instead because:

* That's what every other command uses (not just submodule--helper subcommands).
* Using the "--super-prefix" helper functions makes the "git submodule--helper update" code simpler

= Patch organization

* Patch 1/6 makes sure that display paths are only computed using display path helper functions ([do_]get_submodule_displaypath()) and fixes some display paths that we never realized were broken.
* Patches 2-3/6 simplify and deduplicate some display path computations.
* Patch 4/6 (authored by Ævar) removes SUPPORT_SUPER_PREFIX where it's not needed.
* This doesn't affect correctness, but we want to do this eventually, so do it now to make 5/6 a bit cleaner.
* Patch 5/6 replaces "--recursive-prefix" with "--super-prefix", making do_get_submodule_displaypath() obsolete.
* GGG outputs an odd diff; I recommend viewing it with "--histogram"
* Patch 6/6 removes do_get_submodule_displaypath().

= Series history

Changes in v2:

* Rebase onto ab/submodule-cleanup (previously master)
* Cherry pick https://github.com/avar/git/commit/f445c57490d into 4/6
* Style fixes in .c and tests

= Future work

At the end of this series, get_submodule_displaypath() can be moved to submodule.h, which would make submodule.c:get_super_prefix_or_empty() obsolete. I have a patch that demonstrates this (CI run: [4]), but I decided to keep this series more focused on "git submodule--helper update" so that it's easier to review.

[1] https://lore.kernel.org/git/[email protected] [2]
https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com
[3]
https://lore.kernel.org/git/[email protected]
[4] https://github.com/chooglen/git/actions/runs/2572557584

Glen Choo (6):
submodule--helper: eliminate internal "--update" option submodule--helper update: use display path helper
submodule--helper: don't recreate recursive prefix
submodule--helper: use correct display path helper submodule--helper update: use --super-prefix
submodule--helper: remove display path helper

Ævar Arnfjörð Bjarmason (12):
git-submodule.sh: remove unused sanitize_submodule_env() git-submodule.sh: remove unused $prefix variable git-submodule.sh: make the "$cached" variable a boolean git-submodule.sh: remove unused top-level "--branch" argument
submodule--helper: have --require-init imply --init submodule update: remove "-v" option
submodule--helper: rename "absorb-git-dirs" to "absorbgitdirs"
submodule--helper: report "submodule" as our name in some "-h" output
submodule--helper: understand --checkout, --merge and --rebase
synonyms
git-submodule.sh: use "$quiet", not "$GIT_QUIET" git-sh-setup.sh: remove "say" function, change last users
submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags

 builtin/submodule--helper.c    | 161 +++++++++++++++------------------
 contrib/subtree/git-subtree.sh |  15 ++-
 git-instaweb.sh                |   2 +-
 git-sh-setup.sh                |  16 ----
 git-submodule.sh               |  88 +++++++-----------
 submodule.c                    |   2 +-
 t/t7406-submodule-update.sh    |  58 +++++++++++-
 7 files changed, 175 insertions(+), 167 deletions(-)

base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1282%2Fchooglen%2Fsubmodule%2Fremove-recursive-prefix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1282/chooglen/submodule/remove-recursive-prefix-v2
Pull-Request: https://github.com/git/git/pull/1282

Range-diff vs v1:

  -:  ----------- >  1:  85775255f18 git-submodule.sh: remove unused sanitize_submodule_env()
  -:  ----------- >  2:  960fad98e8a git-submodule.sh: remove unused $prefix variable
  -:  ----------- >  3:  757d0927976 git-submodule.sh: make the "$cached" variable a boolean
  -:  ----------- >  4:  da3aae9e847 git-submodule.sh: remove unused top-level "--branch" argument
  -:  ----------- >  5:  d9c7f69aaa6 submodule--helper: have --require-init imply --init
  -:  ----------- >  6:  0d68ee723e5 submodule update: remove "-v" option
  -:  ----------- >  7:  6e556c412e9 submodule--helper: rename "absorb-git-dirs" to "absorbgitdirs"
  -:  ----------- >  8:  36d45163b6d submodule--helper: report "submodule" as our name in some "-h" output
  -:  ----------- >  9:  8f12108c295 submodule--helper: understand --checkout, --merge and --rebase synonyms
  -:  ----------- > 10:  b788fc671bf submodule--helper: eliminate internal "--update" option
  -:  ----------- > 11:  2eec4637397 git-submodule.sh: use "$quiet", not "$GIT_QUIET"
  -:  ----------- > 12:  5b893f7d81e git-sh-setup.sh: remove "say" function, change last users
  1:  473548f2fa4 ! 13:  64c138df196 submodule--helper update: use display path helper
     @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st
      -	struct strbuf displaypath_sb = STRBUF_INIT;
       	struct strbuf sb = STRBUF_INIT;
      -	const char *displaypath = NULL;
     -+	char *displaypath = NULL;
     ++	char *displaypath;
       	int needs_cloning = 0;
       	int need_free_url = 0;
       
     @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st
      -		else
      -			strbuf_addstr(&sb, ce->name);
      -		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
     --		strbuf_addch(out, '\n');
     -+		strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);
     ++		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
     + 		strbuf_addch(out, '\n');
       		goto cleanup;
       	}
       
     @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets
      +	test_when_finished "rm -fr top-cloned" &&
      +	cp -r top-clean top-cloned &&
      +
     -+	# Create an upstream commit in each repo
     ++	# Create an upstream commit in each repo, starting with bottom
      +	test_commit -C bottom upstream_commit &&
     -+	(cd middle &&
     -+	 git -C bottom fetch &&
     -+	 git -C bottom checkout -f FETCH_HEAD &&
     -+	 git add bottom &&
     -+	 git commit -m "upstream_commit"
     -+	) &&
     -+	(cd top &&
     -+	 git -C middle fetch &&
     -+	 git -C middle checkout -f FETCH_HEAD &&
     -+	 git add middle &&
     -+	 git commit -m "upstream_commit"
     -+	) &&
     ++	# Create middle commit
     ++	git -C middle/bottom fetch &&
     ++	git -C middle/bottom checkout -f FETCH_HEAD &&
     ++	git -C middle add bottom &&
     ++	git -C middle commit -m "upstream_commit" &&
     ++	# Create top commit
     ++	git -C top/middle fetch &&
     ++	git -C top/middle checkout -f FETCH_HEAD &&
     ++	git -C top add middle &&
     ++	git -C top commit -m "upstream_commit" &&
      +
      +	# Create a downstream conflict
     -+	(cd top-cloned/middle &&
     -+	 test_commit -C bottom downstream_commit &&
     -+	 git add bottom &&
     -+	 git commit -m "downstream_commit" &&
     -+	 git fetch --recurse-submodules origin &&
     -+	 test_must_fail git merge origin/main
     -+	) &&
     ++	test_commit -C top-cloned/middle/bottom downstream_commit &&
     ++	git -C top-cloned/middle add bottom &&
     ++	git -C top-cloned/middle commit -m "downstream_commit" &&
     ++	git -C top-cloned/middle fetch --recurse-submodules origin &&
     ++	test_must_fail git -C top-cloned/middle merge origin/main &&
     ++
      +	# Make the update of "middle" a no-op, otherwise we error out
      +	# because of its unmerged state
      +	test_config -C top-cloned submodule.middle.update !true &&
      +	git -C top-cloned submodule update --recursive 2>actual.err &&
     -+	grep "Skipping unmerged submodule .middle/bottom." actual.err
     ++	grep -F "Skipping unmerged submodule middle/bottom" actual.err
      +'
      +
      +test_expect_success 'submodule update --recursive skip submodules with strategy=none' '
     @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets
      +	test_commit -C top-cloned/middle/bottom downstream_commit &&
      +	git -C top-cloned/middle config submodule.bottom.update none &&
      +	git -C top-cloned submodule update --recursive 2>actual.err &&
     -+	grep "Skipping submodule .middle/bottom." actual.err
     ++	grep -F "Skipping submodule ${SQ}middle/bottom${SQ}" actual.err
      +'
      +
       test_done

2: 618053730e1 ! 14: d3e803a4630 submodule--helper: don't recreate recursive prefix
@@ Commit message

## builtin/submodule--helper.c ##
@@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *
- {
+
strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
- if (update_data->recursive_prefix)

  3:  7cd1c46f350 = 15:  1f7cf6ffaf1 submodule--helper: use correct display path helper
  -:  ----------- > 16:  85e65f143b6 submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags
  4:  57988287fc0 ! 17:  1d8b6b244dc submodule--helper update: use --super-prefix

@@ builtin/submodule--helper.c: struct submodule_update_clone {
const char *prefix;
- const char *recursive_prefix;
const char *displaypath;
- const char *update_default;
+ enum submodule_update_type update_default;
struct object_id suboid;
@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,

@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st
+ get_super_prefix());

if (ce_stage(ce)) {
- strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);
-@@ builtin/submodule--helper.c: static void ensure_core_worktree(const char *path)
-
- static void update_data_to_args(struct update_data *update_data, struct strvec *args)
+ strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
+@@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *
{
+ enum submodule_update_type update_type = update_data->update_default;
+
- strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
- strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
if (update_data->displaypath)
@@ builtin/submodule--helper.c: static int module_update(int argc, const char **arg
- N_("path"),
- N_("path into the working tree, across nested "
- "submodule boundaries")),
- OPT_STRING(0, "update", &opt.update_default,
- N_("string"),
- N_("rebase, merge, checkout or none")),
+ OPT_SET_INT(0, "checkout", &opt.update_default,
+ N_("use the 'checkout' update strategy (default)"),
+ SM_UPDATE_CHECKOUT),
@@ builtin/submodule--helper.c: static int module_update(int argc, const char **argv, const char *prefix)
module_list_active(&list);

@@ builtin/submodule--helper.c: struct cmd_struct {
{"name", module_name, 0},
- {"clone", module_clone, 0},
+ {"clone", module_clone, SUPPORT_SUPER_PREFIX},
- {"add", module_add, SUPPORT_SUPER_PREFIX},
+ {"add", module_add, 0},
- {"update", module_update, 0},
+ {"update", module_update, SUPPORT_SUPER_PREFIX},
{"resolve-relative-url-test", resolve_relative_url_test, 0}, {"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
- {"init", module_init, SUPPORT_SUPER_PREFIX},
+ {"init", module_init, 0},
5: 9fa13380b02 ! 18: a21e8cd174d submodule--helper: remove display path helper
@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st
+ get_submodule_displaypath(ce->name, suc->update_data->prefix);

if (ce_stage(ce)) {
- strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);
+ strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)
{
ensure_core_worktree(update_data->sm_path);

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

The sanitize_submodule_env() function was last used before b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15), let's remove it.

This also allows us to remove clear_local_git_env() from git-sh-setup.sh. That function hasn't been documented in Documentation/git-sh-setup.sh, and since 14111fc4927 (git: submodule honor -c credential.* from command line, 2016-02-29) it had only been used in the sanitize_submodule_env() function being removed here.

---
 git-sh-setup.sh  |  7 -------
 git-submodule.sh | 11 -----------
 2 files changed, 18 deletions(-)

index d92df37e992..ecb60d9e3cb 100644
@@ -285,13 +285,6 @@ get_author_ident_from_commit () {
 	parse_ident_from_commit author AUTHOR
 }
 
-# Clear repo-local GIT_* environment variables. Useful when switching to
-# another repository (e.g. when entering a submodule). See also the env
-# list in git_connect()
-clear_local_git_env() {
-	unset $(git rev-parse --local-env-vars)
-}
-
 # Generate a virtual base file for a two-file merge. Uses git apply to
 # remove lines from $1 that are not in $2, leaving only common lines.
 create_virtual_base() {

index fd0b4a2c947..bc436c4ca47 100755
@@ -56,17 +56,6 @@ isnumber()
 	n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
 }
 
-# Sanitize the local git environment for use within a submodule. We
-# can't simply use clear_local_git_env since we want to preserve some
-# of the settings from GIT_CONFIG_PARAMETERS.
-sanitize_submodule_env()
-{
-	save_config=$GIT_CONFIG_PARAMETERS
-	clear_local_git_env
-	GIT_CONFIG_PARAMETERS=$save_config
-	export GIT_CONFIG_PARAMETERS
-}
-
 #
 # Add a new submodule to the working tree, .gitmodules and the index
 #

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Remove the $prefix variable which isn't used anymore, and hasn't been since b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15).

Before that we'd use it to invoke "git submodule--helper" with the "--recursive-prefix" option, but since b3c5f5cb048 that "git submodule--helper" option is only used when it invokes itself.

So the "--recursive-prefix" option is still in use, but at this point only when the helper invokes itself during submodule recursion. See the "--recursive-prefix" option added in
c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24).

---
 git-submodule.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index bc436c4ca47..53847bbf6e2 100755
@@ -41,7 +41,6 @@ files=
 remote=
 nofetch=
 update=
-prefix=
 custom_name=
 depth=
 progress=

@@ -127,7 +126,7 @@ cmd_add()
 		usage
 	fi
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
 }
 
 #

@@ -189,7 +188,7 @@ cmd_init()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
 }
 
 #

@@ -346,7 +345,6 @@ cmd_update()
 		${init:+--init} \
 		${nofetch:+--no-fetch} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
-		${prefix:+--recursive-prefix "$prefix"} \
 		${update:+--update "$update"} \
 		${reference:+"$reference"} \
 		${dissociate:+"--dissociate"} \

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Remove the assignment of "$1" to the "$cached" variable. As seen in the initial implementation in 70c7ac22de6 (Add git-submodule command, 2007-05-26) we only need to keep track of if we've seen the --cached option, not save the "--cached" string for later use.

In 28f9af5d25e (git-submodule summary: code framework, 2008-03-11) "$1" was assigned to it, but since there was no reason to do so let's stop doing it. This trivial change will make it easier to reason about an eventual change that'll remove the cmd_summary() function in favor of dispatching to "git submodule--helper summary" directly.

---
 git-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 53847bbf6e2..b99a00d9f84 100755
@@ -446,7 +446,7 @@ cmd_summary() {
 	do
 		case "$1" in
 		--cached)
-			cached="$1"
+			cached=1
 			;;
 		--files)
 			files="$1"

@@ -583,7 +583,7 @@ do
 		branch="$2"; shift
 		;;
 	--cached)
-		cached="$1"
+		cached=1
 		;;
 	--)
 		break

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

In 5c08dbbdf1a (git-submodule: fix subcommand parser, 2008-01-15) the "--branch" option was supported as an option to "git submodule" itself, i.e. "git submodule --branch" as a side-effect of its implementation.

Then in b57e8119e6e (submodule: teach set-branch subcommand, 2019-02-08) when the "set-branch" subcommand was added the assertion that we shouldn't have "--branch" anywhere except as an argument to "add" and "set-branch" was copy/pasted from the adjacent check for "--cache" added (or rather modified) in 496eeeb19b9 (git-submodule.sh:
avoid "test <cond> -a/-o <cond>", 2014-06-10).

But there's been a logic error in that check, which at a glance looked like it should be supporting:

    git submodule --branch <branch> (add | set-branch) [<options>]

But due to "||" in the condition (as opposed to "&&" for "--cache") if we have "--branch" here already we'll emit usage, even for "add" and "set-branch".

So in addition to never having documented this form, it hasn't worked since b57e8119e6e was released with v2.22.0.

So it's safe to remove this code. I.e. we don't want to support the form noted above, but only:

    git submodule (add | set-branch) --branch <branch> [<options>]

---
 git-submodule.sh | 14 --------------
 1 file changed, 14 deletions(-)

index b99a00d9f84..20fc1b620fa 100755
@@ -574,14 +574,6 @@ do
 	-q|--quiet)
 		GIT_QUIET=1
 		;;
-	-b|--branch)
-		case "$2" in
-		'')
-			usage
-			;;
-		esac
-		branch="$2"; shift
-		;;
 	--cached)
 		cached=1
 		;;

@@ -609,12 +601,6 @@ then
     fi
 fi
 
-# "-b branch" is accepted only by "add" and "set-branch"
-if test -n "$branch" && (test "$command" != add || test "$command" != set-branch)
-then
-	usage
-fi
-
 # "--cached" is accepted only by "status" and "summary"
 if test -n "$cached" && test "$command" != status && test "$command" != summary
 then

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Adjust code added in 0060fd1511b (clone --recurse-submodules: prevent name squatting on Windows, 2019-09-12) to have the internal --require-init option imply --init, rather than having "git-submodule.sh" add it implicitly.

This change doesn't make any difference now, but eliminates another special-case where "git submodule--helper update"'s behavior was different from "git submodule update". This will make it easier to eventually replace the cmd_update() function in git-submodule.sh.

We'll still need to keep the distinction between "--init" and "--require-init" in git-submodule.sh. Once cmd_update() gets re-implemented in C we'll be able to change variables and other code related to that, but not yet.

---
 builtin/submodule--helper.c | 5 ++++-
 git-submodule.sh            | 1 -
 2 files changed, 4 insertions(+), 2 deletions(-)

index 5c77dfcffee..f0702d0cfa2 100644
@@ -2618,7 +2618,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "progress", &opt.progress,
 			    N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &opt.require_init,
-			   N_("disallow cloning into non-empty directory")),
+			   N_("disallow cloning into non-empty directory, implies --init")),
 		OPT_BOOL(0, "single-branch", &opt.single_branch,
 			 N_("clone only one branch, HEAD or --branch")),
 		OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),

@@ -2642,6 +2642,9 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_update_options,
 			     git_submodule_helper_usage, 0);
 
+	if (opt.require_init)
+		opt.init = 1;
+
 	if (filter_options.choice && !opt.init) {
 		usage_with_options(git_submodule_helper_usage,
 				   module_update_options);

index 20fc1b620fa..5b9683bf766 100755
@@ -251,7 +251,6 @@ cmd_update()
 			init=1
 			;;
 		--require-init)
-			init=1
 			require_init=1
 			;;
 		--remote)

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

In e84c3cf3dc3 (git-submodule.sh: accept verbose flag in cmd_update to be non-quiet, 2018-08-14) the "git submodule update" sub-command was made to understand "-v", but the option was never documented.

The only in-tree user has been this test added in 3ad0401e9e6 (submodule update: silence underlying merge/rebase with "--quiet", 2020-09-30), it wasn't per-se testing --quiet, but fixing a bug in e84c3cf3dc3: It used to set "GIT_QUIET=0" instead of unsetting it on "-v", and thus we'd end up passing "--quiet" to "git submodule--helper" on "-v", since the "--quiet" option was passed using the ${parameter:+word} construct.

Furthermore, even if someone had used the "-v" option they'd only be getting the default output. Our default in both git-submodule.sh and "git submodule--helper" has been to be "verbose", so the only way this option could have matter is if it were used as e.g.:

    git submodule --quiet update -v [...]

I.e. to undo the effect of a previous "--quiet" on the command-line.

---
 git-submodule.sh            | 3 ---
 t/t7406-submodule-update.sh | 2 +-
 2 files changed, 1 insertion(+), 4 deletions(-)

index 5b9683bf766..0df6b0fc974 100755
@@ -241,9 +241,6 @@ cmd_update()
 		-q|--quiet)
 			GIT_QUIET=1
 			;;
-		-v)
-			unset GIT_QUIET
-			;;
 		--progress)
 			progress=1
 			;;

index 43f779d751c..06d804e2131 100755
@@ -1074,7 +1074,7 @@ test_expect_success 'submodule update --quiet passes quietness to merge/rebase'
 	 git submodule update --rebase --quiet >out 2>err &&
 	 test_must_be_empty out &&
 	 test_must_be_empty err &&
-	 git submodule update --rebase -v >out 2>err &&
+	 git submodule update --rebase >out 2>err &&
 	 test_file_not_empty out &&
 	 test_must_be_empty err
 	)

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Rename the "absorb-git-dirs" subcommand to "absorbgitdirs", which is what the "git submodule" command itself has called it since the subcommand was implemented in f6f85861400 (submodule: add absorb-git-dir function, 2016-12-12).

Having these two be different will make it more tedious to dispatch to eventually dispatch "git submodule--helper" directly, as we'd need to retain this name mapping. So let's get rid of this needless inconsistency.

---
 builtin/submodule--helper.c | 4 ++--
 git-submodule.sh            | 2 +-
 submodule.c                 | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

index f0702d0cfa2..1a84ae8efd2 100644
@@ -2787,7 +2787,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper absorb-git-dirs [<options>] [<path>...]"),
+		N_("git submodule--helper absorbgitdirs [<options>] [<path>...]"),
 		NULL
 	};
 

@@ -3389,7 +3389,7 @@ static struct cmd_struct commands[] = {
 	{"deinit", module_deinit, 0},
 	{"summary", module_summary, SUPPORT_SUPER_PREFIX},
 	{"push-check", push_check, 0},
-	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
+	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 	{"is-active", is_active, 0},
 	{"check-name", check_name, 0},
 	{"config", module_config, 0},

index 0df6b0fc974..1c1dc320922 100755
@@ -552,7 +552,7 @@ cmd_sync()
 
 cmd_absorbgitdirs()
 {
-	git submodule--helper absorb-git-dirs --prefix "$wt_prefix" "$@"
+	git submodule--helper absorbgitdirs --prefix "$wt_prefix" "$@"
 }
 
 # This loop parses the command line arguments to find the

index 4e299f578f9..2af16c647d5 100644
@@ -2374,7 +2374,7 @@ void absorb_git_dir_into_superproject(const char *path,
 		cp.no_stdin = 1;
 		strvec_pushl(&cp.args, "--super-prefix", sb.buf,
 			     "submodule--helper",
-			     "absorb-git-dirs", NULL);
+			     "absorbgitdirs", NULL);
 		prepare_submodule_repo_env(&cp.env);
 		if (run_command(&cp))
 			die(_("could not recurse into submodule '%s'"), path);

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Change the user-facing "git submodule--helper" commands so that they'll report their name as being "git submodule". To a user these commands are internal implementation details, and it doesn't make sense to emit usage about an internal helper when "git submodule" is invoked with invalid options.

Before this we'd emit e.g.:

	$ git submodule absorbgitdirs --blah
	error: unknown option `blah'
	usage: git submodule--helper absorbgitdirs [<options>] [<path>...]
	[...]
And:

	$ git submodule set-url -- --
	usage: git submodule--helper set-url [--quiet] <path> <newurl>
	[...]

Now we'll start with "usage: git submodule [...]" in both of those cases. This change does not alter the "list", "name", "clone", "config" and "create-branch" commands, those are internal-only (as an aside; their usage info should probably invoke BUG(...)). This only changes the user-facing commands.

The "status", "deinit" and "update" commands are not included in this change, because their usage information already used "submodule" rather than "submodule--helper".

I don't think it's currently possible to emit some of this usage information in practice, as git-submodule.sh will catch unknown options, and e.g. it doesn't seem to be possible to get "add" to emit its usage information from "submodule--helper".

Though that change may be superfluous now, it's also harmless, and will allow us to eventually dispatch further into "git submodule--helper" from git-submodule.sh, while emitting the correct usage output.

---
 builtin/submodule--helper.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

index 1a84ae8efd2..04d2620fce8 100644
@@ -444,7 +444,7 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper foreach [--quiet] [--recursive] [--] <command>"),
+		N_("git submodule foreach [--quiet] [--recursive] [--] <command>"),
 		NULL
 	};
 

@@ -582,7 +582,7 @@ static int module_init(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper init [<options>] [<path>]"),
+		N_("git submodule init [<options>] [<path>]"),
 		NULL
 	};
 

@@ -1185,7 +1185,7 @@ static int module_summary(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper summary [<options>] [<commit>] [--] [<path>]"),
+		N_("git submodule summary [<options>] [<commit>] [--] [<path>]"),
 		NULL
 	};
 

@@ -1349,7 +1349,7 @@ static int module_sync(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper sync [--quiet] [--recursive] [<path>]"),
+		N_("git submodule sync [--quiet] [--recursive] [<path>]"),
 		NULL
 	};
 

@@ -2787,7 +2787,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper absorbgitdirs [<options>] [<path>...]"),
+		N_("git submodule absorbgitdirs [<options>] [<path>...]"),
 		NULL
 	};
 

@@ -2892,7 +2892,7 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 	const char *const usage[] = {
-		N_("git submodule--helper set-url [--quiet] <path> <newurl>"),
+		N_("git submodule set-url [--quiet] <path> <newurl>"),
 		NULL
 	};
 

@@ -2931,8 +2931,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 	const char *const usage[] = {
-		N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"),
-		N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
+		N_("git submodule set-branch [-q|--quiet] (-d|--default) <path>"),
+		N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
 		NULL
 	};
 

@@ -3276,7 +3276,7 @@ static int module_add(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const usage[] = {
-		N_("git submodule--helper add [<options>] [--] <repository> [<path>]"),
+		N_("git submodule add [<options>] [--] <repository> [<path>]"),
 		NULL
 	};
 

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Understand --checkout, --merge and --rebase synonyms for --update={checkout,merge,rebase}, as well as the short options that 'git submodule' itself understands.

This removes a difference between the CLI API of "git submodule" and "git submodule--helper", making it easier to make the latter an alias for the former. See 48308681b07 (git submodule update: have a dedicated helper for cloning, 2016-02-29) for the initial addition of --update.

---
 builtin/submodule--helper.c | 30 ++++++++++++++++++++++++++++++
 git-submodule.sh            | 14 +++++++++-----
 2 files changed, 39 insertions(+), 5 deletions(-)

index 04d2620fce8..53179472d85 100644
@@ -2404,6 +2404,23 @@ static void ensure_core_worktree(const char *path)
 	}
 }
 
+static const char *submodule_update_type_to_label(enum submodule_update_type type)
+{
+	switch (type) {
+	case SM_UPDATE_CHECKOUT:
+		return "checkout";
+	case SM_UPDATE_MERGE:
+		return "merge";
+	case SM_UPDATE_REBASE:
+		return "rebase";
+	case SM_UPDATE_UNSPECIFIED:
+	case SM_UPDATE_NONE:
+	case SM_UPDATE_COMMAND:
+		break;
+	}
+	BUG("unreachable with type %d", type);
+}
+
 static void update_data_to_args(struct update_data *update_data, struct strvec *args)
 {
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);

@@ -2582,6 +2599,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	struct update_data opt = UPDATE_DATA_INIT;
 	struct list_objects_filter_options filter_options;
 	int ret;
+	enum submodule_update_type update_type = SM_UPDATE_UNSPECIFIED;
 
 	struct option module_update_options[] = {
 		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),

@@ -2603,6 +2621,15 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "update", &opt.update_default,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
+		OPT_SET_INT(0, "checkout", &update_type,
+			N_("use the 'checkout' update strategy (default)"),
+			SM_UPDATE_CHECKOUT),
+		OPT_SET_INT('m', "merge", &update_type,
+			N_("use the 'merge' update strategy"),
+			SM_UPDATE_MERGE),
+		OPT_SET_INT('r', "rebase", &update_type,
+			N_("use the 'rebase' update strategy"),
+			SM_UPDATE_REBASE),
 		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
 			   N_("reference repository")),
 		OPT_BOOL(0, "dissociate", &opt.dissociate,

@@ -2652,6 +2679,9 @@ static int module_update(int argc, const char **argv, const char *prefix)
 
 	opt.filter_options = &filter_options;
 
+	if (update_type != SM_UPDATE_UNSPECIFIED)
+		opt.update_default = submodule_update_type_to_label(update_type);
+
 	if (opt.update_default)
 		if (parse_submodule_update_strategy(opt.update_default,
 						    &opt.update_strategy) < 0)

index 1c1dc320922..7fc7119fb21 100755
@@ -40,7 +40,9 @@ require_init=
 files=
 remote=
 nofetch=
-update=
+rebase=
+merge=
+checkout=
 custom_name=
 depth=
 progress=

@@ -260,7 +262,7 @@ cmd_update()
 			force=$1
 			;;
 		-r|--rebase)
-			update="rebase"
+			rebase=1
 			;;
 		--reference)
 			case "$2" in '') usage ;; esac

@@ -274,13 +276,13 @@ cmd_update()
 			dissociate=1
 			;;
 		-m|--merge)
-			update="merge"
+			merge=1
 			;;
 		--recursive)
 			recursive=1
 			;;
 		--checkout)
-			update="checkout"
+			checkout=1
 			;;
 		--recommend-shallow)
 			recommend_shallow="--recommend-shallow"

@@ -341,7 +343,9 @@ cmd_update()
 		${init:+--init} \
 		${nofetch:+--no-fetch} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
-		${update:+--update "$update"} \
+		${rebase:+--rebase} \
+		${merge:+--merge} \
+		${checkout:+--checkout} \
 		${reference:+"$reference"} \
 		${dissociate:+"--dissociate"} \
 		${depth:+"$depth"} \

gitgitgadget

From: Glen Choo <[email protected]>

Follow-up on the preceding commit which taught "git submodule--helper update" to understand "--merge", "--checkout" and "--rebase" and use those options instead of "--update=(rebase|merge|checkout|none)" when the command invokes itself.

Unlike the preceding change this isn't strictly necessary to eventually change "git-submodule.sh" so that it invokes "git submodule--helper update" directly, but let's remove this inconsistency in the command-line interface. We shouldn't need to carry special synonyms for existing options in "git submodule--helper" when that command can use the primary documented names instead.

But, as seen in the post-image this makes the control flow within "builtin/submodule--helper.c" simpler, we can now write directly to the "update_default" member of "struct update_data" when parsing the options in "module_update()".

---
 builtin/submodule--helper.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

index 53179472d85..389b900602f 100644
@@ -1818,7 +1818,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 static void determine_submodule_update_strategy(struct repository *r,
 						int just_cloned,
 						const char *path,
-						const char *update,
+						enum submodule_update_type update,
 						struct submodule_update_strategy *out)
 {
 	const struct submodule *sub = submodule_from_path(r, null_oid(), path);

@@ -1828,9 +1828,7 @@ static void determine_submodule_update_strategy(struct repository *r,
 	key = xstrfmt("submodule.%s.update", sub->name);
 
 	if (update) {
-		if (parse_submodule_update_strategy(update, out) < 0)
-			die(_("Invalid update mode '%s' for submodule path '%s'"),
-				update, path);
+		out->type = update;
 	} else if (!repo_config_get_string_tmp(r, key, &val)) {
 		if (parse_submodule_update_strategy(val, out) < 0)
 			die(_("Invalid update mode '%s' configured for submodule path '%s'"),

@@ -1882,7 +1880,7 @@ struct update_data {
 	const char *prefix;
 	const char *recursive_prefix;
 	const char *displaypath;
-	const char *update_default;
+	enum submodule_update_type update_default;
 	struct object_id suboid;
 	struct string_list references;
 	struct submodule_update_strategy update_strategy;

@@ -2423,6 +2421,8 @@ static const char *submodule_update_type_to_label(enum submodule_update_type typ
 
 static void update_data_to_args(struct update_data *update_data, struct strvec *args)
 {
+	enum submodule_update_type update_type = update_data->update_default;
+
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->recursive_prefix)

@@ -2446,8 +2446,10 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 		strvec_push(args, "--require-init");
 	if (update_data->depth)
 		strvec_pushf(args, "--depth=%d", update_data->depth);
-	if (update_data->update_default)
-		strvec_pushl(args, "--update", update_data->update_default, NULL);
+	if (update_type != SM_UPDATE_UNSPECIFIED)
+		strvec_pushf(args, "--%s",
+			     submodule_update_type_to_label(update_type));
+
 	if (update_data->references.nr) {
 		struct string_list_item *item;
 		for_each_string_list_item(item, &update_data->references)

@@ -2599,7 +2601,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	struct update_data opt = UPDATE_DATA_INIT;
 	struct list_objects_filter_options filter_options;
 	int ret;
-	enum submodule_update_type update_type = SM_UPDATE_UNSPECIFIED;
 
 	struct option module_update_options[] = {
 		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),

@@ -2618,16 +2619,13 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			   N_("path"),
 			   N_("path into the working tree, across nested "
 			      "submodule boundaries")),
-		OPT_STRING(0, "update", &opt.update_default,
-			   N_("string"),
-			   N_("rebase, merge, checkout or none")),
-		OPT_SET_INT(0, "checkout", &update_type,
+		OPT_SET_INT(0, "checkout", &opt.update_default,
 			N_("use the 'checkout' update strategy (default)"),
 			SM_UPDATE_CHECKOUT),
-		OPT_SET_INT('m', "merge", &update_type,
+		OPT_SET_INT('m', "merge", &opt.update_default,
 			N_("use the 'merge' update strategy"),
 			SM_UPDATE_MERGE),
-		OPT_SET_INT('r', "rebase", &update_type,
+		OPT_SET_INT('r', "rebase", &opt.update_default,
 			N_("use the 'rebase' update strategy"),
 			SM_UPDATE_REBASE),
 		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),

@@ -2679,13 +2677,8 @@ static int module_update(int argc, const char **argv, const char *prefix)
 
 	opt.filter_options = &filter_options;
 
-	if (update_type != SM_UPDATE_UNSPECIFIED)
-		opt.update_default = submodule_update_type_to_label(update_type);
-
 	if (opt.update_default)
-		if (parse_submodule_update_strategy(opt.update_default,
-						    &opt.update_strategy) < 0)
-			die(_("bad value for update parameter"));
+		opt.update_strategy.type = opt.update_default;
 
 	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0) {
 		list_objects_filter_release(&filter_options);

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Remove the use of the "$GIT_QUIET" variable in favor of our own "$quiet", ever since b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15) we have not used the "say" function in git-sh-setup.sh, which is the only thing that's affected by using "GIT_QUIET".

We still want to support --quiet for our own use though, but let's use our own variable for that. Now it's obvious that we only care about passing "--quiet" to "git submodule--helper", and not to change the output of any "say" invocation.

---
 git-submodule.sh | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

index 7fc7119fb21..5e5d21c010f 100755
@@ -30,6 +30,7 @@ GIT_PROTOCOL_FROM_USER=0
 export GIT_PROTOCOL_FROM_USER
 
 command=
+quiet=
 branch=
 force=
 reference=

@@ -80,7 +81,7 @@ cmd_add()
 			force=$1
 			;;
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--progress)
 			progress=1

@@ -128,7 +129,7 @@ cmd_add()
 		usage
 	fi
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add ${quiet:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
 }
 
 #

@@ -144,7 +145,7 @@ cmd_foreach()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--recursive)
 			recursive=1

@@ -159,7 +160,7 @@ cmd_foreach()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach ${quiet:+--quiet} ${recursive:+--recursive} -- "$@"
 }
 
 #

@@ -174,7 +175,7 @@ cmd_init()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--)
 			shift

@@ -190,7 +191,7 @@ cmd_init()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${quiet:+--quiet} -- "$@"
 }
 
 #

@@ -207,7 +208,7 @@ cmd_deinit()
 			force=$1
 			;;
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--all)
 			deinit_all=t

@@ -226,7 +227,7 @@ cmd_deinit()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${quiet:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@"
 }
 
 #

@@ -241,7 +242,7 @@ cmd_update()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--progress)
 			progress=1

@@ -335,7 +336,7 @@ cmd_update()
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
-		${GIT_QUIET:+--quiet} \
+		${quiet:+--quiet} \
 		${force:+--force} \
 		${progress:+"--progress"} \
 		${remote:+--remote} \

@@ -396,7 +397,7 @@ cmd_set_branch() {
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch "$branch"} ${default:+--default} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch ${quiet:+--quiet} ${branch:+--branch "$branch"} ${default:+--default} -- "$@"
 }
 
 #

@@ -409,7 +410,7 @@ cmd_set_url() {
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--)
 			shift

@@ -425,7 +426,7 @@ cmd_set_url() {
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url ${quiet:+--quiet} -- "$@"
 }
 
 #

@@ -496,7 +497,7 @@ cmd_status()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--cached)
 			cached=1

@@ -518,7 +519,7 @@ cmd_status()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status ${quiet:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@"
 }
 #
 # Sync remote urls for submodules

@@ -531,7 +532,7 @@ cmd_sync()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			shift
 			;;
 		--recursive)

@@ -551,7 +552,7 @@ cmd_sync()
 		esac
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync ${quiet:+--quiet} ${recursive:+--recursive} -- "$@"
 }
 
 cmd_absorbgitdirs()

@@ -572,7 +573,7 @@ do
 		command=$1
 		;;
 	-q|--quiet)
-		GIT_QUIET=1
+		quiet=1
 		;;
 	--cached)
 		cached=1

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Remove the "say" function, with various rewrites of the remaining git-*.sh code to C and the preceding change to have git-submodule.sh stop using the GIT_QUIET variable there were only four uses in git-subtree.sh. Let's have it use an "arg_quiet" variable instead, and move the "say" function over to it.

The only other use was a trivial message in git-instaweb.sh, since it has never supported the --quiet option (or similar) that code added in 0b624b4ceee (instaweb: restart server if already running, 2009-11-22) can simply use "echo" instead.

The remaining in-tree hits from "say" are all for the sibling function defined in t/test-lib.sh. It's safe to remove this function since it has never been documented in Documentation/git-sh-setup.txt.

---
 contrib/subtree/git-subtree.sh | 15 ++++++++++++---
 git-instaweb.sh                |  2 +-
 git-sh-setup.sh                |  9 ---------
 3 files changed, 13 insertions(+), 13 deletions(-)

index 1af1d9653e9..7562a395c24 100755
@@ -50,6 +50,14 @@ m,message=    use the given message as the commit message for the merge commit
 
 indent=0
 
+# Usage: say [MSG...]
+say () {
+	if test -z "$arg_quiet"
+	then
+		printf '%s\n' "$*"
+	fi
+}
+
 # Usage: debug [MSG...]
 debug () {
 	if test -n "$arg_debug"

@@ -60,7 +68,7 @@ debug () {
 
 # Usage: progress [MSG...]
 progress () {
-	if test -z "$GIT_QUIET"
+	if test -z "$arg_quiet"
 	then
 		if test -z "$arg_debug"
 		then

@@ -146,6 +154,7 @@ main () {
 	eval "$set_args"
 
 	# Begin "real" flag parsing.
+	arg_quiet=
 	arg_debug=
 	arg_prefix=
 	arg_split_branch=

@@ -161,7 +170,7 @@ main () {
 
 		case "$opt" in
 		-q)
-			GIT_QUIET=1
+			arg_quiet=1
 			;;
 		-d)
 			arg_debug=1

@@ -252,7 +261,7 @@ main () {
 	dir="$(dirname "$arg_prefix/.")"
 
 	debug "command: {$arg_command}"
-	debug "quiet: {$GIT_QUIET}"
+	debug "quiet: {$arg_quiet}"
 	debug "dir: {$dir}"
 	debug "opts: {$*}"
 	debug

index 4349566c891..c68f49454cd 100755
@@ -102,7 +102,7 @@ resolve_full_httpd () {
 
 start_httpd () {
 	if test -f "$fqgitdir/pid"; then
-		say "Instance already running. Restarting..."
+		echo "Instance already running. Restarting..."
 		stop_httpd
 	fi
 

index ecb60d9e3cb..ce273fe0e48 100644
@@ -57,15 +57,6 @@ die_with_status () {
 	exit "$status"
 }
 
-GIT_QUIET=
-
-say () {
-	if test -z "$GIT_QUIET"
-	then
-		printf '%s\n' "$*"
-	fi
-}
-
 if test -n "$OPTIONS_SPEC"; then
 	usage() {
 		"$0" -h

gitgitgadget

From: Glen Choo <[email protected]>

There are two locations in prepare_to_clone_next_submodule() that manually calculate the submodule display path, but should just use do_get_submodule_displaypath() for consistency.

Do this replacement and reorder the code slightly to avoid computing the display path twice.

This code was never tested, and adding tests shows that both these sites have been computing the display path incorrectly ever since they were introduced in 48308681b0 (git submodule update: have a dedicated helper for cloning, 2016-02-29) [1]:

- The first hunk puts a "/" between recursive_prefix and ce->name, but recursive_prefix already ends with "/".
- The second hunk calls relative_path() on recursive_prefix and ce->name, but relative_path() only makes sense when both paths share the same base directory. This is never the case here:
- recursive_prefix is the path from the topmost superproject to the current submodule
- ce->name is the path from the root of the current submodule to its submodule.
so, e.g. recursive_prefix="super" and ce->name="submodule" produces displayname="../super" instead of "super/submodule".

While we're fixing the display names, also fix inconsistent quoting of the submodule name.

[1] I verified this by applying the tests to 48308681b0.

---
 builtin/submodule--helper.c | 21 +++++---------
 t/t7406-submodule-update.sh | 56 +++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 14 deletions(-)

index 389b900602f..db2d5ab7998 100644
@@ -1947,30 +1947,23 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	const char *update_string;
 	enum submodule_update_type update_type;
 	char *key;
-	struct strbuf displaypath_sb = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
-	const char *displaypath = NULL;
+	char *displaypath;
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
+	displaypath = do_get_submodule_displaypath(ce->name,
+						   suc->update_data->prefix,
+						   suc->update_data->recursive_prefix);
+
 	if (ce_stage(ce)) {
-		if (suc->update_data->recursive_prefix)
-			strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name);
-		else
-			strbuf_addstr(&sb, ce->name);
-		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
+		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
 		strbuf_addch(out, '\n');
 		goto cleanup;
 	}
 
 	sub = submodule_from_path(the_repository, null_oid(), ce->name);
 
-	if (suc->update_data->recursive_prefix)
-		displaypath = relative_path(suc->update_data->recursive_prefix,
-					    ce->name, &displaypath_sb);
-	else
-		displaypath = ce->name;
-
 	if (!sub) {
 		next_submodule_warn_missing(suc, out, displaypath);
 		goto cleanup;

@@ -2060,7 +2053,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 					      "--no-single-branch");
 
 cleanup:
-	strbuf_release(&displaypath_sb);
+	free(displaypath);
 	strbuf_release(&sb);
 	if (need_free_url)
 		free((void*)url);

gitgitgadget

From: Glen Choo <[email protected]>

update_submodule() uses duplicated code to compute update_data->displaypath and next.recursive_prefix. The latter is just the former with "/" appended to it, and since update_data->displaypath not changed outside of this statement, we can just reuse the already computed result.

We can go one step further and remove the reference to next.recursive_prefix altogether. Since it is only used in update_data_to_args() (to compute the "--recursive-prefix" flag for the recursive update child process) we can just use the already computed .displaypath value of there.

Delete the duplicated code, and remove the unnecessary reference to next.recursive_prefix. As a bonus, this fixes a memory leak where prefixed_path was never freed (this leak was first reported in [1]).

[1] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com

---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

index db2d5ab7998..c104b1a0236 100644
@@ -2418,9 +2418,9 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
-	if (update_data->recursive_prefix)
-		strvec_pushl(args, "--recursive-prefix",
-			     update_data->recursive_prefix, NULL);
+	if (update_data->displaypath)
+		strvec_pushf(args, "--recursive-prefix=%s/",
+			     update_data->displaypath);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)

@@ -2516,14 +2516,6 @@ static int update_submodule(struct update_data *update_data)
 		struct update_data next = *update_data;
 		int res;
 
-		if (update_data->recursive_prefix)
-			prefixed_path = xstrfmt("%s%s/", update_data->recursive_prefix,
-						update_data->sm_path);
-		else
-			prefixed_path = xstrfmt("%s/", update_data->sm_path);
-
-		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
-								  update_data->prefix);
 		next.prefix = NULL;
 		oidcpy(&next.oid, null_oid());
 		oidcpy(&next.suboid, null_oid());

gitgitgadget

From: Glen Choo <[email protected]>

Replace a chunk of code in update_submodule() with an equivalent do_get_submodule_displaypath() invocation. This is already tested by t/t7406-submodule-update.sh:'submodule update --init --recursive from subdirectory', so no tests are added.

The two are equivalent because:

- Exactly one of recursive_prefix|prefix is non-NULL at a time; prefix is set at the superproject level, and recursive_prefix is set when recursing into submodules. There is also a BUG() statement in get_submodule_displaypath() that asserts that both cannot be non-NULL.

- In get_submodule_displaypath(), get_super_prefix() always returns NULL because "--super-prefix" is never passed. Thus calling it is equivalent to calling do_get_submodule_displaypath() with super_prefix = NULL.

Therefore:

- When recursive_prefix is non-NULL, prefix is NULL, and thus get_submodule_displaypath() just returns prefixed_path. This is identical to calling do_get_submodule_displaypath() with super_prefix = recursive_prefix because the return value is still the concatenation of recursive_prefix + update_data->sm_path.

- When prefix is non-NULL, prefixed_path = update_data->sm_path. Thus calling get_submodule_displaypath() with prefixed_path is equivalent to calling do_get_submodule_displaypath() with update_data->sm_path

---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

index c104b1a0236..aad431f898e 100644
@@ -2464,19 +2464,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 
 static int update_submodule(struct update_data *update_data)
 {
-	char *prefixed_path;
-
 	ensure_core_worktree(update_data->sm_path);
 
-	if (update_data->recursive_prefix)
-		prefixed_path = xstrfmt("%s%s", update_data->recursive_prefix,
-					update_data->sm_path);
-	else
-		prefixed_path = xstrdup(update_data->sm_path);
-
-	update_data->displaypath = get_submodule_displaypath(prefixed_path,
-							     update_data->prefix);
-	free(prefixed_path);
+	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
+								update_data->prefix,
+								update_data->recursive_prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Remove the SUPPORT_SUPER_PREFIX flag from "add", "init" and "summary". For the "add" command it hasn't been used since [1], likewise for "init" and "summary" since [2] and [3], respectively.

As implemented in 74866d75793 (git: make super-prefix option, 2016-10-07) the SUPPORT_SUPER_PREFIX flag in git.c applies for the entire command, but as implemented in 89c86265576 (submodule helper:
support super prefix, 2016-12-08) we assert here in cmd_submodule__helper() that we're not getting the flag unexpectedly.

1. 8c8195e9c3e (submodule--helper: introduce add-clone subcommand, 2021-07-10)
2. 6e7c14e65c8 (submodule update --init: display correct path from submodule, 2017-01-06)
3. 1cf823d8f00 (submodule: remove unnecessary prefix based option logic, 2021-06-22)

---
 builtin/submodule--helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index aad431f898e..360309195fb 100644
@@ -3379,15 +3379,15 @@ static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
-	{"add", module_add, SUPPORT_SUPER_PREFIX},
+	{"add", module_add, 0},
 	{"update", module_update, 0},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
-	{"init", module_init, SUPPORT_SUPER_PREFIX},
+	{"init", module_init, 0},
 	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
 	{"deinit", module_deinit, 0},
-	{"summary", module_summary, SUPPORT_SUPER_PREFIX},
+	{"summary", module_summary, 0},
 	{"push-check", push_check, 0},
 	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 	{"is-active", is_active, 0},

gitgitgadget

From: Glen Choo <[email protected]>

Unlike the other subcommands, "git submodule--helper update" uses the "--recursive-prefix" flag instead of "--super-prefix". The two flags are otherwise identical (they only serve to compute the 'display path' of a submodule), except that there is a dedicated helper function to get the value of "--super-prefix".

This inconsistency exists because "git submodule update" used to pass "--recursive-prefix" between shell and C (introduced in [1]) before "--super-prefix" was introduced (in [2]), and for simplicity, we kept this name when "git submodule--helper update" was created.

Remove "--recursive-prefix" and its associated code from "git submodule--helper update", replacing it with "--super-prefix".

To use "--super-prefix", module_update is marked with SUPPORT_SUPER_PREFIX. Note that module_clone must also be marked with SUPPORT_SUPER_PREFIX, otherwise the "git submodule--helper clone" subprocess will fail check because "--super-prefix" is propagated via the environment.

[1] 48308681b0 (git submodule update: have a dedicated helper for cloning, 2016-02-29)
[2] 74866d7579 (git: make super-prefix option, 2016-10-07)

---
 builtin/submodule--helper.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

index 360309195fb..41177332dd8 100644
@@ -477,22 +477,18 @@ static int starts_with_dot_dot_slash(const char *const path)
 
 struct init_cb {
 	const char *prefix;
-	const char *superprefix;
 	unsigned int flags;
 };
 #define INIT_CB_INIT { 0 }
 
 static void init_submodule(const char *path, const char *prefix,
-			   const char *superprefix, unsigned int flags)
+			   unsigned int flags)
 {
 	const struct submodule *sub;
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	/* try superprefix from the environment, if it is not passed explicitly */
-	if (!superprefix)
-		superprefix = get_super_prefix();
-	displaypath = do_get_submodule_displaypath(path, prefix, superprefix);
+	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 

@@ -566,7 +562,7 @@ static void init_submodule(const char *path, const char *prefix,
 static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)
 {
 	struct init_cb *info = cb_data;
-	init_submodule(list_item->name, info->prefix, info->superprefix, info->flags);
+	init_submodule(list_item->name, info->prefix, info->flags);
 }
 
 static int module_init(int argc, const char **argv, const char *prefix)

@@ -1878,7 +1874,6 @@ struct submodule_update_clone {
 
 struct update_data {
 	const char *prefix;
-	const char *recursive_prefix;
 	const char *displaypath;
 	enum submodule_update_type update_default;
 	struct object_id suboid;

@@ -1954,7 +1949,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 
 	displaypath = do_get_submodule_displaypath(ce->name,
 						   suc->update_data->prefix,
-						   suc->update_data->recursive_prefix);
+						   get_super_prefix());
 
 	if (ce_stage(ce)) {
 		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);

@@ -2416,11 +2411,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 {
 	enum submodule_update_type update_type = update_data->update_default;
 
-	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
-	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->displaypath)
-		strvec_pushf(args, "--recursive-prefix=%s/",
+		strvec_pushf(args, "--super-prefix=%s/",
 			     update_data->displaypath);
+	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
+	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)

@@ -2468,7 +2463,7 @@ static int update_submodule(struct update_data *update_data)
 
 	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
 								update_data->prefix,
-								update_data->recursive_prefix);
+								get_super_prefix());
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,

@@ -2592,10 +2587,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "prefix", &opt.prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
-			   N_("path"),
-			   N_("path into the working tree, across nested "
-			      "submodule boundaries")),
 		OPT_SET_INT(0, "checkout", &opt.update_default,
 			N_("use the 'checkout' update strategy (default)"),
 			SM_UPDATE_CHECKOUT),

@@ -2681,7 +2672,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			module_list_active(&list);
 
 		info.prefix = opt.prefix;
-		info.superprefix = opt.recursive_prefix;
 		if (opt.quiet)
 			info.flags |= OPT_QUIET;
 

@@ -3378,9 +3368,9 @@ struct cmd_struct {
 static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
-	{"clone", module_clone, 0},
+	{"clone", module_clone, SUPPORT_SUPER_PREFIX},
 	{"add", module_add, 0},
-	{"update", module_update, 0},
+	{"update", module_update, SUPPORT_SUPER_PREFIX},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"init", module_init, 0},

gitgitgadget

From: Glen Choo <[email protected]>

All invocations of do_get_submodule_displaypath() pass get_super_prefix() as the super_prefix arg, which is exactly the same as get_submodule_displaypath().

Replace all calls to do_get_submodule_displaypath() with get_submodule_displaypath(), and since it has no more callers, remove it.

---
 builtin/submodule--helper.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

index 41177332dd8..3e0ceb52589 100644
@@ -118,10 +118,11 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
-static char *do_get_submodule_displaypath(const char *path,
-					  const char *prefix,
-					  const char *super_prefix)
+/* the result should be freed by the caller. */
+static char *get_submodule_displaypath(const char *path, const char *prefix)
 {
+	const char *super_prefix = get_super_prefix();
+
 	if (prefix && super_prefix) {
 		BUG("cannot have prefix '%s' and superprefix '%s'",
 		    prefix, super_prefix);

@@ -137,13 +138,6 @@ static char *do_get_submodule_displaypath(const char *path,
 	}
 }
 
-/* the result should be freed by the caller. */
-static char *get_submodule_displaypath(const char *path, const char *prefix)
-{
-	const char *super_prefix = get_super_prefix();
-	return do_get_submodule_displaypath(path, prefix, super_prefix);
-}
-
 static char *compute_rev_name(const char *sub_path, const char* object_id)
 {
 	struct strbuf sb = STRBUF_INIT;

@@ -488,7 +482,7 @@ static void init_submodule(const char *path, const char *prefix,
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
+	displaypath = get_submodule_displaypath(path, prefix);
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 

@@ -1947,9 +1941,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
-	displaypath = do_get_submodule_displaypath(ce->name,
-						   suc->update_data->prefix,
-						   get_super_prefix());
+	displaypath =
+		get_submodule_displaypath(ce->name, suc->update_data->prefix);
 
 	if (ce_stage(ce)) {
 		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);

@@ -2461,9 +2454,8 @@ static int update_submodule(struct update_data *update_data)
 {
 	ensure_core_worktree(update_data->sm_path);
 
-	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
-								update_data->prefix,
-								get_super_prefix());
+	update_data->displaypath = get_submodule_displaypath(
+		update_data->sm_path, update_data->prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,

gitgitgadget

Looks like I accidentally submitted all of ab/submodule-cleanup... Let me try this again without GGG.

(resending as v3 because I accidentally included ab/submodule-cleanup in v2)

Thanks Ævar for the feedback! I've incorporated all of the suggestions, except breaking up the tests in 1/6 into their own patch - I wasn't convinced of the value of having prescriptive tests (with test_expect_failure), and I found it difficult to discuss descriptive tests (with test_expect_success) without also having the code in the same diff. FWIW, this version (and the previous one) "git rebase -x 'make test'" cleanly :)

Note for Junio: this version is based on ab/submodule-cleanup (and so will future versions).

= Description

This series is a refactor of "git submodule--helper update" that replaces "--recursive-prefix" with "--super-prefix" (see Background). This was initially motivated by:

* Junio's suggestion to simplify the code [1] (in response to a memory leak found by Johannes Schindelin [2]).
* A desire to use the module_list API + get_submodule_displaypath() outside of builtin/submodule--helper.c (I expect to use this to check out branches in each submodule).

But it also happens to remove some overly complicated/duplicated code that was literally converted from shell :)

= Background

When recursing into nested submodules, Git commands keep track of the path from the superproject to the submodule in order to print a "display path" to the user, e.g.

Submodule path '../super/sub/nested-sub': checked out 'abcdef'

For historical reasons, "git submodule--helper update" uses "--recursive-prefix" for this purpose, but it should use "--super-prefix" instead because:

* That's what every other command uses (not just submodule--helper subcommands).
* Using the "--super-prefix" helper functions makes the "git submodule--helper update" code simpler

= Patch organization

* Patch 1/6 makes sure that display paths are only computed using display path helper functions ([do_]get_submodule_displaypath()) and fixes some display paths that we never realized were broken.
* Patches 2-3/6 simplify and deduplicate some display path computations.
* Patch 4/6 (authored by Ævar) removes SUPPORT_SUPER_PREFIX where it's not needed.
* This doesn't affect correctness, but we want to do this eventually, so do it now to make 5/6 a bit cleaner.
* Patch 5/6 replaces "--recursive-prefix" with "--super-prefix", making do_get_submodule_displaypath() obsolete.
* GGG outputs an odd diff; I recommend viewing it with "--histogram"
* Patch 6/6 removes do_get_submodule_displaypath().

= Series history

Changes in v3:
* None (resend of v2 because v2 accidentally included ab/submodule-cleanup)

Changes in v2:
* Rebase onto ab/submodule-cleanup (previously master)
* Cherry pick https://github.com/avar/git/commit/f445c57490d into 4/6
* Style fixes in .c and tests

= Future work

At the end of this series, get_submodule_displaypath() can be moved to submodule.h, which would make submodule.c:get_super_prefix_or_empty() obsolete. I have a patch that demonstrates this (CI run: [4]), but I decided to keep this series more focused on "git submodule--helper update" so that it's easier to review.

[1] https://lore.kernel.org/git/[email protected]
[2] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com
[3] https://lore.kernel.org/git/[email protected]
[4] https://github.com/chooglen/git/actions/runs/2572557584

Glen Choo (5):
submodule--helper update: use display path helper
submodule--helper: don't recreate recursive prefix
submodule--helper: use correct display path helper submodule--helper update: use --super-prefix
submodule--helper: remove display path helper

Ævar Arnfjörð Bjarmason (1):
submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags

 builtin/submodule--helper.c | 87 ++++++++++---------------------------
 t/t7406-submodule-update.sh | 56 ++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 64 deletions(-)

Range-diff against v1:
1: 473548f2fa ! 1: 64c138df19 submodule--helper update: use display path helper

    @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st
     -	struct strbuf displaypath_sb = STRBUF_INIT;
      	struct strbuf sb = STRBUF_INIT;
     -	const char *displaypath = NULL;
    -+	char *displaypath = NULL;
    ++	char *displaypath;
      	int needs_cloning = 0;
      	int need_free_url = 0;
      
    @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st
     -		else
     -			strbuf_addstr(&sb, ce->name);
     -		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
    --		strbuf_addch(out, '\n');
    -+		strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);
    ++		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
    + 		strbuf_addch(out, '\n');
      		goto cleanup;
      	}
      
    @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets
     +	test_when_finished "rm -fr top-cloned" &&
     +	cp -r top-clean top-cloned &&
     +
    -+	# Create an upstream commit in each repo
    ++	# Create an upstream commit in each repo, starting with bottom
     +	test_commit -C bottom upstream_commit &&
    -+	(cd middle &&
    -+	 git -C bottom fetch &&
    -+	 git -C bottom checkout -f FETCH_HEAD &&
    -+	 git add bottom &&
    -+	 git commit -m "upstream_commit"
    -+	) &&
    -+	(cd top &&
    -+	 git -C middle fetch &&
    -+	 git -C middle checkout -f FETCH_HEAD &&
    -+	 git add middle &&
    -+	 git commit -m "upstream_commit"
    -+	) &&
    ++	# Create middle commit
    ++	git -C middle/bottom fetch &&
    ++	git -C middle/bottom checkout -f FETCH_HEAD &&
    ++	git -C middle add bottom &&
    ++	git -C middle commit -m "upstream_commit" &&
    ++	# Create top commit
    ++	git -C top/middle fetch &&
    ++	git -C top/middle checkout -f FETCH_HEAD &&
    ++	git -C top add middle &&
    ++	git -C top commit -m "upstream_commit" &&
     +
     +	# Create a downstream conflict
    -+	(cd top-cloned/middle &&
    -+	 test_commit -C bottom downstream_commit &&
    -+	 git add bottom &&
    -+	 git commit -m "downstream_commit" &&
    -+	 git fetch --recurse-submodules origin &&
    -+	 test_must_fail git merge origin/main
    -+	) &&
    ++	test_commit -C top-cloned/middle/bottom downstream_commit &&
    ++	git -C top-cloned/middle add bottom &&
    ++	git -C top-cloned/middle commit -m "downstream_commit" &&
    ++	git -C top-cloned/middle fetch --recurse-submodules origin &&
    ++	test_must_fail git -C top-cloned/middle merge origin/main &&
    ++
     +	# Make the update of "middle" a no-op, otherwise we error out
     +	# because of its unmerged state
     +	test_config -C top-cloned submodule.middle.update !true &&
     +	git -C top-cloned submodule update --recursive 2>actual.err &&
    -+	grep "Skipping unmerged submodule .middle/bottom." actual.err
    ++	grep -F "Skipping unmerged submodule middle/bottom" actual.err
     +'
     +
     +test_expect_success 'submodule update --recursive skip submodules with strategy=none' '
    @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets
     +	test_commit -C top-cloned/middle/bottom downstream_commit &&
     +	git -C top-cloned/middle config submodule.bottom.update none &&
     +	git -C top-cloned submodule update --recursive 2>actual.err &&
    -+	grep "Skipping submodule .middle/bottom." actual.err
    ++	grep -F "Skipping submodule ${SQ}middle/bottom${SQ}" actual.err
     +'
     +
      test_done

2: 618053730e ! 2: d3e803a463 submodule--helper: don't recreate recursive prefix
@@ Commit message

## builtin/submodule--helper.c ##
@@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *
- {
+
strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
- if (update_data->recursive_prefix)

3:  7cd1c46f35 = 3:  1f7cf6ffaf submodule--helper: use correct display path helper
-:  ---------- > 4:  85e65f143b submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags
4:  57988287fc ! 5:  1d8b6b244d submodule--helper update: use --super-prefix

@@ builtin/submodule--helper.c: struct submodule_update_clone {
const char *prefix;
- const char *recursive_prefix;
const char *displaypath;
- const char *update_default;
+ enum submodule_update_type update_default;
struct object_id suboid;
@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,

@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st
+ get_super_prefix());

if (ce_stage(ce)) {
- strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);
-@@ builtin/submodule--helper.c: static void ensure_core_worktree(const char *path)
-
- static void update_data_to_args(struct update_data *update_data, struct strvec *args)
+ strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
+@@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *
{
+ enum submodule_update_type update_type = update_data->update_default;
+
- strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
- strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
if (update_data->displaypath)
@@ builtin/submodule--helper.c: static int module_update(int argc, const char **arg
- N_("path"),
- N_("path into the working tree, across nested "
- "submodule boundaries")),
- OPT_STRING(0, "update", &opt.update_default,
- N_("string"),
- N_("rebase, merge, checkout or none")),
+ OPT_SET_INT(0, "checkout", &opt.update_default,
+ N_("use the 'checkout' update strategy (default)"),
+ SM_UPDATE_CHECKOUT),
@@ builtin/submodule--helper.c: static int module_update(int argc, const char **argv, const char *prefix)
module_list_active(&list);

@@ builtin/submodule--helper.c: struct cmd_struct {
{"name", module_name, 0},
- {"clone", module_clone, 0},
+ {"clone", module_clone, SUPPORT_SUPER_PREFIX},
- {"add", module_add, SUPPORT_SUPER_PREFIX},
+ {"add", module_add, 0},
- {"update", module_update, 0},
+ {"update", module_update, SUPPORT_SUPER_PREFIX},
{"resolve-relative-url-test", resolve_relative_url_test, 0}, {"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
- {"init", module_init, SUPPORT_SUPER_PREFIX},
+ {"init", module_init, 0},
5: 9fa13380b0 ! 6: a21e8cd174 submodule--helper: remove display path helper
@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st
+ get_submodule_displaypath(ce->name, suc->update_data->prefix);

if (ce_stage(ce)) {
- strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);
+ strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)
{
ensure_core_worktree(update_data->sm_path);

base-commit: 5b893f7d81eb7feb43662ed8663e2af76a76b4c8

2.37.0.rc0.161.g10f37bed90-goog

There are two locations in prepare_to_clone_next_submodule() that manually calculate the submodule display path, but should just use do_get_submodule_displaypath() for consistency.

Do this replacement and reorder the code slightly to avoid computing the display path twice.

This code was never tested, and adding tests shows that both these sites have been computing the display path incorrectly ever since they were introduced in 48308681b0 (git submodule update: have a dedicated helper for cloning, 2016-02-29) [1]:

- The first hunk puts a "/" between recursive_prefix and ce->name, but recursive_prefix already ends with "/".
- The second hunk calls relative_path() on recursive_prefix and ce->name, but relative_path() only makes sense when both paths share the same base directory. This is never the case here:
- recursive_prefix is the path from the topmost superproject to the current submodule
- ce->name is the path from the root of the current submodule to its submodule.
so, e.g. recursive_prefix="super" and ce->name="submodule" produces displayname="../super" instead of "super/submodule".

While we're fixing the display names, also fix inconsistent quoting of the submodule name.

[1] I verified this by applying the tests to 48308681b0.

---
 builtin/submodule--helper.c | 21 +++++---------
 t/t7406-submodule-update.sh | 56 +++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 14 deletions(-)

index 389b900602..db2d5ab799 100644
@@ -1947,30 +1947,23 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	const char *update_string;
 	enum submodule_update_type update_type;
 	char *key;
-	struct strbuf displaypath_sb = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
-	const char *displaypath = NULL;
+	char *displaypath;
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
+	displaypath = do_get_submodule_displaypath(ce->name,
+						   suc->update_data->prefix,
+						   suc->update_data->recursive_prefix);
+
 	if (ce_stage(ce)) {
-		if (suc->update_data->recursive_prefix)
-			strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name);
-		else
-			strbuf_addstr(&sb, ce->name);
-		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
+		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
 		strbuf_addch(out, '\n');
 		goto cleanup;
 	}
 
 	sub = submodule_from_path(the_repository, null_oid(), ce->name);
 
-	if (suc->update_data->recursive_prefix)
-		displaypath = relative_path(suc->update_data->recursive_prefix,
-					    ce->name, &displaypath_sb);
-	else
-		displaypath = ce->name;
-
 	if (!sub) {
 		next_submodule_warn_missing(suc, out, displaypath);
 		goto cleanup;

@@ -2060,7 +2053,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 					      "--no-single-branch");
 
 cleanup:
-	strbuf_release(&displaypath_sb);
+	free(displaypath);
 	strbuf_release(&sb);
 	if (need_free_url)
 		free((void*)url);

2.37.0.rc0.161.g10f37bed90-goog

update_submodule() uses duplicated code to compute update_data->displaypath and next.recursive_prefix. The latter is just the former with "/" appended to it, and since update_data->displaypath not changed outside of this statement, we can just reuse the already computed result.

We can go one step further and remove the reference to next.recursive_prefix altogether. Since it is only used in update_data_to_args() (to compute the "--recursive-prefix" flag for the recursive update child process) we can just use the already computed .displaypath value of there.

Delete the duplicated code, and remove the unnecessary reference to next.recursive_prefix. As a bonus, this fixes a memory leak where prefixed_path was never freed (this leak was first reported in [1]).

[1] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com

---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

index db2d5ab799..c104b1a023 100644
@@ -2418,9 +2418,9 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
-	if (update_data->recursive_prefix)
-		strvec_pushl(args, "--recursive-prefix",
-			     update_data->recursive_prefix, NULL);
+	if (update_data->displaypath)
+		strvec_pushf(args, "--recursive-prefix=%s/",
+			     update_data->displaypath);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)

@@ -2516,14 +2516,6 @@ static int update_submodule(struct update_data *update_data)
 		struct update_data next = *update_data;
 		int res;
 
-		if (update_data->recursive_prefix)
-			prefixed_path = xstrfmt("%s%s/", update_data->recursive_prefix,
-						update_data->sm_path);
-		else
-			prefixed_path = xstrfmt("%s/", update_data->sm_path);
-
-		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
-								  update_data->prefix);
 		next.prefix = NULL;
 		oidcpy(&next.oid, null_oid());
 		oidcpy(&next.suboid, null_oid());

2.37.0.rc0.161.g10f37bed90-goog

Replace a chunk of code in update_submodule() with an equivalent do_get_submodule_displaypath() invocation. This is already tested by t/t7406-submodule-update.sh:'submodule update --init --recursive from subdirectory', so no tests are added.

The two are equivalent because:

- Exactly one of recursive_prefix|prefix is non-NULL at a time; prefix is set at the superproject level, and recursive_prefix is set when recursing into submodules. There is also a BUG() statement in get_submodule_displaypath() that asserts that both cannot be non-NULL.

- In get_submodule_displaypath(), get_super_prefix() always returns NULL because "--super-prefix" is never passed. Thus calling it is equivalent to calling do_get_submodule_displaypath() with super_prefix = NULL.

Therefore:

- When recursive_prefix is non-NULL, prefix is NULL, and thus get_submodule_displaypath() just returns prefixed_path. This is identical to calling do_get_submodule_displaypath() with super_prefix = recursive_prefix because the return value is still the concatenation of recursive_prefix + update_data->sm_path.

- When prefix is non-NULL, prefixed_path = update_data->sm_path. Thus calling get_submodule_displaypath() with prefixed_path is equivalent to calling do_get_submodule_displaypath() with update_data->sm_path

---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

index c104b1a023..aad431f898 100644
@@ -2464,19 +2464,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 
 static int update_submodule(struct update_data *update_data)
 {
-	char *prefixed_path;
-
 	ensure_core_worktree(update_data->sm_path);
 
-	if (update_data->recursive_prefix)
-		prefixed_path = xstrfmt("%s%s", update_data->recursive_prefix,
-					update_data->sm_path);
-	else
-		prefixed_path = xstrdup(update_data->sm_path);
-
-	update_data->displaypath = get_submodule_displaypath(prefixed_path,
-							     update_data->prefix);
-	free(prefixed_path);
+	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
+								update_data->prefix,
+								update_data->recursive_prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,

2.37.0.rc0.161.g10f37bed90-goog

From: Ævar Arnfjörð Bjarmason <[email protected]>

Remove the SUPPORT_SUPER_PREFIX flag from "add", "init" and "summary". For the "add" command it hasn't been used since [1], likewise for "init" and "summary" since [2] and [3], respectively.

As implemented in 74866d75793 (git: make super-prefix option, 2016-10-07) the SUPPORT_SUPER_PREFIX flag in git.c applies for the entire command, but as implemented in 89c86265576 (submodule helper:
support super prefix, 2016-12-08) we assert here in cmd_submodule__helper() that we're not getting the flag unexpectedly.

1. 8c8195e9c3e (submodule--helper: introduce add-clone subcommand, 2021-07-10)
2. 6e7c14e65c8 (submodule update --init: display correct path from submodule, 2017-01-06)
3. 1cf823d8f00 (submodule: remove unnecessary prefix based option logic, 2021-06-22)

---
 builtin/submodule--helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index aad431f898..360309195f 100644
@@ -3379,15 +3379,15 @@ static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
-	{"add", module_add, SUPPORT_SUPER_PREFIX},
+	{"add", module_add, 0},
 	{"update", module_update, 0},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
-	{"init", module_init, SUPPORT_SUPER_PREFIX},
+	{"init", module_init, 0},
 	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
 	{"deinit", module_deinit, 0},
-	{"summary", module_summary, SUPPORT_SUPER_PREFIX},
+	{"summary", module_summary, 0},
 	{"push-check", push_check, 0},
 	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 	{"is-active", is_active, 0},

2.37.0.rc0.161.g10f37bed90-goog

Unlike the other subcommands, "git submodule--helper update" uses the "--recursive-prefix" flag instead of "--super-prefix". The two flags are otherwise identical (they only serve to compute the 'display path' of a submodule), except that there is a dedicated helper function to get the value of "--super-prefix".

This inconsistency exists because "git submodule update" used to pass "--recursive-prefix" between shell and C (introduced in [1]) before "--super-prefix" was introduced (in [2]), and for simplicity, we kept this name when "git submodule--helper update" was created.

Remove "--recursive-prefix" and its associated code from "git submodule--helper update", replacing it with "--super-prefix".

To use "--super-prefix", module_update is marked with SUPPORT_SUPER_PREFIX. Note that module_clone must also be marked with SUPPORT_SUPER_PREFIX, otherwise the "git submodule--helper clone" subprocess will fail check because "--super-prefix" is propagated via the environment.

[1] 48308681b0 (git submodule update: have a dedicated helper for cloning, 2016-02-29)
[2] 74866d7579 (git: make super-prefix option, 2016-10-07)

---
 builtin/submodule--helper.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

index 360309195f..41177332dd 100644
@@ -477,22 +477,18 @@ static int starts_with_dot_dot_slash(const char *const path)
 
 struct init_cb {
 	const char *prefix;
-	const char *superprefix;
 	unsigned int flags;
 };
 #define INIT_CB_INIT { 0 }
 
 static void init_submodule(const char *path, const char *prefix,
-			   const char *superprefix, unsigned int flags)
+			   unsigned int flags)
 {
 	const struct submodule *sub;
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	/* try superprefix from the environment, if it is not passed explicitly */
-	if (!superprefix)
-		superprefix = get_super_prefix();
-	displaypath = do_get_submodule_displaypath(path, prefix, superprefix);
+	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 

@@ -566,7 +562,7 @@ static void init_submodule(const char *path, const char *prefix,
 static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)
 {
 	struct init_cb *info = cb_data;
-	init_submodule(list_item->name, info->prefix, info->superprefix, info->flags);
+	init_submodule(list_item->name, info->prefix, info->flags);
 }
 
 static int module_init(int argc, const char **argv, const char *prefix)

@@ -1878,7 +1874,6 @@ struct submodule_update_clone {
 
 struct update_data {
 	const char *prefix;
-	const char *recursive_prefix;
 	const char *displaypath;
 	enum submodule_update_type update_default;
 	struct object_id suboid;

@@ -1954,7 +1949,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 
 	displaypath = do_get_submodule_displaypath(ce->name,
 						   suc->update_data->prefix,
-						   suc->update_data->recursive_prefix);
+						   get_super_prefix());
 
 	if (ce_stage(ce)) {
 		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);

@@ -2416,11 +2411,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 {
 	enum submodule_update_type update_type = update_data->update_default;
 
-	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
-	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->displaypath)
-		strvec_pushf(args, "--recursive-prefix=%s/",
+		strvec_pushf(args, "--super-prefix=%s/",
 			     update_data->displaypath);
+	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
+	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)

@@ -2468,7 +2463,7 @@ static int update_submodule(struct update_data *update_data)
 
 	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
 								update_data->prefix,
-								update_data->recursive_prefix);
+								get_super_prefix());
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,

@@ -2592,10 +2587,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "prefix", &opt.prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
-			   N_("path"),
-			   N_("path into the working tree, across nested "
-			      "submodule boundaries")),
 		OPT_SET_INT(0, "checkout", &opt.update_default,
 			N_("use the 'checkout' update strategy (default)"),
 			SM_UPDATE_CHECKOUT),

@@ -2681,7 +2672,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			module_list_active(&list);
 
 		info.prefix = opt.prefix;
-		info.superprefix = opt.recursive_prefix;
 		if (opt.quiet)
 			info.flags |= OPT_QUIET;
 

@@ -3378,9 +3368,9 @@ struct cmd_struct {
 static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
-	{"clone", module_clone, 0},
+	{"clone", module_clone, SUPPORT_SUPER_PREFIX},
 	{"add", module_add, 0},
-	{"update", module_update, 0},
+	{"update", module_update, SUPPORT_SUPER_PREFIX},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"init", module_init, 0},

2.37.0.rc0.161.g10f37bed90-goog

All invocations of do_get_submodule_displaypath() pass get_super_prefix() as the super_prefix arg, which is exactly the same as get_submodule_displaypath().

Replace all calls to do_get_submodule_displaypath() with get_submodule_displaypath(), and since it has no more callers, remove it.

---
 builtin/submodule--helper.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

index 41177332dd..3e0ceb5258 100644
@@ -118,10 +118,11 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
-static char *do_get_submodule_displaypath(const char *path,
-					  const char *prefix,
-					  const char *super_prefix)
+/* the result should be freed by the caller. */
+static char *get_submodule_displaypath(const char *path, const char *prefix)
 {
+	const char *super_prefix = get_super_prefix();
+
 	if (prefix && super_prefix) {
 		BUG("cannot have prefix '%s' and superprefix '%s'",
 		    prefix, super_prefix);

@@ -137,13 +138,6 @@ static char *do_get_submodule_displaypath(const char *path,
 	}
 }
 
-/* the result should be freed by the caller. */
-static char *get_submodule_displaypath(const char *path, const char *prefix)
-{
-	const char *super_prefix = get_super_prefix();
-	return do_get_submodule_displaypath(path, prefix, super_prefix);
-}
-
 static char *compute_rev_name(const char *sub_path, const char* object_id)
 {
 	struct strbuf sb = STRBUF_INIT;

@@ -488,7 +482,7 @@ static void init_submodule(const char *path, const char *prefix,
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
+	displaypath = get_submodule_displaypath(path, prefix);
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 

@@ -1947,9 +1941,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
-	displaypath = do_get_submodule_displaypath(ce->name,
-						   suc->update_data->prefix,
-						   get_super_prefix());
+	displaypath =
+		get_submodule_displaypath(ce->name, suc->update_data->prefix);
 
 	if (ce_stage(ce)) {
 		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);

@@ -2461,9 +2454,8 @@ static int update_submodule(struct update_data *update_data)
 {
 	ensure_core_worktree(update_data->sm_path);
 
-	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
-								update_data->prefix,
-								get_super_prefix());
+	update_data->displaypath = get_submodule_displaypath(
+		update_data->sm_path, update_data->prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,

2.37.0.rc0.161.g10f37bed90-goog

Thanks again! As mentioned, I've taken the amendments Ævar posted in
[1] and fixed the bug mentioned there. The only things I've changed were some punctuation and changing the base commit to upstream's ab/submodule-cleanup (which is based off a slightly older 'master').

Note for Junio: this version is based on ab/submodule-cleanup.

= Description

This series is a refactor of "git submodule--helper update" that replaces "--recursive-prefix" with "--super-prefix" (see Background). This was initially motivated by:

* Junio's suggestion to simplify the code [2] (in response to a memory leak found by Johannes Schindelin [3]).
* A desire to use the module_list API + get_submodule_displaypath() outside of builtin/submodule--helper.c (I expect to use this to check out branches in each submodule).

But it also happens to remove some overly complicated/duplicated code that was literally converted from shell :)

= Background

When recursing into nested submodules, Git commands keep track of the path from the superproject to the submodule in order to print a "display path" to the user, e.g.

Submodule path '../super/sub/nested-sub': checked out 'abcdef'

For historical reasons, "git submodule--helper update" uses "--recursive-prefix" for this purpose, but it should use "--super-prefix" instead because:

* That's what every other command uses (not just submodule--helper subcommands).
* Using the "--super-prefix" helper functions makes the "git submodule--helper update" code simpler

= Patch organization

1/7 and 5/7 were contributed by Ævar (thanks!)

* Patches 1-2/7 makes sure that display paths are only computed using display path helper functions ([do_]get_submodule_displaypath()) and fixes some display paths that we never realized were broken.
* Patches 3-4/7 simplify and deduplicate some display path computations.
* Patch 5/7 removes SUPPORT_SUPER_PREFIX where it's not needed.
* This doesn't affect correctness, but we want to do this eventually, so do it now to make 6/7 a bit cleaner.
* Patch 6/7 replaces "--recursive-prefix" with "--super-prefix", making do_get_submodule_displaypath() obsolete.
* Patch 7/7 removes do_get_submodule_displaypath().

= Series history

Changes in v4:
* Split patch 1 so that the missing test coverage is introduced in its own patch
* Fix a stale commit message in 1/6 (now patch 2/7)
* Use test_cmp instead of "grep -F"
* Style fixes and improvements

Changes in v3:
* None (resend of v2 because v2 accidentally included ab/submodule-cleanup)

Changes in v2:
* Rebase onto ab/submodule-cleanup (previously master)
* Cherry pick https://github.com/avar/git/commit/f445c57490d into 4/6
* Style fixes in .c and tests

= Future work

At the end of this series, get_submodule_displaypath() can be moved to submodule.h, which would make submodule.c:get_super_prefix_or_empty() obsolete. I have a patch that demonstrates this (CI run: [4]), but I decided to keep this series more focused on "git submodule--helper update" so that it's easier to review.

[1] https://lore.kernel.org/git/[email protected]
[2] https://lore.kernel.org/git/[email protected]
[3] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com
[4] https://github.com/chooglen/git/actions/runs/2572557584

Glen Choo (6):
submodule--helper tests: add missing "display path" coverage submodule--helper update: use display path helper
submodule--helper: don't recreate recursive prefix
submodule--helper: use correct display path helper submodule--helper update: use --super-prefix
submodule--helper: remove display path helper

Ævar Arnfjörð Bjarmason (1):
submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags

 builtin/submodule--helper.c | 86 ++++++++++---------------------------
 t/t7406-submodule-update.sh | 62 ++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 64 deletions(-)

Range-diff against v3:
1: 8c82518d33 ! 1: 1f2ef5f6a2 submodule--helper update: use display path helper
@@ Metadata
Author: Glen Choo <[email protected]>

## Commit message ##
- submodule--helper update: use display path helper
+ submodule--helper tests: add missing "display path" coverage

There are two locations in prepare_to_clone_next_submodule() that
- manually calculate the submodule display path, but should just use
- do_get_submodule_displaypath() for consistency.
-
- Do this replacement and reorder the code slightly to avoid computing
- the display path twice.
-
- This code was never tested, and adding tests shows that both these sites
- have been computing the display path incorrectly ever since they were
- introduced in 48308681b0 (git submodule update: have a dedicated helper
- for cloning, 2016-02-29) [1]:
-
- - The first hunk puts a "/" between recursive_prefix and ce->name, but
- recursive_prefix already ends with "/".
- - The second hunk calls relative_path() on recursive_prefix and
- ce->name, but relative_path() only makes sense when both paths share
- the same base directory. This is never the case here:
- - recursive_prefix is the path from the topmost superproject to the
- current submodule
- - ce->name is the path from the root of the current submodule to its
- submodule.
- so, e.g. recursive_prefix="super" and ce->name="submodule" produces
- displayname="../super" instead of "super/submodule".
-
- [1] I verified this by applying the tests to 48308681b0.
+ manually calculate the submodule display path. As discussed in the
+ next commit the "Skipping" output isn't exactly what we want, but
+ let's test how we behave now, before changing the existing behavior.

-
- ## builtin/submodule--helper.c ##
-@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
- const char *update_string;
- enum submodule_update_type update_type;
- char *key;
-- struct strbuf displaypath_sb = STRBUF_INIT;
- struct strbuf sb = STRBUF_INIT;
-- const char *displaypath = NULL;
-+ char *displaypath;
- int needs_cloning = 0;
- int need_free_url = 0;
-
-+ displaypath = do_get_submodule_displaypath(ce->name,
-+ suc->update_data->prefix,
-+ suc->update_data->recursive_prefix);
-+
- if (ce_stage(ce)) {
-- if (suc->update_data->recursive_prefix)
-- strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name);
-- else
-- strbuf_addstr(&sb, ce->name);
-- strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
-+ strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
- strbuf_addch(out, '\n');
- goto cleanup;
- }
-
- sub = submodule_from_path(the_repository, null_oid(), ce->name);
-
-- if (suc->update_data->recursive_prefix)
-- displaypath = relative_path(suc->update_data->recursive_prefix,
-- ce->name, &displaypath_sb);
-- else
-- displaypath = ce->name;
--
- if (!sub) {
- next_submodule_warn_missing(suc, out, displaypath);
- goto cleanup;
-@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
- "--no-single-branch");
-
- cleanup:
-- strbuf_release(&displaypath_sb);
-+ free(displaypath);
- strbuf_release(&sb);
- if (need_free_url)
- free((void*)url);
+ Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>

## t/t7406-submodule-update.sh ##
@@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets partial clone settings' '
@@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets
+ # because of its unmerged state
+ test_config -C top-cloned submodule.middle.update !true &&
+ git -C top-cloned submodule update --recursive 2>actual.err &&
-+ grep -F "Skipping unmerged submodule middle/bottom" actual.err
++ cat >expect.err <<-\EOF &&
++ Skipping unmerged submodule middle//bottom
++ EOF
++ test_cmp expect.err actual.err
+'
+
+test_expect_success 'submodule update --recursive skip submodules with strategy=none' '
@@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets
+ test_commit -C top-cloned/middle/bottom downstream_commit &&
+ git -C top-cloned/middle config submodule.bottom.update none &&
+ git -C top-cloned submodule update --recursive 2>actual.err &&
-+ grep -F "Skipping submodule ${SQ}middle/bottom${SQ}" actual.err
++ cat >expect.err <<-\EOF &&
++ Skipping submodule '\''../middle/'\''
++ EOF
++ test_cmp expect.err actual.err
+'
+
test_done

-:  ---------- > 2:  146b88eaa3 submodule--helper update: use display path helper
2:  102ada974d ! 3:  a744640cd3 submodule--helper: don't recreate recursive prefix
    @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data
     -	if (update_data->recursive_prefix)
     -		strvec_pushl(args, "--recursive-prefix",
     -			     update_data->recursive_prefix, NULL);
    -+	if (update_data->displaypath)
    -+		strvec_pushf(args, "--recursive-prefix=%s/",
    -+			     update_data->displaypath);
    ++	if (update_data->displaypath) {
    ++		strvec_push(args, "--recursive-prefix");
    ++		strvec_pushf(args, "%s/", update_data->displaypath);
    ++	}
      	if (update_data->quiet)
      		strvec_push(args, "--quiet");
      	if (update_data->force)
3:  b6dda65084 = 4:  0b45f2ff2b submodule--helper: use correct display path helper
4:  aa5d389bb8 = 5:  963c8ba07b submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags
5:  4dcfb9889f ! 6:  a777fcf905 submodule--helper update: use --super-prefix
    @@ builtin/submodule--helper.c: struct submodule_update_clone {
      	enum submodule_update_type update_default;
      	struct object_id suboid;
     @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
    - 
    - 	displaypath = do_get_submodule_displaypath(ce->name,
    - 						   suc->update_data->prefix,
    --						   suc->update_data->recursive_prefix);
    -+						   get_super_prefix());
    - 
    - 	if (ce_stage(ce)) {
    - 		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
    + 	char *key;
    + 	struct update_data *ud = suc->update_data;
    + 	char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,
    +-							 ud->recursive_prefix);
    ++							 get_super_prefix());
    + 	struct strbuf sb = STRBUF_INIT;
    + 	int needs_cloning = 0;
    + 	int need_free_url = 0;
     @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *
      {
      	enum submodule_update_type update_type = update_data->update_default;
      
     -	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
     -	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
    - 	if (update_data->displaypath)
    --		strvec_pushf(args, "--recursive-prefix=%s/",
    -+		strvec_pushf(args, "--super-prefix=%s/",
    - 			     update_data->displaypath);
    + 	if (update_data->displaypath) {
    +-		strvec_push(args, "--recursive-prefix");
    ++		strvec_push(args, "--super-prefix");
    + 		strvec_pushf(args, "%s/", update_data->displaypath);
    + 	}
     +	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
     +	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
      	if (update_data->quiet)
6:  109c55236d ! 7:  d47ea17cc2 submodule--helper: remove display path helper
    @@ builtin/submodule--helper.c: static void init_submodule(const char *path, const
      	sub = submodule_from_path(the_repository, null_oid(), path);
      
     @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
    + 	enum submodule_update_type update_type;
    + 	char *key;
    + 	struct update_data *ud = suc->update_data;
    +-	char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,
    +-							 get_super_prefix());
    ++	char *displaypath = get_submodule_displaypath(ce->name, ud->prefix);
    + 	struct strbuf sb = STRBUF_INIT;
      	int needs_cloning = 0;
      	int need_free_url = 0;
    - 
    --	displaypath = do_get_submodule_displaypath(ce->name,
    --						   suc->update_data->prefix,
    --						   get_super_prefix());
    -+	displaypath =
    -+		get_submodule_displaypath(ce->name, suc->update_data->prefix);
    - 
    - 	if (ce_stage(ce)) {
    - 		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
     @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)
      {
      	ensure_core_worktree(update_data->sm_path);

base-commit: 5b893f7d81eb7feb43662ed8663e2af76a76b4c8

2.37.0.rc0.161.g10f37bed90-goog

There are two locations in prepare_to_clone_next_submodule() that manually calculate the submodule display path. As discussed in the next commit the "Skipping" output isn't exactly what we want, but let's test how we behave now, before changing the existing behavior.

---
 t/t7406-submodule-update.sh | 62 +++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

index 06d804e213..9a076e025f 100755
@@ -1116,4 +1116,66 @@ test_expect_success 'submodule update --filter sets partial clone settings' '
 	test_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter
 '
 
+# NEEDSWORK: Clean up the tests so that we can reuse the test setup.
+# Don't reuse the existing repos because the earlier tests have
+# intentionally disruptive configurations.
+test_expect_success 'setup clean recursive superproject' '
+	git init bottom &&
+	test_commit -C bottom "bottom" &&
+	git init middle &&
+	git -C middle submodule add ../bottom bottom &&
+	git -C middle commit -m "middle" &&
+	git init top &&
+	git -C top submodule add ../middle middle &&
+	git -C top commit -m "top" &&
+	git clone --recurse-submodules top top-clean
+'
+
+test_expect_success 'submodule update should skip unmerged submodules' '
+	test_when_finished "rm -fr top-cloned" &&
+	cp -r top-clean top-cloned &&
+
+	# Create an upstream commit in each repo, starting with bottom
+	test_commit -C bottom upstream_commit &&
+	# Create middle commit
+	git -C middle/bottom fetch &&
+	git -C middle/bottom checkout -f FETCH_HEAD &&
+	git -C middle add bottom &&
+	git -C middle commit -m "upstream_commit" &&
+	# Create top commit
+	git -C top/middle fetch &&
+	git -C top/middle checkout -f FETCH_HEAD &&
+	git -C top add middle &&
+	git -C top commit -m "upstream_commit" &&
+
+	# Create a downstream conflict
+	test_commit -C top-cloned/middle/bottom downstream_commit &&
+	git -C top-cloned/middle add bottom &&
+	git -C top-cloned/middle commit -m "downstream_commit" &&
+	git -C top-cloned/middle fetch --recurse-submodules origin &&
+	test_must_fail git -C top-cloned/middle merge origin/main &&
+
+	# Make the update of "middle" a no-op, otherwise we error out
+	# because of its unmerged state
+	test_config -C top-cloned submodule.middle.update !true &&
+	git -C top-cloned submodule update --recursive 2>actual.err &&
+	cat >expect.err <<-\EOF &&
+	Skipping unmerged submodule middle//bottom
+	EOF
+	test_cmp expect.err actual.err
+'
+
+test_expect_success 'submodule update --recursive skip submodules with strategy=none' '
+	test_when_finished "rm -fr top-cloned" &&
+	cp -r top-clean top-cloned &&
+
+	test_commit -C top-cloned/middle/bottom downstream_commit &&
+	git -C top-cloned/middle config submodule.bottom.update none &&
+	git -C top-cloned submodule update --recursive 2>actual.err &&
+	cat >expect.err <<-\EOF &&
+	Skipping submodule '\''../middle/'\''
+	EOF
+	test_cmp expect.err actual.err
+'
+
 test_done

2.37.0.rc0.161.g10f37bed90-goog

There are two locations in prepare_to_clone_next_submodule() that manually calculate the submodule display path, but should just use do_get_submodule_displaypath() for consistency.

Do this replacement and reorder the code slightly to avoid computing the display path twice.

Until the preceding commit this code had never been tested, with our newly added tests we can see that both these sites have been computing the display path incorrectly ever since they were introduced in 48308681b0 (git submodule update: have a dedicated helper for cloning, 2016-02-29) [1]:

- The first hunk puts a "/" between recursive_prefix and ce->name, but recursive_prefix already ends with "/".
- The second hunk calls relative_path() on recursive_prefix and ce->name, but relative_path() only makes sense when both paths share the same base directory. This is never the case here:
- recursive_prefix is the path from the topmost superproject to the current submodule
- ce->name is the path from the root of the current submodule to its submodule.
so, e.g. recursive_prefix="super" and ce->name="submodule" produces displayname="../super" instead of "super/submodule".

[1] I verified this by applying the tests to 48308681b0.

---
 builtin/submodule--helper.c | 19 +++++--------------
 t/t7406-submodule-update.sh |  4 ++--
 2 files changed, 7 insertions(+), 16 deletions(-)

index 389b900602..9381127d56 100644
@@ -1947,30 +1947,21 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	const char *update_string;
 	enum submodule_update_type update_type;
 	char *key;
-	struct strbuf displaypath_sb = STRBUF_INIT;
+	struct update_data *ud = suc->update_data;
+	char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,
+							 ud->recursive_prefix);
 	struct strbuf sb = STRBUF_INIT;
-	const char *displaypath = NULL;
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
 	if (ce_stage(ce)) {
-		if (suc->update_data->recursive_prefix)
-			strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name);
-		else
-			strbuf_addstr(&sb, ce->name);
-		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
+		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
 		strbuf_addch(out, '\n');
 		goto cleanup;
 	}
 
 	sub = submodule_from_path(the_repository, null_oid(), ce->name);
 
-	if (suc->update_data->recursive_prefix)
-		displaypath = relative_path(suc->update_data->recursive_prefix,
-					    ce->name, &displaypath_sb);
-	else
-		displaypath = ce->name;
-
 	if (!sub) {
 		next_submodule_warn_missing(suc, out, displaypath);
 		goto cleanup;

@@ -2060,7 +2051,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 					      "--no-single-branch");
 
 cleanup:
-	strbuf_release(&displaypath_sb);
+	free(displaypath);
 	strbuf_release(&sb);
 	if (need_free_url)
 		free((void*)url);

index 9a076e025f..6cc07460dd 100755
@@ -1160,7 +1160,7 @@ test_expect_success 'submodule update should skip unmerged submodules' '
 	test_config -C top-cloned submodule.middle.update !true &&
 	git -C top-cloned submodule update --recursive 2>actual.err &&
 	cat >expect.err <<-\EOF &&
-	Skipping unmerged submodule middle//bottom
+	Skipping unmerged submodule middle/bottom
 	EOF
 	test_cmp expect.err actual.err
 '

@@ -1173,7 +1173,7 @@ test_expect_success 'submodule update --recursive skip submodules with strategy=
 	git -C top-cloned/middle config submodule.bottom.update none &&
 	git -C top-cloned submodule update --recursive 2>actual.err &&
 	cat >expect.err <<-\EOF &&
-	Skipping submodule '\''../middle/'\''
+	Skipping submodule '\''middle/bottom'\''
 	EOF
 	test_cmp expect.err actual.err
 '

2.37.0.rc0.161.g10f37bed90-goog

update_submodule() uses duplicated code to compute update_data->displaypath and next.recursive_prefix. The latter is just the former with "/" appended to it, and since update_data->displaypath not changed outside of this statement, we can just reuse the already computed result.

We can go one step further and remove the reference to next.recursive_prefix altogether. Since it is only used in update_data_to_args() (to compute the "--recursive-prefix" flag for the recursive update child process) we can just use the already computed .displaypath value of there.

Delete the duplicated code, and remove the unnecessary reference to next.recursive_prefix. As a bonus, this fixes a memory leak where prefixed_path was never freed (this leak was first reported in [1]).

[1] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com

---
 builtin/submodule--helper.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

index 9381127d56..4a0eb05ba2 100644
@@ -2416,9 +2416,10 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
-	if (update_data->recursive_prefix)
-		strvec_pushl(args, "--recursive-prefix",
-			     update_data->recursive_prefix, NULL);
+	if (update_data->displaypath) {
+		strvec_push(args, "--recursive-prefix");
+		strvec_pushf(args, "%s/", update_data->displaypath);
+	}
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)

@@ -2514,14 +2515,6 @@ static int update_submodule(struct update_data *update_data)
 		struct update_data next = *update_data;
 		int res;
 
-		if (update_data->recursive_prefix)
-			prefixed_path = xstrfmt("%s%s/", update_data->recursive_prefix,
-						update_data->sm_path);
-		else
-			prefixed_path = xstrfmt("%s/", update_data->sm_path);
-
-		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
-								  update_data->prefix);
 		next.prefix = NULL;
 		oidcpy(&next.oid, null_oid());
 		oidcpy(&next.suboid, null_oid());

2.37.0.rc0.161.g10f37bed90-goog

Replace a chunk of code in update_submodule() with an equivalent do_get_submodule_displaypath() invocation. This is already tested by t/t7406-submodule-update.sh:'submodule update --init --recursive from subdirectory', so no tests are added.

The two are equivalent because:

- Exactly one of recursive_prefix|prefix is non-NULL at a time; prefix is set at the superproject level, and recursive_prefix is set when recursing into submodules. There is also a BUG() statement in get_submodule_displaypath() that asserts that both cannot be non-NULL.

- In get_submodule_displaypath(), get_super_prefix() always returns NULL because "--super-prefix" is never passed. Thus calling it is equivalent to calling do_get_submodule_displaypath() with super_prefix = NULL.

Therefore:

- When recursive_prefix is non-NULL, prefix is NULL, and thus get_submodule_displaypath() just returns prefixed_path. This is identical to calling do_get_submodule_displaypath() with super_prefix = recursive_prefix because the return value is still the concatenation of recursive_prefix + update_data->sm_path.

- When prefix is non-NULL, prefixed_path = update_data->sm_path. Thus calling get_submodule_displaypath() with prefixed_path is equivalent to calling do_get_submodule_displaypath() with update_data->sm_path

---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

index 4a0eb05ba2..65350bde4b 100644
@@ -2463,19 +2463,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 
 static int update_submodule(struct update_data *update_data)
 {
-	char *prefixed_path;
-
 	ensure_core_worktree(update_data->sm_path);
 
-	if (update_data->recursive_prefix)
-		prefixed_path = xstrfmt("%s%s", update_data->recursive_prefix,
-					update_data->sm_path);
-	else
-		prefixed_path = xstrdup(update_data->sm_path);
-
-	update_data->displaypath = get_submodule_displaypath(prefixed_path,
-							     update_data->prefix);
-	free(prefixed_path);
+	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
+								update_data->prefix,
+								update_data->recursive_prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,

2.37.0.rc0.161.g10f37bed90-goog

From: Ævar Arnfjörð Bjarmason <[email protected]>

Remove the SUPPORT_SUPER_PREFIX flag from "add", "init" and "summary". For the "add" command it hasn't been used since [1], likewise for "init" and "summary" since [2] and [3], respectively.

As implemented in 74866d75793 (git: make super-prefix option, 2016-10-07) the SUPPORT_SUPER_PREFIX flag in git.c applies for the entire command, but as implemented in 89c86265576 (submodule helper:
support super prefix, 2016-12-08) we assert here in cmd_submodule__helper() that we're not getting the flag unexpectedly.

1. 8c8195e9c3e (submodule--helper: introduce add-clone subcommand, 2021-07-10)
2. 6e7c14e65c8 (submodule update --init: display correct path from submodule, 2017-01-06)
3. 1cf823d8f00 (submodule: remove unnecessary prefix based option logic, 2021-06-22)

---
 builtin/submodule--helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 65350bde4b..a7b40482b8 100644
@@ -3378,15 +3378,15 @@ static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
-	{"add", module_add, SUPPORT_SUPER_PREFIX},
+	{"add", module_add, 0},
 	{"update", module_update, 0},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
-	{"init", module_init, SUPPORT_SUPER_PREFIX},
+	{"init", module_init, 0},
 	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
 	{"deinit", module_deinit, 0},
-	{"summary", module_summary, SUPPORT_SUPER_PREFIX},
+	{"summary", module_summary, 0},
 	{"push-check", push_check, 0},
 	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 	{"is-active", is_active, 0},

2.37.0.rc0.161.g10f37bed90-goog

Unlike the other subcommands, "git submodule--helper update" uses the "--recursive-prefix" flag instead of "--super-prefix". The two flags are otherwise identical (they only serve to compute the 'display path' of a submodule), except that there is a dedicated helper function to get the value of "--super-prefix".

This inconsistency exists because "git submodule update" used to pass "--recursive-prefix" between shell and C (introduced in [1]) before "--super-prefix" was introduced (in [2]), and for simplicity, we kept this name when "git submodule--helper update" was created.

Remove "--recursive-prefix" and its associated code from "git submodule--helper update", replacing it with "--super-prefix".

To use "--super-prefix", module_update is marked with SUPPORT_SUPER_PREFIX. Note that module_clone must also be marked with SUPPORT_SUPER_PREFIX, otherwise the "git submodule--helper clone" subprocess will fail check because "--super-prefix" is propagated via the environment.

[1] 48308681b0 (git submodule update: have a dedicated helper for cloning, 2016-02-29)
[2] 74866d7579 (git: make super-prefix option, 2016-10-07)

---
 builtin/submodule--helper.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

index a7b40482b8..eb84e3a98b 100644
@@ -477,22 +477,18 @@ static int starts_with_dot_dot_slash(const char *const path)
 
 struct init_cb {
 	const char *prefix;
-	const char *superprefix;
 	unsigned int flags;
 };
 #define INIT_CB_INIT { 0 }
 
 static void init_submodule(const char *path, const char *prefix,
-			   const char *superprefix, unsigned int flags)
+			   unsigned int flags)
 {
 	const struct submodule *sub;
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	/* try superprefix from the environment, if it is not passed explicitly */
-	if (!superprefix)
-		superprefix = get_super_prefix();
-	displaypath = do_get_submodule_displaypath(path, prefix, superprefix);
+	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 

@@ -566,7 +562,7 @@ static void init_submodule(const char *path, const char *prefix,
 static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)
 {
 	struct init_cb *info = cb_data;
-	init_submodule(list_item->name, info->prefix, info->superprefix, info->flags);
+	init_submodule(list_item->name, info->prefix, info->flags);
 }
 
 static int module_init(int argc, const char **argv, const char *prefix)

@@ -1878,7 +1874,6 @@ struct submodule_update_clone {
 
 struct update_data {
 	const char *prefix;
-	const char *recursive_prefix;
 	const char *displaypath;
 	enum submodule_update_type update_default;
 	struct object_id suboid;

@@ -1949,7 +1944,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	char *key;
 	struct update_data *ud = suc->update_data;
 	char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,
-							 ud->recursive_prefix);
+							 get_super_prefix());
 	struct strbuf sb = STRBUF_INIT;
 	int needs_cloning = 0;
 	int need_free_url = 0;

@@ -2414,12 +2409,12 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 {
 	enum submodule_update_type update_type = update_data->update_default;
 
-	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
-	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->displaypath) {
-		strvec_push(args, "--recursive-prefix");
+		strvec_push(args, "--super-prefix");
 		strvec_pushf(args, "%s/", update_data->displaypath);
 	}
+	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
+	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)

@@ -2467,7 +2462,7 @@ static int update_submodule(struct update_data *update_data)
 
 	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
 								update_data->prefix,
-								update_data->recursive_prefix);
+								get_super_prefix());
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,

@@ -2591,10 +2586,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "prefix", &opt.prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
-			   N_("path"),
-			   N_("path into the working tree, across nested "
-			      "submodule boundaries")),
 		OPT_SET_INT(0, "checkout", &opt.update_default,
 			N_("use the 'checkout' update strategy (default)"),
 			SM_UPDATE_CHECKOUT),

@@ -2680,7 +2671,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			module_list_active(&list);
 
 		info.prefix = opt.prefix;
-		info.superprefix = opt.recursive_prefix;
 		if (opt.quiet)
 			info.flags |= OPT_QUIET;
 

@@ -3377,9 +3367,9 @@ struct cmd_struct {
 static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
-	{"clone", module_clone, 0},
+	{"clone", module_clone, SUPPORT_SUPER_PREFIX},
 	{"add", module_add, 0},
-	{"update", module_update, 0},
+	{"update", module_update, SUPPORT_SUPER_PREFIX},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"init", module_init, 0},

2.37.0.rc0.161.g10f37bed90-goog

All invocations of do_get_submodule_displaypath() pass get_super_prefix() as the super_prefix arg, which is exactly the same as get_submodule_displaypath().

Replace all calls to do_get_submodule_displaypath() with get_submodule_displaypath(), and since it has no more callers, remove it.

---
 builtin/submodule--helper.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

index eb84e3a98b..405a4224c5 100644
@@ -118,10 +118,11 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
-static char *do_get_submodule_displaypath(const char *path,
-					  const char *prefix,
-					  const char *super_prefix)
+/* the result should be freed by the caller. */
+static char *get_submodule_displaypath(const char *path, const char *prefix)
 {
+	const char *super_prefix = get_super_prefix();
+
 	if (prefix && super_prefix) {
 		BUG("cannot have prefix '%s' and superprefix '%s'",
 		    prefix, super_prefix);

@@ -137,13 +138,6 @@ static char *do_get_submodule_displaypath(const char *path,
 	}
 }
 
-/* the result should be freed by the caller. */
-static char *get_submodule_displaypath(const char *path, const char *prefix)
-{
-	const char *super_prefix = get_super_prefix();
-	return do_get_submodule_displaypath(path, prefix, super_prefix);
-}
-
 static char *compute_rev_name(const char *sub_path, const char* object_id)
 {
 	struct strbuf sb = STRBUF_INIT;

@@ -488,7 +482,7 @@ static void init_submodule(const char *path, const char *prefix,
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
+	displaypath = get_submodule_displaypath(path, prefix);
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 

@@ -1943,8 +1937,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	enum submodule_update_type update_type;
 	char *key;
 	struct update_data *ud = suc->update_data;
-	char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,
-							 get_super_prefix());
+	char *displaypath = get_submodule_displaypath(ce->name, ud->prefix);
 	struct strbuf sb = STRBUF_INIT;
 	int needs_cloning = 0;
 	int need_free_url = 0;

@@ -2460,9 +2453,8 @@ static int update_submodule(struct update_data *update_data)
 {
 	ensure_core_worktree(update_data->sm_path);
 
-	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
-								update_data->prefix,
-								get_super_prefix());
+	update_data->displaypath = get_submodule_displaypath(
+		update_data->sm_path, update_data->prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,

2.37.0.rc0.161.g10f37bed90-goog

Thanks for the update, Looks like something happened with GGG to send the base series + yours, I confirmed that 1-13/18 are the same as my series in gitster/ab/submodule-cleanup.

I played around with this a bit, and pushed some amends to
https://github.com/avar/git/tree/avar/pr-git-1282/chooglen/submodule/remove-recursive-prefix-v2;
which you should take more as the results of poking around, I think this is fine as-is if Junio would like to pick it up, sans the possible bug mentioned below.

A range-diff of the changes I made follows below, changes:

* Split up the "add tests" to a new commit :) You mentioned "test_expect_failure", I just assumed we'd use test_expect_succes, and then the 2nd commit has this test change:

	diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
	index 9a076e025f3..6cc07460dd2 100755
	--- a/t/t7406-submodule-update.sh
	+++ b/t/t7406-submodule-update.sh
	@@ -1163 +1163 @@ test_expect_success 'submodule update should skip unmerged submodules' '
	-       Skipping unmerged submodule middle//bottom
	+       Skipping unmerged submodule middle/bottom
	@@ -1176 +1176 @@ test_expect_success 'submodule update --recursive skip submodules with strategy=
	-       Skipping submodule '\''../middle/'\''
	+       Skipping submodule '\''middle/bottom'\''

Which I think makes it much easier to explain the change itself, the first commit basically just says "tests for existing behavior, see the next commit for details".

* Used test_cmp instead of "grep -F", makes for easier to grok output :)

* I had to squint a bit at the change from strvec_pushl() to strvec_pushf().

I.e. we're changing both that we're passing "--foo bar" v.s. "--foo=bar" *and* "bar" v.s. "bar/", but as the commit message notes it's just the latter that matters.

Just a nit, but I think it's a bit easier to reason about just changing the one thing we need to change there, although that means giving the "if" braces. This also leaves the "--super-prefix" code consistent with all the other places where we "strvec.*--super-prefix" at the end.

* We can just declare some of the variables and initialize them at the beginning, but couldn't when they were a strbuf.

* Your 18/18 has some odd code formatting of "var =\n\t\tvalue(..." when we usually do "var = value(...\n...".

* You appear to have (and I haven't "fixed" it here) in 13/18 mentioned "While we're fixing the display names, also fix inconsistent quoting of the submodule name", but as the test_cmp shows we appear not to have done that part at all? Is this referring to a forgotten change where we should be saying (note the added '-quotes):

Skipping unmerged submodule 'middle/bottom'

Either that's what you mean & you forgot, or we're missing a test, or I'm misunderstanding it...

I hope this helps, at least some of it...

1: 64c138df196 ! 1: 3d9977006d3 submodule--helper update: use display path helper
@@ Metadata
Author: Glen Choo <[email protected]>

## Commit message ##
- submodule--helper update: use display path helper
+ submodule--helper tests: add missing "display path" coverage

There are two locations in prepare_to_clone_next_submodule() that
- manually calculate the submodule display path, but should just use
- do_get_submodule_displaypath() for consistency.
-
- Do this replacement and reorder the code slightly to avoid computing
- the display path twice.
-
- This code was never tested, and adding tests shows that both these sites
- have been computing the display path incorrectly ever since they were
- introduced in 48308681b0 (git submodule update: have a dedicated helper
- for cloning, 2016-02-29) [1]:
-
- - The first hunk puts a "/" between recursive_prefix and ce->name, but
- recursive_prefix already ends with "/".
- - The second hunk calls relative_path() on recursive_prefix and
- ce->name, but relative_path() only makes sense when both paths share
- the same base directory. This is never the case here:
- - recursive_prefix is the path from the topmost superproject to the
- current submodule
- - ce->name is the path from the root of the current submodule to its
- submodule.
- so, e.g. recursive_prefix="super" and ce->name="submodule" produces
- displayname="../super" instead of "super/submodule".
-
- While we're fixing the display names, also fix inconsistent quoting of
- the submodule name.
-
- [1] I verified this by applying the tests to 48308681b0.
+ manually calculate the submodule display path, as discussed in the
+ next commit the "Skipping" output isn't exactly what we want, but
+ let's test how we behave now, before changing the existing behavior.

-
- ## builtin/submodule--helper.c ##
-@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
- const char *update_string;
- enum submodule_update_type update_type;
- char *key;
-- struct strbuf displaypath_sb = STRBUF_INIT;
- struct strbuf sb = STRBUF_INIT;
-- const char *displaypath = NULL;
-+ char *displaypath;
- int needs_cloning = 0;
- int need_free_url = 0;
-
-+ displaypath = do_get_submodule_displaypath(ce->name,
-+ suc->update_data->prefix,
-+ suc->update_data->recursive_prefix);
-+
- if (ce_stage(ce)) {
-- if (suc->update_data->recursive_prefix)
-- strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name);
-- else
-- strbuf_addstr(&sb, ce->name);
-- strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
-+ strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
- strbuf_addch(out, '\n');
- goto cleanup;
- }
-
- sub = submodule_from_path(the_repository, null_oid(), ce->name);
-
-- if (suc->update_data->recursive_prefix)
-- displaypath = relative_path(suc->update_data->recursive_prefix,
-- ce->name, &displaypath_sb);
-- else
-- displaypath = ce->name;
--
- if (!sub) {
- next_submodule_warn_missing(suc, out, displaypath);
- goto cleanup;
-@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
- "--no-single-branch");
-
- cleanup:
-- strbuf_release(&displaypath_sb);
-+ free(displaypath);
- strbuf_release(&sb);
- if (need_free_url)
- free((void*)url);
+ Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>

## t/t7406-submodule-update.sh ##
@@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets partial clone settings' '
@@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets
+ # because of its unmerged state
+ test_config -C top-cloned submodule.middle.update !true &&
+ git -C top-cloned submodule update --recursive 2>actual.err &&
-+ grep -F "Skipping unmerged submodule middle/bottom" actual.err
++ cat >expect.err <<-\EOF &&
++ Skipping unmerged submodule middle//bottom
++ EOF
++ test_cmp expect.err actual.err
+'
+
+test_expect_success 'submodule update --recursive skip submodules with strategy=none' '
@@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets
+ test_commit -C top-cloned/middle/bottom downstream_commit &&
+ git -C top-cloned/middle config submodule.bottom.update none &&
+ git -C top-cloned submodule update --recursive 2>actual.err &&
-+ grep -F "Skipping submodule ${SQ}middle/bottom${SQ}" actual.err
++ cat >expect.err <<-\EOF &&
++ Skipping submodule '\''../middle/'\''
++ EOF
++ test_cmp expect.err actual.err
+'
+
test_done

-:  ----------- > 2:  d1a47d302ee submodule--helper update: use display path helper
2:  d3e803a4630 ! 3:  a5b30ac94e6 submodule--helper: don't recreate recursive prefix
    @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data
     -	if (update_data->recursive_prefix)
     -		strvec_pushl(args, "--recursive-prefix",
     -			     update_data->recursive_prefix, NULL);
    -+	if (update_data->displaypath)
    -+		strvec_pushf(args, "--recursive-prefix=%s/",
    -+			     update_data->displaypath);
    ++	if (update_data->displaypath) {
    ++		strvec_push(args, "--recursive-prefix");
    ++		strvec_pushf(args, "%s/", update_data->displaypath);
    ++	}
      	if (update_data->quiet)
      		strvec_push(args, "--quiet");
      	if (update_data->force)
3:  1f7cf6ffaf1 = 4:  0ea102882a7 submodule--helper: use correct display path helper
4:  85e65f143b6 = 5:  8085bc83a0c submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags
5:  1d8b6b244dc ! 6:  4c00a1b496a submodule--helper update: use --super-prefix
    @@ builtin/submodule--helper.c: struct submodule_update_clone {
      	enum submodule_update_type update_default;
      	struct object_id suboid;
     @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
    - 
    - 	displaypath = do_get_submodule_displaypath(ce->name,
    - 						   suc->update_data->prefix,
    --						   suc->update_data->recursive_prefix);
    -+						   get_super_prefix());
    - 
    - 	if (ce_stage(ce)) {
    - 		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
    + 	char *key;
    + 	struct update_data *ud = suc->update_data;
    + 	char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,
    +-							 ud->recursive_prefix);
    ++							 get_super_prefix());
    + 	struct strbuf sb = STRBUF_INIT;
    + 	int needs_cloning = 0;
    + 	int need_free_url = 0;
     @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *
      {
      	enum submodule_update_type update_type = update_data->update_default;
      
     -	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
     -	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
    - 	if (update_data->displaypath)
    --		strvec_pushf(args, "--recursive-prefix=%s/",
    -+		strvec_pushf(args, "--super-prefix=%s/",
    - 			     update_data->displaypath);
    + 	if (update_data->displaypath) {
    +-		strvec_push(args, "--recursive-prefix");
    ++		strvec_push(args, "--super-prefix");
    + 		strvec_pushf(args, "%s/", update_data->displaypath);
    + 	}
     +	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
     +	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
      	if (update_data->quiet)
6:  a21e8cd174d ! 7:  639f07e4b84 submodule--helper: remove display path helper
    @@ builtin/submodule--helper.c: static void init_submodule(const char *path, const
      	sub = submodule_from_path(the_repository, null_oid(), path);
      
     @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
    + 	enum submodule_update_type update_type;
    + 	char *key;
    + 	struct update_data *ud = suc->update_data;
    +-	char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,
    +-							 get_super_prefix());
    ++	char *displaypath = get_submodule_displaypath(ce->name, ud->prefix);
    + 	struct strbuf sb = STRBUF_INIT;
      	int needs_cloning = 0;
      	int need_free_url = 0;
    - 
    --	displaypath = do_get_submodule_displaypath(ce->name,
    --						   suc->update_data->prefix,
    --						   get_super_prefix());
    -+	displaypath =
    -+		get_submodule_displaypath(ce->name, suc->update_data->prefix);
    - 
    - 	if (ce_stage(ce)) {
    - 		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
     @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)
      {
      	ensure_core_worktree(update_data->sm_path);

q

Yes, oops. I resent this as v3 (sans your base), but you can ignore that :P

Ah, I see what you meant now. Makes sense.

Both sound good.

I think this is just "char *displaypath" in 2/7 aka d1a47d302e (submodule--helper update: use display path helper, 2022-06-28)? That change makes sense.

Thanks for catching that. I could have sworn that came from "make style".

Yes, this is a genuine error. In v2, I dropped this change (as you suggested) and I neglected to amend the comit message..

Thanks! Yes it helps a lot. If you don't mind, I'll roll your commits + the commit message fix as v4.