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

Commit Message

Glen Choo June 27, 2022, 11:20 p.m. UTC

From: Glen Choo <[e-mail]>

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 <[e-mail]>
---
 builtin/submodule--helper.c | 22 +++++---------
 t/t7406-submodule-update.sh | 59 +++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 15 deletions(-)

Comments

Ævar Arnfjörð Bjarmason June 28, 2022, 8:23 a.m. UTC | #1

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

> From: Glen Choo <[e-mail]>
>
> 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

Glen Choo June 28, 2022, 5:34 p.m. UTC | #2

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 <[e-mail]> writes:

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

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