[RFC PATCH 0/4] cache parent project's gitdir in submodules

[RFC PATCH 0/4] cache parent project's gitdir in submodules

[Emily Shaffer]

It's necessary for a superproject to know which submodules it contains.
However, historically submodules do not know they are anything but a
normal single-repo Git project (or a superproject, in nested-submodules
cases). This decision does help prevent us from having to support
divergent behaviors in submodule projects vs. superprojects, which makes
sure Git is (somewhat) less confusing for the reader, and helps simplify
our code.

One could imagine, though, some conveniences we could gain from
submodules learning added behavior (though not necessarily *different*
behavior) to provide more context about the state of the project as a
whole, and to make large submodule-based projects easier to work with.
One example is a series[1] I sent some time ago, adding a config to be
shared between the superproject and all its submodules. The RFC[2] I
sent around the same time mentions a few other examples, such as "git
status" run in the submodule noticing whether the superproject has a
commit referencing the submodule's current HEAD.

It's expensive and non-definitive to try and guess whether or not the
current repo is a submodule. submodule.c:get_superproject_working_tree()
does so by essentially running 'git -C .. ls-files -- <own-path>',
invoking an additional process. get_superproject_working_tree() is not
called often, so that's mostly fine. However, [1] attempted to include
an additional config located in the superproject's gitdir by running
'git -C .. rev-parse --git-dir' during startup - a little expensive in
the best case, because it's an extra process, but extremely expensive in
the case when the current repo is *not* a submodule, because we hunt all
the way up the filesystem looking for a '.git'. Adding that cost to
every startup is infeasible.

To that end, in this series I propose caching a path to the
superproject's gitdir - by having the superproject write that relative
path to the submodule's config on creation or update. The goal here is
*not* to say "If I am a submodule, I must have
submodule.superprojectGitDir set" - but instead to say "If I have
submodule.superprojectGitDir set, then I must be a submodule." That is,
I expect we will find edge cases where a submodule was introduced in
some interesting way that bypassed any of the patches below, and
therefore doesn't have the superproject's gitdir cached.

The combination of these two rules:
 - Anything relying on submodule.superprojectGitDir must be nice to
   have, but not essential, because
 - It's possible for a submodule to be valid without having
   submodule.superprojectGitDir set
makes me feel more comfortable with the idea of submodules learning
additional behavior based on this config. I feel pretty unconfident in
our ability to ensure that *every* submodule has this config set.

The series covers a few paths for introducing that config, which I'm
hoping covers most cases.
 - "git submodule update" (which seems to be part of the "git submodule
   init" flow)
 - "git submodule absorbgitdir" to convert a "git init"'d repo into a
   submodule

Notably, we can only really set this config when 'the_repository' is the
superproject - that appears to be the only time when we know the gitdirs
of both the superproject and the submodule.

I'm expecting folks may have a lot to say about this, so I look forward
to discussion :)

 - Emily

1: https://lore.kernel.org/git/20210423001539.4059524-1-emilyshaffer@google.com
2: https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com

Emily Shaffer (4):
  t7400-submodule-basic: modernize inspect() helper
  introduce submodule.superprojectGitDir cache
  submodule: cache superproject gitdir during absorbgitdirs
  submodule: cache superproject gitdir during 'update'

 builtin/submodule--helper.c        |  4 +++
 git-submodule.sh                   |  9 ++++++
 submodule.c                        | 10 ++++++
 t/t7400-submodule-basic.sh         | 49 ++++++++++++++----------------
 t/t7406-submodule-update.sh        | 10 ++++++
 t/t7412-submodule-absorbgitdirs.sh |  1 +
 6 files changed, 57 insertions(+), 26 deletions(-)

-- 
2.32.0.272.g935e593368-goog

[RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper

[Emily Shaffer]

Since the inspect() helper in the submodule-basic test suite was
written, 'git -C <dir>' was added. By using -C, we no longer need a
reference to the base directory for the test. This simplifies callsites,
and will make the addition of other arguments in later patches more
readable.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 t/t7400-submodule-basic.sh | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index a924fdb7a6..f5dc051a6e 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' '
 # generates, which will expand symbolic links.
 submodurl=$(pwd -P)
 
-listbranches() {
-	git for-each-ref --format='%(refname)' 'refs/heads/*'
-}
-
 inspect() {
 	dir=$1 &&
-	dotdot="${2:-..}" &&
 
-	(
-		cd "$dir" &&
-		listbranches >"$dotdot/heads" &&
-		{ git symbolic-ref HEAD || :; } >"$dotdot/head" &&
-		git rev-parse HEAD >"$dotdot/head-sha1" &&
-		git update-index --refresh &&
-		git diff-files --exit-code &&
-		git clean -n -d -x >"$dotdot/untracked"
-	)
+	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
 }
 
+
 test_expect_success 'submodule add' '
 	echo "refs/heads/main" >expect &&
 
@@ -146,7 +139,7 @@ test_expect_success 'submodule add' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/submod ../.. &&
+	inspect addtest/submod &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -237,7 +230,7 @@ test_expect_success 'submodule add --branch' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/submod-branch ../.. &&
+	inspect addtest/submod-branch &&
 	test_cmp expect-heads heads &&
 	test_cmp expect-head head &&
 	test_must_be_empty untracked
@@ -253,7 +246,7 @@ test_expect_success 'submodule add with ./ in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/dotsubmod/frotz ../../.. &&
+	inspect addtest/dotsubmod/frotz &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -269,7 +262,7 @@ test_expect_success 'submodule add with /././ in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/dotslashdotsubmod/frotz ../../.. &&
+	inspect addtest/dotslashdotsubmod/frotz &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -285,7 +278,7 @@ test_expect_success 'submodule add with // in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/slashslashsubmod/frotz ../../.. &&
+	inspect addtest/slashslashsubmod/frotz &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -301,7 +294,7 @@ test_expect_success 'submodule add with /.. in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod ../.. &&
+	inspect addtest/realsubmod &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -317,7 +310,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod2 ../.. &&
+	inspect addtest/realsubmod2 &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -348,7 +341,7 @@ test_expect_success 'submodule add in subdirectory' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod3 ../.. &&
+	inspect addtest/realsubmod3 &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
-- 
2.32.0.272.g935e593368-goog

Re: [RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> Since the inspect() helper in the submodule-basic test suite was
> written, 'git -C <dir>' was added. By using -C, we no longer need a
> reference to the base directory for the test. This simplifies callsites,
> and will make the addition of other arguments in later patches more
> readable.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  t/t7400-submodule-basic.sh | 37 +++++++++++++++----------------------
>  1 file changed, 15 insertions(+), 22 deletions(-)
>
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index a924fdb7a6..f5dc051a6e 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' '
>  # generates, which will expand symbolic links.
>  submodurl=$(pwd -P)
>  
> -listbranches() {
> -	git for-each-ref --format='%(refname)' 'refs/heads/*'
> -}
> -
>  inspect() {
>  	dir=$1 &&
> -	dotdot="${2:-..}" &&
>  
> -	(
> -		cd "$dir" &&
> -		listbranches >"$dotdot/heads" &&
> -		{ git symbolic-ref HEAD || :; } >"$dotdot/head" &&
> -		git rev-parse HEAD >"$dotdot/head-sha1" &&
> -		git update-index --refresh &&
> -		git diff-files --exit-code &&
> -		git clean -n -d -x >"$dotdot/untracked"
> -	)
> +	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
>  }

Quite straight-forward.

> -	inspect addtest/submod ../.. &&
> +	inspect addtest/submod &&

And specifically the losing ../.. is pleasing to the eye, especially
because the old "dotdot" thing was mechanically computable from "dir".

Nicely done.

[RFC PATCH 2/4] introduce submodule.superprojectGitDir cache

[Emily Shaffer]

Teach submodules a reference to their superproject's gitdir. This allows
us to A) know that we're running from a submodule, and B) have a
shortcut to the superproject's vitals, for example, configs.

By using a relative path instead of an absolute path, we can move the
superproject directory around on the filesystem without breaking the
submodule's cache.

Since this cached value is only introduced during new submodule creation
via `git submodule add`, though, there is more work to do to allow the
cache to be created at other times.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 builtin/submodule--helper.c |  4 ++++
 t/t7400-submodule-basic.sh  | 40 ++++++++++++++++++++-----------------
 2 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9d505a6329..3d6fd54c9b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1909,6 +1909,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));
+
 	free(sm_alternate);
 	free(error_strategy);
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index f5dc051a6e..e45f42588f 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' '
 submodurl=$(pwd -P)
 
 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
 }
 
 
@@ -139,7 +143,7 @@ test_expect_success 'submodule add' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/submod &&
+	inspect addtest/submod addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -230,7 +234,7 @@ test_expect_success 'submodule add --branch' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/submod-branch &&
+	inspect addtest/submod-branch addtest &&
 	test_cmp expect-heads heads &&
 	test_cmp expect-head head &&
 	test_must_be_empty untracked
@@ -246,7 +250,7 @@ test_expect_success 'submodule add with ./ in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/dotsubmod/frotz &&
+	inspect addtest/dotsubmod/frotz addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -262,7 +266,7 @@ test_expect_success 'submodule add with /././ in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/dotslashdotsubmod/frotz &&
+	inspect addtest/dotslashdotsubmod/frotz addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -278,7 +282,7 @@ test_expect_success 'submodule add with // in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/slashslashsubmod/frotz &&
+	inspect addtest/slashslashsubmod/frotz addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -294,7 +298,7 @@ test_expect_success 'submodule add with /.. in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod &&
+	inspect addtest/realsubmod addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -310,7 +314,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod2 &&
+	inspect addtest/realsubmod2 addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -341,7 +345,7 @@ test_expect_success 'submodule add in subdirectory' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod3 &&
+	inspect addtest/realsubmod3 addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -482,7 +486,7 @@ test_expect_success 'update should work when path is an empty dir' '
 	git submodule update -q >update.out &&
 	test_must_be_empty update.out &&
 
-	inspect init &&
+	inspect init . &&
 	test_cmp expect head-sha1
 '
 
@@ -541,7 +545,7 @@ test_expect_success 'update should checkout rev1' '
 	echo "$rev1" >expect &&
 
 	git submodule update init &&
-	inspect init &&
+	inspect init . &&
 
 	test_cmp expect head-sha1
 '
-- 
2.32.0.272.g935e593368-goog

Re: [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> Teach submodules a reference to their superproject's gitdir. This allows
> us to A) know that we're running from a submodule, and B) have a
> shortcut to the superproject's vitals, for example, configs.
>
> By using a relative path instead of an absolute path, we can move the
> superproject directory around on the filesystem without breaking the
> submodule's cache.

