[PATCH] submodule--helper absorbgitdirs: no abspaths in "Migrating git..."

[PATCH] submodule--helper absorbgitdirs: no abspaths in "Migrating git..."

[Ævar Arnfjörð Bjarmason]

Change the "Migrating git directory" messages to avoid emitting
absolute paths. We could use "old_git_dir" and "new_gitdir.buf" here
sometimes, but not in all the cases covered by these tests,
i.e. sometimes the latter will be an absolute path with a different
prefix.

So let's just strip off the common prefix of the two strings, which
handles the cases where we have nested submodules nicely. Note that
this case is different than the one in get_submodule_displaypath() in
"builtin/submodule--helper.c" handles, as we're dealing with the paths
to the two ".git" here, not worktree paths.

Before this change we had no tests at all for this "Migrating git
directory" output.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Something I hacked up a while ago, but which I'm prompted to send in
by [1] which added a test for this output, but did so with:

	+  cat >expect.err <<-EOF &&
	+  Migrating git directory of ${SQ}sub1/nested${SQ} from
	+  ${SQ}/Users/chooglen/Code/git/t/trash directory.t7412-submodule-absorbgitdirs/sub1/nested/.git${SQ} to
	+  ${SQ}/Users/chooglen/Code/git/t/trash directory.t7412-submodule-absorbgitdirs/.git/modules/sub1/modules/nested${SQ}

:)

Let's make this message a lot less verbose, and easier to test
instead.

1. https://lore.kernel.org/git/20221109004708.97668-2-chooglen@google.com/

 submodule.c                        |  8 +++++--
 t/t7412-submodule-absorbgitdirs.sh | 36 ++++++++++++++++++++++++------
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/submodule.c b/submodule.c
index b958162d286..1f0032d183a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2274,6 +2274,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;
 	struct strbuf new_gitdir = STRBUF_INIT;
 	const struct submodule *sub;
+	size_t off = 0;
 
 	if (submodule_uses_worktrees(path))
 		die(_("relocate_gitdir for submodule '%s' with "
@@ -2298,9 +2299,12 @@ static void relocate_single_git_dir_into_superproject(const char *path)
 		die(_("could not create directory '%s'"), new_gitdir.buf);
 	real_new_git_dir = real_pathdup(new_gitdir.buf, 1);
 
-	fprintf(stderr, _("Migrating git directory of '%s%s' from\n'%s' to\n'%s'\n"),
+	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, real_new_git_dir);
+		real_old_git_dir + off, real_new_git_dir + off);
 
 	relocate_gitdir(path, real_old_git_dir, real_new_git_dir);
 
diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
index 2859695c6d2..a5cd6db7ac2 100755
--- a/t/t7412-submodule-absorbgitdirs.sh
+++ b/t/t7412-submodule-absorbgitdirs.sh
@@ -18,13 +18,19 @@ test_expect_success 'setup a real submodule' '
 '
 
 test_expect_success 'absorb the git dir' '
+	>expect &&
+	>actual &&
 	>expect.1 &&
 	>expect.2 &&
 	>actual.1 &&
 	>actual.2 &&
 	git status >expect.1 &&
 	git -C sub1 rev-parse HEAD >expect.2 &&
-	git submodule absorbgitdirs &&
+	cat >expect <<-\EOF &&
+	Migrating git directory of '\''sub1'\'' from '\''sub1/.git'\'' to '\''.git/modules/sub1'\''
+	EOF
+	git submodule absorbgitdirs 2>actual &&
+	test_cmp expect actual &&
 	git fsck &&
 	test -f sub1/.git &&
 	test -d .git/modules/sub1 &&
@@ -37,7 +43,8 @@ test_expect_success 'absorb the git dir' '
 test_expect_success 'absorbing does not fail for deinitialized submodules' '
 	test_when_finished "git submodule update --init" &&
 	git submodule deinit --all &&
-	git submodule absorbgitdirs &&
+	git submodule absorbgitdirs 2>err &&
+	test_must_be_empty err &&
 	test -d .git/modules/sub1 &&
 	test -d sub1 &&
 	! test -e sub1/.git
@@ -56,7 +63,11 @@ test_expect_success 'setup nested submodule' '
 test_expect_success 'absorb the git dir in a nested submodule' '
 	git status >expect.1 &&
 	git -C sub1/nested rev-parse HEAD >expect.2 &&
-	git submodule absorbgitdirs &&
+	cat >expect <<-\EOF &&
+	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 &&
 	test -f sub1/nested/.git &&
 	test -d .git/modules/sub1/modules/nested &&
 	git status >actual.1 &&
@@ -87,7 +98,11 @@ test_expect_success 're-setup nested submodule' '
 test_expect_success 'absorb the git dir in a nested submodule' '
 	git status >expect.1 &&
 	git -C sub1/nested rev-parse HEAD >expect.2 &&
-	git submodule absorbgitdirs &&
+	cat >expect <<-\EOF &&
+	Migrating git directory of '\''sub1'\'' from '\''sub1/.git'\'' to '\''.git/modules/sub1'\''
+	EOF
+	git submodule absorbgitdirs 2>actual &&
+	test_cmp expect actual &&
 	test -f sub1/.git &&
 	test -f sub1/nested/.git &&
 	test -d .git/modules/sub1/modules/nested &&
@@ -107,7 +122,11 @@ test_expect_success 'setup a gitlink with missing .gitmodules entry' '
 test_expect_success 'absorbing the git dir fails for incomplete submodules' '
 	git status >expect.1 &&
 	git -C sub2 rev-parse HEAD >expect.2 &&
-	test_must_fail git submodule absorbgitdirs &&
+	cat >expect <<-\EOF &&
+	fatal: could not lookup name for submodule '\''sub2'\''
+	EOF
+	test_must_fail git submodule absorbgitdirs 2>actual &&
+	test_cmp expect actual &&
 	git -C sub2 fsck &&
 	test -d sub2/.git &&
 	git status >actual &&
@@ -127,8 +146,11 @@ test_expect_success 'setup a submodule with multiple worktrees' '
 '
 
 test_expect_success 'absorbing fails for a submodule with multiple worktrees' '
-	test_must_fail git submodule absorbgitdirs sub3 2>error &&
-	test_i18ngrep "not supported" error
+	cat >expect <<-\EOF &&
+	fatal: could not lookup name for submodule '\''sub2'\''
+	EOF
+	test_must_fail git submodule absorbgitdirs 2>actual &&
+	test_cmp expect actual
 '
 
 test_done
-- 
2.38.0.1467.g709fbdff1a9

Re: [PATCH] submodule--helper absorbgitdirs: no abspaths in "Migrating git..."

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Change the "Migrating git directory" messages to avoid emitting
> absolute paths. We could use "old_git_dir" and "new_gitdir.buf" here
> sometimes, but not in all the cases covered by these tests,
> i.e. sometimes the latter will be an absolute path with a different
> prefix.

I'm not sure I follow this bit. Couldn't we use $PWD to make sure we get
the correct path?

> Something I hacked up a while ago, but which I'm prompted to send in
> by [1] which added a test for this output, but did so with:
>
> 	+  cat >expect.err <<-EOF &&
> 	+  Migrating git directory of ${SQ}sub1/nested${SQ} from
> 	+  ${SQ}/Users/chooglen/Code/git/t/trash directory.t7412-submodule-absorbgitdirs/sub1/nested/.git${SQ} to
> 	+  ${SQ}/Users/chooglen/Code/git/t/trash directory.t7412-submodule-absorbgitdirs/.git/modules/sub1/modules/nested${SQ}
>
> :)

