Re: [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs

On Sat, Sep 04, 2021 at 02:27:46PM -0300, Matheus Tavares wrote:

Emily Shaffer [off-list ref] wrote:

quoted

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

quoted

 	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.

quoted

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

quoted

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

quoted

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

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