From: Emily Shaffer <emilyshaffer@google.com>
To: Matheus Tavares <matheus.bernardino@usp.br>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs
Date: Thu, 14 Oct 2021 11:40:42 -0700 [thread overview]
Message-ID: <YWh5qvHdTyjoToYX@google.com> (raw)
In-Reply-To: <20210904172746.3031-1-matheus.bernardino@usp.br>
On Sat, Sep 04, 2021 at 02:27:46PM -0300, Matheus Tavares wrote:
>
> Emily Shaffer <emilyshaffer@google.com> 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@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).
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
next prev parent reply other threads:[~2021-10-14 18:40 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-11 22:54 [RFC PATCH 0/4] cache parent project's gitdir in submodules Emily Shaffer
2021-06-11 22:54 ` [RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer
2021-06-14 4:52 ` Junio C Hamano
2021-06-11 22:54 ` [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer
2021-06-14 5:09 ` Junio C Hamano
2021-06-15 22:00 ` Emily Shaffer
2021-06-11 22:54 ` [RFC PATCH 3/4] submodule: cache superproject gitdir during absorbgitdirs Emily Shaffer
2021-06-14 6:18 ` Junio C Hamano
2021-06-11 22:54 ` [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update' Emily Shaffer
2021-06-14 6:22 ` Junio C Hamano
2021-06-15 21:27 ` Emily Shaffer
2021-06-12 20:12 ` [RFC PATCH 0/4] cache parent project's gitdir in submodules Jacob Keller
2021-06-14 7:26 ` Junio C Hamano
2021-06-15 21:18 ` Emily Shaffer
2021-06-16 0:45 ` [PATCH v2 " Emily Shaffer
2021-06-16 0:45 ` [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer
2021-07-27 17:12 ` Jonathan Tan
2021-08-19 17:46 ` Emily Shaffer
2021-06-16 0:45 ` [PATCH v2 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer
2021-06-16 4:40 ` Junio C Hamano
2021-06-16 4:43 ` Junio C Hamano
2021-06-18 0:03 ` Emily Shaffer
2021-06-18 0:00 ` Emily Shaffer
2021-07-27 17:46 ` Jonathan Tan
2021-08-19 17:53 ` Emily Shaffer
2021-10-14 19:25 ` Ævar Arnfjörð Bjarmason
2021-06-16 0:45 ` [PATCH v2 3/4] submodule: cache superproject gitdir during absorbgitdirs Emily Shaffer
2021-06-16 0:45 ` [PATCH v2 4/4] submodule: cache superproject gitdir during 'update' Emily Shaffer
2021-07-27 17:51 ` Jonathan Tan
2021-08-19 18:02 ` Emily Shaffer
2021-08-19 20:09 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Emily Shaffer
2021-08-19 20:09 ` [PATCH v3 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer
2021-08-19 20:09 ` [PATCH v3 2/4] introduce submodule.superprojectGitDir record Emily Shaffer
2021-08-20 0:38 ` Derrick Stolee
2021-10-13 19:36 ` Emily Shaffer
2021-09-04 17:20 ` Matheus Tavares
2021-10-13 19:39 ` Emily Shaffer
2021-08-19 20:09 ` [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs Emily Shaffer
2021-08-20 0:50 ` Derrick Stolee
2021-10-13 19:42 ` Emily Shaffer
2021-09-04 17:27 ` Matheus Tavares
2021-10-14 18:40 ` Emily Shaffer [this message]
2021-08-19 20:09 ` [PATCH v3 4/4] submodule: record superproject gitdir during 'update' Emily Shaffer
2021-08-20 0:59 ` Derrick Stolee
2021-10-14 18:45 ` Emily Shaffer
2021-08-19 21:56 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Junio C Hamano
2021-08-20 1:09 ` Derrick Stolee
2021-10-13 18:51 ` Emily Shaffer
2021-10-14 17:12 ` Derrick Stolee
2021-10-14 18:52 ` Emily Shaffer
2021-09-04 17:50 ` Matheus Tavares Bernardino
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YWh5qvHdTyjoToYX@google.com \
--to=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=matheus.bernardino@usp.br \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).