Re: [PATCH v3 2/4] introduce submodule.superprojectGitDir record

From: Emily Shaffer <emilyshaffer@google.com>
Date: Oct 13, 2021, 19:36 UTC
Message-ID: <YWc1VJgWymoPado9@google.com>
In-Reply-To: <9fecf160-9646-7e56-478c-aa5f8defa6a9@gmail.com>

On Thu, Aug 19, 2021 at 08:38:19PM -0400, Derrick Stolee wrote:

Show 19 quoted lines

> 
> 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...

Show 23 quoted lines

> 
> 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.

Show 27 quoted lines

> 
> >  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.

Show 15 quoted lines

> 
> >  }
> >  
> >  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