git submodule set-url does not sync when name != path

git submodule set-url does not sync when name != path

[Jan Alexander Steffens (heftig)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hi,

I've encountered this problem with the flatpak-builder repository:

https://github.com/flatpak/flatpak-builder/blob/main/.gitmodules

[submodule "libglnx"]
	path = subprojects/libglnx
	url = https://gitlab.gnome.org/GNOME/libglnx.git
[submodule "debugedit"]
	path = subprojects/debugedit
	url = https://sourceware.org/git/debugedit.git

After 'git submodule init', using 'git submodule set-url libglnx foo'
successfully modifies .gitmodules but does not touch .git/config.
However, a subsequent 'git submodule sync' does sync the modified url
to the local config.

I've investigated a bit and it seems 'git submodule set-url' calls
sync_submodule with "libglnx" as the path, which does not work, while
'git submodule sync' calls it with "subprojects/libglnx" as the path,
which does work.

Greetings,
Jan
-----BEGIN PGP SIGNATURE-----

iIsEARYIADMWIQQGaHodnU+rCLUP2Ss7lKgOUKR3xwUCZL38OhUcaGVmdGlnQGFy
Y2hsaW51eC5vcmcACgkQO5SoDlCkd8cfsAD+M6POceTxAkFpnnmAH7j3RYLBEjFP
w4kzY3U0UPVMU3wBAIPRBGjiwGFp/ws/iMeGQKif1E0ElqcxEbFAQ36HytIG
=4OVq
-----END PGP SIGNATURE-----

Bug: git submodule set-url does not handle name != path correctly

[Jan Alexander Steffens (heftig)]

On Mon, 2023-07-24 at 06:21 +0200, Jan Alexander Steffens (heftig)
wrote:
> Hi,
> 
> I've encountered this problem with the flatpak-builder repository:
> 
> https://github.com/flatpak/flatpak-builder/blob/main/.gitmodules
> 
> [submodule "libglnx"]
>         path = subprojects/libglnx
>         url = https://gitlab.gnome.org/GNOME/libglnx.git
> [submodule "debugedit"]
>         path = subprojects/debugedit
>         url = https://sourceware.org/git/debugedit.git
> 
> After 'git submodule init', using 'git submodule set-url libglnx foo'
> successfully modifies .gitmodules but does not touch .git/config.
> However, a subsequent 'git submodule sync' does sync the modified url
> to the local config.
> 
> I've investigated a bit and it seems 'git submodule set-url' calls
> sync_submodule with "libglnx" as the path, which does not work, while
> 'git submodule sync' calls it with "subprojects/libglnx" as the path,
> which does work.
> 
> Greetings,
> Jan

Friendly bump.

The docs say set-url needs the path to the submodule, not its name, so
it should be 'git submodule set-url subprojects/libglnx foo'.

However, using that actually creates a new
'submodule.subprojects/libglnx.url' option instead of modifying the
existing 'submodule.libglnx.url'.

It looks like set-url needs to translate the path to the name for
modifying .gitmodules but does not do so.

'git submodule set-branch' is also affected by this.

[PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch}

[Jan Alexander Steffens (heftig)]

The commands need a path to a submodule but treated it as the name when
modifying the .gitmodules file, leading to confusion when a submodule's
name does not match its path.

Because calling submodule_from_path initializes the submodule cache, we
need to manually trigger a reread before syncing, as the cache is
missing the config change we just made.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 builtin/submodule--helper.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f6871efd95..f376466a5e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2902,19 +2902,26 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
 		N_("git submodule set-url [--quiet] <path> <newurl>"),
 		NULL
 	};
+	const struct submodule *sub;
 
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))
 		usage_with_options(usage, options);
 
-	config_name = xstrfmt("submodule.%s.url", path);
+	sub = submodule_from_path(the_repository, null_oid(), path);
 
+	if (!sub)
+		die(_("no submodule mapping found in .gitmodules for path '%s'"),
+		    path);
+
+	config_name = xstrfmt("submodule.%s.url", sub->name);
 	config_set_in_gitmodules_file_gently(config_name, newurl);
