From: Emily Shaffer <emilyshaffer@google.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v3 2/4] introduce submodule.superprojectGitDir record
Date: Wed, 13 Oct 2021 12:36:52 -0700 [thread overview]
Message-ID: <YWc1VJgWymoPado9@google.com> (raw)
In-Reply-To: <9fecf160-9646-7e56-478c-aa5f8defa6a9@gmail.com>
On Thu, Aug 19, 2021 at 08:38:19PM -0400, Derrick Stolee wrote:
>
> On 8/19/2021 4:09 PM, Emily Shaffer wrote:
> ...
> > +submodule.superprojectGitDir::
> > + The relative path from the submodule's worktree to its superproject's
> > + gitdir. When Git is run in a repository, it usually makes no difference
> > + whether this repository is standalone or a submodule, but if this
> > + configuration variable is present, additional behavior may be possible,
> > + such as "git status" printing additional information about this
> > + submodule's status with respect to its superproject. This config should
> > + only be present in projects which are submodules, but is not guaranteed
> > + to be present in every submodule, so only optional value-added behavior
> > + should be linked to it. It is set automatically during
> > + submodule creation.
> > ++
> > + Because of this configuration variable, it is forbidden to use the
> > + same submodule worktree shared by multiple superprojects.
>
> nit: this paragraph linked with the "+" line should have no tabbing.
Done.
>
> Also, could we use the same submodule worktree for multiple superprojects
> _before_ this configuration variable? That seems wild to me. Or, is that
> not a new requirement?
I guess it'd be possible to do something pretty evil with symlinks? I'm
not sure why you would want to, though.
But now that I think about it more, I'm not sure that it would work, at
least if we understand submodule to mean "...and the gitdir lives in
.git/modules/ of the superproject".
If superA contains sub and superB contains a symlink to 'sub''s
worktree in superA, then wouldn't superA and superB both be trying to
contain their own gitdirs for sub? And having multiple gitdirs for a
worktree is an unacceptable state anyway.
Or maybe the issue is more like: you have super, which contains sub, and
you have super-wt, which is a worktree of super; to avoid duplicating
sub, you decided to use a symlink. So there's only one sub gitdir, and
only one super gitdir. It's a little awkward, but since submodule
worktrees aren't currently supported, I can see the appeal. In this
configuration, a path from submodule *worktree* to superproject gitdir,
which is what v3 and earlier propose, would be broken in one
superproject worktree or the other And having multiple gitdirs for a
worktree is an unacceptable state anyway.
Or maybe the issue is more like: you have super, which contains sub, and
you have super-wt, which is a worktree of super; to avoid duplicating
sub, you decided to use a symlink. So there's only one sub gitdir, and
only one super gitdir. It's a little awkward, but since submodule
worktrees aren't currently supported, I can see the appeal. In this
configuration, a path from submodule *worktree* to superproject gitdir,
which is what v3 and earlier propose, would be broken in one
superproject worktree or the other. But as I'm proposing in v4, folks in
the review club pointed out to me that a pointer from gitdir to gitdir
makes more sense - and that would fix this concern, too, because sub and
the symlink of sub would both share a single gitdir, and that gitdir
would point to the single gitdir of super and super-wt.
All a long way to say: I think v4 will fix it by originating the
relative path from submodule gitdir, instead. And I will remove the
extra paragraph - I think it is just adding confusion around a case that
nobody would really want to use...
>
> Perhaps you mean something like this instead:
>
> It is forbidden to use the same submodule worktree for multiple
> superprojects, so this configuration variable stores the unique
> superproject and is not multi-valued.
>
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index d55f6262e9..d60fcd2c7d 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)
> > git_config_set_in_file(p, "submodule.alternateErrorStrategy",
> > error_strategy);
> >
> > + git_config_set_in_file(p, "submodule.superprojectGitdir",
> > + relative_path(absolute_path(get_git_dir()),
> > + path, &sb));
> > +
>
> I see that all new submodules will have this configuration set. But we will
> also live in a world where some existing submodules do not have this variable
> set. I'll look elsewhere for compatibility checks.
Yep, the series intended to add them piecemeal where possible, over the
course of a handful of commits.
>
> > inspect() {
> > - dir=$1 &&
> > -
> > - git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&
> > - { git -C "$dir" symbolic-ref HEAD || :; } >head &&
> > - git -C "$dir" rev-parse HEAD >head-sha1 &&
> > - git -C "$dir" update-index --refresh &&
> > - git -C "$dir" diff-files --exit-code &&
> > - git -C "$dir" clean -n -d -x >untracked
> > + sub_dir=$1 &&
> > + super_dir=$2 &&
> > +
> > + git -C "$sub_dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&
> > + { git -C "$sub_dir" symbolic-ref HEAD || :; } >head &&
> > + git -C "$sub_dir" rev-parse HEAD >head-sha1 &&
> > + git -C "$sub_dir" update-index --refresh &&
> > + git -C "$sub_dir" diff-files --exit-code &&
> > + cached_super_dir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" &&
> > + [ "$(git -C "$super_dir" rev-parse --absolute-git-dir)" \
> > + -ef "$sub_dir/$cached_super_dir" ] &&
> > + git -C "$sub_dir" clean -n -d -x >untracked
>
> You rewrote this test in the previous patch, and now every line is changed
> because you renamed 'dir' to 'sub_dir'. Could the previous patch use
> 'sub_dir' from the start so this change only shows the new lines instead of
> many edited lines?
Sure.
>
> > }
> >
> > test_expect_success 'submodule add' '
> > @@ -138,7 +142,7 @@ test_expect_success 'submodule add' '
> > ) &&
> >
> > rm -f heads head untracked &&
> > - inspect addtest/submod &&
> > + inspect addtest/submod addtest &&
>
> Similarly, I would not be upset to see these lines be changed just the
> once, even if the second argument is ignored for a single commit. But
> this nitpick is definitely less important since I could see taste
> swaying things either way.
I feel less interested in that nit; I think a mechanical "strip the
useless arg" change + a mechanical "add an unrelated useful arg" change
is easier to review than doing both at once.
- Emily
next prev parent reply other threads:[~2021-10-13 19:37 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 [this message]
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
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=YWc1VJgWymoPado9@google.com \
--to=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=stolee@gmail.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.