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

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

On 8/19/2021 4:09 PM, Emily Shaffer wrote:
...

quoted

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

quoted

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.

quoted

 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.

quoted

 }
 
 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