On Sat, Sep 04, 2021 at 02:27:46PM -0300, Matheus Tavares wrote:
>
> Emily Shaffer <emilyshaffer@xxxxxxxxxx> wrote:
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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).
Since relocate_single_git_dir() is the only place where the final
submodule gitdir is resolved, I'll leave the code block where it is;
neither side of the if/else in absorb_git_dir...() learns the final
"new" gitdir, so I'd rather not re-derive it.
Anyway, wouldn't the submodule-who-is-also-a-superproject have gone
through this same codepath itself? I'll see if I can find a test to
validate that.
>
> > 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.
Yep, done. Thanks.
>
> > + 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.
Thanks, fixed.
>
> > + 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
Sure.
>
> > +
> > + 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
Ah, thanks. I tried it out, even without the changes you mentioned
above, and this test passes. So I think as I expected, it works because
'sub1' goes through relocate_single_git_dir() as well as 'sub1/nested'.
Thanks for the careful review.
- Emily