Bleh :( It was even more of a rush job than I realized.

> diff --git a/submodule.c b/submodule.c
> index b958162d286..1f0032d183a 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -2274,6 +2274,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;
>  	struct strbuf new_gitdir = STRBUF_INIT;
>  	const struct submodule *sub;
> +	size_t off = 0;
>  
>  	if (submodule_uses_worktrees(path))
>  		die(_("relocate_gitdir for submodule '%s' with "
> @@ -2298,9 +2299,12 @@ static void relocate_single_git_dir_into_superproject(const char *path)
>  		die(_("could not create directory '%s'"), new_gitdir.buf);
>  	real_new_git_dir = real_pathdup(new_gitdir.buf, 1);
>  
> -	fprintf(stderr, _("Migrating git directory of '%s%s' from\n'%s' to\n'%s'\n"),
> +	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, real_new_git_dir);
> +		real_old_git_dir + off, real_new_git_dir + off);

Doesn't this assume that the top level superproject's gitdir and the
worktree share the same parent directory? IOW I think this produces a
wrong result if the user is using a different worktree. Maybe this isn't
a big problem now because worktrees don't interact well with submodules,
so I doubt many users use them, but that's something that we should
improve upon.

Separately, I think the super-prefixed path printed by "git submodule
absorbgitdirs" is just wrong, or at the very least, inconsistent with
every other "git submodule" subcommand. Like I mentioned elsewhere,
subcommands implemented primarily in "git submodule--helper" use
get_submodule_displaypath() which results in the path to the submodule
being relative to the original CWD, but "absorbgitdirs" never passes the
relative path from CWD, which results in a path rooted at the
superproject's tree instead (probably a historical accident because
absorbgitdirs was never implemented in sh). If "absorbgitdirs" did print
the relative path from the CWD (which I think it should at some point),
we can't take this patch since it produces paths relative to the
superproject tree, e.g. the result would be something like:

  git init my-repo
  git init my-repo/submodule
  cd my-repo
  mkdir some-dir
  cd some-dir
  # This would say
  #   "Migrating ../submodule from 'submodule' to '.git/modules/submodule'"
  # instead of
  #   "Migrating ../submodule from '../submodule' to '../.git/modules/submodule'"
  git submodule absorbgitdirs

>  	relocate_gitdir(path, real_old_git_dir, real_new_git_dir);
>  
> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
> index 2859695c6d2..a5cd6db7ac2 100755
> --- a/t/t7412-submodule-absorbgitdirs.sh
> +++ b/t/t7412-submodule-absorbgitdirs.sh
> @@ -18,13 +18,19 @@ test_expect_success 'setup a real submodule' '
>  '
>  
>  test_expect_success 'absorb the git dir' '
> +	>expect &&
> +	>actual &&
>  	>expect.1 &&
>  	>expect.2 &&
>  	>actual.1 &&
>  	>actual.2 &&
>  	git status >expect.1 &&
>  	git -C sub1 rev-parse HEAD >expect.2 &&
> -	git submodule absorbgitdirs &&
> +	cat >expect <<-\EOF &&
> +	Migrating git directory of '\''sub1'\'' from '\''sub1/.git'\'' to '\''.git/modules/sub1'\''
> +	EOF
> +	git submodule absorbgitdirs 2>actual &&
> +	test_cmp expect actual &&
>  	git fsck &&
>  	test -f sub1/.git &&
>  	test -d .git/modules/sub1 &&
> @@ -37,7 +43,8 @@ test_expect_success 'absorb the git dir' '
>  test_expect_success 'absorbing does not fail for deinitialized submodules' '
>  	test_when_finished "git submodule update --init" &&
>  	git submodule deinit --all &&
> -	git submodule absorbgitdirs &&
> +	git submodule absorbgitdirs 2>err &&
> +	test_must_be_empty err &&
>  	test -d .git/modules/sub1 &&
>  	test -d sub1 &&
>  	! test -e sub1/.git
> @@ -56,7 +63,11 @@ test_expect_success 'setup nested submodule' '
>  test_expect_success 'absorb the git dir in a nested submodule' '
>  	git status >expect.1 &&
>  	git -C sub1/nested rev-parse HEAD >expect.2 &&
> -	git submodule absorbgitdirs &&
> +	cat >expect <<-\EOF &&
> +	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 &&
>  	test -f sub1/nested/.git &&
>  	test -d .git/modules/sub1/modules/nested &&
>  	git status >actual.1 &&
> @@ -87,7 +98,11 @@ test_expect_success 're-setup nested submodule' '
>  test_expect_success 'absorb the git dir in a nested submodule' '
>  	git status >expect.1 &&
>  	git -C sub1/nested rev-parse HEAD >expect.2 &&
> -	git submodule absorbgitdirs &&
> +	cat >expect <<-\EOF &&
> +	Migrating git directory of '\''sub1'\'' from '\''sub1/.git'\'' to '\''.git/modules/sub1'\''
> +	EOF
> +	git submodule absorbgitdirs 2>actual &&
> +	test_cmp expect actual &&
>  	test -f sub1/.git &&
>  	test -f sub1/nested/.git &&
>  	test -d .git/modules/sub1/modules/nested &&
> @@ -107,7 +122,11 @@ test_expect_success 'setup a gitlink with missing .gitmodules entry' '
>  test_expect_success 'absorbing the git dir fails for incomplete submodules' '
>  	git status >expect.1 &&
>  	git -C sub2 rev-parse HEAD >expect.2 &&
> -	test_must_fail git submodule absorbgitdirs &&
> +	cat >expect <<-\EOF &&
> +	fatal: could not lookup name for submodule '\''sub2'\''
> +	EOF
> +	test_must_fail git submodule absorbgitdirs 2>actual &&
> +	test_cmp expect actual &&
>  	git -C sub2 fsck &&
>  	test -d sub2/.git &&
>  	git status >actual &&
> @@ -127,8 +146,11 @@ test_expect_success 'setup a submodule with multiple worktrees' '
>  '
>  
>  test_expect_success 'absorbing fails for a submodule with multiple worktrees' '
> -	test_must_fail git submodule absorbgitdirs sub3 2>error &&
> -	test_i18ngrep "not supported" error
> +	cat >expect <<-\EOF &&
> +	fatal: could not lookup name for submodule '\''sub2'\''
> +	EOF
> +	test_must_fail git submodule absorbgitdirs 2>actual &&
> +	test_cmp expect actual
>  '

I think these tests are good-to-have, even if only to check that we're
treating the "--super-prefix" correctly. Maybe we could move them to
before [1].

[1] https://lore.kernel.org/git/patch-v2-02.10-5a35f7b75b3-20221114T100803Z-avarab@gmail.com
>  
>  test_done
> -- 
> 2.38.0.1467.g709fbdff1a9