As the function this new thing is added assumes the modern layout of
having submodule "repository" in .git/modules/* of the repository of
the superproject, it is rather easy to move the whole thing together,
so recording it as relative path is all the more important.

Can a submodule repository be bound to two or more superproject at
the same time?  "We assume no, and we will forbid such a layout, and
that is why we can afford to make submodules aware of their
superprojects" is a totally acceptable answer, but it would make it
easier to follow the reasoning behind a design change like this
series does if such an assumption is recorded somewhere.

> +	git_config_set_in_file(p, "submodule.superprojectGitdir",
> +			       relative_path(absolute_path(get_git_dir()),
> +					     path, &sb));
> +

OK, so even when the superproject is used as a submodule of somebody
else, we could get to the top of its working tree, because (1) the
submodule we are currently working in can find out where the gitdir
of the superproject is, and (2) in that gitdir, which is very likely
different from the ".git/" subdirectory of the working tree of the
superproject (instead, it would be a directory in ".git/modules/" of
its superproject), we could find the core.worktree configuration to
reach the working tree of the superproject.

OK, makes sense.

Thanks.

Re: [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache

[Emily Shaffer]

On Mon, Jun 14, 2021 at 02:09:15PM +0900, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > Teach submodules a reference to their superproject's gitdir. This allows
> > us to A) know that we're running from a submodule, and B) have a
> > shortcut to the superproject's vitals, for example, configs.
> >
> > By using a relative path instead of an absolute path, we can move the
> > superproject directory around on the filesystem without breaking the
> > submodule's cache.
> 
> As the function this new thing is added assumes the modern layout of
> having submodule "repository" in .git/modules/* of the repository of
> the superproject, it is rather easy to move the whole thing together,
> so recording it as relative path is all the more important.
> 
> Can a submodule repository be bound to two or more superproject at
> the same time?  "We assume no, and we will forbid such a layout, and
> that is why we can afford to make submodules aware of their
> superprojects" is a totally acceptable answer, but it would make it
> easier to follow the reasoning behind a design change like this
> series does if such an assumption is recorded somewhere.

Thanks for pointing out the use case - I can see this happening in some
case like adding a source code dependency from a few projects at once,
so it doesn't sound too far-fetched. But no, I don't think this method
of caching can support it; I think forbidding it and saying "if you want
this to work, use a worktree" is pretty reasonable.

Will find a nice place to write it down.

 - Emily

[RFC PATCH 3/4] submodule: cache superproject gitdir during absorbgitdirs

[Emily Shaffer]

Already during 'git submodule add' we cache 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 |  1 +
 2 files changed, 11 insertions(+)

diff --git a/submodule.c b/submodule.c
index 9767ba9893..09dfc4ee38 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2064,6 +2064,7 @@ static void relocate_single_git_dir_into_superproject(const char *path)
 	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 "
@@ -2095,6 +2096,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 */
+	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));
+
+	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..70fc282937 100755
--- a/t/t7412-submodule-absorbgitdirs.sh
+++ b/t/t7412-submodule-absorbgitdirs.sh
@@ -29,6 +29,7 @@ test_expect_success 'absorb the git dir' '
 	test -d .git/modules/sub1 &&
 	git status >actual.1 &&
 	git -C sub1 rev-parse HEAD >actual.2 &&
+	test . -ef "$(git -C sub1 config submodule.superprojectGitDir)" &&
 	test_cmp expect.1 actual.1 &&
 	test_cmp expect.2 actual.2
 '
-- 
2.32.0.272.g935e593368-goog

Re: [RFC PATCH 3/4] submodule: cache superproject gitdir during absorbgitdirs

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> +	test . -ef "$(git -C sub1 config submodule.superprojectGitDir)" &&

Unfortunately "test f1 -ef f2", "test f1 -nt f2", etc. are not part
of POSIX.

When in doubt, check

  https://pubs.opengroup.org/onlinepubs/9699919799/utilities/toc.html

Thanks.

[RFC PATCH 4/4] submodule: cache superproject gitdir during 'update'

[Emily Shaffer]

A cached path to the superproject's gitdir might be added during 'git
submodule add', but in some cases - like submodules which were created
before 'git submodule add' learned to cache that info - it might be
useful to update the cache. Let's do it during 'git submodule update',
when we already have a handle to the superproject while calling
operations on the submodules.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 git-submodule.sh            |  9 +++++++++
 t/t7406-submodule-update.sh | 10 ++++++++++
 2 files changed, 19 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index eb90f18229..ddda751cfa 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -648,6 +648,15 @@ cmd_update()
 			fi
 		fi
 
+		# Cache a pointer to the superproject's gitdir. This may have
+		# changed, so rewrite it unconditionally. Writes it to worktree
+		# if applicable, otherwise to local.
+
+		sp_gitdir="$(git rev-parse --absolute-git-dir)"
+		relative_gitdir="$(realpath --relative-to "$sm_path" "$sp_gitdir")"
+		git -C "$sm_path" config --worktree \
+			submodule.superprojectgitdir "$relative_gitdir"
+
 		if test -n "$recursive"
 		then
 			(
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index ff3ba5422e..96023cbb6a 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1037,4 +1037,14 @@ test_expect_success 'submodule update --quiet passes quietness to merge/rebase'
 	)
 '
 
+test_expect_success 'submodule update adds superproject gitdir to older repos' '
+	(cd super &&
+	 git -C submodule config --unset submodule.superprojectGitdir &&
+	 git submodule update &&
+	 echo "../.git" >expect &&
+	 git -C submodule config submodule.superprojectGitdir >actual &&
+	 test_cmp expect actual
+	)
+'
+
 test_done
-- 
2.32.0.272.g935e593368-goog

