submodule absorbgitdirs: use relative <from> and <to> paths

Commit Message

Ævar Arnfjörð Bjarmason Nov. 22, 2022, 10:45 p.m. UTC

When [1] changed the <from> and <to> paths to from absolute paths by
stripping away their common prefix we started displaying nonsensical
paths in cases where the "gitdir" wasn't "<worktree>/.git".

E.g. a repository at "/repos/repo.git" and a worktree at
"/worktrees/repo-main" would yield paths starting with
"worktrees/repo-main", as the only commonality was the initial "/".

It's harder to narrowly fix that problem than to just have
relocate_single_git_dir_into_superproject() display the same sorts of
paths we do for most other "submodule" commands. I.e. the "<to>"
should be relative to the "<from>" path, rather than relative to the
eventual superproject.

Let's do that, and add a test which checks that we're handling the
"git worktree" case properly. See [2] for the initial bug report.

1. a79e56cb0a6 (submodule--helper absorbgitdirs: no abspaths in
  "Migrating git...", 2022-11-09)
2. https://lore.kernel.org/git/[e-mail]/

Reported-by: Glen Choo <[e-mail]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---

On Mon, Nov 21 2022, Glen Choo wrote:

> Taylor Blau <[e-mail]> writes:
>
>> * ab/submodule-no-abspath (2022-11-09) 1 commit
>>   (merged to 'next' on 2022-11-18 at 34d0accc7b)
>>  + submodule--helper absorbgitdirs: no abspaths in "Migrating git..."
>>  (this branch is used by ab/remove--super-prefix.)
>>
>>  Remove an absolute path in the "Migrating git directory" message.
>>
>>  Will merge to 'master'.
>>  source: <[e-mail]>
>>
>
> (Sorry, I should have spoken up before this got merged to 'next'.)
>
> I have some reservations about this that I mentioned in [1], namely:
>
> - Does this work correctly when using a worktree?
> - If "absorbgitdirs" becomes consistent with other "git submodule"
>   subcommands and prints relative paths to submodules, then this
>   produces the wrong result.
>
> We probably won't see any complaints about this for a while, since
> submodules + worktrees are an uncommon combination, but I expect that
> we'll have to revert this at some point.
>
> [1] https://lore.kernel.org/git/[e-mail]

Hi, sorry about the delay in getting back to you on that. I think the
below should fix the bug you noted, and also improve the output in
general so we use the same sort of relative paths we use for other
"submodule" commands.

Branch & passing CI at:
https://github.com/avar/git/tree/avar/submodule--helper-absorbgitdirs-no-abspath-in-message-fixup

 submodule.c                        | 18 +++++++++++-------
 t/t7412-submodule-absorbgitdirs.sh | 25 ++++++++++++++++++++++---
 2 files changed, 33 insertions(+), 10 deletions(-)

Comments

Glen Choo Nov. 23, 2022, 12:43 a.m. UTC | #1

I think we might be better off reverting a79e56cb0a6 than trying to fix
forward. But before getting into that, thanks for reading the report and
sending a fix :)

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

Rearranging the thread slightly,

>> - If "absorbgitdirs" becomes consistent with other "git submodule"
>>   subcommands and prints relative paths to submodules, then this
>>   produces the wrong result.
> It's harder to narrowly fix that problem than to just have
> relocate_single_git_dir_into_superproject() display the same sorts of
> paths we do for most other "submodule" commands. I.e. the "<to>"
> should be relative to the "<from>" path, rather than relative to the
> eventual superproject.

As I noted here and in the initial report [1], the relative path is from
the original CWD to the submodule, not from 'old submodule gitdir' to
'new submodule gitdir', so I wouldn't really consider this consistent
with other "submodule" commands.

Besides that, I also don't find the output intuitive..

[1] https://lore.kernel.org/git/[e-mail]/

