[PATCH 0/5] submodule: remove "--recursive-prefix"

[PATCH 0/5] submodule: remove "--recursive-prefix"

[Glen Choo via GitGitGadget]

= 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
-- 
gitgitgadget

[PATCH 1/5] submodule--helper update: use display path helper

[Glen Choo via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c597df7528e..63c661b26a6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1949,30 +1949,22 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	const char *update_string;
 	enum submodule_update_type update_type;
 	char *key;
-	struct strbuf displaypath_sb = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
-	const char *displaypath = NULL;
+	char *displaypath = NULL;
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
+	displaypath = do_get_submodule_displaypath(ce->name,
+						   suc->update_data->prefix,
+						   suc->update_data->recursive_prefix);
+
 	if (ce_stage(ce)) {
-		if (suc->update_data->recursive_prefix)
-			strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name);
-		else
-			strbuf_addstr(&sb, ce->name);
-		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
-		strbuf_addch(out, '\n');
+		strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);
 		goto cleanup;
 	}
 
 	sub = submodule_from_path(the_repository, null_oid(), ce->name);
 
-	if (suc->update_data->recursive_prefix)
-		displaypath = relative_path(suc->update_data->recursive_prefix,
-					    ce->name, &displaypath_sb);
-	else
-		displaypath = ce->name;
-
 	if (!sub) {
 		next_submodule_warn_missing(suc, out, displaypath);
 		goto cleanup;
@@ -2062,7 +2054,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 					      "--no-single-branch");
 
 cleanup:
-	strbuf_release(&displaypath_sb);
+	free(displaypath);
 	strbuf_release(&sb);
 	if (need_free_url)
 		free((void*)url);
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 43f779d751c..e1dc3b1041b 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1116,4 +1116,63 @@ test_expect_success 'submodule update --filter sets partial clone settings' '
 	test_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter
 '
 
+# NEEDSWORK: Clean up the tests so that we can reuse the test setup.
+# Don't reuse the existing repos because the earlier tests have
+# intentionally disruptive configurations.
+test_expect_success 'setup clean recursive superproject' '
+	git init bottom &&
+	test_commit -C bottom "bottom" &&
+	git init middle &&
+	git -C middle submodule add ../bottom bottom &&
+	git -C middle commit -m "middle" &&
+	git init top &&
+	git -C top submodule add ../middle middle &&
+	git -C top commit -m "top" &&
+	git clone --recurse-submodules top top-clean
+'
+
+test_expect_success 'submodule update should skip unmerged submodules' '
+	test_when_finished "rm -fr top-cloned" &&
+	cp -r top-clean top-cloned &&
+
+	# Create an upstream commit in each repo
+	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 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
+	) &&
+	# 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
+'
+
+test_expect_success 'submodule update --recursive skip submodules with strategy=none' '
+	test_when_finished "rm -fr top-cloned" &&
+	cp -r top-clean top-cloned &&
+
+	test_commit -C top-cloned/middle/bottom downstream_commit &&
+	git -C top-cloned/middle config submodule.bottom.update none &&
+	git -C top-cloned submodule update --recursive 2>actual.err &&
+	grep "Skipping submodule .middle/bottom." actual.err
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 1/5] submodule--helper update: use display path helper

[Ævar Arnfjörð Bjarmason]

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

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

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.

> [...]
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c597df7528e..63c661b26a6 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1949,30 +1949,22 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
>  	const char *update_string;
>  	enum submodule_update_type update_type;
>  	char *key;
> -	struct strbuf displaypath_sb = STRBUF_INIT;
>  	struct strbuf sb = STRBUF_INIT;
> -	const char *displaypath = NULL;
> +	char *displaypath = NULL;

Don't init to NULL?...

>  	int needs_cloning = 0;
>  	int need_free_url = 0;
>  
> +	displaypath = do_get_submodule_displaypath(ce->name,

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

> +						   suc->update_data->prefix,
> +						   suc->update_data->recursive_prefix);
> +
>  	if (ce_stage(ce)) {
> -		if (suc->update_data->recursive_prefix)
> -			strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name);
> -		else
> -			strbuf_addstr(&sb, ce->name);
> -		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
> -		strbuf_addch(out, '\n');
> +		strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);

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?

>  		goto cleanup;
>  	}
>  
>  	sub = submodule_from_path(the_repository, null_oid(), ce->name);
>  
> -	if (suc->update_data->recursive_prefix)
> -		displaypath = relative_path(suc->update_data->recursive_prefix,
> -					    ce->name, &displaypath_sb);
> -	else
> -		displaypath = ce->name;
> -
>  	if (!sub) {
>  		next_submodule_warn_missing(suc, out, displaypath);
>  		goto cleanup;
> @@ -2062,7 +2054,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
>  					      "--no-single-branch");
>  
>  cleanup:
> -	strbuf_release(&displaypath_sb);
> +	free(displaypath);
>  	strbuf_release(&sb);
>  	if (need_free_url)
>  		free((void*)url);

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

> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 43f779d751c..e1dc3b1041b 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -1116,4 +1116,63 @@ test_expect_success 'submodule update --filter sets partial clone settings' '
>  	test_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter
>  '
>  
> +# NEEDSWORK: Clean up the tests so that we can reuse the test setup.
> +# Don't reuse the existing repos because the earlier tests have
> +# intentionally disruptive configurations.

Yeah this does seem a bit repetative...

> +test_expect_success 'setup clean recursive superproject' '
> +	git init bottom &&
> +	test_commit -C bottom "bottom" &&
> +	git init middle &&
> +	git -C middle submodule add ../bottom bottom &&
> +	git -C middle commit -m "middle" &&
> +	git init top &&
> +	git -C top submodule add ../middle middle &&
> +	git -C top commit -m "top" &&
> +	git clone --recurse-submodules top top-clean
> +'
> +
> +test_expect_success 'submodule update should skip unmerged submodules' '
> +	test_when_finished "rm -fr top-cloned" &&
> +	cp -r top-clean top-cloned &&
> +
> +	# Create an upstream commit in each repo
> +	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"
> +	) &&

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

> +
> +	# 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
> +	) &&
> +	# 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

Maybe use grep -F with ${SQ} instead?
> +'
> +
> +test_expect_success 'submodule update --recursive skip submodules with strategy=none' '
> +	test_when_finished "rm -fr top-cloned" &&
> +	cp -r top-clean top-cloned &&
> +
> +	test_commit -C top-cloned/middle/bottom downstream_commit &&
> +	git -C top-cloned/middle config submodule.bottom.update none &&
> +	git -C top-cloned submodule update --recursive 2>actual.err &&
> +	grep "Skipping submodule .middle/bottom." actual.err
> +'
> +
>  test_done

Re: [PATCH 1/5] submodule--helper update: use display path helper

[Glen Choo]

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:

> On Mon, Jun 27 2022, Glen Choo via GitGitGadget wrote:
>
>> 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]:
>
> 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.

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.

>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index 43f779d751c..e1dc3b1041b 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -1116,4 +1116,63 @@ test_expect_success 'submodule update --filter sets partial clone settings' '
>>  	test_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter
>>  '
>>  
>> +# NEEDSWORK: Clean up the tests so that we can reuse the test setup.
>> +# Don't reuse the existing repos because the earlier tests have
>> +# intentionally disruptive configurations.
>
> Yeah this does seem a bit repetative...
>
>> +test_expect_success 'setup clean recursive superproject' '
>> +	git init bottom &&
>> +	test_commit -C bottom "bottom" &&
>> +	git init middle &&
>> +	git -C middle submodule add ../bottom bottom &&
>> +	git -C middle commit -m "middle" &&
>> +	git init top &&
>> +	git -C top submodule add ../middle middle &&
>> +	git -C top commit -m "top" &&
>> +	git clone --recurse-submodules top top-clean
>> +'
>> +
>> +test_expect_success 'submodule update should skip unmerged submodules' '
>> +	test_when_finished "rm -fr top-cloned" &&
>> +	cp -r top-clean top-cloned &&
>> +
>> +	# Create an upstream commit in each repo
>> +	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"
>> +	) &&
>
> 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 


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 also wonder if this can't be a for-loop with the right params

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

[PATCH 2/5] submodule--helper: don't recreate recursive prefix

[Glen Choo via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 63c661b26a6..7a963a967b8 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2401,9 +2401,9 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 {
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
-	if (update_data->recursive_prefix)
-		strvec_pushl(args, "--recursive-prefix",
-			     update_data->recursive_prefix, NULL);
+	if (update_data->displaypath)
+		strvec_pushf(args, "--recursive-prefix=%s/",
+			     update_data->displaypath);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)
@@ -2497,14 +2497,6 @@ static int update_submodule(struct update_data *update_data)
 		struct update_data next = *update_data;
 		int res;
 
-		if (update_data->recursive_prefix)
-			prefixed_path = xstrfmt("%s%s/", update_data->recursive_prefix,
-						update_data->sm_path);
-		else
-			prefixed_path = xstrfmt("%s/", update_data->sm_path);
-
-		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
-								  update_data->prefix);
 		next.prefix = NULL;
 		oidcpy(&next.oid, null_oid());
 		oidcpy(&next.suboid, null_oid());