Re: [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update'

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> A cached path to the superproject's gitdir might be added during 'git
> submodule add', but in some cases - like submodules which were created
> before 'git submodule add' learned to cache that info - it might be
> useful to update the cache. Let's do it during 'git submodule update',
> when we already have a handle to the superproject while calling
> operations on the submodules.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  git-submodule.sh            |  9 +++++++++
>  t/t7406-submodule-update.sh | 10 ++++++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index eb90f18229..ddda751cfa 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -648,6 +648,15 @@ cmd_update()
>  			fi
>  		fi
>  
> +		# Cache a pointer to the superproject's gitdir. This may have
> +		# changed, so rewrite it unconditionally. Writes it to worktree
> +		# if applicable, otherwise to local.
> +
> +		sp_gitdir="$(git rev-parse --absolute-git-dir)"
> +		relative_gitdir="$(realpath --relative-to "$sm_path" "$sp_gitdir")"

realpath may not exist on the target system.  Discussions on the
patch [*1*] may be of interest.

It might be a good idea to push to your github repository to trigger
CI on macOS (I am guessing that you do not test locally on macs from
the two issues we saw in this series).

Thanks.


[Reference]

*1*
https://lore.kernel.org/git/20201206225349.3392790-3-sandals@crustytoothpaste.net/

Re: [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update'

[Emily Shaffer]

On Mon, Jun 14, 2021 at 03:22:29PM +0900, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > A cached path to the superproject's gitdir might be added during 'git
> > submodule add', but in some cases - like submodules which were created
> > before 'git submodule add' learned to cache that info - it might be
> > useful to update the cache. Let's do it during 'git submodule update',
> > when we already have a handle to the superproject while calling
> > operations on the submodules.
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> >  git-submodule.sh            |  9 +++++++++
> >  t/t7406-submodule-update.sh | 10 ++++++++++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index eb90f18229..ddda751cfa 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -648,6 +648,15 @@ cmd_update()
> >  			fi
> >  		fi
> >  
> > +		# Cache a pointer to the superproject's gitdir. This may have
> > +		# changed, so rewrite it unconditionally. Writes it to worktree
> > +		# if applicable, otherwise to local.
> > +
> > +		sp_gitdir="$(git rev-parse --absolute-git-dir)"
> > +		relative_gitdir="$(realpath --relative-to "$sm_path" "$sp_gitdir")"
> 
> realpath may not exist on the target system.  Discussions on the
> patch [*1*] may be of interest.
> 
> It might be a good idea to push to your github repository to trigger
> CI on macOS (I am guessing that you do not test locally on macs from
> the two issues we saw in this series).

I typically do, sorry for forgetting to do so this time.

> 
> Thanks.
> 
> 
> [Reference]
> 
> *1*
> https://lore.kernel.org/git/20201206225349.3392790-3-sandals@crustytoothpaste.net/

Re: [RFC PATCH 0/4] cache parent project's gitdir in submodules

[Jacob Keller]

On Fri, Jun 11, 2021 at 3:54 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> It's necessary for a superproject to know which submodules it contains.
> However, historically submodules do not know they are anything but a
> normal single-repo Git project (or a superproject, in nested-submodules
> cases). This decision does help prevent us from having to support
> divergent behaviors in submodule projects vs. superprojects, which makes
> sure Git is (somewhat) less confusing for the reader, and helps simplify
> our code.
>
> One could imagine, though, some conveniences we could gain from
> submodules learning added behavior (though not necessarily *different*
> behavior) to provide more context about the state of the project as a
> whole, and to make large submodule-based projects easier to work with.
> One example is a series[1] I sent some time ago, adding a config to be
> shared between the superproject and all its submodules. The RFC[2] I
> sent around the same time mentions a few other examples, such as "git
> status" run in the submodule noticing whether the superproject has a
> commit referencing the submodule's current HEAD.
>
> It's expensive and non-definitive to try and guess whether or not the
> current repo is a submodule. submodule.c:get_superproject_working_tree()
> does so by essentially running 'git -C .. ls-files -- <own-path>',
> invoking an additional process. get_superproject_working_tree() is not
> called often, so that's mostly fine. However, [1] attempted to include
> an additional config located in the superproject's gitdir by running
> 'git -C .. rev-parse --git-dir' during startup - a little expensive in
> the best case, because it's an extra process, but extremely expensive in
> the case when the current repo is *not* a submodule, because we hunt all
> the way up the filesystem looking for a '.git'. Adding that cost to
> every startup is infeasible.
>

It also adds cost for no benefit to the normal case where it's not a
submodule. The cost added for non-submodules ought to be as near-zero
as possible.

> To that end, in this series I propose caching a path to the
> superproject's gitdir - by having the superproject write that relative
> path to the submodule's config on creation or update. The goal here is
> *not* to say "If I am a submodule, I must have
> submodule.superprojectGitDir set" - but instead to say "If I have
> submodule.superprojectGitDir set, then I must be a submodule." That is,
> I expect we will find edge cases where a submodule was introduced in
> some interesting way that bypassed any of the patches below, and
> therefore doesn't have the superproject's gitdir cached.
>
> The combination of these two rules:
>  - Anything relying on submodule.superprojectGitDir must be nice to
>    have, but not essential, because
>  - It's possible for a submodule to be valid without having
>    submodule.superprojectGitDir set
> makes me feel more comfortable with the idea of submodules learning
> additional behavior based on this config. I feel pretty unconfident in
> our ability to ensure that *every* submodule has this config set.
>

I think this is a good direction. I do think having some information
about being a submodule could be very useful for tools such as git
status, and making it more of a "if we know for sure, we get some
small benefit, but if we don't know then it's no harm" is a good
direction.

> The series covers a few paths for introducing that config, which I'm
> hoping covers most cases.
>  - "git submodule update" (which seems to be part of the "git submodule
>    init" flow)
>  - "git submodule absorbgitdir" to convert a "git init"'d repo into a
>    submodule
>
> Notably, we can only really set this config when 'the_repository' is the
> superproject - that appears to be the only time when we know the gitdirs
> of both the superproject and the submodule.

We could also presumably add a new command for setting this?

>
> I'm expecting folks may have a lot to say about this, so I look forward
> to discussion :)
>
>  - Emily
>
> 1: https://lore.kernel.org/git/20210423001539.4059524-1-emilyshaffer@google.com
> 2: https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com
>
> Emily Shaffer (4):
>   t7400-submodule-basic: modernize inspect() helper
>   introduce submodule.superprojectGitDir cache
>   submodule: cache superproject gitdir during absorbgitdirs
>   submodule: cache superproject gitdir during 'update'
>
>  builtin/submodule--helper.c        |  4 +++
>  git-submodule.sh                   |  9 ++++++
>  submodule.c                        | 10 ++++++
>  t/t7400-submodule-basic.sh         | 49 ++++++++++++++----------------
>  t/t7406-submodule-update.sh        | 10 ++++++
>  t/t7412-submodule-absorbgitdirs.sh |  1 +
>  6 files changed, 57 insertions(+), 26 deletions(-)
>
> --
> 2.32.0.272.g935e593368-goog
>

Re: [RFC PATCH 0/4] cache parent project's gitdir in submodules

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> It's necessary for a superproject to know which submodules it contains.
> However, historically submodules do not know they are anything but a
> normal single-repo Git project (or a superproject, in nested-submodules
> cases). This decision does help prevent us from having to support
> divergent behaviors in submodule projects vs. superprojects, which makes
> sure Git is (somewhat) less confusing for the reader, and helps simplify
> our code.

https://github.com/git/git/actions/runs/934803365 is the CI run of
'seen' that passes.  With this and another topic merged to 'seen',
as can be seen in https://github.com/git/git/actions/runs/934680687,
a handful of tests fail.

Thanks.

Re: [RFC PATCH 0/4] cache parent project's gitdir in submodules

[Emily Shaffer]

On Mon, Jun 14, 2021 at 04:26:17PM +0900, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > It's necessary for a superproject to know which submodules it contains.
> > However, historically submodules do not know they are anything but a
> > normal single-repo Git project (or a superproject, in nested-submodules
> > cases). This decision does help prevent us from having to support
> > divergent behaviors in submodule projects vs. superprojects, which makes
> > sure Git is (somewhat) less confusing for the reader, and helps simplify
> > our code.
> 
> https://github.com/git/git/actions/runs/934803365 is the CI run of
> 'seen' that passes.  With this and another topic merged to 'seen',
> as can be seen in https://github.com/git/git/actions/runs/934680687,
> a handful of tests fail.

Thanks, I will investigate.

[PATCH v2 0/4] cache parent project's gitdir in submodules

[Emily Shaffer]

The reception for this series seemed pretty good in v1, so I'm dropping
the RFC.

Tested: https://github.com/nasamuffin/git/actions/runs/941100646

Sinec v1, mostly platform-friendliness fixes. Also added documentation
for the new config option - wordsmithing help is always welcome.

 - Emily

Emily Shaffer (4):
  t7400-submodule-basic: modernize inspect() helper
  introduce submodule.superprojectGitDir cache
  submodule: cache superproject gitdir during absorbgitdirs
  submodule: cache superproject gitdir during 'update'

 Documentation/config/submodule.txt | 12 ++++++++
 builtin/submodule--helper.c        |  4 +++
 git-submodule.sh                   | 10 ++++++
 submodule.c                        | 10 ++++++
 t/t7400-submodule-basic.sh         | 49 ++++++++++++++----------------
 t/t7406-submodule-update.sh        | 10 ++++++
 t/t7412-submodule-absorbgitdirs.sh |  9 +++++-
 7 files changed, 77 insertions(+), 27 deletions(-)

Range-diff against v1:
1:  d6284438fb = 1:  a6718eea80 t7400-submodule-basic: modernize inspect() helper
2:  56470e2eab ! 2:  4cebe7bcb5 introduce submodule.superprojectGitDir cache
    @@ Commit message
     
         Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
     
    + ## Documentation/config/submodule.txt ##
    +@@ Documentation/config/submodule.txt: submodule.alternateErrorStrategy::
    + 	`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`
    + 	or `info`, and if there is an error with the computed alternate, the
    + 	clone proceeds as if no alternate was specified.
    ++
    ++submodule.superprojectGitDir::
    ++	The relative path from the submodule's worktree  to the superproject's
    ++	gitdir. This config should only be present in projects which are
    ++	submodules, but is not guaranteed to be present in every submodule. It
    ++	is set automatically during submodule creation.
    +++
    ++	In situations where more than one superproject references the same
    ++	submodule worktree, the value of this config and the behavior of
    ++	operations which use it are undefined. To reference a single project
    ++	from multiple superprojects, it is better to create a worktree of the
    ++	submodule for each superproject.
    +
      ## builtin/submodule--helper.c ##
     @@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv, const char *prefix)
      		git_config_set_in_file(p, "submodule.alternateErrorStrategy",
3:  42f954f523 ! 3:  df97a9c2bb submodule: cache superproject gitdir during absorbgitdirs
    @@ submodule.c: static void relocate_single_git_dir_into_superproject(const char *p
     
      ## t/t7412-submodule-absorbgitdirs.sh ##
     @@ t/t7412-submodule-absorbgitdirs.sh: test_expect_success 'absorb the git dir' '
    - 	test -d .git/modules/sub1 &&
      	git status >actual.1 &&
      	git -C sub1 rev-parse HEAD >actual.2 &&
    -+	test . -ef "$(git -C sub1 config submodule.superprojectGitDir)" &&
      	test_cmp expect.1 actual.1 &&
    - 	test_cmp expect.2 actual.2
    +-	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 &&
    ++	test-tool path-utils real_path \
    ++		"$(git -C sub1 config submodule.superprojectGitDir)" >actual &&
    ++
    ++	test_cmp expect actual
      '
    + 
    + test_expect_success 'absorbing does not fail for deinitialized submodules' '
4:  4f55ab42c7 ! 4:  a3f3be58ad submodule: cache superproject gitdir during 'update'
    @@ git-submodule.sh: cmd_update()
     +		# Cache a pointer to the superproject's gitdir. This may have
     +		# changed, so rewrite it unconditionally. Writes it to worktree
     +		# if applicable, otherwise to local.
    ++		relative_gitdir="$(git rev-parse --path-format=relative \
    ++						 --prefix "${sm_path}" \
    ++						 --git-dir)"
     +
    -+		sp_gitdir="$(git rev-parse --absolute-git-dir)"
    -+		relative_gitdir="$(realpath --relative-to "$sm_path" "$sp_gitdir")"
     +		git -C "$sm_path" config --worktree \
     +			submodule.superprojectgitdir "$relative_gitdir"
     +
    @@ git-submodule.sh: cmd_update()
      			(
     
      ## t/t7406-submodule-update.sh ##
    -@@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --quiet passes quietness to merge/rebase'
    +@@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --quiet passes quietness to fetch with a s
      	)
      '
      
-- 
2.32.0.272.g935e593368-goog

[PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper

[Emily Shaffer]

Since the inspect() helper in the submodule-basic test suite was
written, 'git -C <dir>' was added. By using -C, we no longer need a
reference to the base directory for the test. This simplifies callsites,
and will make the addition of other arguments in later patches more
readable.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 t/t7400-submodule-basic.sh | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index a924fdb7a6..f5dc051a6e 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' '
 # generates, which will expand symbolic links.
 submodurl=$(pwd -P)
 
-listbranches() {
-	git for-each-ref --format='%(refname)' 'refs/heads/*'
-}
-
 inspect() {
 	dir=$1 &&
-	dotdot="${2:-..}" &&
 
-	(
-		cd "$dir" &&
-		listbranches >"$dotdot/heads" &&
-		{ git symbolic-ref HEAD || :; } >"$dotdot/head" &&
-		git rev-parse HEAD >"$dotdot/head-sha1" &&
-		git update-index --refresh &&
-		git diff-files --exit-code &&
-		git clean -n -d -x >"$dotdot/untracked"
-	)
+	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
 }
 
+
 test_expect_success 'submodule add' '
 	echo "refs/heads/main" >expect &&
 
@@ -146,7 +139,7 @@ test_expect_success 'submodule add' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/submod ../.. &&
+	inspect addtest/submod &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -237,7 +230,7 @@ test_expect_success 'submodule add --branch' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/submod-branch ../.. &&
+	inspect addtest/submod-branch &&
 	test_cmp expect-heads heads &&
 	test_cmp expect-head head &&
 	test_must_be_empty untracked
@@ -253,7 +246,7 @@ test_expect_success 'submodule add with ./ in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/dotsubmod/frotz ../../.. &&
+	inspect addtest/dotsubmod/frotz &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -269,7 +262,7 @@ test_expect_success 'submodule add with /././ in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/dotslashdotsubmod/frotz ../../.. &&
+	inspect addtest/dotslashdotsubmod/frotz &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -285,7 +278,7 @@ test_expect_success 'submodule add with // in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/slashslashsubmod/frotz ../../.. &&
+	inspect addtest/slashslashsubmod/frotz &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -301,7 +294,7 @@ test_expect_success 'submodule add with /.. in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod ../.. &&
+	inspect addtest/realsubmod &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -317,7 +310,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod2 ../.. &&
+	inspect addtest/realsubmod2 &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -348,7 +341,7 @@ test_expect_success 'submodule add in subdirectory' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod3 ../.. &&
+	inspect addtest/realsubmod3 &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
-- 
2.32.0.272.g935e593368-goog

Re: [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper

[Jonathan Tan]

> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index a924fdb7a6..f5dc051a6e 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' '
>  # generates, which will expand symbolic links.
>  submodurl=$(pwd -P)
>  
> -listbranches() {
> -	git for-each-ref --format='%(refname)' 'refs/heads/*'
> -}
> -
>  inspect() {
>  	dir=$1 &&
> -	dotdot="${2:-..}" &&
>  
> -	(
> -		cd "$dir" &&
> -		listbranches >"$dotdot/heads" &&
> -		{ git symbolic-ref HEAD || :; } >"$dotdot/head" &&
> -		git rev-parse HEAD >"$dotdot/head-sha1" &&
> -		git update-index --refresh &&
> -		git diff-files --exit-code &&
> -		git clean -n -d -x >"$dotdot/untracked"
> -	)
> +	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
>  }
>  
> +

Stray extra line.

Other than that, this is definitely a good simplification.

Re: [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper

[Emily Shaffer]

On Tue, Jul 27, 2021 at 10:12:25AM -0700, Jonathan Tan wrote:
> 
> > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> > index a924fdb7a6..f5dc051a6e 100755
> > --- a/t/t7400-submodule-basic.sh
> > +++ b/t/t7400-submodule-basic.sh
> > @@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' '
> >  # generates, which will expand symbolic links.
> >  submodurl=$(pwd -P)
> >  
> > -listbranches() {
> > -	git for-each-ref --format='%(refname)' 'refs/heads/*'
> > -}
> > -
> >  inspect() {
> >  	dir=$1 &&
> > -	dotdot="${2:-..}" &&
> >  
> > -	(
> > -		cd "$dir" &&
> > -		listbranches >"$dotdot/heads" &&
> > -		{ git symbolic-ref HEAD || :; } >"$dotdot/head" &&
> > -		git rev-parse HEAD >"$dotdot/head-sha1" &&
> > -		git update-index --refresh &&
> > -		git diff-files --exit-code &&
> > -		git clean -n -d -x >"$dotdot/untracked"
> > -	)
> > +	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
> >  }
> >  
> > +
> 
> Stray extra line.
> 
> Other than that, this is definitely a good simplification.

Ack, fixed locally. Thanks.

 - Emily

[PATCH v2 2/4] introduce submodule.superprojectGitDir cache

[Emily Shaffer]

Teach submodules a reference to their superproject's gitdir. This allows
us to A) know that we're running from a submodule, and B) have a
shortcut to the superproject's vitals, for example, configs.

By using a relative path instead of an absolute path, we can move the
superproject directory around on the filesystem without breaking the
submodule's cache.

Since this cached value is only introduced during new submodule creation
via `git submodule add`, though, there is more work to do to allow the
cache to be created at other times.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/config/submodule.txt | 12 +++++++++
 builtin/submodule--helper.c        |  4 +++
 t/t7400-submodule-basic.sh         | 40 ++++++++++++++++--------------
 3 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
index d7a63c8c12..7c459cc19e 100644
--- a/Documentation/config/submodule.txt
+++ b/Documentation/config/submodule.txt
@@ -90,3 +90,15 @@ submodule.alternateErrorStrategy::
 	`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`
 	or `info`, and if there is an error with the computed alternate, the
 	clone proceeds as if no alternate was specified.
+
+submodule.superprojectGitDir::
+	The relative path from the submodule's worktree  to the superproject's
+	gitdir. This config should only be present in projects which are
+	submodules, but is not guaranteed to be present in every submodule. It
+	is set automatically during submodule creation.
++
+	In situations where more than one superproject references the same
+	submodule worktree, the value of this config and the behavior of
+	operations which use it are undefined. To reference a single project
+	from multiple superprojects, it is better to create a worktree of the
+	submodule for each superproject.
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));
+
 	free(sm_alternate);
 	free(error_strategy);
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index f5dc051a6e..e45f42588f 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' '
 submodurl=$(pwd -P)
 
 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
 }
 
 
@@ -139,7 +143,7 @@ test_expect_success 'submodule add' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/submod &&
+	inspect addtest/submod addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -230,7 +234,7 @@ test_expect_success 'submodule add --branch' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/submod-branch &&
+	inspect addtest/submod-branch addtest &&
 	test_cmp expect-heads heads &&
 	test_cmp expect-head head &&
 	test_must_be_empty untracked
@@ -246,7 +250,7 @@ test_expect_success 'submodule add with ./ in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/dotsubmod/frotz &&
+	inspect addtest/dotsubmod/frotz addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -262,7 +266,7 @@ test_expect_success 'submodule add with /././ in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/dotslashdotsubmod/frotz &&
+	inspect addtest/dotslashdotsubmod/frotz addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -278,7 +282,7 @@ test_expect_success 'submodule add with // in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/slashslashsubmod/frotz &&
+	inspect addtest/slashslashsubmod/frotz addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -294,7 +298,7 @@ test_expect_success 'submodule add with /.. in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod &&
+	inspect addtest/realsubmod addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -310,7 +314,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod2 &&
+	inspect addtest/realsubmod2 addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -341,7 +345,7 @@ test_expect_success 'submodule add in subdirectory' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod3 &&
+	inspect addtest/realsubmod3 addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -482,7 +486,7 @@ test_expect_success 'update should work when path is an empty dir' '
 	git submodule update -q >update.out &&
 	test_must_be_empty update.out &&
 
-	inspect init &&
+	inspect init . &&
 	test_cmp expect head-sha1
 '
 
@@ -541,7 +545,7 @@ test_expect_success 'update should checkout rev1' '
 	echo "$rev1" >expect &&
 
 	git submodule update init &&
-	inspect init &&
+	inspect init . &&
 
 	test_cmp expect head-sha1
 '
-- 
2.32.0.272.g935e593368-goog

Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> Teach submodules a reference to their superproject's gitdir. This allows
> us to A) know that we're running from a submodule, and B) have a
> shortcut to the superproject's vitals, for example, configs.
>
> By using a relative path instead of an absolute path, we can move the
> superproject directory around on the filesystem without breaking the
> submodule's cache.
>
> Since this cached value is only introduced during new submodule creation
> via `git submodule add`, though, there is more work to do to allow the
> cache to be created at other times.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  Documentation/config/submodule.txt | 12 +++++++++
>  builtin/submodule--helper.c        |  4 +++
>  t/t7400-submodule-basic.sh         | 40 ++++++++++++++++--------------
>  3 files changed, 38 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
> index d7a63c8c12..7c459cc19e 100644
> --- a/Documentation/config/submodule.txt
> +++ b/Documentation/config/submodule.txt
> @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy::
>  	`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`
>  	or `info`, and if there is an error with the computed alternate, the
>  	clone proceeds as if no alternate was specified.
> +
> +submodule.superprojectGitDir::
> +	The relative path from the submodule's worktree  to the superproject's
> +	gitdir. This config should only be present in projects which are
> +	submodules, but is not guaranteed to be present in every submodule. It
> +	is set automatically during submodule creation.
> ++
> +	In situations where more than one superproject references the same
> +	submodule worktree, the value of this config and the behavior of
> +	operations which use it are undefined. To reference a single project
> +	from multiple superprojects, it is better to create a worktree of the
> +	submodule for each superproject.

You'd need to dedent the second paragraph that follows a lone '+'
sign to typeset this correctly.

The new paragraph suggests separate worktrees for the same submodule
repository, but for that to work correctly,

 - "git clone [--recurse-submodules]" that clones the second
   superproject that shares the same submodule with a superproject
   that we already locally has to support a way for users to tell
   where to grab that existing submodule from and arrange a new
   worktree, instead of creating another instance of the submodule
   repository by cloning it afresh.

 - The "submodule.superprojectGitDir" needs to be set to
   per-worktree half of the repo-local configuration file.

Because I usually do not pay much attention to the submodule part of
the toolset, I may well be mistaken, but I suspect that the former
does not exist yet.  If I recall correctly, the latter was a NEEDSWORK
item in the previous round of this patchset?

As I said, I think it is OK for now to stop at declaring that you
cannot simply do it without hinting something that may not fully
work.

Thanks.

Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> As I said, I think it is OK for now to stop at declaring that you
> cannot simply do it without hinting something that may not fully
> work.

I'll add the following to the tip of the topic for now.


 Documentation/config/submodule.txt | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git c/Documentation/config/submodule.txt w/Documentation/config/submodule.txt
index 7c459cc19e..58ba63a75e 100644
--- c/Documentation/config/submodule.txt
+++ w/Documentation/config/submodule.txt
@@ -97,8 +97,5 @@ submodule.superprojectGitDir::
 	submodules, but is not guaranteed to be present in every submodule. It
 	is set automatically during submodule creation.
 +
-	In situations where more than one superproject references the same
-	submodule worktree, the value of this config and the behavior of
-	operations which use it are undefined. To reference a single project
-	from multiple superprojects, it is better to create a worktree of the
-	submodule for each superproject.
+Because of this configuration variable, it is forbidden to use the
+same submodule worktree shared by multiple superprojects.

Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache

[Emily Shaffer]

On Wed, Jun 16, 2021 at 01:43:36PM +0900, Junio C Hamano wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > As I said, I think it is OK for now to stop at declaring that you
> > cannot simply do it without hinting something that may not fully
> > work.
> 
> I'll add the following to the tip of the topic for now.

Just saw this - yes, it looks fine to me. I'll squash that locally in
case anybody has more comments and wants a reroll.

> 
> 
>  Documentation/config/submodule.txt | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git c/Documentation/config/submodule.txt w/Documentation/config/submodule.txt
> index 7c459cc19e..58ba63a75e 100644
> --- c/Documentation/config/submodule.txt
> +++ w/Documentation/config/submodule.txt
> @@ -97,8 +97,5 @@ submodule.superprojectGitDir::
>  	submodules, but is not guaranteed to be present in every submodule. It
>  	is set automatically during submodule creation.
>  +
> -	In situations where more than one superproject references the same
> -	submodule worktree, the value of this config and the behavior of
> -	operations which use it are undefined. To reference a single project
> -	from multiple superprojects, it is better to create a worktree of the
> -	submodule for each superproject.
> +Because of this configuration variable, it is forbidden to use the
> +same submodule worktree shared by multiple superprojects.

Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache

[Emily Shaffer]

On Wed, Jun 16, 2021 at 01:40:36PM +0900, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> > +submodule.superprojectGitDir::
> > +	The relative path from the submodule's worktree  to the superproject's
> > +	gitdir. This config should only be present in projects which are
> > +	submodules, but is not guaranteed to be present in every submodule. It
> > +	is set automatically during submodule creation.
> > ++
> > +	In situations where more than one superproject references the same
> > +	submodule worktree, the value of this config and the behavior of
> > +	operations which use it are undefined. To reference a single project
> > +	from multiple superprojects, it is better to create a worktree of the
> > +	submodule for each superproject.
> 
> You'd need to dedent the second paragraph that follows a lone '+'
> sign to typeset this correctly.

Ok.

> 
> The new paragraph suggests separate worktrees for the same submodule
> repository, but for that to work correctly,
> 
>  - "git clone [--recurse-submodules]" that clones the second
>    superproject that shares the same submodule with a superproject
>    that we already locally has to support a way for users to tell
>    where to grab that existing submodule from and arrange a new
>    worktree, instead of creating another instance of the submodule
>    repository by cloning it afresh.
> 
>  - The "submodule.superprojectGitDir" needs to be set to
>    per-worktree half of the repo-local configuration file.
> 
> Because I usually do not pay much attention to the submodule part of
> the toolset, I may well be mistaken, but I suspect that the former
> does not exist yet.  If I recall correctly, the latter was a NEEDSWORK
> item in the previous round of this patchset?
> 
> As I said, I think it is OK for now to stop at declaring that you
> cannot simply do it without hinting something that may not fully
> work.

Yeah, that is all correct. Ok, I will drop the broken suggestion.

 - Emily

Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache

[Jonathan Tan]

> Teach submodules a reference to their superproject's gitdir. This allows
> us to A) know that we're running from a submodule, and B) have a
> shortcut to the superproject's vitals, for example, configs.

