diff mbox series

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

Message ID 20210611225428.1208973-3-emilyshaffer@google.com (mailing list archive)
State Superseded
Headers show
Series cache parent project's gitdir in submodules | expand

Commit Message

Emily Shaffer June 11, 2021, 10:54 p.m. UTC 
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(-)

Comments

Junio C Hamano June 14, 2021, 5:09 a.m. UTC | #1 
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.
Emily Shaffer June 15, 2021, 10 p.m. UTC | #2 
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
diff mbox series

Patch

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
 '