-- 
gitgitgadget

[PATCH 3/5] submodule--helper: use correct display path helper

[Glen Choo via GitGitGadget]

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

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

[PATCH 4/5] submodule--helper update: use --super-prefix

[Glen Choo via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fa8256526e9..81ea4669aab 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -477,22 +477,18 @@ static int starts_with_dot_dot_slash(const char *const path)
 
 struct init_cb {
 	const char *prefix;
-	const char *superprefix;
 	unsigned int flags;
 };
 #define INIT_CB_INIT { 0 }
 
 static void init_submodule(const char *path, const char *prefix,
-			   const char *superprefix, unsigned int flags)
+			   unsigned int flags)
 {
 	const struct submodule *sub;
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	/* try superprefix from the environment, if it is not passed explicitly */
-	if (!superprefix)
-		superprefix = get_super_prefix();
-	displaypath = do_get_submodule_displaypath(path, prefix, superprefix);
+	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 
@@ -566,7 +562,7 @@ static void init_submodule(const char *path, const char *prefix,
 static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)
 {
 	struct init_cb *info = cb_data;
-	init_submodule(list_item->name, info->prefix, info->superprefix, info->flags);
+	init_submodule(list_item->name, info->prefix, info->flags);
 }
 
 static int module_init(int argc, const char **argv, const char *prefix)
@@ -1880,7 +1876,6 @@ struct submodule_update_clone {
 
 struct update_data {
 	const char *prefix;
-	const char *recursive_prefix;
 	const char *displaypath;
 	const char *update_default;
 	struct object_id suboid;
@@ -1956,7 +1951,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 
 	displaypath = do_get_submodule_displaypath(ce->name,
 						   suc->update_data->prefix,
-						   suc->update_data->recursive_prefix);
+						   get_super_prefix());
 
 	if (ce_stage(ce)) {
 		strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);
@@ -2399,11 +2394,11 @@ static void ensure_core_worktree(const char *path)
 
 static void update_data_to_args(struct update_data *update_data, struct strvec *args)
 {
-	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
-	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->displaypath)
-		strvec_pushf(args, "--recursive-prefix=%s/",
+		strvec_pushf(args, "--super-prefix=%s/",
 			     update_data->displaypath);
+	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
+	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)
@@ -2449,7 +2444,7 @@ static int update_submodule(struct update_data *update_data)
 
 	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
 								update_data->prefix,
-								update_data->recursive_prefix);
+								get_super_prefix());
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,
@@ -2573,10 +2568,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "prefix", &opt.prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
-			   N_("path"),
-			   N_("path into the working tree, across nested "
-			      "submodule boundaries")),
 		OPT_STRING(0, "update", &opt.update_default,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
@@ -2655,7 +2646,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			module_list_active(&list);
 
 		info.prefix = opt.prefix;
-		info.superprefix = opt.recursive_prefix;
 		if (opt.quiet)
 			info.flags |= OPT_QUIET;
 
@@ -3352,9 +3342,9 @@ struct cmd_struct {
 static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
-	{"clone", module_clone, 0},
+	{"clone", module_clone, SUPPORT_SUPER_PREFIX},
 	{"add", module_add, SUPPORT_SUPER_PREFIX},
-	{"update", module_update, 0},
+	{"update", module_update, SUPPORT_SUPER_PREFIX},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
-- 
gitgitgadget

Re: [PATCH 4/5] submodule--helper update: use --super-prefix

[Ævar Arnfjörð Bjarmason]

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

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

>  static void update_data_to_args(struct update_data *update_data, struct strvec *args)
>  {
> -	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
> -	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
>  	if (update_data->displaypath)
> -		strvec_pushf(args, "--recursive-prefix=%s/",
> +		strvec_pushf(args, "--super-prefix=%s/",
>  			     update_data->displaypath);
> +	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
> +	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);

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

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

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

Re: [PATCH 4/5] submodule--helper update: use --super-prefix

[Glen Choo]

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

> On Mon, Jun 27 2022, Glen Choo via GitGitGadget wrote:
>
>> 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 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.

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.

>>  static void update_data_to_args(struct update_data *update_data, struct strvec *args)
>>  {
>> -	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
>> -	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
>>  	if (update_data->displaypath)
>> -		strvec_pushf(args, "--recursive-prefix=%s/",
>> +		strvec_pushf(args, "--super-prefix=%s/",
>>  			     update_data->displaypath);
>> +	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
>> +	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
>
> 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 :)

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

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

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

[PATCH 5/5] submodule--helper: remove display path helper

[Glen Choo via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 81ea4669aab..73b36f47600 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -118,10 +118,11 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
-static char *do_get_submodule_displaypath(const char *path,
-					  const char *prefix,
-					  const char *super_prefix)
+/* the result should be freed by the caller. */
+static char *get_submodule_displaypath(const char *path, const char *prefix)
 {
+	const char *super_prefix = get_super_prefix();
+
 	if (prefix && super_prefix) {
 		BUG("cannot have prefix '%s' and superprefix '%s'",
 		    prefix, super_prefix);
@@ -137,13 +138,6 @@ static char *do_get_submodule_displaypath(const char *path,
 	}
 }
 
-/* the result should be freed by the caller. */
-static char *get_submodule_displaypath(const char *path, const char *prefix)
-{
-	const char *super_prefix = get_super_prefix();
-	return do_get_submodule_displaypath(path, prefix, super_prefix);
-}
-
 static char *compute_rev_name(const char *sub_path, const char* object_id)
 {
 	struct strbuf sb = STRBUF_INIT;
@@ -488,7 +482,7 @@ static void init_submodule(const char *path, const char *prefix,
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
+	displaypath = get_submodule_displaypath(path, prefix);
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 
@@ -1949,9 +1943,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
-	displaypath = do_get_submodule_displaypath(ce->name,
-						   suc->update_data->prefix,
-						   get_super_prefix());
+	displaypath =
+		get_submodule_displaypath(ce->name, suc->update_data->prefix);
 
 	if (ce_stage(ce)) {
 		strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath);
@@ -2442,9 +2435,8 @@ static int update_submodule(struct update_data *update_data)
 {
 	ensure_core_worktree(update_data->sm_path);
 
-	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
-								update_data->prefix,
-								get_super_prefix());
+	update_data->displaypath = get_submodule_displaypath(
+		update_data->sm_path, update_data->prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,
-- 
gitgitgadget

Re: [PATCH 0/5] submodule: remove "--recursive-prefix"

[Glen Choo]

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

> = 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" | | |

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

[PATCH v2 00/18] submodule: remove "--recursive-prefix"

[Glen Choo via GitGitGadget]

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

-- 
gitgitgadget

[PATCH v2 01/18] git-submodule.sh: remove unused sanitize_submodule_env()

[Ævar Arnfjörð Bjarmason via GitGitGadget]

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

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index d92df37e992..ecb60d9e3cb 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -285,13 +285,6 @@ get_author_ident_from_commit () {
 	parse_ident_from_commit author AUTHOR
 }
 
-# Clear repo-local GIT_* environment variables. Useful when switching to
-# another repository (e.g. when entering a submodule). See also the env
-# list in git_connect()
-clear_local_git_env() {
-	unset $(git rev-parse --local-env-vars)
-}
-
 # Generate a virtual base file for a two-file merge. Uses git apply to
 # remove lines from $1 that are not in $2, leaving only common lines.
 create_virtual_base() {
diff --git a/git-submodule.sh b/git-submodule.sh
index fd0b4a2c947..bc436c4ca47 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -56,17 +56,6 @@ isnumber()
 	n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
 }
 
-# Sanitize the local git environment for use within a submodule. We
-# can't simply use clear_local_git_env since we want to preserve some
-# of the settings from GIT_CONFIG_PARAMETERS.
-sanitize_submodule_env()
-{
-	save_config=$GIT_CONFIG_PARAMETERS
-	clear_local_git_env
-	GIT_CONFIG_PARAMETERS=$save_config
-	export GIT_CONFIG_PARAMETERS
-}
-
 #
 # Add a new submodule to the working tree, .gitmodules and the index
 #
-- 
gitgitgadget

[PATCH v2 02/18] git-submodule.sh: remove unused $prefix variable

[Ævar Arnfjörð Bjarmason via GitGitGadget]

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

diff --git a/git-submodule.sh b/git-submodule.sh
index bc436c4ca47..53847bbf6e2 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -41,7 +41,6 @@ files=
 remote=
 nofetch=
 update=
-prefix=
 custom_name=
 depth=
 progress=
@@ -127,7 +126,7 @@ cmd_add()
 		usage
 	fi
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
 }
 
 #
@@ -189,7 +188,7 @@ cmd_init()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
 }
 
 #
@@ -346,7 +345,6 @@ cmd_update()
 		${init:+--init} \
 		${nofetch:+--no-fetch} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
-		${prefix:+--recursive-prefix "$prefix"} \
 		${update:+--update "$update"} \
 		${reference:+"$reference"} \
 		${dissociate:+"--dissociate"} \
-- 
gitgitgadget

[PATCH v2 03/18] git-submodule.sh: make the "$cached" variable a boolean

[Ævar Arnfjörð Bjarmason via GitGitGadget]

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

diff --git a/git-submodule.sh b/git-submodule.sh
index 53847bbf6e2..b99a00d9f84 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -446,7 +446,7 @@ cmd_summary() {
 	do
 		case "$1" in
 		--cached)
-			cached="$1"
+			cached=1
 			;;
 		--files)
 			files="$1"
@@ -583,7 +583,7 @@ do
 		branch="$2"; shift
 		;;
 	--cached)
-		cached="$1"
+		cached=1
 		;;
 	--)
 		break
-- 
gitgitgadget

[PATCH v2 04/18] git-submodule.sh: remove unused top-level "--branch" argument

[Ævar Arnfjörð Bjarmason via GitGitGadget]

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

diff --git a/git-submodule.sh b/git-submodule.sh
index b99a00d9f84..20fc1b620fa 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -574,14 +574,6 @@ do
 	-q|--quiet)
 		GIT_QUIET=1
 		;;
-	-b|--branch)
-		case "$2" in
-		'')
-			usage
-			;;
-		esac
-		branch="$2"; shift
-		;;
 	--cached)
 		cached=1
 		;;
