Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx
>
> test_done
> --
> 2.38.0.1467.g709fbdff1a9