The first sentence is probably better "Introduce a new config variable
storing a submodule's reference to its superproject's gitdir, and teach
'git submodule add' to write it".

Also, I think there should be more detail about the planned use both
here in the commit message and in the config documentation. Is the plan
just to use it for best-effort explanatory messages? (Using it as a true
cache is probably too performance-intensive, I would think, since in its
absence, we have no idea whether the repo is a submodule and would
always have to search to the root of the filesystem.) If it is just for
best-effort explanatory messages, maybe write:

  If present, commands like "git status" in this submodule may print
  additional information about this submodule's status with respect to
  its superproject.

This would further reinforce that this variable being missing is OK.

> diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
> index d7a63c8c12..7c459cc19e 100644
> --- a/Documentation/config/submodule.txt
> +++ b/Documentation/config/submodule.txt
> @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy::
>  	`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`
>  	or `info`, and if there is an error with the computed alternate, the
>  	clone proceeds as if no alternate was specified.
> +
> +submodule.superprojectGitDir::
> +	The relative path from the submodule's worktree  to the superproject's
> +	gitdir. This config should only be present in projects which are
> +	submodules, but is not guaranteed to be present in every submodule. It
> +	is set automatically during submodule creation.
> ++
> +	In situations where more than one superproject references the same
> +	submodule worktree, the value of this config and the behavior of
> +	operations which use it are undefined. To reference a single project
> +	from multiple superprojects, it is better to create a worktree of the
> +	submodule for each superproject.