@@ -609,12 +601,6 @@ then
     fi
 fi
 
-# "-b branch" is accepted only by "add" and "set-branch"
-if test -n "$branch" && (test "$command" != add || test "$command" != set-branch)
-then
-	usage
-fi
-
 # "--cached" is accepted only by "status" and "summary"
 if test -n "$cached" && test "$command" != status && test "$command" != summary
 then
-- 
gitgitgadget

[PATCH v2 05/18] submodule--helper: have --require-init imply --init

[Ævar Arnfjörð Bjarmason via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5c77dfcffee..f0702d0cfa2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2618,7 +2618,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "progress", &opt.progress,
 			    N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &opt.require_init,
-			   N_("disallow cloning into non-empty directory")),
+			   N_("disallow cloning into non-empty directory, implies --init")),
 		OPT_BOOL(0, "single-branch", &opt.single_branch,
 			 N_("clone only one branch, HEAD or --branch")),
 		OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
@@ -2642,6 +2642,9 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_update_options,
 			     git_submodule_helper_usage, 0);
 
+	if (opt.require_init)
+		opt.init = 1;
+
 	if (filter_options.choice && !opt.init) {
 		usage_with_options(git_submodule_helper_usage,
 				   module_update_options);
diff --git a/git-submodule.sh b/git-submodule.sh
index 20fc1b620fa..5b9683bf766 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -251,7 +251,6 @@ cmd_update()
 			init=1
 			;;
 		--require-init)
-			init=1
 			require_init=1
 			;;
 		--remote)
-- 
gitgitgadget

[PATCH v2 06/18] submodule update: remove "-v" option

[Ævar Arnfjörð Bjarmason via GitGitGadget]

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

diff --git a/git-submodule.sh b/git-submodule.sh
index 5b9683bf766..0df6b0fc974 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -241,9 +241,6 @@ cmd_update()
 		-q|--quiet)
 			GIT_QUIET=1
 			;;
-		-v)
-			unset GIT_QUIET
-			;;
 		--progress)
 			progress=1
 			;;
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 43f779d751c..06d804e2131 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1074,7 +1074,7 @@ test_expect_success 'submodule update --quiet passes quietness to merge/rebase'
 	 git submodule update --rebase --quiet >out 2>err &&
 	 test_must_be_empty out &&
 	 test_must_be_empty err &&
-	 git submodule update --rebase -v >out 2>err &&
+	 git submodule update --rebase >out 2>err &&
 	 test_file_not_empty out &&
 	 test_must_be_empty err
 	)
-- 
gitgitgadget

[PATCH v2 07/18] submodule--helper: rename "absorb-git-dirs" to "absorbgitdirs"

[Ævar Arnfjörð Bjarmason via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f0702d0cfa2..1a84ae8efd2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2787,7 +2787,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper absorb-git-dirs [<options>] [<path>...]"),
+		N_("git submodule--helper absorbgitdirs [<options>] [<path>...]"),
 		NULL
 	};
 
@@ -3389,7 +3389,7 @@ static struct cmd_struct commands[] = {
 	{"deinit", module_deinit, 0},
 	{"summary", module_summary, SUPPORT_SUPER_PREFIX},
 	{"push-check", push_check, 0},
-	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
+	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 	{"is-active", is_active, 0},
 	{"check-name", check_name, 0},
 	{"config", module_config, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 0df6b0fc974..1c1dc320922 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -552,7 +552,7 @@ cmd_sync()
 
 cmd_absorbgitdirs()
 {
-	git submodule--helper absorb-git-dirs --prefix "$wt_prefix" "$@"
+	git submodule--helper absorbgitdirs --prefix "$wt_prefix" "$@"
 }
 
 # This loop parses the command line arguments to find the
diff --git a/submodule.c b/submodule.c
index 4e299f578f9..2af16c647d5 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2374,7 +2374,7 @@ void absorb_git_dir_into_superproject(const char *path,
 		cp.no_stdin = 1;
 		strvec_pushl(&cp.args, "--super-prefix", sb.buf,
 			     "submodule--helper",
-			     "absorb-git-dirs", NULL);
+			     "absorbgitdirs", NULL);
 		prepare_submodule_repo_env(&cp.env);
 		if (run_command(&cp))
 			die(_("could not recurse into submodule '%s'"), path);
-- 
gitgitgadget

[PATCH v2 08/18] submodule--helper: report "submodule" as our name in some "-h" output

[Ævar Arnfjörð Bjarmason via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1a84ae8efd2..04d2620fce8 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -444,7 +444,7 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper foreach [--quiet] [--recursive] [--] <command>"),
+		N_("git submodule foreach [--quiet] [--recursive] [--] <command>"),
 		NULL
 	};
 
@@ -582,7 +582,7 @@ static int module_init(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper init [<options>] [<path>]"),
+		N_("git submodule init [<options>] [<path>]"),
 		NULL
 	};
 
@@ -1185,7 +1185,7 @@ static int module_summary(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper summary [<options>] [<commit>] [--] [<path>]"),
+		N_("git submodule summary [<options>] [<commit>] [--] [<path>]"),
 		NULL
 	};
 
@@ -1349,7 +1349,7 @@ static int module_sync(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper sync [--quiet] [--recursive] [<path>]"),
+		N_("git submodule sync [--quiet] [--recursive] [<path>]"),
 		NULL
 	};
 
@@ -2787,7 +2787,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper absorbgitdirs [<options>] [<path>...]"),
+		N_("git submodule absorbgitdirs [<options>] [<path>...]"),
 		NULL
 	};
 
@@ -2892,7 +2892,7 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 	const char *const usage[] = {
-		N_("git submodule--helper set-url [--quiet] <path> <newurl>"),
+		N_("git submodule set-url [--quiet] <path> <newurl>"),
 		NULL
 	};
 
@@ -2931,8 +2931,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 	const char *const usage[] = {
-		N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"),
-		N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
+		N_("git submodule set-branch [-q|--quiet] (-d|--default) <path>"),
+		N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
 		NULL
 	};
 
@@ -3276,7 +3276,7 @@ static int module_add(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const usage[] = {
-		N_("git submodule--helper add [<options>] [--] <repository> [<path>]"),
+		N_("git submodule add [<options>] [--] <repository> [<path>]"),
 		NULL
 	};
 
-- 
gitgitgadget

[PATCH v2 09/18] submodule--helper: understand --checkout, --merge and --rebase synonyms

[Ævar Arnfjörð Bjarmason via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 04d2620fce8..53179472d85 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2404,6 +2404,23 @@ static void ensure_core_worktree(const char *path)
 	}
 }
 
+static const char *submodule_update_type_to_label(enum submodule_update_type type)
+{
+	switch (type) {
+	case SM_UPDATE_CHECKOUT:
+		return "checkout";
+	case SM_UPDATE_MERGE:
+		return "merge";
+	case SM_UPDATE_REBASE:
+		return "rebase";
+	case SM_UPDATE_UNSPECIFIED:
+	case SM_UPDATE_NONE:
+	case SM_UPDATE_COMMAND:
+		break;
+	}
+	BUG("unreachable with type %d", type);
+}
+
 static void update_data_to_args(struct update_data *update_data, struct strvec *args)
 {
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
@@ -2582,6 +2599,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	struct update_data opt = UPDATE_DATA_INIT;
 	struct list_objects_filter_options filter_options;
 	int ret;
+	enum submodule_update_type update_type = SM_UPDATE_UNSPECIFIED;
 
 	struct option module_update_options[] = {
 		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),
@@ -2603,6 +2621,15 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "update", &opt.update_default,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
+		OPT_SET_INT(0, "checkout", &update_type,
+			N_("use the 'checkout' update strategy (default)"),
+			SM_UPDATE_CHECKOUT),
+		OPT_SET_INT('m', "merge", &update_type,
+			N_("use the 'merge' update strategy"),
+			SM_UPDATE_MERGE),
+		OPT_SET_INT('r', "rebase", &update_type,
+			N_("use the 'rebase' update strategy"),
+			SM_UPDATE_REBASE),
 		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
 			   N_("reference repository")),
 		OPT_BOOL(0, "dissociate", &opt.dissociate,
@@ -2652,6 +2679,9 @@ static int module_update(int argc, const char **argv, const char *prefix)
 
 	opt.filter_options = &filter_options;
 
+	if (update_type != SM_UPDATE_UNSPECIFIED)
+		opt.update_default = submodule_update_type_to_label(update_type);
+
 	if (opt.update_default)
 		if (parse_submodule_update_strategy(opt.update_default,
 						    &opt.update_strategy) < 0)
diff --git a/git-submodule.sh b/git-submodule.sh
index 1c1dc320922..7fc7119fb21 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -40,7 +40,9 @@ require_init=
 files=
 remote=
 nofetch=
-update=
+rebase=
+merge=
+checkout=
 custom_name=
 depth=
 progress=
@@ -260,7 +262,7 @@ cmd_update()
 			force=$1
 			;;
 		-r|--rebase)
-			update="rebase"
+			rebase=1
 			;;
 		--reference)
 			case "$2" in '') usage ;; esac
