Merge branch 'js/submodule-fix-misuse-of-path-and-name'

In .gitmodules files, submodules are keyed by their names, and the
path to the submodule whose name is $name is specified by the
submodule.$name.path variable.  There were a few codepaths that
mixed the name and path up when consulting the submodule database,
which have been corrected.  It took long for these bugs to be found
as the name of a submodule initially is the same as its path, and
the problem does not surface until it is moved to a different path,
which apparently happens very rarely.

* js/submodule-fix-misuse-of-path-and-name:
  t7420: test that we correctly handle renamed submodules
  t7419: test that we correctly handle renamed submodules
  t7419, t7420: use test_cmp_config instead of grepping .gitmodules
  t7419: actually test the branch switching
  submodule--helper: return error from set-url when modifying failed
  submodule--helper: use submodule_from_path in set-{url,branch}

builtin/submodule--helper.c

@@ -2889,7 +2889,7 @@ cleanup:
 
 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;
@@ -2901,20 +2901,29 @@ 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);
 
-	config_set_in_gitmodules_file_gently(config_name, newurl);
+	if (!sub)
-	sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
+		die(_("no submodule mapping found in .gitmodules for path '%s'"),
+		    path);
+
+	config_name = xstrfmt("submodule.%s.url", sub->name);
+	ret = config_set_in_gitmodules_file_gently(config_name, newurl);
+
+	if (!ret) {
+		repo_read_gitmodules(the_repository, 0);
+		sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);
+	}
 
 	free(config_name);
-
+	return !!ret;
-	return 0;
 }
 
 static int module_set_branch(int argc, const char **argv, const char *prefix)
@@ -2941,6 +2950,7 @@ 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);
 
@@ -2953,7 +2963,13 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	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);

t/t7419-submodule-set-branch.sh

@@ -11,6 +11,10 @@ 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' '
@@ -27,26 +31,28 @@ test_expect_success 'submodule config cache setup' '
 		git checkout -b topic &&
 		echo b >a &&
 		git add . &&
-		git commit -mb
+		git commit -mb &&
+		git checkout main
 	) &&
 	mkdir super &&
 	(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"
 	)
 '
 
 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
@@ -57,13 +63,12 @@ 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 &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
-		c
+		a
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
@@ -71,10 +76,9 @@ test_expect_success 'test submodule set-branch --default' '
 '
 
 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 &&
+		test_cmp_config topic -f .gitmodules submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		b
@@ -85,17 +89,43 @@ test_expect_success 'test submodule set-branch -b' '
 '
 
 test_expect_success 'test submodule set-branch -d' '
-	test_commit -C submodule 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 &&
-		d
+		a
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
+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

t/t7420-submodule-set-url.sh

@@ -25,17 +25,26 @@ 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 &&
@@ -44,15 +53,28 @@ test_expect_success 'test submodule set-url' '
 	) &&
 	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 &&
-		grep -F "url = ../newsubmodule" .gitmodules &&
+		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
 '