-	sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
+
+	repo_read_gitmodules (the_repository, 0);
+	sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);
 
 	free(config_name);
-
 	return 0;
 }
 
@@ -2942,19 +2949,26 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 		N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
 		NULL
 	};
+	const struct submodule *sub;
 
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	if (!opt_branch && !opt_default)
 		die(_("--branch or --default required"));
 
 	if (opt_branch && opt_default)
 		die(_("options '%s' and '%s' cannot be used together"), "--branch", "--default");
 
 	if (argc != 1 || !(path = argv[0]))
 		usage_with_options(usage, options);
 
-	config_name = xstrfmt("submodule.%s.branch", path);
+	sub = submodule_from_path(the_repository, null_oid(), path);
+
+	if (!sub)
+		die(_("no submodule mapping found in .gitmodules for path '%s'"),
+		    path);
+
+	config_name = xstrfmt("submodule.%s.branch", sub->name);
 	ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);
 
 	free(config_name);
-- 
2.42.0

[PATCH 2/6] submodule--helper: return error from set-url when modifying failed

[Jan Alexander Steffens (heftig)]

set-branch will return an error when setting the config fails so I don't
see why set-url shouldn't. Also skip the sync in this case.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 builtin/submodule--helper.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f376466a5e..e2175083a6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2890,39 +2890,41 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 
 static int module_set_url(int argc, const char **argv, const char *prefix)
 {
-	int quiet = 0;
+	int quiet = 0, ret;
 	const char *newurl;
 	const char *path;
 	char *config_name;
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("suppress output for setting url of a submodule")),
 		OPT_END()
 	};
 	const char *const usage[] = {
 		N_("git submodule set-url [--quiet] <path> <newurl>"),
 		NULL
 	};
 	const struct submodule *sub;
 
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))
 		usage_with_options(usage, options);
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 
 	if (!sub)
 		die(_("no submodule mapping found in .gitmodules for path '%s'"),
 		    path);
 
 	config_name = xstrfmt("submodule.%s.url", sub->name);
-	config_set_in_gitmodules_file_gently(config_name, newurl);
+	ret = config_set_in_gitmodules_file_gently(config_name, newurl);
 
-	repo_read_gitmodules (the_repository, 0);
-	sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);
+	if (!ret) {
+		repo_read_gitmodules(the_repository, 0);
+		sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);
+	}
 
 	free(config_name);
-	return 0;
+	return !!ret;
 }
 
 static int module_set_branch(int argc, const char **argv, const char *prefix)
-- 
2.42.0

[PATCH 3/6] t7419: Actually test the branch switching

[Jan Alexander Steffens (heftig)]

The submodule repo the test set up had the 'topic' branch checked out,
meaning the repo's default branch (HEAD) is the 'topic' branch.

The following tests then pretended to switch between the default branch
and the 'topic' branch. This was papered over by continually adding
commits to the 'topic' branch and checking if the submodule gets updated
to this new commit.