@@ -274,13 +276,13 @@ cmd_update()
 			dissociate=1
 			;;
 		-m|--merge)
-			update="merge"
+			merge=1
 			;;
 		--recursive)
 			recursive=1
 			;;
 		--checkout)
-			update="checkout"
+			checkout=1
 			;;
 		--recommend-shallow)
 			recommend_shallow="--recommend-shallow"
@@ -341,7 +343,9 @@ cmd_update()
 		${init:+--init} \
 		${nofetch:+--no-fetch} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
-		${update:+--update "$update"} \
+		${rebase:+--rebase} \
+		${merge:+--merge} \
+		${checkout:+--checkout} \
 		${reference:+"$reference"} \
 		${dissociate:+"--dissociate"} \
 		${depth:+"$depth"} \
-- 
gitgitgadget

[PATCH v2 10/18] submodule--helper: eliminate internal "--update" option

[Glen Choo via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53179472d85..389b900602f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1818,7 +1818,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 static void determine_submodule_update_strategy(struct repository *r,
 						int just_cloned,
 						const char *path,
-						const char *update,
+						enum submodule_update_type update,
 						struct submodule_update_strategy *out)
 {
 	const struct submodule *sub = submodule_from_path(r, null_oid(), path);
@@ -1828,9 +1828,7 @@ static void determine_submodule_update_strategy(struct repository *r,
 	key = xstrfmt("submodule.%s.update", sub->name);
 
 	if (update) {
-		if (parse_submodule_update_strategy(update, out) < 0)
-			die(_("Invalid update mode '%s' for submodule path '%s'"),
-				update, path);
+		out->type = update;
 	} else if (!repo_config_get_string_tmp(r, key, &val)) {
 		if (parse_submodule_update_strategy(val, out) < 0)
 			die(_("Invalid update mode '%s' configured for submodule path '%s'"),
@@ -1882,7 +1880,7 @@ struct update_data {
 	const char *prefix;
 	const char *recursive_prefix;
 	const char *displaypath;
-	const char *update_default;
+	enum submodule_update_type update_default;
 	struct object_id suboid;
 	struct string_list references;
 	struct submodule_update_strategy update_strategy;
@@ -2423,6 +2421,8 @@ static const char *submodule_update_type_to_label(enum submodule_update_type typ
 
 static void update_data_to_args(struct update_data *update_data, struct strvec *args)
 {
+	enum submodule_update_type update_type = update_data->update_default;
+
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->recursive_prefix)
@@ -2446,8 +2446,10 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 		strvec_push(args, "--require-init");
 	if (update_data->depth)
 		strvec_pushf(args, "--depth=%d", update_data->depth);
-	if (update_data->update_default)
-		strvec_pushl(args, "--update", update_data->update_default, NULL);
+	if (update_type != SM_UPDATE_UNSPECIFIED)
+		strvec_pushf(args, "--%s",
+			     submodule_update_type_to_label(update_type));
+
 	if (update_data->references.nr) {
 		struct string_list_item *item;
 		for_each_string_list_item(item, &update_data->references)
@@ -2599,7 +2601,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	struct update_data opt = UPDATE_DATA_INIT;
 	struct list_objects_filter_options filter_options;
 	int ret;
-	enum submodule_update_type update_type = SM_UPDATE_UNSPECIFIED;
 
 	struct option module_update_options[] = {
 		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),
@@ -2618,16 +2619,13 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			   N_("path"),
 			   N_("path into the working tree, across nested "
 			      "submodule boundaries")),
-		OPT_STRING(0, "update", &opt.update_default,
-			   N_("string"),
-			   N_("rebase, merge, checkout or none")),
-		OPT_SET_INT(0, "checkout", &update_type,
+		OPT_SET_INT(0, "checkout", &opt.update_default,
 			N_("use the 'checkout' update strategy (default)"),
 			SM_UPDATE_CHECKOUT),
-		OPT_SET_INT('m', "merge", &update_type,
+		OPT_SET_INT('m', "merge", &opt.update_default,
 			N_("use the 'merge' update strategy"),
 			SM_UPDATE_MERGE),
-		OPT_SET_INT('r', "rebase", &update_type,
+		OPT_SET_INT('r', "rebase", &opt.update_default,
 			N_("use the 'rebase' update strategy"),
 			SM_UPDATE_REBASE),
 		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
@@ -2679,13 +2677,8 @@ static int module_update(int argc, const char **argv, const char *prefix)
 
 	opt.filter_options = &filter_options;
 
-	if (update_type != SM_UPDATE_UNSPECIFIED)
-		opt.update_default = submodule_update_type_to_label(update_type);
-
 	if (opt.update_default)
-		if (parse_submodule_update_strategy(opt.update_default,
-						    &opt.update_strategy) < 0)
-			die(_("bad value for update parameter"));
+		opt.update_strategy.type = opt.update_default;
 
 	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0) {
 		list_objects_filter_release(&filter_options);
-- 
gitgitgadget

[PATCH v2 11/18] git-submodule.sh: use "$quiet", not "$GIT_QUIET"

[Ævar Arnfjörð Bjarmason via GitGitGadget]

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

diff --git a/git-submodule.sh b/git-submodule.sh
index 7fc7119fb21..5e5d21c010f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -30,6 +30,7 @@ GIT_PROTOCOL_FROM_USER=0
 export GIT_PROTOCOL_FROM_USER
 
 command=
+quiet=
 branch=
 force=
 reference=
@@ -80,7 +81,7 @@ cmd_add()
 			force=$1
 			;;
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--progress)
 			progress=1
@@ -128,7 +129,7 @@ cmd_add()
 		usage
 	fi
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add ${quiet:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
 }
 
 #
@@ -144,7 +145,7 @@ cmd_foreach()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--recursive)
 			recursive=1
@@ -159,7 +160,7 @@ cmd_foreach()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach ${quiet:+--quiet} ${recursive:+--recursive} -- "$@"
 }
 
 #
@@ -174,7 +175,7 @@ cmd_init()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--)
 			shift
@@ -190,7 +191,7 @@ cmd_init()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${quiet:+--quiet} -- "$@"
 }
 
 #
@@ -207,7 +208,7 @@ cmd_deinit()
 			force=$1
 			;;
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--all)
 			deinit_all=t
@@ -226,7 +227,7 @@ cmd_deinit()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${quiet:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@"
 }
 
 #
@@ -241,7 +242,7 @@ cmd_update()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--progress)
 			progress=1
@@ -335,7 +336,7 @@ cmd_update()
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
-		${GIT_QUIET:+--quiet} \
+		${quiet:+--quiet} \
 		${force:+--force} \
 		${progress:+"--progress"} \
 		${remote:+--remote} \
@@ -396,7 +397,7 @@ cmd_set_branch() {
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch "$branch"} ${default:+--default} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch ${quiet:+--quiet} ${branch:+--branch "$branch"} ${default:+--default} -- "$@"
 }
 
 #
@@ -409,7 +410,7 @@ cmd_set_url() {
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--)
 			shift
@@ -425,7 +426,7 @@ cmd_set_url() {
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url ${quiet:+--quiet} -- "$@"
 }
 
 #
@@ -496,7 +497,7 @@ cmd_status()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--cached)
 			cached=1
@@ -518,7 +519,7 @@ cmd_status()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status ${quiet:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@"
 }
 #
 # Sync remote urls for submodules
@@ -531,7 +532,7 @@ cmd_sync()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			shift
 			;;
 		--recursive)