> diff --git a/submodule.c b/submodule.c
> index c47358097fd..a464c99a27f 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -2271,9 +2271,12 @@ int validate_submodule_git_dir(char *git_dir, const char *submodule_name)
>  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 *rel_old_git_dir;
> +	const char *rel_new_git_dir;
>  	struct strbuf new_gitdir = STRBUF_INIT;
>  	const struct submodule *sub;
> -	size_t off = 0;
> +	const char *super_prefix = get_super_prefix();
> +	const char *sp = super_prefix ? super_prefix : "";
>  
>  	if (submodule_uses_worktrees(path))
>  		die(_("relocate_gitdir for submodule '%s' with "
> @@ -2285,6 +2288,7 @@ static void relocate_single_git_dir_into_superproject(const char *path)
>  		return;
>  
>  	real_old_git_dir = real_pathdup(old_git_dir, 1);
> +	rel_old_git_dir = xstrfmt("%s%s", sp, old_git_dir);

rel_old_git_dir is relative to the root of the superproject's worktree.

> @@ -2293,23 +2297,23 @@ static void relocate_single_git_dir_into_superproject(const char *path)
>  	submodule_name_to_gitdir(&new_gitdir, the_repository, sub->name);
>  	if (validate_submodule_git_dir(new_gitdir.buf, sub->name) < 0)
>  		die(_("refusing to move '%s' into an existing git dir"),
> -		    real_old_git_dir);
> +		    rel_old_git_dir);
>  	if (safe_create_leading_directories_const(new_gitdir.buf) < 0)
>  		die(_("could not create directory '%s'"), new_gitdir.buf);
> +
>  	real_new_git_dir = real_pathdup(new_gitdir.buf, 1);
> +	rel_new_git_dir = relative_path(real_new_git_dir, real_old_git_dir,
> +					&new_gitdir);

rel_new_git_dir is relative to rel_old_git_dir

>  
> -	while (real_old_git_dir[off] && real_new_git_dir[off] &&
> -	       real_old_git_dir[off] == real_new_git_dir[off])
> -		off++;
>  	fprintf(stderr, _("Migrating git directory of '%s%s' from '%s' to '%s'\n"),
> -		get_super_prefix_or_empty(), path,
> -		real_old_git_dir + off, real_new_git_dir + off);
> +		sp, path, rel_old_git_dir, rel_new_git_dir);

and the submodule is also relative to the root of the superproject's
worktree...

> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
> index a5cd6db7ac2..0afa0fe3a83 100755
> --- a/t/t7412-submodule-absorbgitdirs.sh
> +++ b/t/t7412-submodule-absorbgitdirs.sh
> @@ -27,7 +27,7 @@ test_expect_success 'absorb the git dir' '
>  	git status >expect.1 &&
>  	git -C sub1 rev-parse HEAD >expect.2 &&
>  	cat >expect <<-\EOF &&
> -	Migrating git directory of '\''sub1'\'' from '\''sub1/.git'\'' to '\''.git/modules/sub1'\''
> +	Migrating git directory of '\''sub1'\'' from '\''sub1/.git'\'' to '\''../../.git/modules/sub1'\''
>  	EOF
>  	git submodule absorbgitdirs 2>actual &&
>  	test_cmp expect actual &&

So when we print the 3 relative paths here, they don't all share the
same base, which I find quite unintuitive to parse.

The 'obvious' solution to make this relative to the original CWD is to
plumb the relative path in --super-prefix (like the other "git
submodule" commands), but that won't give the right result in all cases
either, since we found a call site that gets --super-prefix from "git
read-tree" [2], in which case, --super-prefix is relative to the root of
the worktree and not the original CWD. I don't think we should pursue
this.

A more workable fix (and a possible future direction for removing
--super-prefix) would be to pass "--original-cwd" instead of
"--super-prefix", which would let the child process resolve the relative
path correctly. That's a big change though, and I don't think it's worth
doing right now, especially with 'remove --super-prefix' underway.

So for now, I'd strongly prefer we either eject or revert a79e56cb0a6.

[2] https://lore.kernel.org/git/[e-mail]

Patch

diff --git a/submodule.c b/submodule.c
index c47358097fd..a464c99a27f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2271,9 +2271,12 @@  int validate_submodule_git_dir(char *git_dir, const char *submodule_name)
 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 *rel_old_git_dir;
+	const char *rel_new_git_dir;
 	struct strbuf new_gitdir = STRBUF_INIT;
 	const struct submodule *sub;
-	size_t off = 0;
+	const char *super_prefix = get_super_prefix();
+	const char *sp = super_prefix ? super_prefix : "";
 
 	if (submodule_uses_worktrees(path))
 		die(_("relocate_gitdir for submodule '%s' with "
@@ -2285,6 +2288,7 @@  static void relocate_single_git_dir_into_superproject(const char *path)
 		return;
 
 	real_old_git_dir = real_pathdup(old_git_dir, 1);
+	rel_old_git_dir = xstrfmt("%s%s", sp, old_git_dir);
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 	if (!sub)
@@ -2293,23 +2297,23 @@  static void relocate_single_git_dir_into_superproject(const char *path)
 	submodule_name_to_gitdir(&new_gitdir, the_repository, sub->name);
 	if (validate_submodule_git_dir(new_gitdir.buf, sub->name) < 0)
 		die(_("refusing to move '%s' into an existing git dir"),
-		    real_old_git_dir);
+		    rel_old_git_dir);
 	if (safe_create_leading_directories_const(new_gitdir.buf) < 0)
 		die(_("could not create directory '%s'"), new_gitdir.buf);
+
 	real_new_git_dir = real_pathdup(new_gitdir.buf, 1);
+	rel_new_git_dir = relative_path(real_new_git_dir, real_old_git_dir,
+					&new_gitdir);
 
-	while (real_old_git_dir[off] && real_new_git_dir[off] &&
-	       real_old_git_dir[off] == real_new_git_dir[off])
-		off++;
 	fprintf(stderr, _("Migrating git directory of '%s%s' from '%s' to '%s'\n"),
-		get_super_prefix_or_empty(), path,
-		real_old_git_dir + off, real_new_git_dir + off);
+		sp, path, rel_old_git_dir, rel_new_git_dir);
 
 	relocate_gitdir(path, real_old_git_dir, real_new_git_dir);
 
 	free(old_git_dir);
 	free(real_old_git_dir);
 	free(real_new_git_dir);
+	free(rel_old_git_dir);
 	strbuf_release(&new_gitdir);
 }
 
diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
index a5cd6db7ac2..0afa0fe3a83 100755
--- a/t/t7412-submodule-absorbgitdirs.sh
+++ b/t/t7412-submodule-absorbgitdirs.sh
@@ -27,7 +27,7 @@  test_expect_success 'absorb the git dir' '
 	git status >expect.1 &&
 	git -C sub1 rev-parse HEAD >expect.2 &&
 	cat >expect <<-\EOF &&
-	Migrating git directory of '\''sub1'\'' from '\''sub1/.git'\'' to '\''.git/modules/sub1'\''
+	Migrating git directory of '\''sub1'\'' from '\''sub1/.git'\'' to '\''../../.git/modules/sub1'\''
 	EOF
 	git submodule absorbgitdirs 2>actual &&
 	test_cmp expect actual &&
@@ -64,7 +64,7 @@  test_expect_success 'absorb the git dir in a nested submodule' '
 	git status >expect.1 &&
 	git -C sub1/nested rev-parse HEAD >expect.2 &&
 	cat >expect <<-\EOF &&
-	Migrating git directory of '\''sub1/nested'\'' from '\''sub1/nested/.git'\'' to '\''.git/modules/sub1/modules/nested'\''
+	Migrating git directory of '\''sub1/nested'\'' from '\''sub1/nested/.git'\'' to '\''../../../.git/modules/sub1/modules/nested'\''
 	EOF
 	git submodule absorbgitdirs 2>actual &&
 	test_cmp expect actual &&
@@ -99,7 +99,7 @@  test_expect_success 'absorb the git dir in a nested submodule' '
 	git status >expect.1 &&
 	git -C sub1/nested rev-parse HEAD >expect.2 &&
 	cat >expect <<-\EOF &&
-	Migrating git directory of '\''sub1'\'' from '\''sub1/.git'\'' to '\''.git/modules/sub1'\''
+	Migrating git directory of '\''sub1'\'' from '\''sub1/.git'\'' to '\''../../.git/modules/sub1'\''
 	EOF
 	git submodule absorbgitdirs 2>actual &&
 	test_cmp expect actual &&
@@ -112,6 +112,25 @@  test_expect_success 'absorb the git dir in a nested submodule' '
 	test_cmp expect.2 actual.2
 '
 
+test_expect_success 'absorb the git dir outside of primary worktree' '
+	test_when_finished "rm -rf repo-bare.git" &&
+	git clone --bare . repo-bare.git &&
+	test_when_finished "rm -rf repo-wt" &&
+	git -C repo-bare.git worktree add ../repo-wt &&
+
+	test_when_finished "rm -f .gitconfig" &&
+	test_config_global protocol.file.allow always &&
+	git -C repo-wt submodule update --init &&
+	git init repo-wt/sub2 &&
+	test_commit -C repo-wt/sub2 A &&
+	git -C repo-wt submodule add ./sub2 sub2 &&
+	cat >expect <<-EOF &&
+	Migrating git directory of '\''sub2'\'' from '\''sub2/.git'\'' to '\''../../../repo-bare.git/worktrees/repo-wt/modules/sub2'\''
+	EOF
+	DO_IT=1 git -C repo-wt submodule absorbgitdirs 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup a gitlink with missing .gitmodules entry' '
 	git init sub2 &&
 	test_commit -C sub2 first &&