[PATCH] submodule absorbgitdirs: use relative <from> and <to> paths

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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/kl6lmt8qv9gc.fsf@chooglen-macbookpro.roam.corp.google.com/

Reported-by: Glen Choo <chooglen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Mon, Nov 21 2022, Glen Choo wrote:

> Taylor Blau <me@ttaylorr.com> 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: <patch-1.1-34b54fdd9bb-20221109T020347Z-avarab@gmail.com>
>>
>
> (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/kl6lmt8qv9gc.fsf@chooglen-macbookpro.roam.corp.google.com

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

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 &&
-- 
2.38.0.1524.gdb7bac9ecc9