patch, 5 partssubmodule: 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/xmqq35g5xmmv.fsf@gitster.g [2] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com [3] https://lore.kernel.org/git/patch-v3-02.12-082e015781e-20220622T142012Z-avarab@gmail.com [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

From: Glen Choo <chooglen@google.com>

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 22 +++++---------
 t/t7406-submodule-update.sh | 59 +++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 15 deletions(-)

On Mon, Jun 27 2022, Glen Choo via GitGitGadget wrote:

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.

Don't init to NULL?...

...because this will either die or return a valid string, and the init is just suppressing compiler flow analysis, no?

Nit: The removal of strbuf_addch() here is bad, we try not to expose
translators to such formatting concerns, so let's leave the \n out of
the message still.

But more imporantly let's also split up the %s to '%s' change up, or perhaps just drop it for now? Isn't it entirely unrelated?

I must admit I didn't spend too much time on the *actual* logic change here, but nothing seems obviously incorrect...

Yeah this does seem a bit repetative...

E.g. here the mixture of "cd" and "-C" is a bit odd, can't we just use:

    git -C middle/bottom ...
    ...
    git -C middle add 

I also wonder if this can't be a for-loop with the right params

Maybe use grep -F with ${SQ} instead?

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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 <chooglen@google.com>

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

From: Glen Choo <chooglen@google.com>

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

From: Glen Choo <chooglen@google.com>

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)

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

On Mon, Jun 27 2022, Glen Choo via GitGitGadget wrote:

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.

I did a double-take at this, but it's just one of these cases where "diff" is being overly helpful in trying to find us the most minimal diff possible :)

I did my own spelunking into --super-prefix recently, and went a bit overboard, I don't think I'll ever submit all of these, but they're in my avar/git github fork:

	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)

So, this is a digressio, but after doing those I figured we could eventually get rid of --super-prefix, but it'll require some more make-things-a-built-in, or make-things-a-library.

But I think out of those perhaps you'd be interested in cherry-picking f445c57490d (submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags, 2022-06-27) before this 4/5? I.e. before adding a new SUPPORT_SUPER_PREFIX flag we can remove it from those commands that don't use it, which clears things up a bit.

The others are all mostly unrelated cleanup, and I'm only noting them in case you're overly curious. A web view for f445c57490d is at: https://github.com/avar/git/commit/f445c57490d

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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 <chooglen@google.com>

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

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/xmqq35g5xmmv.fsf@gitster.g [2] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com [3] https://lore.kernel.org/git/patch-v3-02.12-082e015781e-20220622T142012Z-avarab@gmail.com [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);

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-sh-setup.sh  |  7 -------
 git-submodule.sh | 11 -----------
 2 files changed, 18 deletions(-)

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

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

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-submodule.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

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>]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-submodule.sh | 14 --------------
 1 file changed, 14 deletions(-)

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 5 ++++-
 git-submodule.sh            | 1 -
 2 files changed, 4 insertions(+), 2 deletions(-)

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-submodule.sh            | 3 ---
 t/t7406-submodule-update.sh | 2 +-
 2 files changed, 1 insertion(+), 4 deletions(-)

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 4 ++--
 git-submodule.sh            | 2 +-
 submodule.c                 | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 30 ++++++++++++++++++++++++++++++
 git-submodule.sh            | 14 +++++++++-----
 2 files changed, 39 insertions(+), 5 deletions(-)

From: Glen Choo <chooglen@google.com>

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()".

Signed-off-by: Glen Choo <chooglen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-submodule.sh | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 contrib/subtree/git-subtree.sh | 15 ++++++++++++---
 git-instaweb.sh                |  2 +-
 git-sh-setup.sh                |  9 ---------
 3 files changed, 13 insertions(+), 13 deletions(-)

From: Glen Choo <chooglen@google.com>

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 21 +++++---------
 t/t7406-submodule-update.sh | 56 +++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 14 deletions(-)

From: Glen Choo <chooglen@google.com>

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

From: Glen Choo <chooglen@google.com>

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

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)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

From: Glen Choo <chooglen@google.com>

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

From: Glen Choo <chooglen@google.com>

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)

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

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/xmqq35g5xmmv.fsf@gitster.g [2] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com [3] https://lore.kernel.org/git/patch-v3-02.12-082e015781e-20220622T142012Z-avarab@gmail.com [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

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 21 +++++---------
 t/t7406-submodule-update.sh | 56 +++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 14 deletions(-)

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

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)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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)

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

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/220701.865ykhdboc.gmgdl@evledraar.gmail.com [2] https://lore.kernel.org/git/xmqq35g5xmmv.fsf@gitster.g [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 <chooglen@google.com>
     
      ## 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.
     
         Signed-off-by: Glen Choo <chooglen@google.com>
    -
    - ## 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 <avarab@gmail.com>
     
      ## 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

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.

Signed-off-by: Glen Choo <chooglen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7406-submodule-update.sh | 62 +++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 19 +++++--------------
 t/t7406-submodule-update.sh |  4 ++--
 2 files changed, 7 insertions(+), 16 deletions(-)

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

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)

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

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)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

On Thu, Jun 30 2022, Glen Choo via GitGitGadget wrote:

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 <chooglen@google.com>
     
      ## 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.
     
         Signed-off-by: Glen Choo <chooglen@google.com>
    -
    - ## 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 <avarab@gmail.com>
     
      ## 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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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.