* [PATCH 2/6] submodule--helper: return error from set-url when modifying failed
2023-10-03 18:50 ` [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch} Jan Alexander Steffens (heftig)
@ 2023-10-03 18:50 ` Jan Alexander Steffens (heftig)
2023-10-03 23:10 ` Junio C Hamano
2023-10-03 18:50 ` [PATCH 3/6] t7419: Actually test the branch switching Jan Alexander Steffens (heftig)
` (5 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
To: git; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/6] submodule--helper: return error from set-url when modifying failed
2023-10-03 18:50 ` [PATCH 2/6] submodule--helper: return error from set-url when modifying failed Jan Alexander Steffens (heftig)
@ 2023-10-03 23:10 ` Junio C Hamano
0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2023-10-03 23:10 UTC (permalink / raw)
To: Jan Alexander Steffens (heftig); +Cc: git
"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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/6] t7419: Actually test the branch switching
2023-10-03 18:50 ` [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch} Jan Alexander Steffens (heftig)
2023-10-03 18:50 ` [PATCH 2/6] submodule--helper: return error from set-url when modifying failed Jan Alexander Steffens (heftig)
@ 2023-10-03 18:50 ` Jan Alexander Steffens (heftig)
2023-10-04 0:20 ` Junio C Hamano
2023-10-03 18:50 ` [PATCH 4/6] t7419, t7420: Use test_cmp_config instead of grepping .gitmodules Jan Alexander Steffens (heftig)
` (4 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
To: git; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/6] t7419: Actually test the branch switching
2023-10-03 18:50 ` [PATCH 3/6] t7419: Actually test the branch switching Jan Alexander Steffens (heftig)
@ 2023-10-04 0:20 ` Junio C Hamano
0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2023-10-04 0:20 UTC (permalink / raw)
To: Jan Alexander Steffens (heftig); +Cc: git
"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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/6] t7419, t7420: Use test_cmp_config instead of grepping .gitmodules
2023-10-03 18:50 ` [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch} Jan Alexander Steffens (heftig)
2023-10-03 18:50 ` [PATCH 2/6] submodule--helper: return error from set-url when modifying failed Jan Alexander Steffens (heftig)
2023-10-03 18:50 ` [PATCH 3/6] t7419: Actually test the branch switching Jan Alexander Steffens (heftig)
@ 2023-10-03 18:50 ` Jan Alexander Steffens (heftig)
2023-10-03 18:50 ` [PATCH 5/6] t7419: Test that we correctly handle renamed submodules Jan Alexander Steffens (heftig)
` (3 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
To: git; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 5/6] t7419: Test that we correctly handle renamed submodules
2023-10-03 18:50 ` [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch} Jan Alexander Steffens (heftig)
` (2 preceding siblings ...)
2023-10-03 18:50 ` [PATCH 4/6] t7419, t7420: Use test_cmp_config instead of grepping .gitmodules Jan Alexander Steffens (heftig)
@ 2023-10-03 18:50 ` Jan Alexander Steffens (heftig)
2023-10-03 18:50 ` [PATCH 6/6] t7420: " Jan Alexander Steffens (heftig)
` (2 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
To: git; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 6/6] t7420: Test that we correctly handle renamed submodules
2023-10-03 18:50 ` [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch} Jan Alexander Steffens (heftig)
` (3 preceding siblings ...)
2023-10-03 18:50 ` [PATCH 5/6] t7419: Test that we correctly handle renamed submodules Jan Alexander Steffens (heftig)
@ 2023-10-03 18:50 ` Jan Alexander Steffens (heftig)
2023-10-04 1:10 ` [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch} Junio C Hamano
2023-11-21 20:32 ` [PATCH v2 " Jan Alexander Steffens (heftig)
6 siblings, 0 replies; 19+ messages in thread
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
To: git; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch}
2023-10-03 18:50 ` [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch} Jan Alexander Steffens (heftig)
` (4 preceding siblings ...)
2023-10-03 18:50 ` [PATCH 6/6] t7420: " Jan Alexander Steffens (heftig)
@ 2023-10-04 1:10 ` Junio C Hamano
2023-11-21 20:32 ` [PATCH v2 " Jan Alexander Steffens (heftig)
6 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2023-10-04 1:10 UTC (permalink / raw)
To: Jan Alexander Steffens (heftig); +Cc: git
"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.
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v2 1/6] submodule--helper: use submodule_from_path in set-{url,branch}
2023-10-03 18:50 ` [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch} Jan Alexander Steffens (heftig)
` (5 preceding siblings ...)
2023-10-04 1:10 ` [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch} Junio C Hamano
@ 2023-11-21 20:32 ` Jan Alexander Steffens (heftig)
2023-11-21 20:32 ` [PATCH v2 2/6] submodule--helper: return error from set-url when modifying failed Jan Alexander Steffens (heftig)
` (5 more replies)
6 siblings, 6 replies; 19+ messages in thread
From: Jan Alexander Steffens (heftig) @ 2023-11-21 20:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Shourya Shukla,
Ævar Arnfjörð Bjarmason, Denton Liu,
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 2/6] submodule--helper: return error from set-url when modifying failed
2023-11-21 20:32 ` [PATCH v2 " Jan Alexander Steffens (heftig)
@ 2023-11-21 20:32 ` Jan Alexander Steffens (heftig)
2023-11-21 20:32 ` [PATCH v2 3/6] t7419: actually test the branch switching Jan Alexander Steffens (heftig)
` (4 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Jan Alexander Steffens (heftig) @ 2023-11-21 20:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Shourya Shukla,
Ævar Arnfjörð Bjarmason, Denton Liu,
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 3/6] t7419: actually test the branch switching
2023-11-21 20:32 ` [PATCH v2 " Jan Alexander Steffens (heftig)
2023-11-21 20:32 ` [PATCH v2 2/6] submodule--helper: return error from set-url when modifying failed Jan Alexander Steffens (heftig)
@ 2023-11-21 20:32 ` Jan Alexander Steffens (heftig)
2023-11-21 20:32 ` [PATCH v2 4/6] t7419, t7420: use test_cmp_config instead of grepping .gitmodules Jan Alexander Steffens (heftig)
` (3 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Jan Alexander Steffens (heftig) @ 2023-11-21 20:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Shourya Shukla,
Ævar Arnfjörð Bjarmason, Denton Liu,
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 4/6] t7419, t7420: use test_cmp_config instead of grepping .gitmodules
2023-11-21 20:32 ` [PATCH v2 " Jan Alexander Steffens (heftig)
2023-11-21 20:32 ` [PATCH v2 2/6] submodule--helper: return error from set-url when modifying failed Jan Alexander Steffens (heftig)
2023-11-21 20:32 ` [PATCH v2 3/6] t7419: actually test the branch switching Jan Alexander Steffens (heftig)
@ 2023-11-21 20:32 ` Jan Alexander Steffens (heftig)
2023-11-21 20:32 ` [PATCH v2 5/6] t7419: test that we correctly handle renamed submodules Jan Alexander Steffens (heftig)
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Jan Alexander Steffens (heftig) @ 2023-11-21 20:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Shourya Shukla,
Ævar Arnfjörð Bjarmason, Denton Liu,
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 5/6] t7419: test that we correctly handle renamed submodules
2023-11-21 20:32 ` [PATCH v2 " Jan Alexander Steffens (heftig)
` (2 preceding siblings ...)
2023-11-21 20:32 ` [PATCH v2 4/6] t7419, t7420: use test_cmp_config instead of grepping .gitmodules Jan Alexander Steffens (heftig)
@ 2023-11-21 20:32 ` Jan Alexander Steffens (heftig)
2023-11-21 20:32 ` [PATCH v2 6/6] t7420: " Jan Alexander Steffens (heftig)
2023-11-22 7:54 ` [PATCH v2 1/6] submodule--helper: use submodule_from_path in set-{url,branch} Junio C Hamano
5 siblings, 0 replies; 19+ messages in thread
From: Jan Alexander Steffens (heftig) @ 2023-11-21 20:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Shourya Shukla,
Ævar Arnfjörð Bjarmason, Denton Liu,
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 6/6] t7420: test that we correctly handle renamed submodules
2023-11-21 20:32 ` [PATCH v2 " Jan Alexander Steffens (heftig)
` (3 preceding siblings ...)
2023-11-21 20:32 ` [PATCH v2 5/6] t7419: test that we correctly handle renamed submodules Jan Alexander Steffens (heftig)
@ 2023-11-21 20:32 ` Jan Alexander Steffens (heftig)
2023-11-22 7:54 ` [PATCH v2 1/6] submodule--helper: use submodule_from_path in set-{url,branch} Junio C Hamano
5 siblings, 0 replies; 19+ messages in thread
From: Jan Alexander Steffens (heftig) @ 2023-11-21 20:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Shourya Shukla,
Ævar Arnfjörð Bjarmason, Denton Liu,
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/6] submodule--helper: use submodule_from_path in set-{url,branch}
2023-11-21 20:32 ` [PATCH v2 " Jan Alexander Steffens (heftig)
` (4 preceding siblings ...)
2023-11-21 20:32 ` [PATCH v2 6/6] t7420: " Jan Alexander Steffens (heftig)
@ 2023-11-22 7:54 ` Junio C Hamano
2023-11-22 9:50 ` Jan Alexander Steffens (heftig)
5 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2023-11-22 7:54 UTC (permalink / raw)
To: Jan Alexander Steffens (heftig)
Cc: git, Shourya Shukla, Ævar Arnfjörð Bjarmason,
Denton Liu
"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.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/6] submodule--helper: use submodule_from_path in set-{url,branch}
2023-11-22 7:54 ` [PATCH v2 1/6] submodule--helper: use submodule_from_path in set-{url,branch} Junio C Hamano
@ 2023-11-22 9:50 ` Jan Alexander Steffens (heftig)
0 siblings, 0 replies; 19+ messages in thread
From: Jan Alexander Steffens (heftig) @ 2023-11-22 9:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Shourya Shukla, Ævar Arnfjörð Bjarmason,
Denton Liu
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.
^ permalink raw reply [flat|nested] 19+ messages in thread