So my suggestion would be:

  The relative path from this repository'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, commands
  like "git status" in this submodule may print additional information
  about this submodule's status with respect to its superproject.

(I believe Junio has commented on the second paragraph - I don't have
additional comments on that.)

Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache

[Emily Shaffer]

On Tue, Jul 27, 2021 at 10:46:50AM -0700, Jonathan Tan wrote:
> 
> > Teach submodules a reference to their superproject's gitdir. This allows
> > us to A) know that we're running from a submodule, and B) have a
> > shortcut to the superproject's vitals, for example, configs.
> 
> The first sentence is probably better "Introduce a new config variable
> storing a submodule's reference to its superproject's gitdir, and teach
> 'git submodule add' to write it".
> 
> Also, I think there should be more detail about the planned use both
> here in the commit message and in the config documentation. Is the plan
> just to use it for best-effort explanatory messages? (Using it as a true
> cache is probably too performance-intensive, I would think, since in its
> absence, we have no idea whether the repo is a submodule and would
> always have to search to the root of the filesystem.) If it is just for
> best-effort explanatory messages, maybe write:
> 
>   If present, commands like "git status" in this submodule may print
>   additional information about this submodule's status with respect to
>   its superproject.
> 
> This would further reinforce that this variable being missing is OK.

Ok, I'll expand the commit message. The first use case for this extra
pointer is for a shared config between superproject and submodule, which
I've sent a series for already; I'll mention that in the commit message
too.

> 
> > diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
> > index d7a63c8c12..7c459cc19e 100644
> > --- a/Documentation/config/submodule.txt
> > +++ b/Documentation/config/submodule.txt
> > @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy::
> >  	`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`
> >  	or `info`, and if there is an error with the computed alternate, the
> >  	clone proceeds as if no alternate was specified.
> > +
> > +submodule.superprojectGitDir::
> > +	The relative path from the submodule's worktree  to the superproject's
> > +	gitdir. This config should only be present in projects which are
> > +	submodules, but is not guaranteed to be present in every submodule. It
> > +	is set automatically during submodule creation.
> > ++
> > +	In situations where more than one superproject references the same
> > +	submodule worktree, the value of this config and the behavior of
> > +	operations which use it are undefined. To reference a single project
> > +	from multiple superprojects, it is better to create a worktree of the
> > +	submodule for each superproject.
> 
> So my suggestion would be:
> 
>   The relative path from this repository'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, commands
>   like "git status" in this submodule may print additional information
>   about this submodule's status with respect to its superproject.
> 
> (I believe Junio has commented on the second paragraph - I don't have
> additional comments on that.)

The spirit of this suggestion seems to be "describe a possible side
effect of this config", so I'll do that, although the wording may not be
exactly the same. Thanks.

 - Emily

Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 15 2021, Emily Shaffer wrote:

> Teach submodules a reference to their superproject's gitdir. This allows
> us to A) know that we're running from a submodule, and B) have a
> shortcut to the superproject's vitals, for example, configs.
>
> By using a relative path instead of an absolute path, we can move the
> superproject directory around on the filesystem without breaking the
> submodule's cache.
>
> Since this cached value is only introduced during new submodule creation
> via `git submodule add`, though, there is more work to do to allow the
> cache to be created at other times.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  Documentation/config/submodule.txt | 12 +++++++++
>  builtin/submodule--helper.c        |  4 +++
>  t/t7400-submodule-basic.sh         | 40 ++++++++++++++++--------------
>  3 files changed, 38 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
> index d7a63c8c12..7c459cc19e 100644
> --- a/Documentation/config/submodule.txt
> +++ b/Documentation/config/submodule.txt
> @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy::
>  	`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`
>  	or `info`, and if there is an error with the computed alternate, the
>  	clone proceeds as if no alternate was specified.
> +
> +submodule.superprojectGitDir::
> +	The relative path from the submodule's worktree  to the superproject's
> +	gitdir. This config should only be present in projects which are
> +	submodules, but is not guaranteed to be present in every submodule. It
> +	is set automatically during submodule creation.
> ++
> +	In situations where more than one superproject references the same
> +	submodule worktree, the value of this config and the behavior of
> +	operations which use it are undefined. To reference a single project
> +	from multiple superprojects, it is better to create a worktree of the
> +	submodule for each superproject.
> 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));
> +
>  	free(sm_alternate);
>  	free(error_strategy);
>  

Am I correct that what this series really does is avoid needing to:

1. Run the equivalent of $(git rev-parse --absolute-git-dir), let's call
   the result of that $X.
2. Feed that to the equvialent of $(git -C $X/../ rev-parse
   --absolute-git-dir)
3. Check if the relationship between the two is really that of a
   submodule, i.e. running "git submodule status", check if $X contains
   ".git/modules/" etc.

I see an earlier iteration of this series had such a shell-out, and that
this is the "cache":
https://lore.kernel.org/git/20210423001539.4059524-5-emilyshaffer@google.com/;
and your v1 cover letter seems to support the above summary:
https://lore.kernel.org/git/20210611225428.1208973-1-emilyshaffer@google.com/

I think it's fine to fine to add such a cache in principle if it's needed.

But I'm a bit iffy on a series that's structured in a way as to not
start by asserting that we should have given semantics without the
cache, and then adds the cache later as an optional optimization.

Particularly as the whole submodule business is moving to C, so isn't
this whole caching business something we can avoid doing, and instead
just call a C function? The optimization part of this was not calling
"git rev-parse" on every submodule invocation wasn't it, not avoiding a
few syscalls that deal with the FS.

Your initial RFC had modifications to git-submodule.sh, in the interim
it seems that's been moved sufficiently to C that we're modifying just
the submodule.c here.

I'm not very familiar with setup_git_directory_gently_1(),
discover_git_directory() etc, but wherever you are in a git worktree
we'll chdir() to the top-level when running built-ins.

So that gives us step #1 of the above. And #2 would be adding "/../" to
the end of that path and running those functions again? Perhaps with a
#3 for "is there a submodule relationship?".

Even if we still have some bits in shellscript etc, couldn't we then
setenv() that in some GIT_* variable, e.g. GIT_SUPERPROJECT_DIR?

Or is the problem really that this isn't a cache, because we can't say
with absolute certainty that there is such a gitdir/submodule
relationship, except at the time of running "git submodule" in the
former for some reason?

[PATCH v2 3/4] submodule: cache superproject gitdir during absorbgitdirs

[Emily Shaffer]

Already during 'git submodule add' we cache 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)
 	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 */
+	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));
+
+	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 &&
+	test-tool path-utils real_path \
+		"$(git -C sub1 config submodule.superprojectGitDir)" >actual &&
+
+	test_cmp expect actual
 '
 
 test_expect_success 'absorbing does not fail for deinitialized submodules' '
-- 
2.32.0.272.g935e593368-goog

[PATCH v2 4/4] submodule: cache superproject gitdir during 'update'

[Emily Shaffer]

A cached path to the superproject's gitdir might be added during 'git
submodule add', but in some cases - like submodules which were created
before 'git submodule add' learned to cache that info - it might be
useful to update the cache. Let's do it during 'git submodule update',
when we already have a handle to the superproject while calling
operations on the submodules.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 git-submodule.sh            | 10 ++++++++++
 t/t7406-submodule-update.sh | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index 4678378424..f98dcc16ae 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -648,6 +648,16 @@ cmd_update()
 			fi
 		fi
 
+		# Cache a pointer to the superproject's gitdir. This may have
+		# changed, so rewrite it unconditionally. Writes it to worktree
+		# if applicable, otherwise to local.
+		relative_gitdir="$(git rev-parse --path-format=relative \
+						 --prefix "${sm_path}" \
+						 --git-dir)"
+
+		git -C "$sm_path" config --worktree \
+			submodule.superprojectgitdir "$relative_gitdir"
+
 		if test -n "$recursive"
 		then
 			(
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f4f61fe554..c39821ba8e 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1061,4 +1061,14 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s
 	)
 '
 
+test_expect_success 'submodule update adds superproject gitdir to older repos' '
+	(cd super &&
+	 git -C submodule config --unset submodule.superprojectGitdir &&
+	 git submodule update &&
+	 echo "../.git" >expect &&
+	 git -C submodule config submodule.superprojectGitdir >actual &&
+	 test_cmp expect actual
+	)
+'
+
 test_done
-- 
2.32.0.272.g935e593368-goog

Re: [PATCH v2 4/4] submodule: cache superproject gitdir during 'update'

[Jonathan Tan]

> A cached path to the superproject's gitdir might be added during 'git

[snip]

> +		# Cache a pointer to the superproject's gitdir. This may have

Patches 3 and 4 look good to me except that for me, a cache is something
that lets us avoid an expensive computation. But as far as I know, we're
not planning to perform the expensive computation in the absence of a
cache. Maybe the word "record" would fit better.

This is nit-picky, though, and if others think that the word "cache"
fits here, that's fine by me too.

Re: [PATCH v2 4/4] submodule: cache superproject gitdir during 'update'

[Emily Shaffer]

On Tue, Jul 27, 2021 at 10:51:51AM -0700, Jonathan Tan wrote:
> 
> > A cached path to the superproject's gitdir might be added during 'git
> 
> [snip]
> 
> > +		# Cache a pointer to the superproject's gitdir. This may have
> 
> Patches 3 and 4 look good to me except that for me, a cache is something
> that lets us avoid an expensive computation. But as far as I know, we're
> not planning to perform the expensive computation in the absence of a
> cache. Maybe the word "record" would fit better.
> 
> This is nit-picky, though, and if others think that the word "cache"
> fits here, that's fine by me too.

