| Message ID | 20220301002613.1459916-3-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | teach submodules to know they're submodules | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > + /* > + * Note location of superproject's gitdir. Because the submodule already > + * has a gitdir and local config, we can store this pointer from > + * worktree config to worktree config, if the submodule has > + * extensions.worktreeConfig set. > + */ Probably the comment is a bit stale. There is no longer a pointer or location of superproject's gitdir recorded anywhere. > + strbuf_addf(&config_path, "%s/config", real_new_git_dir); > + git_configset_init(&sub_cs); > + git_configset_add_file(&sub_cs, config_path.buf); > + > + git_config_set_in_file(config_path.buf, "submodule.hasSuperproject", > + "true"); > + > + git_configset_clear(&sub_cs); > + strbuf_release(&config_path); > + strbuf_release(&sb); > free(old_git_dir); > free(real_old_git_dir); > free(real_new_git_dir); > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index 40cf8d89aa..833fa01961 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -115,6 +115,10 @@ inspect() { > git -C "$sub_dir" rev-parse HEAD >head-sha1 && > git -C "$sub_dir" update-index --refresh && > git -C "$sub_dir" diff-files --exit-code && > + > + # Ensure that submodule.hasSuperproject is set. > + git -C "$sub_dir" config "submodule.hasSuperproject" Are we sufficiently happy to see the variable is set to anything, or do we require it to be set to boolean true? If the former, the above is fine, with trailing && added. If the latter, then something like val=$(git config --type=bool "submodule.hasSuperproject") && test "$val" = true && would be more appropriate, but I wonder something like test_config_is () { local var expect val var="$1" expect="$2" shift 2 val=$(git "$@" config --type=bool "$var") && test "$val" = "$expect" } would be in order. That would allow us to write test_config_is submodule.hasSuperproject true -C "$sub_dir" && > git -C "$sub_dir" clean -n -d -x >untracked > } > > diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh > index 11cccbb333..422c3cc343 100755 > --- a/t/t7406-submodule-update.sh > +++ b/t/t7406-submodule-update.sh > @@ -1061,4 +1061,12 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s > ) > ' > > +test_expect_success 'submodule update adds submodule.hasSuperproject to older repos' ' > + (cd super && > + git -C submodule config --unset submodule.hasSuperproject && Are we testing that submodule.hasSuperproject is set, and that it can successfully be unset? "config --unset no.such.var" will exit with non-zero status. > + git submodule update && > + git -C submodule config submodule.hasSuperproject Ditto.
On Mon, Feb 28, 2022 at 11:00:57PM -0800, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > + /* > > + * Note location of superproject's gitdir. Because the submodule already > > + * has a gitdir and local config, we can store this pointer from > > + * worktree config to worktree config, if the submodule has > > + * extensions.worktreeConfig set. > > + */ > > Probably the comment is a bit stale. There is no longer a pointer > or location of superproject's gitdir recorded anywhere. Thanks. I considered replacing it with a new comment about "now we'll note that it's got a superproject", but I think that's clear enough from the config set line, so I deleted the comment entirely. > > > + strbuf_addf(&config_path, "%s/config", real_new_git_dir); > > + git_configset_init(&sub_cs); > > + git_configset_add_file(&sub_cs, config_path.buf); > > + > > + git_config_set_in_file(config_path.buf, "submodule.hasSuperproject", > > + "true"); > > + > > + git_configset_clear(&sub_cs); > > + strbuf_release(&config_path); > > + strbuf_release(&sb); > > free(old_git_dir); > > free(real_old_git_dir); > > free(real_new_git_dir); > > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > > index 40cf8d89aa..833fa01961 100755 > > --- a/t/t7400-submodule-basic.sh > > +++ b/t/t7400-submodule-basic.sh > > @@ -115,6 +115,10 @@ inspect() { > > git -C "$sub_dir" rev-parse HEAD >head-sha1 && > > git -C "$sub_dir" update-index --refresh && > > git -C "$sub_dir" diff-files --exit-code && > > + > > + # Ensure that submodule.hasSuperproject is set. > > + git -C "$sub_dir" config "submodule.hasSuperproject" > > Are we sufficiently happy to see the variable is set to anything, or > do we require it to be set to boolean true? > > If the former, the above is fine, with trailing && added. > > If the latter, then something like > > val=$(git config --type=bool "submodule.hasSuperproject") && > test "$val" = true && > > would be more appropriate, but I wonder something like > > test_config_is () { > local var expect val > var="$1" expect="$2" > shift 2 > val=$(git "$@" config --type=bool "$var") && > test "$val" = "$expect" > } > > would be in order. That would allow us to write > > test_config_is submodule.hasSuperproject true -C "$sub_dir" && > This seemed neat, so I started to look into implementing it, and found `test_cmp_config()` which takes additional args to pass to `git config` - so I should be able to achieve this same thing with `test_config_is -C "$sub_dir" --type=bool true submodule.hasSuperproject`. > > git -C "$sub_dir" clean -n -d -x >untracked > > } > > > > diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh > > index 11cccbb333..422c3cc343 100755 > > --- a/t/t7406-submodule-update.sh > > +++ b/t/t7406-submodule-update.sh > > @@ -1061,4 +1061,12 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s > > ) > > ' > > > > +test_expect_success 'submodule update adds submodule.hasSuperproject to older repos' ' > > + (cd super && > > + git -C submodule config --unset submodule.hasSuperproject && > > Are we testing that submodule.hasSuperproject is set, and that > it can successfully be unset? "config --unset no.such.var" will > exit with non-zero status. > > > + git submodule update && > > + git -C submodule config submodule.hasSuperproject > > Ditto. Ah, thanks.
Reviewing this series lightly because I will need to base 'ar/submodule-update reroll pt 2' on this (pt.1 is at https://lore.kernel.org/git/20220305001401.20888-1-chooglen@google.com). Emily Shaffer <emilyshaffer@google.com> writes: > diff --git a/git-submodule.sh b/git-submodule.sh > index 652861aa66..59dffda775 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -449,6 +449,9 @@ cmd_update() > ;; > esac > > + # Note that the submodule is a submodule. > + git -C "$sm_path" config submodule.hasSuperproject "true" > + > if test -n "$recursive" > then > ( This hunk has a textual conflict with 'ar/submodule-update reroll pt 2', but the fix is easy - just teach "git submodule--helper update" to set the config in C. > diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh > index 11cccbb333..422c3cc343 100755 > --- a/t/t7406-submodule-update.sh > +++ b/t/t7406-submodule-update.sh > @@ -1061,4 +1061,12 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s > ) > ' > > +test_expect_success 'submodule update adds submodule.hasSuperproject to older repos' ' > + (cd super && > + git -C submodule config --unset submodule.hasSuperproject && > + git submodule update && > + git -C submodule config submodule.hasSuperproject > + ) > +' > + > test_done I think there is a gap in the test coverage. I notice that this doesn't test that we set submodule.hasSuperproject when the submodule is cloned for the first time with 'git submodule update'. I thought that maybe the test for this was here... > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index 40cf8d89aa..833fa01961 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -115,6 +115,10 @@ inspect() { > git -C "$sub_dir" rev-parse HEAD >head-sha1 && > git -C "$sub_dir" update-index --refresh && > git -C "$sub_dir" diff-files --exit-code && > + > + # Ensure that submodule.hasSuperproject is set. > + git -C "$sub_dir" config "submodule.hasSuperproject" > + > git -C "$sub_dir" clean -n -d -x >untracked > } > But when I removed the "set submodule.hasSuperproject in submodule" line, i.e. git -C "$sm_path" config submodule.hasSuperproject "true" t7400 still passes.
Glen Choo <chooglen@google.com> writes: > Emily Shaffer <emilyshaffer@google.com> writes: > >> diff --git a/git-submodule.sh b/git-submodule.sh >> index 652861aa66..59dffda775 100755 >> --- a/git-submodule.sh >> +++ b/git-submodule.sh >> @@ -449,6 +449,9 @@ cmd_update() >> ;; >> esac >> >> + # Note that the submodule is a submodule. >> + git -C "$sm_path" config submodule.hasSuperproject "true" >> + >> if test -n "$recursive" >> then >> ( > > This hunk has a textual conflict with 'ar/submodule-update reroll pt > 2', but the fix is easy - just teach "git submodule--helper update" to > set the config in C. From our dicussion (offline), it turns out this statement isn't really correct because we do set the config in C, but we do it in clone_submodule(): diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index c5d3fc3817..92986646bc 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1839,6 +1839,11 @@ static int clone_submodule(struct module_clone_data *clone_data) git_config_set_in_file(p, "submodule.alternateErrorStrategy", error_strategy); + /* + * Teach the submodule that it's a submodule. + */ + git_config_set_in_file(p, "submodule.hasSuperproject", "true"); + free(sm_alternate); free(error_strategy); >> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh >> index 11cccbb333..422c3cc343 100755 >> --- a/t/t7406-submodule-update.sh >> +++ b/t/t7406-submodule-update.sh >> @@ -1061,4 +1061,12 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s >> ) >> ' >> >> +test_expect_success 'submodule update adds submodule.hasSuperproject to older repos' ' >> + (cd super && >> + git -C submodule config --unset submodule.hasSuperproject && >> + git submodule update && >> + git -C submodule config submodule.hasSuperproject >> + ) >> +' >> + >> test_done > > > I think there is a gap in the test coverage. I notice that this doesn't > test that we set submodule.hasSuperproject when the submodule is cloned > for the first time with 'git submodule update'. I thought that maybe the > test for this was here... > >> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh >> index 40cf8d89aa..833fa01961 100755 >> --- a/t/t7400-submodule-basic.sh >> +++ b/t/t7400-submodule-basic.sh >> @@ -115,6 +115,10 @@ inspect() { >> git -C "$sub_dir" rev-parse HEAD >head-sha1 && >> git -C "$sub_dir" update-index --refresh && >> git -C "$sub_dir" diff-files --exit-code && >> + >> + # Ensure that submodule.hasSuperproject is set. >> + git -C "$sub_dir" config "submodule.hasSuperproject" >> + >> git -C "$sub_dir" clean -n -d -x >untracked >> } >> > > But when I removed the "set submodule.hasSuperproject in submodule" > line, i.e. > > git -C "$sm_path" config submodule.hasSuperproject "true" > > t7400 still passes. So we would expect that newly cloned submodules would pass even without this .sh line. I don't think we need to do this twice in C and in shell. We can move this line: + git_config_set_in_file(p, "submodule.hasSuperproject", "true"); into run-update-procedure (and out of clone_submodule()). This way it's guaranteed to touch every submodule (newly cloned or not).
diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt index ee454f8126..99d5260b8e 100644 --- a/Documentation/config/submodule.txt +++ b/Documentation/config/submodule.txt @@ -91,3 +91,9 @@ submodule.alternateErrorStrategy:: `ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore` or `info`, and if there is an error with the computed alternate, the clone proceeds as if no alternate was specified. + +submodule.hasSuperproject:: + Indicates whether this repository is a submodule. If this config is set + to 'true', Git may traverse the filesystem above this submodule in order + to identify the superproject. It is set automatically during submodule + creation, update, and 'git submodule absorbgitdir'. diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index c5d3fc3817..92986646bc 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1839,6 +1839,11 @@ static int clone_submodule(struct module_clone_data *clone_data) git_config_set_in_file(p, "submodule.alternateErrorStrategy", error_strategy); + /* + * Teach the submodule that it's a submodule. + */ + git_config_set_in_file(p, "submodule.hasSuperproject", "true"); + free(sm_alternate); free(error_strategy); diff --git a/git-submodule.sh b/git-submodule.sh index 652861aa66..59dffda775 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -449,6 +449,9 @@ cmd_update() ;; esac + # Note that the submodule is a submodule. + git -C "$sm_path" config submodule.hasSuperproject "true" + if test -n "$recursive" then ( diff --git a/submodule.c b/submodule.c index c689070524..741104af8a 100644 --- a/submodule.c +++ b/submodule.c @@ -2097,6 +2097,8 @@ static void relocate_single_git_dir_into_superproject(const char *path) char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL; struct strbuf new_gitdir = STRBUF_INIT; const struct submodule *sub; + struct config_set sub_cs; + struct strbuf config_path = STRBUF_INIT, sb = STRBUF_INIT; if (submodule_uses_worktrees(path)) die(_("relocate_gitdir for submodule '%s' with " @@ -2127,6 +2129,22 @@ static void relocate_single_git_dir_into_superproject(const char *path) relocate_gitdir(path, real_old_git_dir, real_new_git_dir); + /* + * Note location of superproject's gitdir. Because the submodule already + * has a gitdir and local config, we can store this pointer from + * worktree config to worktree config, if the submodule has + * extensions.worktreeConfig set. + */ + strbuf_addf(&config_path, "%s/config", real_new_git_dir); + git_configset_init(&sub_cs); + git_configset_add_file(&sub_cs, config_path.buf); + + git_config_set_in_file(config_path.buf, "submodule.hasSuperproject", + "true"); + + git_configset_clear(&sub_cs); + strbuf_release(&config_path); + strbuf_release(&sb); free(old_git_dir); free(real_old_git_dir); free(real_new_git_dir); diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 40cf8d89aa..833fa01961 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -115,6 +115,10 @@ inspect() { git -C "$sub_dir" rev-parse HEAD >head-sha1 && git -C "$sub_dir" update-index --refresh && git -C "$sub_dir" diff-files --exit-code && + + # Ensure that submodule.hasSuperproject is set. + git -C "$sub_dir" config "submodule.hasSuperproject" + git -C "$sub_dir" clean -n -d -x >untracked } diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index 11cccbb333..422c3cc343 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -1061,4 +1061,12 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s ) ' +test_expect_success 'submodule update adds submodule.hasSuperproject to older repos' ' + (cd super && + git -C submodule config --unset submodule.hasSuperproject && + git submodule update && + git -C submodule config submodule.hasSuperproject + ) +' + test_done diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh index 1cfa150768..187fb6bbbc 100755 --- a/t/t7412-submodule-absorbgitdirs.sh +++ b/t/t7412-submodule-absorbgitdirs.sh @@ -30,7 +30,9 @@ test_expect_success 'absorb the git dir' ' git status >actual.1 && git -C sub1 rev-parse HEAD >actual.2 && test_cmp expect.1 actual.1 && - test_cmp expect.2 actual.2 + test_cmp expect.2 actual.2 && + + git -C sub1 config submodule.hasSuperproject ' test_expect_success 'absorbing does not fail for deinitialized submodules' ' @@ -61,7 +63,9 @@ test_expect_success 'absorb the git dir in a nested submodule' ' git status >actual.1 && git -C sub1/nested rev-parse HEAD >actual.2 && test_cmp expect.1 actual.1 && - test_cmp expect.2 actual.2 + test_cmp expect.2 actual.2 && + + git -C sub1/nested config submodule.hasSuperproject ' test_expect_success 're-setup nested submodule' ' @@ -130,4 +134,46 @@ test_expect_success 'absorbing fails for a submodule with multiple worktrees' ' test_i18ngrep "not supported" error ' +test_expect_success 'absorbgitdirs works when called from a superproject worktree' ' + # set up a worktree of the superproject + git worktree add wt && + ( + cd wt && + + # create a new unembedded git dir + git init sub4 && + test_commit -C sub4 first && + git submodule add ./sub4 && + test_tick && + + # absorb the git dir + git submodule absorbgitdirs sub4 && + + # make sure the submodule noted the superproject + git -C sub4 config submodule.hasSuperproject + ) +' + +test_expect_success 'absorbgitdirs works with a submodule with worktree config' ' + # reuse the worktree of the superproject + ( + cd wt && + + # create a new unembedded git dir + git init sub5 && + test_commit -C sub5 first && + git submodule add ./sub5 && + test_tick && + + # turn on worktree configs for submodule + git -C sub5 config extensions.worktreeConfig true && + + # absorb the git dir + git submodule absorbgitdirs sub5 && + + # make sure the submodule noted the superproject + git -C sub5 config submodule.hasSuperproject + ) +' + test_done
Teach submodules a config variable indicating the fact that they are a submodule. If this config is set to false or unset, Git may assume the current repo is not a submodule. Git commands can use this variable to decide whether to traverse the filesystem and look for a superproject at all. 'git rev-parse --show-superproject-working-tree' can learn to exit early if this config is unset or false. Other newly added or implicit behavior - like "git status" showing the submodule's status in relation to the superproject, or a config shared between the superproject and submodule - can use this config to decide whether to search the parent directory to find a superproject. Introduce this config everywhere we add a new submodule, or touch one that already exists, so that we can proliferate it in repos which are already out in the world using submodules. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Helped-by: Junio C Hamano <gitster@pobox.com> --- Documentation/config/submodule.txt | 6 ++++ builtin/submodule--helper.c | 5 +++ git-submodule.sh | 3 ++ submodule.c | 18 +++++++++++ t/t7400-submodule-basic.sh | 4 +++ t/t7406-submodule-update.sh | 8 +++++ t/t7412-submodule-absorbgitdirs.sh | 50 ++++++++++++++++++++++++++++-- 7 files changed, 92 insertions(+), 2 deletions(-)