Return the submodule repo to the 'main' branch after setup so we can
actually test the switching behavior.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 t/t7419-submodule-set-branch.sh | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 232065504c..5ac16d0eb7 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -11,23 +11,28 @@ as expected.
 
 TEST_PASSES_SANITIZE_LEAK=true
 TEST_NO_CREATE_REPO=1
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
 . ./test-lib.sh
 
 test_expect_success 'setup' '
 	git config --global protocol.file.allow always
 '
 
 test_expect_success 'submodule config cache setup' '
 	mkdir submodule &&
 	(cd submodule &&
 		git init &&
 		echo a >a &&
 		git add . &&
 		git commit -ma &&
 		git checkout -b topic &&
 		echo b >a &&
 		git add . &&
-		git commit -mb
+		git commit -mb &&
+		git checkout main
 	) &&
 	mkdir super &&
 	(cd super &&
@@ -57,41 +62,38 @@ test_expect_success 'test submodule set-branch --branch' '
 '
 
 test_expect_success 'test submodule set-branch --default' '
-	test_commit -C submodule c &&
 	(cd super &&
 		git submodule set-branch --default submodule &&
 		! grep branch .gitmodules &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
-		c
+		a
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch -b' '
-	test_commit -C submodule b &&
 	(cd super &&
 		git submodule set-branch -b topic submodule &&
 		grep "branch = topic" .gitmodules &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		b
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch -d' '
-	test_commit -C submodule d &&
 	(cd super &&
 		git submodule set-branch -d submodule &&
 		! grep branch .gitmodules &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
-		d
+		a
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
-- 
2.42.0

[PATCH 4/6] t7419, t7420: Use test_cmp_config instead of grepping .gitmodules

[Jan Alexander Steffens (heftig)]

We have a test function to verify config files. Use it as it's more
precise.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 t/t7419-submodule-set-branch.sh | 10 +++++-----
 t/t7420-submodule-set-url.sh    |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 5ac16d0eb7..3cd30865a7 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -44,53 +44,53 @@ test_expect_success 'submodule config cache setup' '
 
 test_expect_success 'ensure submodule branch is unset' '
 	(cd super &&
-		! grep branch .gitmodules
+		test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch
 	)
 '
 
 test_expect_success 'test submodule set-branch --branch' '
 	(cd super &&
 		git submodule set-branch --branch topic submodule &&
-		grep "branch = topic" .gitmodules &&
+		test_cmp_config topic -f .gitmodules submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		b
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch --default' '
 	(cd super &&
 		git submodule set-branch --default submodule &&
-		! grep branch .gitmodules &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		a
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch -b' '
 	(cd super &&
 		git submodule set-branch -b topic submodule &&
-		grep "branch = topic" .gitmodules &&
+		test_cmp_config topic -f .gitmodules submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		b
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch -d' '
 	(cd super &&
 		git submodule set-branch -d submodule &&
-		! grep branch .gitmodules &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		a
diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
index d6bf62b3ac..aa63d806fe 100755
--- a/t/t7420-submodule-set-url.sh
+++ b/t/t7420-submodule-set-url.sh
@@ -49,7 +49,7 @@ test_expect_success 'test submodule set-url' '
 		cd super &&
 		test_must_fail git submodule update --remote &&
 		git submodule set-url submodule ../newsubmodule &&
-		grep -F "url = ../newsubmodule" .gitmodules &&
+		test_cmp_config ../newsubmodule -f .gitmodules submodule.submodule.url &&
 		git submodule update --remote
 	) &&
 	git -C super/submodule show >actual &&
-- 
2.42.0

[PATCH 5/6] t7419: Test that we correctly handle renamed submodules

[Jan Alexander Steffens (heftig)]

Add the submodule again with an explicitly different name and path. Test
that calling set-branch modifies the correct .gitmodules entries. Make
sure we don't create a section named after the path instead of the name.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 t/t7419-submodule-set-branch.sh | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 3cd30865a7..a5d1bc5c54 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -38,7 +38,8 @@ test_expect_success 'submodule config cache setup' '
 	(cd super &&
 		git init &&
 		git submodule add ../submodule &&
-		git commit -m "add submodule"
+		git submodule add --name thename ../submodule thepath &&
+		git commit -m "add submodules"
 	)
 '
 
@@ -100,4 +101,31 @@ test_expect_success 'test submodule set-branch -d' '
 	)
 '
 
+test_expect_success 'test submodule set-branch --branch with named submodule' '
+	(cd super &&
+		git submodule set-branch --branch topic thepath &&
+		test_cmp_config topic -f .gitmodules submodule.thename.branch &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.thepath.branch &&
+		git submodule update --remote &&
+		cat <<-\EOF >expect &&
+		b
+		EOF
+		git -C thepath show -s --pretty=%s >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'test submodule set-branch --default with named submodule' '
+	(cd super &&
+		git submodule set-branch --default thepath &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.thename.branch &&
+		git submodule update --remote &&
+		cat <<-\EOF >expect &&
+		a
+		EOF
+		git -C thepath show -s --pretty=%s >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
-- 
2.42.0

[PATCH 6/6] t7420: Test that we correctly handle renamed submodules

[Jan Alexander Steffens (heftig)]

Create a second submodule with a name that differs from its path. Test
that calling set-url modifies the correct .gitmodules entries. Make sure
we don't create a section named after the path instead of the name.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 t/t7420-submodule-set-url.sh | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
index aa63d806fe..bf7f15ee79 100755
--- a/t/t7420-submodule-set-url.sh
+++ b/t/t7420-submodule-set-url.sh
@@ -25,34 +25,56 @@ test_expect_success 'submodule config cache setup' '
 		git add file &&
 		git commit -ma
 	) &&
+	mkdir namedsubmodule &&
+	(
+		cd namedsubmodule &&
+		git init &&
+		echo 1 >file &&
+		git add file &&
+		git commit -m1
+	) &&
 	mkdir super &&
 	(
 		cd super &&
 		git init &&
 		git submodule add ../submodule &&
-		git commit -m "add submodule"
+		git submodule add --name thename ../namedsubmodule thepath &&
+		git commit -m "add submodules"
 	)
 '
 
 test_expect_success 'test submodule set-url' '
-	# add a commit and move the submodule (change the url)
+	# add commits and move the submodules (change the urls)
 	(
 		cd submodule &&
 		echo b >>file &&
 		git add file &&
 		git commit -mb
 	) &&
 	mv submodule newsubmodule &&
 
+	(
+		cd namedsubmodule &&
+		echo 2 >>file &&
+		git add file &&
+		git commit -m2
+	) &&
+	mv namedsubmodule newnamedsubmodule &&
+
 	git -C newsubmodule show >expect &&
+	git -C newnamedsubmodule show >>expect &&
 	(
 		cd super &&
 		test_must_fail git submodule update --remote &&
 		git submodule set-url submodule ../newsubmodule &&
 		test_cmp_config ../newsubmodule -f .gitmodules submodule.submodule.url &&
+		git submodule set-url thepath ../newnamedsubmodule &&
+		test_cmp_config ../newnamedsubmodule -f .gitmodules submodule.thename.url &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.thepath.url &&
 		git submodule update --remote
 	) &&
 	git -C super/submodule show >actual &&
+	git -C super/thepath show >>actual &&
 	test_cmp expect actual
 '
 
-- 
2.42.0

Re: [PATCH 2/6] submodule--helper: return error from set-url when modifying failed

[Junio C Hamano]

"Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:

> set-branch will return an error when setting the config fails so I don't
> see why set-url shouldn't. Also skip the sync in this case.

Two other failures in this helper function gives end-user readable
message (parameter errors are greeted with usage, giving wrong path
is greeted with a die()), but this new error condition will silently
die unless config_set_in_gitmodules_file_gently() complains.

I wonder if we should give an error message in this case, too.

In any case, not calling sync after failed set_in_gitmodules is a
sensible design decision.

Re: [PATCH 3/6] t7419: Actually test the branch switching

[Junio C Hamano]

"Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:

> Subject: Re: [PATCH 3/6] t7419: Actually test the branch switching

"Actually" -> "actually".

> The submodule repo the test set up had the 'topic' branch checked out,
> meaning the repo's default branch (HEAD) is the 'topic' branch.
>
> The following tests then pretended to switch between the default branch
> and the 'topic' branch. This was papered over by continually adding
> commits to the 'topic' branch and checking if the submodule gets updated
> to this new commit.
>
> Return the submodule repo to the 'main' branch after setup so we can
> actually test the switching behavior.

Nicely spotted.

> diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
> index 232065504c..5ac16d0eb7 100755
> --- a/t/t7419-submodule-set-branch.sh
> +++ b/t/t7419-submodule-set-branch.sh
> @@ -11,23 +11,28 @@ as expected.
>  
>  TEST_PASSES_SANITIZE_LEAK=true
>  TEST_NO_CREATE_REPO=1
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

And it is good that this variable is used to prepare the test for
both kinds of CI runs that use 'main' or 'master' as the default
branch.

Re: [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch}

[Junio C Hamano]

"Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:

> The commands need a path to a submodule but treated it as the name when
> modifying the .gitmodules file, leading to confusion when a submodule's
> name does not match its path.

Thanks for noticing and fixing this common mix-up.

> Because calling submodule_from_path initializes the submodule cache, we
> need to manually trigger a reread before syncing, as the cache is
> missing the config change we just made.

> Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f6871efd95..f376466a5e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2902,19 +2902,26 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
>  		N_("git submodule set-url [--quiet] <path> <newurl>"),
>  		NULL
>  	};
> +	const struct submodule *sub;
>  
>  	argc = parse_options(argc, argv, prefix, options, usage, 0);
>  
>  	if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))
>  		usage_with_options(usage, options);
>  
> -	config_name = xstrfmt("submodule.%s.url", path);
> +	sub = submodule_from_path(the_repository, null_oid(), path);
>  
> +	if (!sub)
> +		die(_("no submodule mapping found in .gitmodules for path '%s'"),
> +		    path);
> +
> +	config_name = xstrfmt("submodule.%s.url", sub->name);

Looks correct.

>  	config_set_in_gitmodules_file_gently(config_name, newurl);
> -	sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
> +
> +	repo_read_gitmodules (the_repository, 0);

Style.  No extra space between function name and "(".

But more importantly, is this sufficient?  repo_read_gitmodules()
does not seem to clear the cache and build its contents from scratch
(as submodule_cache_check_init() bypasses itself upon second call).

The code is correct because submodule-config.c::config_from() does
set .overwrite to true, so submodule.name.url would be overwritten
to the new value, I think, but somebody else who is more familiar
with the recent submodule code may want to sanity check my analysis.

> +	sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);

Is the use of "sub" still safe here?  

I think it is safe as repo_read_gitmodules() did not rebuild the
in-core cache from scratch but did selective overwrite, so the
in-core instance "sub" is still valid, but again somebody else who
is more familiar with the recent submodule code may want to sanity
check.

> @@ -2942,19 +2949,26 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
>  		N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
>  		NULL
>  	};
> +	const struct submodule *sub;
>  
>  	argc = parse_options(argc, argv, prefix, options, usage, 0);
>  
>  	if (!opt_branch && !opt_default)
>  		die(_("--branch or --default required"));
>  
>  	if (opt_branch && opt_default)
>  		die(_("options '%s' and '%s' cannot be used together"), "--branch", "--default");
>  
>  	if (argc != 1 || !(path = argv[0]))
>  		usage_with_options(usage, options);
>  
> -	config_name = xstrfmt("submodule.%s.branch", path);
> +	sub = submodule_from_path(the_repository, null_oid(), path);
> +
> +	if (!sub)
> +		die(_("no submodule mapping found in .gitmodules for path '%s'"),
> +		    path);
> +
> +	config_name = xstrfmt("submodule.%s.branch", sub->name);
>  	ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);

This side happens not to require re-reading of gitmodules file,
because, unlike the URL helper, we do not care what we have in the
in-core cache is stale.  It is correct but feels a bit brittle.

[PATCH v2 1/6] submodule--helper: use submodule_from_path in set-{url,branch}

[Jan Alexander Steffens (heftig)]

The commands need a path to a submodule but treated it as the name when
modifying the .gitmodules file, leading to confusion when a submodule's
name does not match its path.

Because calling submodule_from_path initializes the submodule cache, we
need to manually trigger a reread before syncing, as the cache is
missing the config change we just made.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---

Notes:
    v2 changes:
        - fixed code style
        - replaced potentially unsafe use of `sub->path` with `path`

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6f3bf33e61..af461ada8b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2901,19 +2901,26 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
 		N_("git submodule set-url [--quiet] <path> <newurl>"),
 		NULL
 	};
