From 6327085aa027b7d5936912bed596a3cf86953439 Mon Sep 17 00:00:00 2001 From: "Jan Alexander Steffens (heftig)" Date: Tue, 3 Oct 2023 20:50:42 +0200 Subject: [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch} 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) Signed-off-by: Junio C Hamano --- 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 6f3bf33e61..0c1509ad6e 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] "), 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; } @@ -2941,6 +2948,7 @@ static int module_set_branch(int argc, const char **argv, const char *prefix) N_("git submodule set-branch [-q|--quiet] (-b|--branch) "), NULL }; + const struct submodule *sub; argc = parse_options(argc, argv, prefix, options, usage, 0); @@ -2953,7 +2961,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); From 387c122131a9f8e4a67122d53955133d099b64a7 Mon Sep 17 00:00:00 2001 From: "Jan Alexander Steffens (heftig)" Date: Tue, 3 Oct 2023 20:50:43 +0200 Subject: [PATCH 2/6] submodule--helper: return error from set-url when modifying failed 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) Signed-off-by: Junio C Hamano --- 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 0c1509ad6e..cce46450ab 100644 --- a/builtin/submodule--helper.c +++ b/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; @@ -2915,13 +2915,15 @@ static int module_set_url(int argc, const char **argv, const char *prefix) 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) From b027fb07848bdfd84abdf1d6af2cf5d9e4592ffc Mon Sep 17 00:00:00 2001 From: "Jan Alexander Steffens (heftig)" Date: Tue, 3 Oct 2023 20:50:44 +0200 Subject: [PATCH 3/6] t7419: actually test the branch switching 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) Signed-off-by: Junio C Hamano --- 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,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,7 +31,8 @@ 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 && @@ -57,13 +62,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 && git submodule update --remote && cat <<-\EOF >expect && - c + a EOF git -C submodule show -s --pretty=%s >actual && test_cmp expect actual @@ -71,7 +75,6 @@ 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 && @@ -85,13 +88,12 @@ 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 && git submodule update --remote && cat <<-\EOF >expect && - d + a EOF git -C submodule show -s --pretty=%s >actual && test_cmp expect actual From 5fc880632d5e9ef6fd40a5ef9b5e01d61cdc320a Mon Sep 17 00:00:00 2001 From: "Jan Alexander Steffens (heftig)" Date: Tue, 3 Oct 2023 20:50:45 +0200 Subject: [PATCH 4/6] t7419, t7420: use test_cmp_config instead of grepping .gitmodules We have a test function to verify config files. Use it as it's more precise. Signed-off-by: Jan Alexander Steffens (heftig) Signed-off-by: Junio C Hamano --- 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,14 +44,14 @@ 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 @@ -64,7 +64,7 @@ test_expect_success 'test submodule set-branch --branch' ' 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 @@ -77,7 +77,7 @@ test_expect_success 'test submodule set-branch --default' ' 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 @@ -90,7 +90,7 @@ test_expect_success 'test submodule set-branch -b' ' 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 && From 32bff3675e6b4dc5ae93f0777e5da709f6576275 Mon Sep 17 00:00:00 2001 From: "Jan Alexander Steffens (heftig)" Date: Tue, 3 Oct 2023 20:50:46 +0200 Subject: [PATCH 5/6] t7419: test that we correctly handle renamed submodules 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) Signed-off-by: Junio C Hamano --- 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 From bd1c20ccd70d7452566bbc56ca2cf1fbb73d747c Mon Sep 17 00:00:00 2001 From: "Jan Alexander Steffens (heftig)" Date: Tue, 3 Oct 2023 20:50:47 +0200 Subject: [PATCH 6/6] t7420: test that we correctly handle renamed submodules 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) Signed-off-by: Junio C Hamano --- 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,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 && 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 '