Re: [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs
- From
Matheus Tavares <matheus.bernardino@usp.br>
- Date
- Sep 4, 2021, 17:27 UTC
- Message-ID
- <20210904172746.3031-1-matheus.bernardino@usp.br>
- In-Reply-To
- <20210819200953.2105230-4-emilyshaffer@google.com>
Emily Shaffer <emilyshaffer@google.com> wrote:
Show 18 quoted lines
> > Already during 'git submodule add' we record a pointer to the > superproject's gitdir. However, this doesn't help brand-new > submodules created with 'git init' and later absorbed with 'git > submodule absorbgitdir'. Let's start adding that pointer during 'git > submodule absorbgitdir' too. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > submodule.c | 10 ++++++++++ > t/t7412-submodule-absorbgitdirs.sh | 9 ++++++++- > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/submodule.c b/submodule.c > index 0b1d9c1dde..4b314bf09c 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -2065,6 +2065,7 @@ static void relocate_single_git_dir_into_superproject(const char *path)
This function has only one caller, `absorb_git_dir_into_superproject()`. Perhaps it would be better to set submodule.superprojectGitdir in the caller, right after the `else` block in which `relocate...()` is called. This way, the config would also be set in the case of nested submodules where the inner one already had its gitdir absorbed (which is the case handled by the code in the `if` block).
Show 13 quoted lines
> char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
> char *new_git_dir;
> const struct submodule *sub;
> + struct strbuf config_path = STRBUF_INIT, sb = STRBUF_INIT;
>
> if (submodule_uses_worktrees(path))
> die(_("relocate_gitdir for submodule '%s' with "
> @@ -2096,6 +2097,15 @@ static void relocate_single_git_dir_into_superproject(const char *path)
>
> relocate_gitdir(path, real_old_git_dir, real_new_git_dir);
>
> + /* cache pointer to superproject's gitdir */
> + /* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */s/experimental/extensions/
On the Review Club, I mentioned we might want to save submodule.superprojectGitdir at the worktree config file. But Jonathan and Josh gave a better suggestion, which is to cache the superproject gitdir relative to the submodule's gitdir, instead of its working tree.
This way, the worktree config wouldn't be an issue. And more importantly, this would prevent `git mv` from making the cached path stale, as Stolee pointed out upthread.
> + strbuf_addf(&config_path, "%s/config", real_new_git_dir); > + git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir", > + relative_path(get_super_prefix_or_empty(), > + path, &sb));
In this code, `the_repository` corresponds to the superproject, right? I think `get_super_prefix_or_empty()` should instead be `absolute_path(get_git_dir())`, like you did on the previous patch.
And since the first argument to `relative_path()` will be an absolute path, I believe we also need to convert the second one to an absolute path. Otherwise, `relative_path()` would return the first argument as-is [1]. (I played around with using `get_git_dir()` directly as the first argument, but it seems this can sometimes already be absolute, in case of nested submodules.)
If we store the path as relative to the submodule's gitdir, it should be simpler, as `real_new_git_dir` is already absolute: git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir", relative_path(absolute_path(get_git_dir()) real_new_git_dir, &sb));
[1]: I'm not sure if this is intended or if it's a bug. I was expecting that, before comparing its two arguments, `relative_path()` would convert any relative path given as argument to absolute, using the current working dir path. But that's not the case.
Show 18 quoted lines
> + 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/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh > index 1cfa150768..e2d78e01df 100755 > --- a/t/t7412-submodule-absorbgitdirs.sh > +++ b/t/t7412-submodule-absorbgitdirs.sh > @@ -30,7 +30,14 @@ 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 && > + > + # make sure the submodule cached the superproject gitdir correctly > + test-tool path-utils real_path . >expect &&
This should be '.git' instead of '.', since the config caches the path to the superproject's gitdir. But ...
> + test-tool path-utils real_path \ > + "$(git -C sub1 config submodule.superprojectGitDir)" >actual &&
... I think we could also avoid converting to an absolute path here, so that we can test whether the setting is really caching a relative path. I.e., the test could be:
super_gitdir="$(git rev-parse --absolute-git-dir)" && test-tool path-utils relative_path "$super_gitdir" "$PWD/sub1" >expect && git -C sub1 config submodule.superprojectGitDir >actual && test_cmp expect actual
> + > + test_cmp expect actual > '
It may also be interesting to test if the config is correctly set when absorbing the gitdir of nested submodules:
diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh index e2d78e01df..c2e5e7dd1c 100755 --- a/t/t7412-submodule-absorbgitdirs.sh +++ b/t/t7412-submodule-absorbgitdirs.sh @@ -70,3 +70,8 @@ test_expect_success 'absorb the git dir in a nested submodule' ' test_cmp expect.1 actual.1 && - test_cmp expect.2 actual.2 + test_cmp expect.2 actual.2 && + + sub1_gitdir="$(git -C sub1 rev-parse --absolute-git-dir)" && + test-tool path-utils relative_path "$sub1_gitdir" "$PWD/sub1/nested" >expect && + git -C sub1/nested config submodule.superprojectGitDir >actual && + test_cmp expect actual ' --- >8 ---