+	const struct submodule *sub;
 
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))
 		usage_with_options(usage, options);
 
-	config_name = xstrfmt("submodule.%s.url", path);
+	sub = submodule_from_path(the_repository, null_oid(), path);
 
+	if (!sub)
+		die(_("no submodule mapping found in .gitmodules for path '%s'"),
+		    path);
+
+	config_name = xstrfmt("submodule.%s.url", sub->name);
 	config_set_in_gitmodules_file_gently(config_name, newurl);
+
+	repo_read_gitmodules(the_repository, 0);
 	sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
 
 	free(config_name);
-
 	return 0;
 }
 
@@ -2941,19 +2948,26 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 		N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
 		NULL
 	};
+	const struct submodule *sub;
 
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	if (!opt_branch && !opt_default)
 		die(_("--branch or --default required"));
 
 	if (opt_branch && opt_default)
 		die(_("options '%s' and '%s' cannot be used together"), "--branch", "--default");
 
 	if (argc != 1 || !(path = argv[0]))
 		usage_with_options(usage, options);
 
-	config_name = xstrfmt("submodule.%s.branch", path);
+	sub = submodule_from_path(the_repository, null_oid(), path);
+
+	if (!sub)
+		die(_("no submodule mapping found in .gitmodules for path '%s'"),
+		    path);
+
+	config_name = xstrfmt("submodule.%s.branch", sub->name);
 	ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);
 
 	free(config_name);