Thanks. I switched the commit messages around to use 'record' and 'hint'
when appropriate instead. I'll send a reroll with those nits as soon as
it passes CI.

 - Emily

[PATCH v3 0/4] cache parent project's gitdir in submodules

[Emily Shaffer]

Since v2, mostly documentation changes and a handful of small nits from
Junio and Jonathan Tan. Thanks for the reviews, both.

CI run here: https://github.com/nasamuffin/git/actions/runs/1147984974

 - Emily

Emily Shaffer (4):
  t7400-submodule-basic: modernize inspect() helper
  introduce submodule.superprojectGitDir record
  submodule: record superproject gitdir during absorbgitdirs
  submodule: record superproject gitdir during 'update'

 Documentation/config/submodule.txt | 15 ++++++++++
 builtin/submodule--helper.c        |  4 +++
 git-submodule.sh                   | 10 +++++++
 submodule.c                        | 10 +++++++
 t/t7400-submodule-basic.sh         | 48 ++++++++++++++----------------
 t/t7406-submodule-update.sh        | 10 +++++++
 t/t7412-submodule-absorbgitdirs.sh |  9 +++++-
 7 files changed, 79 insertions(+), 27 deletions(-)

Range-diff against v2:
1:  a6718eea80 ! 1:  f1236dc9d7 t7400-submodule-basic: modernize inspect() helper
    @@ t/t7400-submodule-basic.sh: test_expect_success 'setup - repository to add submo
     +	git -C "$dir" clean -n -d -x >untracked
      }
      
    -+
      test_expect_success 'submodule add' '
    - 	echo "refs/heads/main" >expect &&
    - 
     @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule add' '
      	) &&
      
2:  4cebe7bcb5 ! 2:  2caf9eb8ee introduce submodule.superprojectGitDir cache
    @@ Metadata
     Author: Emily Shaffer <emilyshaffer@google.com>
     
      ## Commit message ##
    -    introduce submodule.superprojectGitDir cache
    +    introduce submodule.superprojectGitDir record
     
         Teach submodules a reference to their superproject's gitdir. This allows
         us to A) know that we're running from a submodule, and B) have a
    @@ Commit message
         superproject directory around on the filesystem without breaking the
         submodule's cache.
     
    -    Since this cached value is only introduced during new submodule creation
    +    Since this hint value is only introduced during new submodule creation
         via `git submodule add`, though, there is more work to do to allow the
    -    cache to be created at other times.
    +    record to be created at other times.
    +
    +    If the new config is present, we can do some optional value-added
    +    behavior, like letting "git status" print additional info about the
    +    submodule's status in relation to its superproject, or like letting the
    +    superproject and submodule share an additional config file separate from
    +    either one's local config.
     
         Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
     
      ## Documentation/config/submodule.txt ##
     @@ Documentation/config/submodule.txt: submodule.alternateErrorStrategy::
    @@ Documentation/config/submodule.txt: submodule.alternateErrorStrategy::
      	clone proceeds as if no alternate was specified.
     +
     +submodule.superprojectGitDir::
    -+	The relative path from the submodule's worktree  to the superproject's
    -+	gitdir. This config should only be present in projects which are
    -+	submodules, but is not guaranteed to be present in every submodule. It
    -+	is set automatically during submodule creation.
    ++	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.
     ++
    -+	In situations where more than one superproject references the same
    -+	submodule worktree, the value of this config and the behavior of
    -+	operations which use it are undefined. To reference a single project
    -+	from multiple superprojects, it is better to create a worktree of the
    -+	submodule for each superproject.
    ++	Because of this configuration variable, it is forbidden to use the
    ++	same submodule worktree shared by multiple superprojects.
     
      ## builtin/submodule--helper.c ##
     @@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv, const char *prefix)
    @@ t/t7400-submodule-basic.sh: test_expect_success 'setup - repository to add submo
     +	git -C "$sub_dir" clean -n -d -x >untracked
      }
      
    - 
    + test_expect_success 'submodule add' '
     @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule add' '
      	) &&
      
3:  df97a9c2bb ! 3:  d278568a8e submodule: cache superproject gitdir during absorbgitdirs
    @@ Metadata
     Author: Emily Shaffer <emilyshaffer@google.com>
     
      ## Commit message ##
    -    submodule: cache superproject gitdir during absorbgitdirs
    +    submodule: record superproject gitdir during absorbgitdirs
     
    -    Already during 'git submodule add' we cache a pointer to the
    +    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
4:  a3f3be58ad ! 4:  6866c36620 submodule: cache superproject gitdir during 'update'
    @@ Metadata
     Author: Emily Shaffer <emilyshaffer@google.com>
     
      ## Commit message ##
    -    submodule: cache superproject gitdir during 'update'
    +    submodule: record superproject gitdir during 'update'
     
    -    A cached path to the superproject's gitdir might be added during 'git
    -    submodule add', but in some cases - like submodules which were created
    -    before 'git submodule add' learned to cache that info - it might be
    -    useful to update the cache. Let's do it during 'git submodule update',
    -    when we already have a handle to the superproject while calling
    +    A recorded hint path to the superproject's gitdir might be added during
    +    'git submodule add', but in some cases - like submodules which were
    +    created before 'git submodule add' learned to record that info - it might
    +    be useful to update the hint. Let's do it during 'git submodule
    +    update', when we already have a handle to the superproject while calling
         operations on the submodules.
     
         Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
-- 
2.33.0.rc2.250.ged5fa647cd-goog

[PATCH v3 1/4] t7400-submodule-basic: modernize inspect() helper

[Emily Shaffer]

Since the inspect() helper in the submodule-basic test suite was
written, 'git -C <dir>' was added. By using -C, we no longer need a
reference to the base directory for the test. This simplifies callsites,
and will make the addition of other arguments in later patches more
readable.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 t/t7400-submodule-basic.sh | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index a924fdb7a6..4bc6b6c886 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -107,23 +107,15 @@ test_expect_success 'setup - repository to add submodules to' '
 # generates, which will expand symbolic links.
 submodurl=$(pwd -P)
 
-listbranches() {
-	git for-each-ref --format='%(refname)' 'refs/heads/*'
-}
-
 inspect() {
 	dir=$1 &&
-	dotdot="${2:-..}" &&
 
-	(
-		cd "$dir" &&
-		listbranches >"$dotdot/heads" &&
-		{ git symbolic-ref HEAD || :; } >"$dotdot/head" &&
-		git rev-parse HEAD >"$dotdot/head-sha1" &&
-		git update-index --refresh &&
-		git diff-files --exit-code &&
-		git clean -n -d -x >"$dotdot/untracked"
-	)
+	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
 }
 
 test_expect_success 'submodule add' '
@@ -146,7 +138,7 @@ test_expect_success 'submodule add' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/submod ../.. &&
+	inspect addtest/submod &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -237,7 +229,7 @@ test_expect_success 'submodule add --branch' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/submod-branch ../.. &&
+	inspect addtest/submod-branch &&
 	test_cmp expect-heads heads &&
 	test_cmp expect-head head &&
 	test_must_be_empty untracked
@@ -253,7 +245,7 @@ test_expect_success 'submodule add with ./ in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/dotsubmod/frotz ../../.. &&
+	inspect addtest/dotsubmod/frotz &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -269,7 +261,7 @@ test_expect_success 'submodule add with /././ in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/dotslashdotsubmod/frotz ../../.. &&
+	inspect addtest/dotslashdotsubmod/frotz &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -285,7 +277,7 @@ test_expect_success 'submodule add with // in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/slashslashsubmod/frotz ../../.. &&
+	inspect addtest/slashslashsubmod/frotz &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -301,7 +293,7 @@ test_expect_success 'submodule add with /.. in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod ../.. &&
+	inspect addtest/realsubmod &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -317,7 +309,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod2 ../.. &&
+	inspect addtest/realsubmod2 &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -348,7 +340,7 @@ test_expect_success 'submodule add in subdirectory' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod3 ../.. &&
+	inspect addtest/realsubmod3 &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
-- 
2.33.0.rc2.250.ged5fa647cd-goog

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

[Emily Shaffer]

Teach submodules a reference to their superproject's gitdir. This allows
us to A) know that we're running from a submodule, and B) have a
shortcut to the superproject's vitals, for example, configs.

By using a relative path instead of an absolute path, we can move the
superproject directory around on the filesystem without breaking the
submodule's cache.

Since this hint value is only introduced during new submodule creation
via `git submodule add`, though, there is more work to do to allow the
record to be created at other times.

If the new config is present, we can do some optional value-added
behavior, like letting "git status" print additional info about the
submodule's status in relation to its superproject, or like letting the
superproject and submodule share an additional config file separate from
either one's local config.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config/submodule.txt | 15 +++++++++++
 builtin/submodule--helper.c        |  4 +++
 t/t7400-submodule-basic.sh         | 40 ++++++++++++++++--------------
 3 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
index d7a63c8c12..23e0a01d90 100644
--- a/Documentation/config/submodule.txt
+++ b/Documentation/config/submodule.txt
@@ -90,3 +90,18 @@ submodule.alternateErrorStrategy::
 	`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`
 	or `info`, and if there is an error with the computed alternate, the
 	clone proceeds as if no alternate was specified.
+
+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.
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));
+
 	free(sm_alternate);
 	free(error_strategy);
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 4bc6b6c886..e407329d81 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' '
 submodurl=$(pwd -P)
 
 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
 }
 
 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 &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -229,7 +233,7 @@ test_expect_success 'submodule add --branch' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/submod-branch &&
+	inspect addtest/submod-branch addtest &&
 	test_cmp expect-heads heads &&
 	test_cmp expect-head head &&
 	test_must_be_empty untracked
@@ -245,7 +249,7 @@ test_expect_success 'submodule add with ./ in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/dotsubmod/frotz &&
+	inspect addtest/dotsubmod/frotz addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -261,7 +265,7 @@ test_expect_success 'submodule add with /././ in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/dotslashdotsubmod/frotz &&
+	inspect addtest/dotslashdotsubmod/frotz addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -277,7 +281,7 @@ test_expect_success 'submodule add with // in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/slashslashsubmod/frotz &&
+	inspect addtest/slashslashsubmod/frotz addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -293,7 +297,7 @@ test_expect_success 'submodule add with /.. in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod &&
+	inspect addtest/realsubmod addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -309,7 +313,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod2 &&
+	inspect addtest/realsubmod2 addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -340,7 +344,7 @@ test_expect_success 'submodule add in subdirectory' '
 	) &&
 
 	rm -f heads head untracked &&
-	inspect addtest/realsubmod3 &&
+	inspect addtest/realsubmod3 addtest &&
 	test_cmp expect heads &&
 	test_cmp expect head &&
 	test_must_be_empty untracked
@@ -481,7 +485,7 @@ test_expect_success 'update should work when path is an empty dir' '
 	git submodule update -q >update.out &&
 	test_must_be_empty update.out &&
 
