{"thread":{"id":"58775","subject":"[PATCH] submodule--helper absorbgitdirs: no abspaths in \"Migrating git...\"","startedAt":"2022-11-09T02:35:54Z","lastAt":"2022-11-16T22:08:54Z","messageCount":2,"participants":["Ævar Arnfjörð Bjarmason","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"466930","messageId":"patch-1.1-34b54fdd9bb-20221109T020347Z-avarab@gmail.com","threadId":"58775","inReplyTo":null,"subject":"[PATCH] submodule--helper absorbgitdirs: no abspaths in \"Migrating git...\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-09T02:35:39Z","receivedAt":"2022-11-09T02:35:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"Migrating git directory\" messages to avoid emitting\nabsolute paths. We could use \"old_git_dir\" and \"new_gitdir.buf\" here\nsometimes, but not in all the cases covered by these tests,\ni.e. sometimes the latter will be an absolute path with a different\nprefix.\n\nSo let's just strip off the common prefix of the two strings, which\nhandles the cases where we have nested submodules nicely. Note that\nthis case is different than the one in get_submodule_displaypath() in\n\"builtin/submodule--helper.c\" handles, as we're dealing with the paths\nto the two \".git\" here, not worktree paths.\n\nBefore this change we had no tests at all for this \"Migrating git\ndirectory\" output.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nSomething I hacked up a while ago, but which I'm prompted to send in\nby [1] which added a test for this output, but did so with:\n\n\t+  cat >expect.err <<-EOF &&\n\t+  Migrating git directory of ${SQ}sub1/nested${SQ} from\n\t+  ${SQ}/Users/chooglen/Code/git/t/trash directory.t7412-submodule-absorbgitdirs/sub1/nested/.git${SQ} to\n\t+  ${SQ}/Users/chooglen/Code/git/t/trash directory.t7412-submodule-absorbgitdirs/.git/modules/sub1/modules/nested${SQ}\n\n:)\n\nLet's make this message a lot less verbose, and easier to test\ninstead.\n\n1. https://lore.kernel.org/git/20221109004708.97668-2-chooglen@google.com/\n\n submodule.c                        |  8 +++++--\n t/t7412-submodule-absorbgitdirs.sh | 36 ++++++++++++++++++++++++------\n 2 files changed, 35 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b958162d286..1f0032d183a 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2274,6 +2274,7 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n \tstruct strbuf new_gitdir = STRBUF_INIT;\n \tconst struct submodule *sub;\n+\tsize_t off = 0;\n \n \tif (submodule_uses_worktrees(path))\n \t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n@@ -2298,9 +2299,12 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n \t\tdie(_(\"could not create directory '%s'\"), new_gitdir.buf);\n \treal_new_git_dir = real_pathdup(new_gitdir.buf, 1);\n \n-\tfprintf(stderr, _(\"Migrating git directory of '%s%s' from\\n'%s' to\\n'%s'\\n\"),\n+\twhile (real_old_git_dir[off] && real_new_git_dir[off] &&\n+\t       real_old_git_dir[off] == real_new_git_dir[off])\n+\t\toff++;\n+\tfprintf(stderr, _(\"Migrating git directory of '%s%s' from '%s' to '%s'\\n\"),\n \t\tget_super_prefix_or_empty(), path,\n-\t\treal_old_git_dir, real_new_git_dir);\n+\t\treal_old_git_dir + off, real_new_git_dir + off);\n \n \trelocate_gitdir(path, real_old_git_dir, real_new_git_dir);\n \ndiff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\nindex 2859695c6d2..a5cd6db7ac2 100755\n--- a/t/t7412-submodule-absorbgitdirs.sh\n+++ b/t/t7412-submodule-absorbgitdirs.sh\n@@ -18,13 +18,19 @@ test_expect_success 'setup a real submodule' '\n '\n \n test_expect_success 'absorb the git dir' '\n+\t>expect &&\n+\t>actual &&\n \t>expect.1 &&\n \t>expect.2 &&\n \t>actual.1 &&\n \t>actual.2 &&\n \tgit status >expect.1 &&\n \tgit -C sub1 rev-parse HEAD >expect.2 &&\n-\tgit submodule absorbgitdirs &&\n+\tcat >expect <<-\\EOF &&\n+\tMigrating git directory of '\\''sub1'\\'' from '\\''sub1/.git'\\'' to '\\''.git/modules/sub1'\\''\n+\tEOF\n+\tgit submodule absorbgitdirs 2>actual &&\n+\ttest_cmp expect actual &&\n \tgit fsck &&\n \ttest -f sub1/.git &&\n \ttest -d .git/modules/sub1 &&\n@@ -37,7 +43,8 @@ test_expect_success 'absorb the git dir' '\n test_expect_success 'absorbing does not fail for deinitialized submodules' '\n \ttest_when_finished \"git submodule update --init\" &&\n \tgit submodule deinit --all &&\n-\tgit submodule absorbgitdirs &&\n+\tgit submodule absorbgitdirs 2>err &&\n+\ttest_must_be_empty err &&\n \ttest -d .git/modules/sub1 &&\n \ttest -d sub1 &&\n \t! test -e sub1/.git\n@@ -56,7 +63,11 @@ test_expect_success 'setup nested submodule' '\n test_expect_success 'absorb the git dir in a nested submodule' '\n \tgit status >expect.1 &&\n \tgit -C sub1/nested rev-parse HEAD >expect.2 &&\n-\tgit submodule absorbgitdirs &&\n+\tcat >expect <<-\\EOF &&\n+\tMigrating git directory of '\\''sub1/nested'\\'' from '\\''sub1/nested/.git'\\'' to '\\''.git/modules/sub1/modules/nested'\\''\n+\tEOF\n+\tgit submodule absorbgitdirs 2>actual &&\n+\ttest_cmp expect actual &&\n \ttest -f sub1/nested/.git &&\n \ttest -d .git/modules/sub1/modules/nested &&\n \tgit status >actual.1 &&\n@@ -87,7 +98,11 @@ test_expect_success 're-setup nested submodule' '\n test_expect_success 'absorb the git dir in a nested submodule' '\n \tgit status >expect.1 &&\n \tgit -C sub1/nested rev-parse HEAD >expect.2 &&\n-\tgit submodule absorbgitdirs &&\n+\tcat >expect <<-\\EOF &&\n+\tMigrating git directory of '\\''sub1'\\'' from '\\''sub1/.git'\\'' to '\\''.git/modules/sub1'\\''\n+\tEOF\n+\tgit submodule absorbgitdirs 2>actual &&\n+\ttest_cmp expect actual &&\n \ttest -f sub1/.git &&\n \ttest -f sub1/nested/.git &&\n \ttest -d .git/modules/sub1/modules/nested &&\n@@ -107,7 +122,11 @@ test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n test_expect_success 'absorbing the git dir fails for incomplete submodules' '\n \tgit status >expect.1 &&\n \tgit -C sub2 rev-parse HEAD >expect.2 &&\n-\ttest_must_fail git submodule absorbgitdirs &&\n+\tcat >expect <<-\\EOF &&\n+\tfatal: could not lookup name for submodule '\\''sub2'\\''\n+\tEOF\n+\ttest_must_fail git submodule absorbgitdirs 2>actual &&\n+\ttest_cmp expect actual &&\n \tgit -C sub2 fsck &&\n \ttest -d sub2/.git &&\n \tgit status >actual &&\n@@ -127,8 +146,11 @@ test_expect_success 'setup a submodule with multiple worktrees' '\n '\n \n test_expect_success 'absorbing fails for a submodule with multiple worktrees' '\n-\ttest_must_fail git submodule absorbgitdirs sub3 2>error &&\n-\ttest_i18ngrep \"not supported\" error\n+\tcat >expect <<-\\EOF &&\n+\tfatal: could not lookup name for submodule '\\''sub2'\\''\n+\tEOF\n+\ttest_must_fail git submodule absorbgitdirs 2>actual &&\n+\ttest_cmp expect actual\n '\n \n test_done\n-- \n2.38.0.1467.g709fbdff1a9\n\n"},{"id":"467399","messageId":"kl6lmt8qv9gc.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58775","inReplyTo":"patch-1.1-34b54fdd9bb-20221109T020347Z-avarab@gmail.com","subject":"Re: [PATCH] submodule--helper absorbgitdirs: no abspaths in \"Migrating git...\"","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-16T22:08:35Z","receivedAt":"2022-11-16T22:08:54Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Change the \"Migrating git directory\" messages to avoid emitting\n> absolute paths. We could use \"old_git_dir\" and \"new_gitdir.buf\" here\n> sometimes, but not in all the cases covered by these tests,\n> i.e. sometimes the latter will be an absolute path with a different\n> prefix.\n\nI'm not sure I follow this bit. Couldn't we use $PWD to make sure we get\nthe correct path?\n\n> Something I hacked up a while ago, but which I'm prompted to send in\n> by [1] which added a test for this output, but did so with:\n>\n> \t+  cat >expect.err <<-EOF &&\n> \t+  Migrating git directory of ${SQ}sub1/nested${SQ} from\n> \t+  ${SQ}/Users/chooglen/Code/git/t/trash directory.t7412-submodule-absorbgitdirs/sub1/nested/.git${SQ} to\n> \t+  ${SQ}/Users/chooglen/Code/git/t/trash directory.t7412-submodule-absorbgitdirs/.git/modules/sub1/modules/nested${SQ}\n>\n> :)\n\nBleh :( It was even more of a rush job than I realized.\n\n> diff --git a/submodule.c b/submodule.c\n> index b958162d286..1f0032d183a 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -2274,6 +2274,7 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n>  \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n>  \tstruct strbuf new_gitdir = STRBUF_INIT;\n>  \tconst struct submodule *sub;\n> +\tsize_t off = 0;\n>  \n>  \tif (submodule_uses_worktrees(path))\n>  \t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n> @@ -2298,9 +2299,12 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n>  \t\tdie(_(\"could not create directory '%s'\"), new_gitdir.buf);\n>  \treal_new_git_dir = real_pathdup(new_gitdir.buf, 1);\n>  \n> -\tfprintf(stderr, _(\"Migrating git directory of '%s%s' from\\n'%s' to\\n'%s'\\n\"),\n> +\twhile (real_old_git_dir[off] && real_new_git_dir[off] &&\n> +\t       real_old_git_dir[off] == real_new_git_dir[off])\n> +\t\toff++;\n> +\tfprintf(stderr, _(\"Migrating git directory of '%s%s' from '%s' to '%s'\\n\"),\n>  \t\tget_super_prefix_or_empty(), path,\n> -\t\treal_old_git_dir, real_new_git_dir);\n> +\t\treal_old_git_dir + off, real_new_git_dir + off);\n\nDoesn't this assume that the top level superproject's gitdir and the\nworktree share the same parent directory? IOW I think this produces a\nwrong result if the user is using a different worktree. Maybe this isn't\na big problem now because worktrees don't interact well with submodules,\nso I doubt many users use them, but that's something that we should\nimprove upon.\n\nSeparately, I think the super-prefixed path printed by \"git submodule\nabsorbgitdirs\" is just wrong, or at the very least, inconsistent with\nevery other \"git submodule\" subcommand. Like I mentioned elsewhere,\nsubcommands implemented primarily in \"git submodule--helper\" use\nget_submodule_displaypath() which results in the path to the submodule\nbeing relative to the original CWD, but \"absorbgitdirs\" never passes the\nrelative path from CWD, which results in a path rooted at the\nsuperproject's tree instead (probably a historical accident because\nabsorbgitdirs was never implemented in sh). If \"absorbgitdirs\" did print\nthe relative path from the CWD (which I think it should at some point),\nwe can't take this patch since it produces paths relative to the\nsuperproject tree, e.g. the result would be something like:\n\n  git init my-repo\n  git init my-repo/submodule\n  cd my-repo\n  mkdir some-dir\n  cd some-dir\n  # This would say\n  #   \"Migrating ../submodule from 'submodule' to '.git/modules/submodule'\"\n  # instead of\n  #   \"Migrating ../submodule from '../submodule' to '../.git/modules/submodule'\"\n  git submodule absorbgitdirs\n\n>  \trelocate_gitdir(path, real_old_git_dir, real_new_git_dir);\n>  \n> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\n> index 2859695c6d2..a5cd6db7ac2 100755\n> --- a/t/t7412-submodule-absorbgitdirs.sh\n> +++ b/t/t7412-submodule-absorbgitdirs.sh\n> @@ -18,13 +18,19 @@ test_expect_success 'setup a real submodule' '\n>  '\n>  \n>  test_expect_success 'absorb the git dir' '\n> +\t>expect &&\n> +\t>actual &&\n>  \t>expect.1 &&\n>  \t>expect.2 &&\n>  \t>actual.1 &&\n>  \t>actual.2 &&\n>  \tgit status >expect.1 &&\n>  \tgit -C sub1 rev-parse HEAD >expect.2 &&\n> -\tgit submodule absorbgitdirs &&\n> +\tcat >expect <<-\\EOF &&\n> +\tMigrating git directory of '\\''sub1'\\'' from '\\''sub1/.git'\\'' to '\\''.git/modules/sub1'\\''\n> +\tEOF\n> +\tgit submodule absorbgitdirs 2>actual &&\n> +\ttest_cmp expect actual &&\n>  \tgit fsck &&\n>  \ttest -f sub1/.git &&\n>  \ttest -d .git/modules/sub1 &&\n> @@ -37,7 +43,8 @@ test_expect_success 'absorb the git dir' '\n>  test_expect_success 'absorbing does not fail for deinitialized submodules' '\n>  \ttest_when_finished \"git submodule update --init\" &&\n>  \tgit submodule deinit --all &&\n> -\tgit submodule absorbgitdirs &&\n> +\tgit submodule absorbgitdirs 2>err &&\n> +\ttest_must_be_empty err &&\n>  \ttest -d .git/modules/sub1 &&\n>  \ttest -d sub1 &&\n>  \t! test -e sub1/.git\n> @@ -56,7 +63,11 @@ test_expect_success 'setup nested submodule' '\n>  test_expect_success 'absorb the git dir in a nested submodule' '\n>  \tgit status >expect.1 &&\n>  \tgit -C sub1/nested rev-parse HEAD >expect.2 &&\n> -\tgit submodule absorbgitdirs &&\n> +\tcat >expect <<-\\EOF &&\n> +\tMigrating git directory of '\\''sub1/nested'\\'' from '\\''sub1/nested/.git'\\'' to '\\''.git/modules/sub1/modules/nested'\\''\n> +\tEOF\n> +\tgit submodule absorbgitdirs 2>actual &&\n> +\ttest_cmp expect actual &&\n>  \ttest -f sub1/nested/.git &&\n>  \ttest -d .git/modules/sub1/modules/nested &&\n>  \tgit status >actual.1 &&\n> @@ -87,7 +98,11 @@ test_expect_success 're-setup nested submodule' '\n>  test_expect_success 'absorb the git dir in a nested submodule' '\n>  \tgit status >expect.1 &&\n>  \tgit -C sub1/nested rev-parse HEAD >expect.2 &&\n> -\tgit submodule absorbgitdirs &&\n> +\tcat >expect <<-\\EOF &&\n> +\tMigrating git directory of '\\''sub1'\\'' from '\\''sub1/.git'\\'' to '\\''.git/modules/sub1'\\''\n> +\tEOF\n> +\tgit submodule absorbgitdirs 2>actual &&\n> +\ttest_cmp expect actual &&\n>  \ttest -f sub1/.git &&\n>  \ttest -f sub1/nested/.git &&\n>  \ttest -d .git/modules/sub1/modules/nested &&\n> @@ -107,7 +122,11 @@ test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n>  test_expect_success 'absorbing the git dir fails for incomplete submodules' '\n>  \tgit status >expect.1 &&\n>  \tgit -C sub2 rev-parse HEAD >expect.2 &&\n> -\ttest_must_fail git submodule absorbgitdirs &&\n> +\tcat >expect <<-\\EOF &&\n> +\tfatal: could not lookup name for submodule '\\''sub2'\\''\n> +\tEOF\n> +\ttest_must_fail git submodule absorbgitdirs 2>actual &&\n> +\ttest_cmp expect actual &&\n>  \tgit -C sub2 fsck &&\n>  \ttest -d sub2/.git &&\n>  \tgit status >actual &&\n> @@ -127,8 +146,11 @@ test_expect_success 'setup a submodule with multiple worktrees' '\n>  '\n>  \n>  test_expect_success 'absorbing fails for a submodule with multiple worktrees' '\n> -\ttest_must_fail git submodule absorbgitdirs sub3 2>error &&\n> -\ttest_i18ngrep \"not supported\" error\n> +\tcat >expect <<-\\EOF &&\n> +\tfatal: could not lookup name for submodule '\\''sub2'\\''\n> +\tEOF\n> +\ttest_must_fail git submodule absorbgitdirs 2>actual &&\n> +\ttest_cmp expect actual\n>  '\n\nI think these tests are good-to-have, even if only to check that we're\ntreating the \"--super-prefix\" correctly. Maybe we could move them to\nbefore [1].\n\n[1] https://lore.kernel.org/git/patch-v2-02.10-5a35f7b75b3-20221114T100803Z-avarab@gmail.com\n>  \n>  test_done\n> -- \n> 2.38.0.1467.g709fbdff1a9\n"}]}