-- 
2.43.0

[PATCH v2 2/6] submodule--helper: return error from set-url when modifying failed

[Jan Alexander Steffens (heftig)]

set-branch will return an error when setting the config fails so I don't
see why set-url shouldn't. Also skip the sync in this case.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 builtin/submodule--helper.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index af461ada8b..0013ea1ab0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2889,39 +2889,41 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 
 static int module_set_url(int argc, const char **argv, const char *prefix)
 {
-	int quiet = 0;
+	int quiet = 0, ret;
 	const char *newurl;
 	const char *path;
 	char *config_name;
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("suppress output for setting url of a submodule")),
 		OPT_END()
 	};
 	const char *const usage[] = {
 		N_("git submodule set-url [--quiet] <path> <newurl>"),
 		NULL
 	};
 	const struct submodule *sub;
 
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))
 		usage_with_options(usage, options);
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 
 	if (!sub)
 		die(_("no submodule mapping found in .gitmodules for path '%s'"),
 		    path);
 
 	config_name = xstrfmt("submodule.%s.url", sub->name);
-	config_set_in_gitmodules_file_gently(config_name, newurl);
+	ret = config_set_in_gitmodules_file_gently(config_name, newurl);
 
-	repo_read_gitmodules(the_repository, 0);
-	sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
+	if (!ret) {
+		repo_read_gitmodules(the_repository, 0);
+		sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
+	}
 
 	free(config_name);