@@ -551,7 +552,7 @@ cmd_sync()
 		esac
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync ${quiet:+--quiet} ${recursive:+--recursive} -- "$@"
 }
 
 cmd_absorbgitdirs()
@@ -572,7 +573,7 @@ do
 		command=$1
 		;;
 	-q|--quiet)
-		GIT_QUIET=1
+		quiet=1
 		;;
 	--cached)
 		cached=1
-- 
gitgitgadget

[PATCH v2 12/18] git-sh-setup.sh: remove "say" function, change last users

[Ævar Arnfjörð Bjarmason via GitGitGadget]

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

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 1af1d9653e9..7562a395c24 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -50,6 +50,14 @@ m,message=    use the given message as the commit message for the merge commit
 
 indent=0
 
+# Usage: say [MSG...]
+say () {
+	if test -z "$arg_quiet"
+	then
+		printf '%s\n' "$*"
+	fi
+}
+
 # Usage: debug [MSG...]
 debug () {
 	if test -n "$arg_debug"
@@ -60,7 +68,7 @@ debug () {
 
 # Usage: progress [MSG...]
 progress () {
-	if test -z "$GIT_QUIET"
+	if test -z "$arg_quiet"
 	then
 		if test -z "$arg_debug"
 		then
@@ -146,6 +154,7 @@ main () {
 	eval "$set_args"
 
 	# Begin "real" flag parsing.
+	arg_quiet=
 	arg_debug=
 	arg_prefix=
 	arg_split_branch=
@@ -161,7 +170,7 @@ main () {
 
 		case "$opt" in
 		-q)
-			GIT_QUIET=1
+			arg_quiet=1
 			;;
 		-d)
 			arg_debug=1
@@ -252,7 +261,7 @@ main () {
 	dir="$(dirname "$arg_prefix/.")"
 
 	debug "command: {$arg_command}"
-	debug "quiet: {$GIT_QUIET}"
+	debug "quiet: {$arg_quiet}"
 	debug "dir: {$dir}"
 	debug "opts: {$*}"
 	debug
diff --git a/git-instaweb.sh b/git-instaweb.sh
index 4349566c891..c68f49454cd 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -102,7 +102,7 @@ resolve_full_httpd () {
 
 start_httpd () {
 	if test -f "$fqgitdir/pid"; then
-		say "Instance already running. Restarting..."
+		echo "Instance already running. Restarting..."
 		stop_httpd
 	fi
 
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index ecb60d9e3cb..ce273fe0e48 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -57,15 +57,6 @@ die_with_status () {
 	exit "$status"
 }
 
-GIT_QUIET=
-
-say () {
-	if test -z "$GIT_QUIET"
-	then
-		printf '%s\n' "$*"
-	fi
-}
-
 if test -n "$OPTIONS_SPEC"; then
 	usage() {
 		"$0" -h
-- 
gitgitgadget

[PATCH v2 13/18] submodule--helper update: use display path helper

[Glen Choo via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 389b900602f..db2d5ab7998 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1947,30 +1947,23 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	const char *update_string;
 	enum submodule_update_type update_type;
 	char *key;
-	struct strbuf displaypath_sb = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
-	const char *displaypath = NULL;
+	char *displaypath;
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
+	displaypath = do_get_submodule_displaypath(ce->name,
+						   suc->update_data->prefix,
+						   suc->update_data->recursive_prefix);
+
 	if (ce_stage(ce)) {
-		if (suc->update_data->recursive_prefix)
-			strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name);
-		else
-			strbuf_addstr(&sb, ce->name);
-		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
+		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
 		strbuf_addch(out, '\n');
 		goto cleanup;
 	}
 
 	sub = submodule_from_path(the_repository, null_oid(), ce->name);
 
-	if (suc->update_data->recursive_prefix)
-		displaypath = relative_path(suc->update_data->recursive_prefix,
-					    ce->name, &displaypath_sb);
-	else
-		displaypath = ce->name;
-
 	if (!sub) {
 		next_submodule_warn_missing(suc, out, displaypath);
 		goto cleanup;
@@ -2060,7 +2053,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 					      "--no-single-branch");
 
 cleanup:
-	strbuf_release(&displaypath_sb);
+	free(displaypath);
 	strbuf_release(&sb);
 	if (need_free_url)
 		free((void*)url);
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 06d804e2131..f0408c5cc4a 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1116,4 +1116,60 @@ test_expect_success 'submodule update --filter sets partial clone settings' '
 	test_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter
 '
 
+# NEEDSWORK: Clean up the tests so that we can reuse the test setup.
+# Don't reuse the existing repos because the earlier tests have
+# intentionally disruptive configurations.
+test_expect_success 'setup clean recursive superproject' '
+	git init bottom &&
+	test_commit -C bottom "bottom" &&
+	git init middle &&
+	git -C middle submodule add ../bottom bottom &&
+	git -C middle commit -m "middle" &&
+	git init top &&
+	git -C top submodule add ../middle middle &&
+	git -C top commit -m "top" &&
+	git clone --recurse-submodules top top-clean
+'
+
+test_expect_success 'submodule update should skip unmerged submodules' '
+	test_when_finished "rm -fr top-cloned" &&
+	cp -r top-clean top-cloned &&
+
+	# Create an upstream commit in each repo, starting with bottom
+	test_commit -C bottom upstream_commit &&
+	# Create middle commit
+	git -C middle/bottom fetch &&
+	git -C middle/bottom checkout -f FETCH_HEAD &&
+	git -C middle add bottom &&
+	git -C middle commit -m "upstream_commit" &&
+	# Create top commit
+	git -C top/middle fetch &&
+	git -C top/middle checkout -f FETCH_HEAD &&
+	git -C top add middle &&
+	git -C top commit -m "upstream_commit" &&
+
+	# Create a downstream conflict
+	test_commit -C top-cloned/middle/bottom downstream_commit &&
+	git -C top-cloned/middle add bottom &&
+	git -C top-cloned/middle commit -m "downstream_commit" &&
+	git -C top-cloned/middle fetch --recurse-submodules origin &&
+	test_must_fail git -C top-cloned/middle merge origin/main &&
+
+	# Make the update of "middle" a no-op, otherwise we error out
+	# because of its unmerged state
+	test_config -C top-cloned submodule.middle.update !true &&
+	git -C top-cloned submodule update --recursive 2>actual.err &&
+	grep -F "Skipping unmerged submodule middle/bottom" actual.err
+'
+
+test_expect_success 'submodule update --recursive skip submodules with strategy=none' '
+	test_when_finished "rm -fr top-cloned" &&
+	cp -r top-clean top-cloned &&
+
+	test_commit -C top-cloned/middle/bottom downstream_commit &&
+	git -C top-cloned/middle config submodule.bottom.update none &&
+	git -C top-cloned submodule update --recursive 2>actual.err &&
+	grep -F "Skipping submodule ${SQ}middle/bottom${SQ}" actual.err
+'
+
 test_done
-- 
gitgitgadget

[PATCH v2 14/18] submodule--helper: don't recreate recursive prefix

[Glen Choo via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index db2d5ab7998..c104b1a0236 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2418,9 +2418,9 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
-	if (update_data->recursive_prefix)
-		strvec_pushl(args, "--recursive-prefix",
-			     update_data->recursive_prefix, NULL);
+	if (update_data->displaypath)
+		strvec_pushf(args, "--recursive-prefix=%s/",
+			     update_data->displaypath);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)
@@ -2516,14 +2516,6 @@ static int update_submodule(struct update_data *update_data)
 		struct update_data next = *update_data;
 		int res;
 
-		if (update_data->recursive_prefix)
-			prefixed_path = xstrfmt("%s%s/", update_data->recursive_prefix,
-						update_data->sm_path);
-		else
-			prefixed_path = xstrfmt("%s/", update_data->sm_path);
-
-		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
-								  update_data->prefix);
 		next.prefix = NULL;
 		oidcpy(&next.oid, null_oid());
 		oidcpy(&next.suboid, null_oid());
-- 
gitgitgadget

[PATCH v2 15/18] submodule--helper: use correct display path helper

[Glen Choo via GitGitGadget]

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

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

[PATCH v2 16/18] submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags

[Ævar Arnfjörð Bjarmason via GitGitGadget]

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

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

[PATCH v2 17/18] submodule--helper update: use --super-prefix

[Glen Choo via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 360309195fb..41177332dd8 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -477,22 +477,18 @@ static int starts_with_dot_dot_slash(const char *const path)
 
 struct init_cb {
 	const char *prefix;
-	const char *superprefix;
 	unsigned int flags;
 };
 #define INIT_CB_INIT { 0 }
 
 static void init_submodule(const char *path, const char *prefix,
-			   const char *superprefix, unsigned int flags)
+			   unsigned int flags)
 {
 	const struct submodule *sub;
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	/* try superprefix from the environment, if it is not passed explicitly */
-	if (!superprefix)
-		superprefix = get_super_prefix();
-	displaypath = do_get_submodule_displaypath(path, prefix, superprefix);
+	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 
@@ -566,7 +562,7 @@ static void init_submodule(const char *path, const char *prefix,
 static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)
 {
 	struct init_cb *info = cb_data;
-	init_submodule(list_item->name, info->prefix, info->superprefix, info->flags);
+	init_submodule(list_item->name, info->prefix, info->flags);
 }
 
 static int module_init(int argc, const char **argv, const char *prefix)
@@ -1878,7 +1874,6 @@ struct submodule_update_clone {
 
 struct update_data {
 	const char *prefix;
-	const char *recursive_prefix;
 	const char *displaypath;
 	enum submodule_update_type update_default;
 	struct object_id suboid;
@@ -1954,7 +1949,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 
 	displaypath = do_get_submodule_displaypath(ce->name,
 						   suc->update_data->prefix,
-						   suc->update_data->recursive_prefix);
+						   get_super_prefix());
 
 	if (ce_stage(ce)) {
 		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
@@ -2416,11 +2411,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 {
 	enum submodule_update_type update_type = update_data->update_default;
 
-	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
-	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->displaypath)
-		strvec_pushf(args, "--recursive-prefix=%s/",
+		strvec_pushf(args, "--super-prefix=%s/",
 			     update_data->displaypath);
+	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
+	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)
@@ -2468,7 +2463,7 @@ static int update_submodule(struct update_data *update_data)
 
 	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
 								update_data->prefix,
-								update_data->recursive_prefix);
+								get_super_prefix());
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,
@@ -2592,10 +2587,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "prefix", &opt.prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
-			   N_("path"),
-			   N_("path into the working tree, across nested "
-			      "submodule boundaries")),
 		OPT_SET_INT(0, "checkout", &opt.update_default,
 			N_("use the 'checkout' update strategy (default)"),
 			SM_UPDATE_CHECKOUT),
@@ -2681,7 +2672,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			module_list_active(&list);
 
 		info.prefix = opt.prefix;
-		info.superprefix = opt.recursive_prefix;
 		if (opt.quiet)
 			info.flags |= OPT_QUIET;
 
@@ -3378,9 +3368,9 @@ struct cmd_struct {
 static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
-	{"clone", module_clone, 0},
+	{"clone", module_clone, SUPPORT_SUPER_PREFIX},
 	{"add", module_add, 0},
-	{"update", module_update, 0},
+	{"update", module_update, SUPPORT_SUPER_PREFIX},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"init", module_init, 0},
-- 
gitgitgadget

[PATCH v2 18/18] submodule--helper: remove display path helper

[Glen Choo via GitGitGadget]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 41177332dd8..3e0ceb52589 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -118,10 +118,11 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
-static char *do_get_submodule_displaypath(const char *path,
-					  const char *prefix,
-					  const char *super_prefix)
+/* the result should be freed by the caller. */
+static char *get_submodule_displaypath(const char *path, const char *prefix)
 {
+	const char *super_prefix = get_super_prefix();
+
 	if (prefix && super_prefix) {
 		BUG("cannot have prefix '%s' and superprefix '%s'",
 		    prefix, super_prefix);
@@ -137,13 +138,6 @@ static char *do_get_submodule_displaypath(const char *path,
 	}
 }
 
-/* the result should be freed by the caller. */
-static char *get_submodule_displaypath(const char *path, const char *prefix)
-{
-	const char *super_prefix = get_super_prefix();
-	return do_get_submodule_displaypath(path, prefix, super_prefix);
-}
-
 static char *compute_rev_name(const char *sub_path, const char* object_id)
 {
 	struct strbuf sb = STRBUF_INIT;
@@ -488,7 +482,7 @@ static void init_submodule(const char *path, const char *prefix,
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
+	displaypath = get_submodule_displaypath(path, prefix);
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 
@@ -1947,9 +1941,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
-	displaypath = do_get_submodule_displaypath(ce->name,
-						   suc->update_data->prefix,
-						   get_super_prefix());
+	displaypath =
+		get_submodule_displaypath(ce->name, suc->update_data->prefix);
 
 	if (ce_stage(ce)) {
 		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
@@ -2461,9 +2454,8 @@ static int update_submodule(struct update_data *update_data)
 {
 	ensure_core_worktree(update_data->sm_path);
 
-	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
-								update_data->prefix,
-								get_super_prefix());
+	update_data->displaypath = get_submodule_displaypath(
+		update_data->sm_path, update_data->prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,
-- 
gitgitgadget

Re: [PATCH v2 00/18] submodule: remove "--recursive-prefix"

[Glen Choo]

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

> 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

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

[PATCH v3 0/6] submodule: remove "--recursive-prefix"

[Glen Choo]

(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
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v3 1/6] submodule--helper update: use display path helper

[Glen Choo]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 389b900602..db2d5ab799 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1947,30 +1947,23 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	const char *update_string;
 	enum submodule_update_type update_type;
 	char *key;
-	struct strbuf displaypath_sb = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
-	const char *displaypath = NULL;
+	char *displaypath;
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
+	displaypath = do_get_submodule_displaypath(ce->name,
+						   suc->update_data->prefix,
+						   suc->update_data->recursive_prefix);
+
 	if (ce_stage(ce)) {
-		if (suc->update_data->recursive_prefix)
-			strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name);
-		else
-			strbuf_addstr(&sb, ce->name);
-		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
+		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
 		strbuf_addch(out, '\n');
 		goto cleanup;
 	}
 
 	sub = submodule_from_path(the_repository, null_oid(), ce->name);
 
-	if (suc->update_data->recursive_prefix)
-		displaypath = relative_path(suc->update_data->recursive_prefix,
-					    ce->name, &displaypath_sb);
-	else
-		displaypath = ce->name;
-
 	if (!sub) {
 		next_submodule_warn_missing(suc, out, displaypath);
 		goto cleanup;
@@ -2060,7 +2053,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 					      "--no-single-branch");
 
 cleanup:
-	strbuf_release(&displaypath_sb);
+	free(displaypath);
 	strbuf_release(&sb);
 	if (need_free_url)
 		free((void*)url);
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 06d804e213..f0408c5cc4 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1116,4 +1116,60 @@ test_expect_success 'submodule update --filter sets partial clone settings' '
 	test_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter
 '
 
+# NEEDSWORK: Clean up the tests so that we can reuse the test setup.
+# Don't reuse the existing repos because the earlier tests have
+# intentionally disruptive configurations.
+test_expect_success 'setup clean recursive superproject' '
+	git init bottom &&
+	test_commit -C bottom "bottom" &&
+	git init middle &&
+	git -C middle submodule add ../bottom bottom &&
+	git -C middle commit -m "middle" &&
+	git init top &&
+	git -C top submodule add ../middle middle &&
+	git -C top commit -m "top" &&
+	git clone --recurse-submodules top top-clean
+'
+
+test_expect_success 'submodule update should skip unmerged submodules' '
+	test_when_finished "rm -fr top-cloned" &&
+	cp -r top-clean top-cloned &&
+
+	# Create an upstream commit in each repo, starting with bottom
+	test_commit -C bottom upstream_commit &&
+	# Create middle commit
+	git -C middle/bottom fetch &&
+	git -C middle/bottom checkout -f FETCH_HEAD &&
+	git -C middle add bottom &&
+	git -C middle commit -m "upstream_commit" &&
+	# Create top commit
+	git -C top/middle fetch &&
+	git -C top/middle checkout -f FETCH_HEAD &&
+	git -C top add middle &&
+	git -C top commit -m "upstream_commit" &&
+
+	# Create a downstream conflict
+	test_commit -C top-cloned/middle/bottom downstream_commit &&
+	git -C top-cloned/middle add bottom &&
+	git -C top-cloned/middle commit -m "downstream_commit" &&
+	git -C top-cloned/middle fetch --recurse-submodules origin &&
+	test_must_fail git -C top-cloned/middle merge origin/main &&
+
+	# Make the update of "middle" a no-op, otherwise we error out
+	# because of its unmerged state
+	test_config -C top-cloned submodule.middle.update !true &&
+	git -C top-cloned submodule update --recursive 2>actual.err &&
+	grep -F "Skipping unmerged submodule middle/bottom" actual.err
+'
+
+test_expect_success 'submodule update --recursive skip submodules with strategy=none' '
+	test_when_finished "rm -fr top-cloned" &&
+	cp -r top-clean top-cloned &&
+
+	test_commit -C top-cloned/middle/bottom downstream_commit &&
+	git -C top-cloned/middle config submodule.bottom.update none &&
+	git -C top-cloned submodule update --recursive 2>actual.err &&
+	grep -F "Skipping submodule ${SQ}middle/bottom${SQ}" actual.err
+'
+
 test_done
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v3 2/6] submodule--helper: don't recreate recursive prefix

[Glen Choo]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index db2d5ab799..c104b1a023 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2418,9 +2418,9 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
-	if (update_data->recursive_prefix)
-		strvec_pushl(args, "--recursive-prefix",
-			     update_data->recursive_prefix, NULL);
+	if (update_data->displaypath)
+		strvec_pushf(args, "--recursive-prefix=%s/",
+			     update_data->displaypath);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)
@@ -2516,14 +2516,6 @@ static int update_submodule(struct update_data *update_data)
 		struct update_data next = *update_data;
 		int res;
 
-		if (update_data->recursive_prefix)
-			prefixed_path = xstrfmt("%s%s/", update_data->recursive_prefix,
-						update_data->sm_path);
-		else
-			prefixed_path = xstrfmt("%s/", update_data->sm_path);
-
-		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
-								  update_data->prefix);
 		next.prefix = NULL;
 		oidcpy(&next.oid, null_oid());
 		oidcpy(&next.suboid, null_oid());
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v3 3/6] submodule--helper: use correct display path helper

[Glen Choo]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c104b1a023..aad431f898 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2464,19 +2464,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 
 static int update_submodule(struct update_data *update_data)
 {
-	char *prefixed_path;
-
 	ensure_core_worktree(update_data->sm_path);
 
-	if (update_data->recursive_prefix)
-		prefixed_path = xstrfmt("%s%s", update_data->recursive_prefix,
-					update_data->sm_path);
-	else
-		prefixed_path = xstrdup(update_data->sm_path);
-
-	update_data->displaypath = get_submodule_displaypath(prefixed_path,
-							     update_data->prefix);
-	free(prefixed_path);
+	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
+								update_data->prefix,
+								update_data->recursive_prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v3 4/6] submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags

[Glen Choo]

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

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

[PATCH v3 5/6] submodule--helper update: use --super-prefix

[Glen Choo]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 360309195f..41177332dd 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -477,22 +477,18 @@ static int starts_with_dot_dot_slash(const char *const path)
 
 struct init_cb {
 	const char *prefix;
-	const char *superprefix;
 	unsigned int flags;
 };
 #define INIT_CB_INIT { 0 }
 
 static void init_submodule(const char *path, const char *prefix,
-			   const char *superprefix, unsigned int flags)
+			   unsigned int flags)
 {
 	const struct submodule *sub;
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	/* try superprefix from the environment, if it is not passed explicitly */
-	if (!superprefix)
-		superprefix = get_super_prefix();
-	displaypath = do_get_submodule_displaypath(path, prefix, superprefix);
+	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 
@@ -566,7 +562,7 @@ static void init_submodule(const char *path, const char *prefix,
 static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)
 {
 	struct init_cb *info = cb_data;
-	init_submodule(list_item->name, info->prefix, info->superprefix, info->flags);
+	init_submodule(list_item->name, info->prefix, info->flags);
 }
 
 static int module_init(int argc, const char **argv, const char *prefix)
@@ -1878,7 +1874,6 @@ struct submodule_update_clone {
 
 struct update_data {
 	const char *prefix;
-	const char *recursive_prefix;
 	const char *displaypath;
 	enum submodule_update_type update_default;
 	struct object_id suboid;
@@ -1954,7 +1949,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 
 	displaypath = do_get_submodule_displaypath(ce->name,
 						   suc->update_data->prefix,
-						   suc->update_data->recursive_prefix);
+						   get_super_prefix());
 
 	if (ce_stage(ce)) {
 		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
@@ -2416,11 +2411,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 {
 	enum submodule_update_type update_type = update_data->update_default;
 
-	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
-	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->displaypath)
-		strvec_pushf(args, "--recursive-prefix=%s/",
+		strvec_pushf(args, "--super-prefix=%s/",
 			     update_data->displaypath);
+	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
+	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)
@@ -2468,7 +2463,7 @@ static int update_submodule(struct update_data *update_data)
 
 	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
 								update_data->prefix,
-								update_data->recursive_prefix);
+								get_super_prefix());
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,
@@ -2592,10 +2587,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "prefix", &opt.prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
-			   N_("path"),
-			   N_("path into the working tree, across nested "
-			      "submodule boundaries")),
 		OPT_SET_INT(0, "checkout", &opt.update_default,
 			N_("use the 'checkout' update strategy (default)"),
 			SM_UPDATE_CHECKOUT),
@@ -2681,7 +2672,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			module_list_active(&list);
 
 		info.prefix = opt.prefix;
-		info.superprefix = opt.recursive_prefix;
 		if (opt.quiet)
 			info.flags |= OPT_QUIET;
 
@@ -3378,9 +3368,9 @@ struct cmd_struct {
 static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
-	{"clone", module_clone, 0},
+	{"clone", module_clone, SUPPORT_SUPER_PREFIX},
 	{"add", module_add, 0},
-	{"update", module_update, 0},
+	{"update", module_update, SUPPORT_SUPER_PREFIX},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"init", module_init, 0},
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v3 6/6] submodule--helper: remove display path helper

[Glen Choo]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 41177332dd..3e0ceb5258 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -118,10 +118,11 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
-static char *do_get_submodule_displaypath(const char *path,
-					  const char *prefix,
-					  const char *super_prefix)
+/* the result should be freed by the caller. */
+static char *get_submodule_displaypath(const char *path, const char *prefix)
 {
+	const char *super_prefix = get_super_prefix();
+
 	if (prefix && super_prefix) {
 		BUG("cannot have prefix '%s' and superprefix '%s'",
 		    prefix, super_prefix);
@@ -137,13 +138,6 @@ static char *do_get_submodule_displaypath(const char *path,
 	}
 }
 
-/* the result should be freed by the caller. */
-static char *get_submodule_displaypath(const char *path, const char *prefix)
-{
-	const char *super_prefix = get_super_prefix();
-	return do_get_submodule_displaypath(path, prefix, super_prefix);
-}
-
 static char *compute_rev_name(const char *sub_path, const char* object_id)
 {
 	struct strbuf sb = STRBUF_INIT;
@@ -488,7 +482,7 @@ static void init_submodule(const char *path, const char *prefix,
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
+	displaypath = get_submodule_displaypath(path, prefix);
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 
@@ -1947,9 +1941,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
-	displaypath = do_get_submodule_displaypath(ce->name,
-						   suc->update_data->prefix,
-						   get_super_prefix());
+	displaypath =
+		get_submodule_displaypath(ce->name, suc->update_data->prefix);
 
 	if (ce_stage(ce)) {
 		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
@@ -2461,9 +2454,8 @@ static int update_submodule(struct update_data *update_data)
 {
 	ensure_core_worktree(update_data->sm_path);
 
-	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
-								update_data->prefix,
-								get_super_prefix());
+	update_data->displaypath = get_submodule_displaypath(
+		update_data->sm_path, update_data->prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v4 0/7] submodule: remove "--recursive-prefix"

[Glen Choo]

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
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v4 1/7] submodule--helper tests: add missing "display path" coverage

[Glen Choo]

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(+)

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

[PATCH v4 2/7] submodule--helper update: use display path helper

[Glen Choo]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 389b900602..9381127d56 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1947,30 +1947,21 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	const char *update_string;
 	enum submodule_update_type update_type;
 	char *key;
-	struct strbuf displaypath_sb = STRBUF_INIT;
+	struct update_data *ud = suc->update_data;
+	char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,
+							 ud->recursive_prefix);
 	struct strbuf sb = STRBUF_INIT;
-	const char *displaypath = NULL;
 	int needs_cloning = 0;
 	int need_free_url = 0;
 
 	if (ce_stage(ce)) {
-		if (suc->update_data->recursive_prefix)
-			strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name);
-		else
-			strbuf_addstr(&sb, ce->name);
-		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
+		strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
 		strbuf_addch(out, '\n');
 		goto cleanup;
 	}
 
 	sub = submodule_from_path(the_repository, null_oid(), ce->name);
 
-	if (suc->update_data->recursive_prefix)
-		displaypath = relative_path(suc->update_data->recursive_prefix,
-					    ce->name, &displaypath_sb);
-	else
-		displaypath = ce->name;
-
 	if (!sub) {
 		next_submodule_warn_missing(suc, out, displaypath);
 		goto cleanup;
@@ -2060,7 +2051,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 					      "--no-single-branch");
 
 cleanup:
-	strbuf_release(&displaypath_sb);
+	free(displaypath);
 	strbuf_release(&sb);
 	if (need_free_url)
 		free((void*)url);
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 9a076e025f..6cc07460dd 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1160,7 +1160,7 @@ test_expect_success 'submodule update should skip unmerged submodules' '
 	test_config -C top-cloned submodule.middle.update !true &&
 	git -C top-cloned submodule update --recursive 2>actual.err &&
 	cat >expect.err <<-\EOF &&
-	Skipping unmerged submodule middle//bottom
+	Skipping unmerged submodule middle/bottom
 	EOF
 	test_cmp expect.err actual.err
 '
@@ -1173,7 +1173,7 @@ test_expect_success 'submodule update --recursive skip submodules with strategy=
 	git -C top-cloned/middle config submodule.bottom.update none &&
 	git -C top-cloned submodule update --recursive 2>actual.err &&
 	cat >expect.err <<-\EOF &&
-	Skipping submodule '\''../middle/'\''
+	Skipping submodule '\''middle/bottom'\''
 	EOF
 	test_cmp expect.err actual.err
 '
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v4 3/7] submodule--helper: don't recreate recursive prefix

[Glen Choo]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9381127d56..4a0eb05ba2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2416,9 +2416,10 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
-	if (update_data->recursive_prefix)
-		strvec_pushl(args, "--recursive-prefix",
-			     update_data->recursive_prefix, NULL);
+	if (update_data->displaypath) {
+		strvec_push(args, "--recursive-prefix");
+		strvec_pushf(args, "%s/", update_data->displaypath);
+	}
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)
@@ -2514,14 +2515,6 @@ static int update_submodule(struct update_data *update_data)
 		struct update_data next = *update_data;
 		int res;
 
-		if (update_data->recursive_prefix)
-			prefixed_path = xstrfmt("%s%s/", update_data->recursive_prefix,
-						update_data->sm_path);
-		else
-			prefixed_path = xstrfmt("%s/", update_data->sm_path);
-
-		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
-								  update_data->prefix);
 		next.prefix = NULL;
 		oidcpy(&next.oid, null_oid());
 		oidcpy(&next.suboid, null_oid());
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v4 4/7] submodule--helper: use correct display path helper

[Glen Choo]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4a0eb05ba2..65350bde4b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2463,19 +2463,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 
 static int update_submodule(struct update_data *update_data)
 {
-	char *prefixed_path;
-
 	ensure_core_worktree(update_data->sm_path);
 
-	if (update_data->recursive_prefix)
-		prefixed_path = xstrfmt("%s%s", update_data->recursive_prefix,
-					update_data->sm_path);
-	else
-		prefixed_path = xstrdup(update_data->sm_path);
-
-	update_data->displaypath = get_submodule_displaypath(prefixed_path,
-							     update_data->prefix);
-	free(prefixed_path);
+	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
+								update_data->prefix,
+								update_data->recursive_prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v4 5/7] submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags

[Glen Choo]

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

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

[PATCH v4 6/7] submodule--helper update: use --super-prefix

[Glen Choo]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a7b40482b8..eb84e3a98b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -477,22 +477,18 @@ static int starts_with_dot_dot_slash(const char *const path)
 
 struct init_cb {
 	const char *prefix;
-	const char *superprefix;
 	unsigned int flags;
 };
 #define INIT_CB_INIT { 0 }
 
 static void init_submodule(const char *path, const char *prefix,
-			   const char *superprefix, unsigned int flags)
+			   unsigned int flags)
 {
 	const struct submodule *sub;
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	/* try superprefix from the environment, if it is not passed explicitly */
-	if (!superprefix)
-		superprefix = get_super_prefix();
-	displaypath = do_get_submodule_displaypath(path, prefix, superprefix);
+	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 
@@ -566,7 +562,7 @@ static void init_submodule(const char *path, const char *prefix,
 static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)
 {
 	struct init_cb *info = cb_data;
-	init_submodule(list_item->name, info->prefix, info->superprefix, info->flags);
+	init_submodule(list_item->name, info->prefix, info->flags);
 }
 
 static int module_init(int argc, const char **argv, const char *prefix)
@@ -1878,7 +1874,6 @@ struct submodule_update_clone {
 
 struct update_data {
 	const char *prefix;
-	const char *recursive_prefix;
 	const char *displaypath;
 	enum submodule_update_type update_default;
 	struct object_id suboid;
@@ -1949,7 +1944,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	char *key;
 	struct update_data *ud = suc->update_data;
 	char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,
-							 ud->recursive_prefix);
+							 get_super_prefix());
 	struct strbuf sb = STRBUF_INIT;
 	int needs_cloning = 0;
 	int need_free_url = 0;
@@ -2414,12 +2409,12 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 {
 	enum submodule_update_type update_type = update_data->update_default;
 
-	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
-	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->displaypath) {
-		strvec_push(args, "--recursive-prefix");
+		strvec_push(args, "--super-prefix");
 		strvec_pushf(args, "%s/", update_data->displaypath);
 	}
+	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
+	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->quiet)
 		strvec_push(args, "--quiet");
 	if (update_data->force)