-	inspect init &&
+	inspect init . &&
 	test_cmp expect head-sha1
 '
 
@@ -540,7 +544,7 @@ test_expect_success 'update should checkout rev1' '
 	echo "$rev1" >expect &&
 
 	git submodule update init &&
-	inspect init &&
+	inspect init . &&
 
 	test_cmp expect head-sha1
 '
-- 
2.33.0.rc2.250.ged5fa647cd-goog

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

[Derrick Stolee]

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.

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?

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.

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

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

Thanks,
-Stolee

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

[Emily Shaffer]

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

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

[Matheus Tavares]

Emily Shaffer <emilyshaffer@google.com> wrote:
>
> 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));

This will be executed when cloning a submodule with
`git submodule add <url/path> <path>`. Do we also want to set
submodule.superprojectGitdir when adding a repository that already exists in
the working tree as a submodule? I.e., something like:

git init super
git init super/sub
[ make commits in super/sub ]
git -C super submodule add ./sub

I don't know if this workflow is so commonly used, though... It may not be
worth the additional work.

Another option, which I believe was suggested by Jonathan Nieder on the Review
Club, is to change the code to absorb the gitdir when adding the local
submodule. Then, the configuration would already be set by the
`absorb_git_dir...()` function itself.

>  	free(sm_alternate);
>  	free(error_strategy);
> 
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 4bc6b6c886..e407329d81 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' '
>  submodurl=$(pwd -P)
> 
>  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" ] &&

To avoid the non-POSIX `-ef`, we could perhaps do something like: 

	super_gitdir="$(git -C "$super_dir" rev-parse --absolute-git-dir)" &&
	cached_gitdir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" &&
	test "$cached_gitdir" = "$(test-tool path-utils relative_path "$super_gitdir" "$PWD/$sub_dir")" &&

(We need the "$PWD/" at the last command because `path.c:relative_path()`
returns the first argument as-is when one of the two paths given to it is
absolute and the other is not.)

One bonus of testing the cached path this way is that we also check that
it is indeed being stored as a relative path :)

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

[Emily Shaffer]

On Sat, Sep 04, 2021 at 02:20:51PM -0300, Matheus Tavares wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> wrote:
> >
> > 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));
> 
> This will be executed when cloning a submodule with
> `git submodule add <url/path> <path>`. Do we also want to set
> submodule.superprojectGitdir when adding a repository that already exists in
> the working tree as a submodule? I.e., something like:
> 
> git init super
> git init super/sub
> [ make commits in super/sub ]
> git -C super submodule add ./sub
> 
> I don't know if this workflow is so commonly used, though... It may not be
> worth the additional work.

Yeah, I think it is covered in the next patch with 'git submodule absorbgitdirs'.

> 
> Another option, which I believe was suggested by Jonathan Nieder on the Review
> Club, is to change the code to absorb the gitdir when adding the local
> submodule. Then, the configuration would already be set by the
> `absorb_git_dir...()` function itself.
> 
> >  	free(sm_alternate);
> >  	free(error_strategy);
> > 
> > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> > index 4bc6b6c886..e407329d81 100755
> > --- a/t/t7400-submodule-basic.sh
> > +++ b/t/t7400-submodule-basic.sh
> > @@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' '
> >  submodurl=$(pwd -P)
> > 
> >  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" ] &&
> 
> To avoid the non-POSIX `-ef`, we could perhaps do something like: 
> 
> 	super_gitdir="$(git -C "$super_dir" rev-parse --absolute-git-dir)" &&
> 	cached_gitdir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" &&
> 	test "$cached_gitdir" = "$(test-tool path-utils relative_path "$super_gitdir" "$PWD/$sub_dir")" &&
> 
> (We need the "$PWD/" at the last command because `path.c:relative_path()`
> returns the first argument as-is when one of the two paths given to it is
> absolute and the other is not.)
> 
> One bonus of testing the cached path this way is that we also check that
> it is indeed being stored as a relative path :)

Yep, that is what I settled on. Thanks.

 - Emily

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

[Emily Shaffer]

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)
 	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 */
+	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));
+
+	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 &&
+	test-tool path-utils real_path \
+		"$(git -C sub1 config submodule.superprojectGitDir)" >actual &&
+
+	test_cmp expect actual
 '
 
 test_expect_success 'absorbing does not fail for deinitialized submodules' '
-- 
2.33.0.rc2.250.ged5fa647cd-goog

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

[Derrick Stolee]

On 8/19/2021 4:09 PM, Emily Shaffer wrote:
> +	/* cache pointer to superproject's gitdir */
> +	/* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */
> +	strbuf_addf(&config_path, "%s/config", real_new_git_dir);

Regarding this NEEDSWORK: you are specifying the config file that
covers every worktree of this submodule, which seems to make sense
to me. The submodule shouldn't have worktrees to itself, right? It
might become a worktree if its superproject has a worktree.

But also: it's still correct to write to the local, non-worktree
config file even if experimental.worktreeConfig is set. That
config just means "we need to read the worktree config as well
as the local config".

Or, am I misunderstanding something?

(I'm advocating for the removal of the NEEDSWORK, in case that
wasn't clear.)

> +	git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir",
> +			       relative_path(get_super_prefix_or_empty(),
> +					     path, &sb));

Thanks,
-Stolee

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

[Emily Shaffer]

On Thu, Aug 19, 2021 at 08:50:27PM -0400, Derrick Stolee wrote:
> 
> On 8/19/2021 4:09 PM, Emily Shaffer wrote:
> > +	/* cache pointer to superproject's gitdir */
> > +	/* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */
> > +	strbuf_addf(&config_path, "%s/config", real_new_git_dir);
> 
> Regarding this NEEDSWORK: you are specifying the config file that
> covers every worktree of this submodule, which seems to make sense
> to me. The submodule shouldn't have worktrees to itself, right? It
> might become a worktree if its superproject has a worktree.
> 
> But also: it's still correct to write to the local, non-worktree
> config file even if experimental.worktreeConfig is set. That
> config just means "we need to read the worktree config as well
> as the local config".
> 
> Or, am I misunderstanding something?
> 
> (I'm advocating for the removal of the NEEDSWORK, in case that
> wasn't clear.)

Yeah, I think by pointing from submodule gitdir to superproject gitdir
this point goes away, anyways. Regardless of which worktree is in use,
the gitdir->gitdir pointer will still be valid. I'll delete the
NEEDSWORK.

> 
> > +	git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir",
> > +			       relative_path(get_super_prefix_or_empty(),
> > +					     path, &sb));
> 
> Thanks,
> -Stolee
> 

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

[Matheus Tavares]

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

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

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

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

> +
> +	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
 '
--- >8 ---

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

[Emily Shaffer]

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

[PATCH v3 4/4] submodule: record superproject gitdir during 'update'

[Emily Shaffer]

A recorded hint path to the superproject's gitdir might be added during
'git submodule add', but in some cases - like submodules which were
created before 'git submodule add' learned to record that info - it might
be useful to update the hint. Let's do it during 'git submodule
update', when we already have a handle to the superproject while calling
operations on the submodules.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 git-submodule.sh            | 10 ++++++++++
 t/t7406-submodule-update.sh | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index 4678378424..f98dcc16ae 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -648,6 +648,16 @@ cmd_update()
 			fi
 		fi
 
+		# Cache a pointer to the superproject's gitdir. This may have
+		# changed, so rewrite it unconditionally. Writes it to worktree
+		# if applicable, otherwise to local.
+		relative_gitdir="$(git rev-parse --path-format=relative \
+						 --prefix "${sm_path}" \
+						 --git-dir)"
+
+		git -C "$sm_path" config --worktree \
+			submodule.superprojectgitdir "$relative_gitdir"
+
 		if test -n "$recursive"
 		then
 			(
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f4f61fe554..c39821ba8e 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1061,4 +1061,14 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s
 	)
 '
 
+test_expect_success 'submodule update adds superproject gitdir to older repos' '
+	(cd super &&
+	 git -C submodule config --unset submodule.superprojectGitdir &&
+	 git submodule update &&
+	 echo "../.git" >expect &&
+	 git -C submodule config submodule.superprojectGitdir >actual &&
+	 test_cmp expect actual
+	)
+'
+
 test_done
-- 
2.33.0.rc2.250.ged5fa647cd-goog

Re: [PATCH v3 4/4] submodule: record superproject gitdir during 'update'

[Derrick Stolee]

On 8/19/2021 4:09 PM, Emily Shaffer wrote:
> +		# Cache a pointer to the superproject's gitdir. This may have
> +		# changed, so rewrite it unconditionally. Writes it to worktree
> +		# if applicable, otherwise to local.
> +		relative_gitdir="$(git rev-parse --path-format=relative \
> +						 --prefix "${sm_path}" \
> +						 --git-dir)"
> +
> +		git -C "$sm_path" config --worktree \> +			submodule.superprojectgitdir "$relative_gitdir"

Ok, I see now why you care about the worktree config. The scenario you
are covering is something like moving a submodule within a worktree and
not wanting to change the relative path of other copies of that submodule
within other worktrees, yes?

For commands such as 'init' and 'add', we don't have the possibility of
colliding with other worktrees because the submodule is being created
for the first time, so the relative path should be safe to place in the
non-worktree config.

I do struggle with the fact that these are inconsistent across the
two commits. It makes me think that there should only be one way to
do it, and either the NEEDSWORK needs to be fixed now, or this line
shouldn't include --worktree. Much of this can depend on the reason
the worktree config exists for a submodule. I expect you have more
context than me, so could you help me understand?

Moving to a different concern I am now realizing with this config:
What happens if a submodule moves, and then a user runs 'git checkout'
in the superproject to move it back? How do we make this config value
update across those movements? Is there a possibility of integrating
with unpack_trees() to notice that a submodule has moved and hence we
should update this config value?

Thanks,
-Stoolee

Re: [PATCH v3 4/4] submodule: record superproject gitdir during 'update'

[Emily Shaffer]

On Thu, Aug 19, 2021 at 08:59:55PM -0400, Derrick Stolee wrote:
> 
> On 8/19/2021 4:09 PM, Emily Shaffer wrote:
> > +		# Cache a pointer to the superproject's gitdir. This may have
> > +		# changed, so rewrite it unconditionally. Writes it to worktree
> > +		# if applicable, otherwise to local.
> > +		relative_gitdir="$(git rev-parse --path-format=relative \
> > +						 --prefix "${sm_path}" \
> > +						 --git-dir)"
> > +
> > +		git -C "$sm_path" config --worktree \> +			submodule.superprojectgitdir "$relative_gitdir"
> 
> Ok, I see now why you care about the worktree config. The scenario you
> are covering is something like moving a submodule within a worktree and
> not wanting to change the relative path of other copies of that submodule
> within other worktrees, yes?
> 
> For commands such as 'init' and 'add', we don't have the possibility of
> colliding with other worktrees because the submodule is being created
> for the first time, so the relative path should be safe to place in the
> non-worktree config.
> 
> I do struggle with the fact that these are inconsistent across the
> two commits. It makes me think that there should only be one way to
> do it, and either the NEEDSWORK needs to be fixed now, or this line
> shouldn't include --worktree. Much of this can depend on the reason
> the worktree config exists for a submodule. I expect you have more
> context than me, so could you help me understand?
> 
> Moving to a different concern I am now realizing with this config:
> What happens if a submodule moves, and then a user runs 'git checkout'
> in the superproject to move it back? How do we make this config value
> update across those movements? Is there a possibility of integrating
> with unpack_trees() to notice that a submodule has moved and hence we
> should update this config value?

