git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Matheus Tavares <matheus.bernardino@usp.br>
To: emilyshaffer@google.com
Cc: git@vger.kernel.org
Subject: Re: [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs
Date: Sat,  4 Sep 2021 14:27:46 -0300	[thread overview]
Message-ID: <20210904172746.3031-1-matheus.bernardino@usp.br> (raw)
In-Reply-To: <20210819200953.2105230-4-emilyshaffer@google.com>

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

  parent reply	other threads:[~2021-09-04 17:27 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-11 22:54 [RFC PATCH 0/4] cache parent project's gitdir in submodules Emily Shaffer
2021-06-11 22:54 ` [RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer
2021-06-14  4:52   ` Junio C Hamano
2021-06-11 22:54 ` [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer
2021-06-14  5:09   ` Junio C Hamano
2021-06-15 22:00     ` Emily Shaffer
2021-06-11 22:54 ` [RFC PATCH 3/4] submodule: cache superproject gitdir during absorbgitdirs Emily Shaffer
2021-06-14  6:18   ` Junio C Hamano
2021-06-11 22:54 ` [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update' Emily Shaffer
2021-06-14  6:22   ` Junio C Hamano
2021-06-15 21:27     ` Emily Shaffer
2021-06-12 20:12 ` [RFC PATCH 0/4] cache parent project's gitdir in submodules Jacob Keller
2021-06-14  7:26 ` Junio C Hamano
2021-06-15 21:18   ` Emily Shaffer
2021-06-16  0:45 ` [PATCH v2 " Emily Shaffer
2021-06-16  0:45   ` [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer
2021-07-27 17:12     ` Jonathan Tan
2021-08-19 17:46       ` Emily Shaffer
2021-06-16  0:45   ` [PATCH v2 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer
2021-06-16  4:40     ` Junio C Hamano
2021-06-16  4:43       ` Junio C Hamano
2021-06-18  0:03         ` Emily Shaffer
2021-06-18  0:00       ` Emily Shaffer
2021-07-27 17:46     ` Jonathan Tan
2021-08-19 17:53       ` Emily Shaffer
2021-10-14 19:25     ` Ævar Arnfjörð Bjarmason
2021-06-16  0:45   ` [PATCH v2 3/4] submodule: cache superproject gitdir during absorbgitdirs Emily Shaffer
2021-06-16  0:45   ` [PATCH v2 4/4] submodule: cache superproject gitdir during 'update' Emily Shaffer
2021-07-27 17:51     ` Jonathan Tan
2021-08-19 18:02       ` Emily Shaffer
2021-08-19 20:09   ` [PATCH v3 0/4] cache parent project's gitdir in submodules Emily Shaffer
2021-08-19 20:09     ` [PATCH v3 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer
2021-08-19 20:09     ` [PATCH v3 2/4] introduce submodule.superprojectGitDir record Emily Shaffer
2021-08-20  0:38       ` Derrick Stolee
2021-10-13 19:36         ` Emily Shaffer
2021-09-04 17:20       ` Matheus Tavares
2021-10-13 19:39         ` Emily Shaffer
2021-08-19 20:09     ` [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs Emily Shaffer
2021-08-20  0:50       ` Derrick Stolee
2021-10-13 19:42         ` Emily Shaffer
2021-09-04 17:27       ` Matheus Tavares [this message]
2021-10-14 18:40         ` Emily Shaffer
2021-08-19 20:09     ` [PATCH v3 4/4] submodule: record superproject gitdir during 'update' Emily Shaffer
2021-08-20  0:59       ` Derrick Stolee
2021-10-14 18:45         ` Emily Shaffer
2021-08-19 21:56     ` [PATCH v3 0/4] cache parent project's gitdir in submodules Junio C Hamano
2021-08-20  1:09     ` Derrick Stolee
2021-10-13 18:51       ` Emily Shaffer
2021-10-14 17:12         ` Derrick Stolee
2021-10-14 18:52           ` Emily Shaffer
2021-09-04 17:50     ` Matheus Tavares Bernardino

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210904172746.3031-1-matheus.bernardino@usp.br \
    --to=matheus.bernardino@usp.br \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).