@@ -2467,7 +2462,7 @@ static int update_submodule(struct update_data *update_data)
 
 	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
 								update_data->prefix,
-								update_data->recursive_prefix);
+								get_super_prefix());
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,
@@ -2591,10 +2586,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "prefix", &opt.prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
-			   N_("path"),
-			   N_("path into the working tree, across nested "
-			      "submodule boundaries")),
 		OPT_SET_INT(0, "checkout", &opt.update_default,
 			N_("use the 'checkout' update strategy (default)"),
 			SM_UPDATE_CHECKOUT),
@@ -2680,7 +2671,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			module_list_active(&list);
 
 		info.prefix = opt.prefix;
-		info.superprefix = opt.recursive_prefix;
 		if (opt.quiet)
 			info.flags |= OPT_QUIET;
 
@@ -3377,9 +3367,9 @@ struct cmd_struct {
 static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
-	{"clone", module_clone, 0},
+	{"clone", module_clone, SUPPORT_SUPER_PREFIX},
 	{"add", module_add, 0},
-	{"update", module_update, 0},
+	{"update", module_update, SUPPORT_SUPER_PREFIX},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"init", module_init, 0},
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v4 7/7] submodule--helper: remove display path helper

[Glen Choo]

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index eb84e3a98b..405a4224c5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -118,10 +118,11 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
-static char *do_get_submodule_displaypath(const char *path,
-					  const char *prefix,
-					  const char *super_prefix)
+/* the result should be freed by the caller. */
+static char *get_submodule_displaypath(const char *path, const char *prefix)
 {
+	const char *super_prefix = get_super_prefix();
+
 	if (prefix && super_prefix) {
 		BUG("cannot have prefix '%s' and superprefix '%s'",
 		    prefix, super_prefix);
@@ -137,13 +138,6 @@ static char *do_get_submodule_displaypath(const char *path,
 	}
 }
 
-/* the result should be freed by the caller. */
-static char *get_submodule_displaypath(const char *path, const char *prefix)
-{
-	const char *super_prefix = get_super_prefix();
-	return do_get_submodule_displaypath(path, prefix, super_prefix);
-}
-
 static char *compute_rev_name(const char *sub_path, const char* object_id)
 {
 	struct strbuf sb = STRBUF_INIT;
@@ -488,7 +482,7 @@ static void init_submodule(const char *path, const char *prefix,
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	displaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());
+	displaypath = get_submodule_displaypath(path, prefix);
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 
@@ -1943,8 +1937,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	enum submodule_update_type update_type;
 	char *key;
 	struct update_data *ud = suc->update_data;
-	char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,
-							 get_super_prefix());
+	char *displaypath = get_submodule_displaypath(ce->name, ud->prefix);
 	struct strbuf sb = STRBUF_INIT;
 	int needs_cloning = 0;
 	int need_free_url = 0;
@@ -2460,9 +2453,8 @@ static int update_submodule(struct update_data *update_data)
 {
 	ensure_core_worktree(update_data->sm_path);
 
-	update_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,
-								update_data->prefix,
-								get_super_prefix());
+	update_data->displaypath = get_submodule_displaypath(
+		update_data->sm_path, update_data->prefix);
 
 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
 					    update_data->sm_path, update_data->update_default,
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH v2 00/18] submodule: remove "--recursive-prefix"

[Ævar Arnfjörð Bjarmason]

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

> 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

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

Re: [PATCH v2 00/18] submodule: remove "--recursive-prefix"

[Glen Choo]

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

> On Thu, Jun 30 2022, Glen Choo via GitGitGadget wrote:
>
>> = 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
>
> 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.

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

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

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

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

Both sound good.

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

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.

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

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

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

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

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

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

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