I think that switching from "sub worktree to super gitdir" to "sub
gitdir to super gitdir" fixes this neatly - the gitdir-to-gitdir path
will be identical regardless of worktree, so we can set it in the main
submodule config (lose the '--worktree' arg).

This also will fix the case you describe, moving around the submodule in
the superproject's tree from checkout to checkout, as the gitdir will
not move.

Thanks a bunch for the review and sorry it took me so long to reply. v4
incoming.

 - Emily

Re: [PATCH v3 0/4] cache parent project's gitdir in submodules

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> Since v2, mostly documentation changes and a handful of small nits from
> Junio and Jonathan Tan. Thanks for the reviews, both.

Will queue, but ...

>      +submodule.superprojectGitDir::
>     -+	The relative path from the submodule's worktree  to the superproject's
>     -+	gitdir. This config should only be present in projects which are
>     -+	submodules, but is not guaranteed to be present in every submodule. It
>     -+	is set automatically during submodule creation.
>     ++	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.
>      ++
>     -+	In situations where more than one superproject references the same
>     -+	submodule worktree, the value of this config and the behavior of
>     -+	operations which use it are undefined. To reference a single project
>     -+	from multiple superprojects, it is better to create a worktree of the
>     -+	submodule for each superproject.
>     ++	Because of this configuration variable, it is forbidden to use the
>     ++	same submodule worktree shared by multiple superprojects.

... I think this will regress the format from what I've queued in
'seen' unless you dedent the "Because of this..." paragraph.  It is
unfortunate but AsciiDoc wants the follow-up paragraphs that follow
the single '+' line to start at the left edge without indentation.

Re: [PATCH v3 0/4] cache parent project's gitdir in submodules

[Derrick Stolee]

On 8/19/2021 4:09 PM, Emily Shaffer wrote:
> Since v2, mostly documentation changes and a handful of small nits from
> Junio and Jonathan Tan. Thanks for the reviews, both.

Sorry to show up late with many questions (mostly because my understanding
of submodules is not as strong as yours), but I do have some concerns that
we have not covered all the cases that could lead to the relative path
needing an update, such as a 'git checkout' across commits in the super-
project which moves a submodule.

Leading more to my confusion is that I thought 'git submodule update'
was the way to move a submodule, but that does not appear to be the case.
I used 'git mv' to move a submodule and that correctly updated the
.gitmodules file, but did not run any 'git submodule' subcommands (based
on GIT_TRACE2_PERF=1).

You mention in an earlier cover letter that this config does not need to
exist, but it seems to me that if it exists it needs to be correct for us
to depend on it for future features. I'm not convinced that we can rely
on it as-written, but you might be able to convince me by pointing out
why these submodule movements are safe.

Thanks,
-Stolee

Re: [PATCH v3 0/4] cache parent project's gitdir in submodules

[Emily Shaffer]

On Thu, Aug 19, 2021 at 09:09:46PM -0400, Derrick Stolee wrote:
> 
> On 8/19/2021 4:09 PM, Emily Shaffer wrote:
> > Since v2, mostly documentation changes and a handful of small nits from
> > Junio and Jonathan Tan. Thanks for the reviews, both.
> 
> Sorry to show up late with many questions (mostly because my understanding
> of submodules is not as strong as yours),

Well, here I am apologizing for showing up even later with a reply ;)

> but I do have some concerns that
> we have not covered all the cases that could lead to the relative path
> needing an update, such as a 'git checkout' across commits in the super-
> project which moves a submodule.
> 
> Leading more to my confusion is that I thought 'git submodule update'
> was the way to move a submodule, but that does not appear to be the case.
> I used 'git mv' to move a submodule and that correctly updated the
> .gitmodules file, but did not run any 'git submodule' subcommands (based
> on GIT_TRACE2_PERF=1).

During a live review a few weeks ago it was pointed out, I forget by
who, that this whole series would make a lot more sense if it was
treated as the path from the submodule's gitdir to the superproject's
gitdir. I think this would also fix your 'git mv' example here, as the
submodule gitdir would not change.

> 
> You mention in an earlier cover letter that this config does not need to
> exist, but it seems to me that if it exists it needs to be correct for us
> to depend on it for future features. I'm not convinced that we can rely
> on it as-written, but you might be able to convince me by pointing out
> why these submodule movements are safe.

Yeah, that is a good point, and I wonder how to be defensive about
this... Maybe it makes sense to BUG() if we don't find the
superproject's gitdir from this config? Maybe it makes sense to warn
(asking users to 'git submodule update') and erase the incorrect config
line?

Both those approaches would require a wrapper around
'git_config_get_string()' to cope with a failure, but it might be worth
it.

I'll try proposing such a wrapper in the reroll, unless I hear back
sooner.

 - Emily

Re: [PATCH v3 0/4] cache parent project's gitdir in submodules

[Derrick Stolee]

On 10/13/2021 2:51 PM, Emily Shaffer wrote:
> On Thu, Aug 19, 2021 at 09:09:46PM -0400, Derrick Stolee wrote:
>> but I do have some concerns that
>> we have not covered all the cases that could lead to the relative path
>> needing an update, such as a 'git checkout' across commits in the super-
>> project which moves a submodule.
>>
>> Leading more to my confusion is that I thought 'git submodule update'
>> was the way to move a submodule, but that does not appear to be the case.
>> I used 'git mv' to move a submodule and that correctly updated the
>> .gitmodules file, but did not run any 'git submodule' subcommands (based
>> on GIT_TRACE2_PERF=1).
> 
> During a live review a few weeks ago it was pointed out, I forget by
> who, that this whole series would make a lot more sense if it was
> treated as the path from the submodule's gitdir to the superproject's
> gitdir. I think this would also fix your 'git mv' example here, as the
> submodule gitdir would not change.

I think that's a great way to deliver similar functionality without
the issues I was thinking about.

>> You mention in an earlier cover letter that this config does not need to
>> exist, but it seems to me that if it exists it needs to be correct for us
>> to depend on it for future features. I'm not convinced that we can rely
>> on it as-written, but you might be able to convince me by pointing out
>> why these submodule movements are safe.
> 
> Yeah, that is a good point, and I wonder how to be defensive about
> this... Maybe it makes sense to BUG() if we don't find the
> superproject's gitdir from this config? Maybe it makes sense to warn
> (asking users to 'git submodule update') and erase the incorrect config
> line?

I think we can complain with something like a die() if the config points
to data that doesn't make sense (not a .git directory), but a BUG() is
too heavy-handed because it can just be a user modifying their config
file incorrectly.

I'm happy to shut down a process because a user messed with config
incorrectly. Since your proposed change allows operations like 'git mv'
to move submodules without needing to change this config, I'm much
happier with the design. It becomes impossible for the config to get
out of sync unless the user does something outside of a Git command.

Thanks,
-Stolee

Re: [PATCH v3 0/4] cache parent project's gitdir in submodules

[Emily Shaffer]

On Thu, Oct 14, 2021 at 01:12:11PM -0400, Derrick Stolee wrote:
> 
> On 10/13/2021 2:51 PM, Emily Shaffer wrote:
> > On Thu, Aug 19, 2021 at 09:09:46PM -0400, Derrick Stolee wrote:
> >> but I do have some concerns that
> >> we have not covered all the cases that could lead to the relative path
> >> needing an update, such as a 'git checkout' across commits in the super-
> >> project which moves a submodule.
> >>
> >> Leading more to my confusion is that I thought 'git submodule update'
> >> was the way to move a submodule, but that does not appear to be the case.
> >> I used 'git mv' to move a submodule and that correctly updated the
> >> .gitmodules file, but did not run any 'git submodule' subcommands (based
> >> on GIT_TRACE2_PERF=1).
> > 
> > During a live review a few weeks ago it was pointed out, I forget by
> > who, that this whole series would make a lot more sense if it was
> > treated as the path from the submodule's gitdir to the superproject's
> > gitdir. I think this would also fix your 'git mv' example here, as the
> > submodule gitdir would not change.
> 
> I think that's a great way to deliver similar functionality without
> the issues I was thinking about.
> 
> >> You mention in an earlier cover letter that this config does not need to
> >> exist, but it seems to me that if it exists it needs to be correct for us
> >> to depend on it for future features. I'm not convinced that we can rely
> >> on it as-written, but you might be able to convince me by pointing out
> >> why these submodule movements are safe.
> > 
> > Yeah, that is a good point, and I wonder how to be defensive about
> > this... Maybe it makes sense to BUG() if we don't find the
> > superproject's gitdir from this config? Maybe it makes sense to warn
> > (asking users to 'git submodule update') and erase the incorrect config
> > line?
> 
> I think we can complain with something like a die() if the config points
> to data that doesn't make sense (not a .git directory), but a BUG() is
> too heavy-handed because it can just be a user modifying their config
> file incorrectly.

Ok. Having re-thought since the other day, I think I will skip the
wrapper for this reroll and instead propose it in a series that actually
uses this pointer (so, the shared submodule-and-superproject config), if
it looks like we'd want one. I expect doing that work in the context of
a caller will make the correct behavior a little more clear - in the
context of this series I'm not totally sure what we'd want to do.

> 
> I'm happy to shut down a process because a user messed with config
> incorrectly. Since your proposed change allows operations like 'git mv'
> to move submodules without needing to change this config, I'm much
> happier with the design. It becomes impossible for the config to get
> out of sync unless the user does something outside of a Git command.

Thanks for pulling the context back up here. I appreciate it. Like I
mentioned in reply to your comment on v4, look for a reroll in the next
30 minutes or so (assuming the CI passes ok).

 - Emily

Re: [PATCH v3 0/4] cache parent project's gitdir in submodules

[Matheus Tavares Bernardino]

Hi, Emily

On Thu, Aug 19, 2021 at 5:10 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> Emily Shaffer (4):
>   t7400-submodule-basic: modernize inspect() helper
>   introduce submodule.superprojectGitDir record
>   submodule: record superproject gitdir during absorbgitdirs
>   submodule: record superproject gitdir during 'update'

I left a few comments on patches 2 and 3. They are mostly about things
we have already discussed on the Review Club, but I got the chance to
experiment with the code a bit more after that, and I thought it could
be helpful to provide these inline suggestions. Hope it helps, and
thanks for the series!

Thanks,
Matheus