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

From: Glen Choo <chooglen@google.com>
Date: Nov 16, 2022, 22:08 UTC
Message-ID: <kl6lmt8qv9gc.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-1.1-34b54fdd9bb-20221109T020347Z-avarab@gmail.com>

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

Show 5 quoted lines

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

Show 9 quoted lines

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

Show 24 quoted lines

> 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

Show 88 quoted lines

>  	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