-	return 0;
+	return !!ret;
 }
 
 static int module_set_branch(int argc, const char **argv, const char *prefix)
-- 
2.43.0

[PATCH v2 3/6] t7419: actually test the branch switching

[Jan Alexander Steffens (heftig)]

The submodule repo the test set up had the 'topic' branch checked out,
meaning the repo's default branch (HEAD) is the 'topic' branch.

The following tests then pretended to switch between the default branch
and the 'topic' branch. This was papered over by continually adding
commits to the 'topic' branch and checking if the submodule gets updated
to this new commit.

Return the submodule repo to the 'main' branch after setup so we can
actually test the switching behavior.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---

Notes:
    v2 changes:
        - fixed subject

 t/t7419-submodule-set-branch.sh | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 232065504c..5ac16d0eb7 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -11,23 +11,28 @@ as expected.
 
 TEST_PASSES_SANITIZE_LEAK=true
 TEST_NO_CREATE_REPO=1
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
 . ./test-lib.sh
 
 test_expect_success 'setup' '
 	git config --global protocol.file.allow always
 '
 
 test_expect_success 'submodule config cache setup' '
 	mkdir submodule &&
 	(cd submodule &&
 		git init &&
 		echo a >a &&
 		git add . &&
 		git commit -ma &&
 		git checkout -b topic &&
 		echo b >a &&
 		git add . &&
-		git commit -mb
+		git commit -mb &&
+		git checkout main
 	) &&
 	mkdir super &&
 	(cd super &&
@@ -57,41 +62,38 @@ test_expect_success 'test submodule set-branch --branch' '
 '
 
 test_expect_success 'test submodule set-branch --default' '
-	test_commit -C submodule c &&
 	(cd super &&
 		git submodule set-branch --default submodule &&
 		! grep branch .gitmodules &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
-		c
+		a
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch -b' '
-	test_commit -C submodule b &&
 	(cd super &&
 		git submodule set-branch -b topic submodule &&
 		grep "branch = topic" .gitmodules &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		b
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch -d' '
-	test_commit -C submodule d &&
 	(cd super &&
 		git submodule set-branch -d submodule &&
 		! grep branch .gitmodules &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
-		d
+		a
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
-- 
2.43.0

[PATCH v2 4/6] t7419, t7420: use test_cmp_config instead of grepping .gitmodules

[Jan Alexander Steffens (heftig)]

We have a test function to verify config files. Use it as it's more
precise.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 t/t7419-submodule-set-branch.sh | 10 +++++-----
 t/t7420-submodule-set-url.sh    |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 5ac16d0eb7..3cd30865a7 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -44,53 +44,53 @@ test_expect_success 'submodule config cache setup' '
 
 test_expect_success 'ensure submodule branch is unset' '
 	(cd super &&
-		! grep branch .gitmodules
+		test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch
 	)
 '
 
 test_expect_success 'test submodule set-branch --branch' '
 	(cd super &&
 		git submodule set-branch --branch topic submodule &&
-		grep "branch = topic" .gitmodules &&
+		test_cmp_config topic -f .gitmodules submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		b
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch --default' '
 	(cd super &&
 		git submodule set-branch --default submodule &&
-		! grep branch .gitmodules &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		a
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch -b' '
 	(cd super &&
 		git submodule set-branch -b topic submodule &&
-		grep "branch = topic" .gitmodules &&
+		test_cmp_config topic -f .gitmodules submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		b
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch -d' '
 	(cd super &&
 		git submodule set-branch -d submodule &&
-		! grep branch .gitmodules &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		a
diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
index d6bf62b3ac..aa63d806fe 100755
--- a/t/t7420-submodule-set-url.sh
+++ b/t/t7420-submodule-set-url.sh
@@ -49,7 +49,7 @@ test_expect_success 'test submodule set-url' '
 		cd super &&
 		test_must_fail git submodule update --remote &&
 		git submodule set-url submodule ../newsubmodule &&
-		grep -F "url = ../newsubmodule" .gitmodules &&
+		test_cmp_config ../newsubmodule -f .gitmodules submodule.submodule.url &&
 		git submodule update --remote
 	) &&
 	git -C super/submodule show >actual &&
-- 
2.43.0

[PATCH v2 5/6] t7419: test that we correctly handle renamed submodules

[Jan Alexander Steffens (heftig)]

Add the submodule again with an explicitly different name and path. Test
that calling set-branch modifies the correct .gitmodules entries. Make
sure we don't create a section named after the path instead of the name.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 t/t7419-submodule-set-branch.sh | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 3cd30865a7..a5d1bc5c54 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -38,7 +38,8 @@ test_expect_success 'submodule config cache setup' '
 	(cd super &&
 		git init &&
 		git submodule add ../submodule &&
-		git commit -m "add submodule"
+		git submodule add --name thename ../submodule thepath &&
+		git commit -m "add submodules"
 	)
 '
 
@@ -100,4 +101,31 @@ test_expect_success 'test submodule set-branch -d' '
 	)
 '
 
+test_expect_success 'test submodule set-branch --branch with named submodule' '
+	(cd super &&
+		git submodule set-branch --branch topic thepath &&
+		test_cmp_config topic -f .gitmodules submodule.thename.branch &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.thepath.branch &&
+		git submodule update --remote &&
+		cat <<-\EOF >expect &&
+		b
+		EOF
+		git -C thepath show -s --pretty=%s >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'test submodule set-branch --default with named submodule' '
+	(cd super &&
+		git submodule set-branch --default thepath &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.thename.branch &&
+		git submodule update --remote &&
+		cat <<-\EOF >expect &&
+		a
+		EOF
+		git -C thepath show -s --pretty=%s >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
-- 
2.43.0

[PATCH v2 6/6] t7420: test that we correctly handle renamed submodules

[Jan Alexander Steffens (heftig)]

Create a second submodule with a name that differs from its path. Test
that calling set-url modifies the correct .gitmodules entries. Make sure
we don't create a section named after the path instead of the name.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 t/t7420-submodule-set-url.sh | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
index aa63d806fe..bf7f15ee79 100755
--- a/t/t7420-submodule-set-url.sh
+++ b/t/t7420-submodule-set-url.sh
@@ -25,34 +25,56 @@ test_expect_success 'submodule config cache setup' '
 		git add file &&
 		git commit -ma
 	) &&
+	mkdir namedsubmodule &&
+	(
+		cd namedsubmodule &&
+		git init &&
+		echo 1 >file &&
+		git add file &&
+		git commit -m1
+	) &&
 	mkdir super &&
 	(
 		cd super &&
 		git init &&
 		git submodule add ../submodule &&
-		git commit -m "add submodule"
+		git submodule add --name thename ../namedsubmodule thepath &&
+		git commit -m "add submodules"
 	)
 '
 
 test_expect_success 'test submodule set-url' '
-	# add a commit and move the submodule (change the url)
+	# add commits and move the submodules (change the urls)
 	(
 		cd submodule &&
 		echo b >>file &&
 		git add file &&
 		git commit -mb
 	) &&
 	mv submodule newsubmodule &&
 
+	(
+		cd namedsubmodule &&
+		echo 2 >>file &&
+		git add file &&
+		git commit -m2
+	) &&
+	mv namedsubmodule newnamedsubmodule &&
+
 	git -C newsubmodule show >expect &&
+	git -C newnamedsubmodule show >>expect &&
 	(
 		cd super &&
 		test_must_fail git submodule update --remote &&
 		git submodule set-url submodule ../newsubmodule &&
 		test_cmp_config ../newsubmodule -f .gitmodules submodule.submodule.url &&
+		git submodule set-url thepath ../newnamedsubmodule &&
+		test_cmp_config ../newnamedsubmodule -f .gitmodules submodule.thename.url &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.thepath.url &&
 		git submodule update --remote
 	) &&
 	git -C super/submodule show >actual &&
+	git -C super/thepath show >>actual &&
 	test_cmp expect actual
 '
 
-- 
2.43.0

Re: [PATCH v2 1/6] submodule--helper: use submodule_from_path in set-{url,branch}

[Junio C Hamano]

"Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:

> Notes:
>     v2 changes:
>         - fixed code style
>         - replaced potentially unsafe use of `sub->path` with `path`

Hasn't the previous iteration of this topic long been merged to not
just 'next' but to 'master' and appears in a released version of Git?

We are all human, so mistakes are inevitable, but if we discover a
mistake that needs fixing after a topic hits 'next', we take it as a
sign that the particular mistake was easy to make and hard to spot,
and the fix for it deserves its own explanation.  Please make an
incremental update on top of what has already been merged with a
good explanation (explain why sub->path is "potentially unsafe" and
why using path is better, for example).

Thanks.

Re: [PATCH v2 1/6] submodule--helper: use submodule_from_path in set-{url,branch}

[Jan Alexander Steffens (heftig)]

On Wed, 2023-11-22 at 16:54 +0900, Junio C Hamano wrote:
> "Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:
> 
> > Notes:
> >     v2 changes:
> >         - fixed code style
> >         - replaced potentially unsafe use of `sub->path` with
> > `path`
> 
> Hasn't the previous iteration of this topic long been merged to not
> just 'next' but to 'master' and appears in a released version of Git?
> 
> We are all human, so mistakes are inevitable, but if we discover a
> mistake that needs fixing after a topic hits 'next', we take it as a
> sign that the particular mistake was easy to make and hard to spot,
> and the fix for it deserves its own explanation.  Please make an
> incremental update on top of what has already been merged with a
> good explanation (explain why sub->path is "potentially unsafe" and
> why using path is better, for example).
> 
> Thanks.

So it has. I'm sorry. I was only keeping track of the 'maint' branch.