{"thread":{"id":"44961","subject":"[PATCH] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves","startedAt":"2017-01-24T21:04:00Z","lastAt":"2017-01-25T23:40:01Z","messageCount":13,"participants":["Stefan Beller","Brandon Williams","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"310112","messageId":"20170124210346.12060-1-sbeller@google.com","threadId":"44961","inReplyTo":null,"subject":"[PATCH] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-24T21:03:46Z","receivedAt":"2017-01-24T21:04:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.\nWhen nested is already absorbed into sub, but sub is not absorbed into\nits superproject, then we need to fixup the gitfile and core.worktree\nsetting for 'nested' when absorbing 'sub', but we do not need to move\nits git dir around.\n\nPreviously 'nested's gitfile contained \"gitdir: ../.git/modules/nested\";\nit has to be corrected to \"gitdir: ../../.git/modules/sub1/modules/nested\".\n\nAn alternative I considered to do this work lazily, i.e. when resolving\n\"../.git/modules/nested\", we would notice the \".git\" being a gitfile\nlinking to another path.  That seemed to be robuster by design, but harder\nto get the implementation right.  Maybe we have to do that anyway once we\ntry to have submodules and worktrees working nicely together, but for now\njust produce 'correct' (i.e. direct) pointers.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n  cc Jeff and Brandon, who worked on the setup code recently,\n  and the alternative design mentioned was messing around a lot in setup.c.\n  \n  Thanks,\n  Stefan\n\n submodule.c                        | 41 +++++++++++++++++++-------------------\n t/t7412-submodule-absorbgitdirs.sh | 27 +++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 20 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 4c4f033e8a..7deb0fca6a 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1393,16 +1393,9 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n \tconst char *new_git_dir;\n \tconst struct submodule *sub;\n-\n-\tif (submodule_uses_worktrees(path))\n-\t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n-\t\t      \"more than one worktree not supported\"), path);\n+\tint err_code;\n \n \told_git_dir = xstrfmt(\"%s/.git\", path);\n-\tif (read_gitfile(old_git_dir))\n-\t\t/* If it is an actual gitfile, it doesn't need migration. */\n-\t\treturn;\n-\n \treal_old_git_dir = real_pathdup(old_git_dir);\n \n \tsub = submodule_from_path(null_sha1, path);\n@@ -1414,6 +1407,24 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n \t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n \treal_new_git_dir = real_pathdup(new_git_dir);\n \n+\tif (read_gitfile_gently(old_git_dir, &err_code) ||\n+\t    err_code == READ_GITFILE_ERR_NOT_A_REPO) {\n+\t\t/*\n+\t\t * If it is an actual gitfile, it doesn't need migration,\n+\t\t * however in case of a recursively nested submodule, the\n+\t\t * gitfile content may be stale, as its superproject\n+\t\t * (which may be a submodule of another superproject)\n+\t\t * may have been moved. So expect a bogus pointer to be read,\n+\t\t * which materializes as error READ_GITFILE_ERR_NOT_A_REPO.\n+\t\t */\n+\t\tconnect_work_tree_and_git_dir(path, real_new_git_dir);\n+\t\treturn;\n+\t}\n+\n+\tif (submodule_uses_worktrees(path))\n+\t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n+\t\t      \"more than one worktree not supported\"), path);\n+\n \tif (!prefix)\n \t\tprefix = get_super_prefix();\n \n@@ -1437,22 +1448,14 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \t\t\t\t      const char *path,\n \t\t\t\t      unsigned flags)\n {\n-\tconst char *sub_git_dir, *v;\n-\tchar *real_sub_git_dir = NULL, *real_common_git_dir = NULL;\n \tstruct strbuf gitdir = STRBUF_INIT;\n-\n \tstrbuf_addf(&gitdir, \"%s/.git\", path);\n-\tsub_git_dir = resolve_gitdir(gitdir.buf);\n \n \t/* Not populated? */\n-\tif (!sub_git_dir)\n+\tif (!file_exists(gitdir.buf))\n \t\tgoto out;\n \n-\t/* Is it already absorbed into the superprojects git dir? */\n-\treal_sub_git_dir = real_pathdup(sub_git_dir);\n-\treal_common_git_dir = real_pathdup(get_git_common_dir());\n-\tif (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n-\t\trelocate_single_git_dir_into_superproject(prefix, path);\n+\trelocate_single_git_dir_into_superproject(prefix, path);\n \n \tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1481,6 +1484,4 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \n out:\n \tstrbuf_release(&gitdir);\n-\tfree(real_sub_git_dir);\n-\tfree(real_common_git_dir);\n }\ndiff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\nindex 1c47780e2b..e2bbb449b6 100755\n--- a/t/t7412-submodule-absorbgitdirs.sh\n+++ b/t/t7412-submodule-absorbgitdirs.sh\n@@ -64,6 +64,33 @@ test_expect_success 'absorb the git dir in a nested submodule' '\n \ttest_cmp expect.2 actual.2\n '\n \n+test_expect_success 're-setup nested submodule' '\n+\t# un-absorb the direct submodule, to test if the nested submodule\n+\t# is still correct (needs a rewrite of the gitfile only)\n+\trm -rf sub1/.git &&\n+\tmv .git/modules/sub1 sub1/.git &&\n+\tGIT_WORK_TREE=. git -C sub1 config --unset core.worktree &&\n+\t# fixup the nested submodule\n+\techo \"gitdir: ../.git/modules/nested\" >sub1/nested/.git &&\n+\tGIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \\\n+\t\tcore.worktree \"../../../nested\" &&\n+\t# make sure this re-setup is correct\n+\tgit status --ignore-submodules=none\n+'\n+\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+\ttest -f sub1/.git &&\n+\ttest -f sub1/nested/.git &&\n+\ttest -d .git/modules/sub1/modules/nested &&\n+\tgit status >actual.1 &&\n+\tgit -C sub1/nested rev-parse HEAD >actual.2 &&\n+\ttest_cmp expect.1 actual.1 &&\n+\ttest_cmp expect.2 actual.2\n+'\n+\n test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n \tgit init sub2 &&\n \ttest_commit -C sub2 first &&\n-- \n2.11.0.486.g67830dbe1c\n\n"},{"id":"310114","messageId":"20170124215851.GA58021@google.com","threadId":"44961","inReplyTo":"20170124210346.12060-1-sbeller@google.com","subject":"Re: [PATCH] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-01-24T21:58:51Z","receivedAt":"2017-01-24T21:58:59Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/24, Stefan Beller wrote:\n> +\tif (read_gitfile_gently(old_git_dir, &err_code) ||\n> +\t    err_code == READ_GITFILE_ERR_NOT_A_REPO) {\n> +\t\t/*\n> +\t\t * If it is an actual gitfile, it doesn't need migration,\n> +\t\t * however in case of a recursively nested submodule, the\n> +\t\t * gitfile content may be stale, as its superproject\n> +\t\t * (which may be a submodule of another superproject)\n> +\t\t * may have been moved. So expect a bogus pointer to be read,\n> +\t\t * which materializes as error READ_GITFILE_ERR_NOT_A_REPO.\n> +\t\t */\n> +\t\tconnect_work_tree_and_git_dir(path, real_new_git_dir);\n\nSo connect_work_tree_and_git_dir() will update the .gitfile if it is\nstale.\n\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (submodule_uses_worktrees(path))\n> +\t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n> +\t\t      \"more than one worktree not supported\"), path);\n\nNo current support for worktrees (yet!).\n\n> +\n>  \tif (!prefix)\n>  \t\tprefix = get_super_prefix();\n>  \n> @@ -1437,22 +1448,14 @@ void absorb_git_dir_into_superproject(const char *prefix,\n>  \t\t\t\t      const char *path,\n>  \t\t\t\t      unsigned flags)\n>  {\n> -\tconst char *sub_git_dir, *v;\n> -\tchar *real_sub_git_dir = NULL, *real_common_git_dir = NULL;\n>  \tstruct strbuf gitdir = STRBUF_INIT;\n> -\n>  \tstrbuf_addf(&gitdir, \"%s/.git\", path);\n> -\tsub_git_dir = resolve_gitdir(gitdir.buf);\n>  \n>  \t/* Not populated? */\n> -\tif (!sub_git_dir)\n> +\tif (!file_exists(gitdir.buf))\n>  \t\tgoto out;\n\nThere should be a is_submodule_populated() function now, maybe\nwe should start using it when performing population checks?\n\n>  \n> -\t/* Is it already absorbed into the superprojects git dir? */\n> -\treal_sub_git_dir = real_pathdup(sub_git_dir);\n> -\treal_common_git_dir = real_pathdup(get_git_common_dir());\n> -\tif (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n> -\t\trelocate_single_git_dir_into_superproject(prefix, path);\n> +\trelocate_single_git_dir_into_superproject(prefix, path);\n\nSo the check was just pushed into the relocation function.\n\n>  \n>  \tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n>  \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> @@ -1481,6 +1484,4 @@ void absorb_git_dir_into_superproject(const char *prefix,\n>  \n>  out:\n>  \tstrbuf_release(&gitdir);\n> -\tfree(real_sub_git_dir);\n> -\tfree(real_common_git_dir);\n>  }\n> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\n> index 1c47780e2b..e2bbb449b6 100755\n> --- a/t/t7412-submodule-absorbgitdirs.sh\n> +++ b/t/t7412-submodule-absorbgitdirs.sh\n> @@ -64,6 +64,33 @@ test_expect_success 'absorb the git dir in a nested submodule' '\n>  \ttest_cmp expect.2 actual.2\n>  '\n>  \n> +test_expect_success 're-setup nested submodule' '\n> +\t# un-absorb the direct submodule, to test if the nested submodule\n> +\t# is still correct (needs a rewrite of the gitfile only)\n> +\trm -rf sub1/.git &&\n> +\tmv .git/modules/sub1 sub1/.git &&\n> +\tGIT_WORK_TREE=. git -C sub1 config --unset core.worktree &&\n> +\t# fixup the nested submodule\n> +\techo \"gitdir: ../.git/modules/nested\" >sub1/nested/.git &&\n> +\tGIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \\\n> +\t\tcore.worktree \"../../../nested\" &&\n> +\t# make sure this re-setup is correct\n> +\tgit status --ignore-submodules=none\n> +'\n> +\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> +\ttest -f sub1/.git &&\n> +\ttest -f sub1/nested/.git &&\n> +\ttest -d .git/modules/sub1/modules/nested &&\n> +\tgit status >actual.1 &&\n> +\tgit -C sub1/nested rev-parse HEAD >actual.2 &&\n> +\ttest_cmp expect.1 actual.1 &&\n> +\ttest_cmp expect.2 actual.2\n> +'\n> +\n>  test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n>  \tgit init sub2 &&\n>  \ttest_commit -C sub2 first &&\n> -- \n> 2.11.0.486.g67830dbe1c\n\n\nAside from my one question the rest of this looks good to me.\n\n-- \nBrandon Williams\n"},{"id":"310116","messageId":"20170124221948.GB58021@google.com","threadId":"44961","inReplyTo":"CAGZ79kYKkx441bbU5Oy9Ernb1FmbcTybYbL_M_+yWG_ycfPwrA@mail.gmail.com","subject":"Re: [PATCH] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-01-24T22:19:48Z","receivedAt":"2017-01-24T22:20:06Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/24, Stefan Beller wrote:\n> On Tue, Jan 24, 2017 at 1:58 PM, Brandon Williams <bmwill@google.com> wrote:\n> > On 01/24, Stefan Beller wrote:\n> >> +     if (read_gitfile_gently(old_git_dir, &err_code) ||\n> >> +         err_code == READ_GITFILE_ERR_NOT_A_REPO) {\n> >> +             /*\n> >> +              * If it is an actual gitfile, it doesn't need migration,\n> >> +              * however in case of a recursively nested submodule, the\n> >> +              * gitfile content may be stale, as its superproject\n> >> +              * (which may be a submodule of another superproject)\n> >> +              * may have been moved. So expect a bogus pointer to be read,\n> >> +              * which materializes as error READ_GITFILE_ERR_NOT_A_REPO.\n> >> +              */\n> >> +             connect_work_tree_and_git_dir(path, real_new_git_dir);\n> >\n> > So connect_work_tree_and_git_dir() will update the .gitfile if it is\n> > stale.\n> >\n> >> +             return;\n> >> +     }\n> >> +\n> >> +     if (submodule_uses_worktrees(path))\n> >> +             die(_(\"relocate_gitdir for submodule '%s' with \"\n> >> +                   \"more than one worktree not supported\"), path);\n> >\n> > No current support for worktrees (yet!).\n> >\n> >> +\n> >>       if (!prefix)\n> >>               prefix = get_super_prefix();\n> >>\n> >> @@ -1437,22 +1448,14 @@ void absorb_git_dir_into_superproject(const char *prefix,\n> >>                                     const char *path,\n> >>                                     unsigned flags)\n> >>  {\n> >> -     const char *sub_git_dir, *v;\n> >> -     char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;\n> >>       struct strbuf gitdir = STRBUF_INIT;\n> >> -\n> >>       strbuf_addf(&gitdir, \"%s/.git\", path);\n> >> -     sub_git_dir = resolve_gitdir(gitdir.buf);\n> >>\n> >>       /* Not populated? */\n> >> -     if (!sub_git_dir)\n> >> +     if (!file_exists(gitdir.buf))\n> >>               goto out;\n> >\n> > There should be a is_submodule_populated() function now, maybe\n> > we should start using it when performing population checks?\n> \n> Yes I am aware of that, but the problem is we cannot use it here.\n> is_submodule_populated[1], just like the code here, uses\n> resolve_gitdir, which is\n> \n>     const char *resolve_gitdir(const char *suspect)\n>     {\n>         if (is_git_directory(suspect))\n>            return suspect;\n>         return read_gitfile(suspect);\n>     }\n> \n> And there you see the problem: read_gitfile will die on error.\n> we'd have to have use read_gitfile_gently(old_git_dir, &err_code),\n> and then allow READ_GITFILE_ERR_NOT_A_REPO to go through,\n> just as above.\n\nHmm, then maybe is_submodule_populated should be rewritten to not die on\nan error then?\n\n> \n> And that is also the reason why we had to move submodule_uses_worktrees\n> down, as it also uses no gentle function to look for a git directory\n> (read: it would die as well). When you have bogus content in your\n> .git file, there is really nothing you can do to determine if the submodule\n> is part of a worktree setup, so it is fine to postpone the check until after we\n> fixed up the link.\n> \n> So here is the bug you spotted: If it is a worktree already, then\n> read_gitfile_gently would work fine, no need to \"fix\" it.\n> \n> I'll resend with logic as follows:\n> \n>     char *retvalue = read_gitfile_gently(old_git_dir, &err_code);\n>     if (retvalue)\n>         // return early; a worktree is fine here, no need to check\n>         // because we do nothing\n> \n>     if (err_code == READ_GITFILE_ERR_NOT_A_REPO)\n>         // connect; then check for worktree and return early;\n> \n>     // do the actual relocation.\n> \n> \n> [1] as found e.g. at\n> https://public-inbox.org/git/1481915002-162130-2-git-send-email-bmwill@google.com/\n> \n> >\n> >>\n> >> -     /* Is it already absorbed into the superprojects git dir? */\n> >> -     real_sub_git_dir = real_pathdup(sub_git_dir);\n> >> -     real_common_git_dir = real_pathdup(get_git_common_dir());\n> >> -     if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n> >> -             relocate_single_git_dir_into_superproject(prefix, path);\n> >> +     relocate_single_git_dir_into_superproject(prefix, path);\n> >\n> > So the check was just pushed into the relocation function.\n> \n> The check was pushed down, so we can use the\n> connect_work_tree_and_git_dir instead.\n> \n> >\n> >>\n> >>       if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n> >>               struct child_process cp = CHILD_PROCESS_INIT;\n> >> @@ -1481,6 +1484,4 @@ void absorb_git_dir_into_superproject(const char *prefix,\n> >>\n> >>  out:\n> >>       strbuf_release(&gitdir);\n> >> -     free(real_sub_git_dir);\n> >> -     free(real_common_git_dir);\n> >>  }\n> >> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\n> >> index 1c47780e2b..e2bbb449b6 100755\n> >> --- a/t/t7412-submodule-absorbgitdirs.sh\n> >> +++ b/t/t7412-submodule-absorbgitdirs.sh\n> >> @@ -64,6 +64,33 @@ test_expect_success 'absorb the git dir in a nested submodule' '\n> >>       test_cmp expect.2 actual.2\n> >>  '\n> >>\n> >> +test_expect_success 're-setup nested submodule' '\n> >> +     # un-absorb the direct submodule, to test if the nested submodule\n> >> +     # is still correct (needs a rewrite of the gitfile only)\n> >> +     rm -rf sub1/.git &&\n> >> +     mv .git/modules/sub1 sub1/.git &&\n> >> +     GIT_WORK_TREE=. git -C sub1 config --unset core.worktree &&\n> >> +     # fixup the nested submodule\n> >> +     echo \"gitdir: ../.git/modules/nested\" >sub1/nested/.git &&\n> >> +     GIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \\\n> >> +             core.worktree \"../../../nested\" &&\n> >> +     # make sure this re-setup is correct\n> >> +     git status --ignore-submodules=none\n> >> +'\n> >> +\n> >> +test_expect_success 'absorb the git dir in a nested submodule' '\n> >> +     git status >expect.1 &&\n> >> +     git -C sub1/nested rev-parse HEAD >expect.2 &&\n> >> +     git submodule absorbgitdirs &&\n> >> +     test -f sub1/.git &&\n> >> +     test -f sub1/nested/.git &&\n> >> +     test -d .git/modules/sub1/modules/nested &&\n> >> +     git status >actual.1 &&\n> >> +     git -C sub1/nested rev-parse HEAD >actual.2 &&\n> >> +     test_cmp expect.1 actual.1 &&\n> >> +     test_cmp expect.2 actual.2\n> >> +'\n> >> +\n> >>  test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n> >>       git init sub2 &&\n> >>       test_commit -C sub2 first &&\n> >> --\n> >> 2.11.0.486.g67830dbe1c\n> >\n> >\n> > Aside from my one question the rest of this looks good to me.\n> >\n> > --\n> > Brandon Williams\n\n-- \nBrandon Williams\n"},{"id":"310117","messageId":"CAGZ79kYKkx441bbU5Oy9Ernb1FmbcTybYbL_M_+yWG_ycfPwrA@mail.gmail.com","threadId":"44961","inReplyTo":"20170124215851.GA58021@google.com","subject":"Re: [PATCH] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-24T22:13:53Z","receivedAt":"2017-01-24T22:20:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 24, 2017 at 1:58 PM, Brandon Williams <bmwill@google.com> wrote:\n> On 01/24, Stefan Beller wrote:\n>> +     if (read_gitfile_gently(old_git_dir, &err_code) ||\n>> +         err_code == READ_GITFILE_ERR_NOT_A_REPO) {\n>> +             /*\n>> +              * If it is an actual gitfile, it doesn't need migration,\n>> +              * however in case of a recursively nested submodule, the\n>> +              * gitfile content may be stale, as its superproject\n>> +              * (which may be a submodule of another superproject)\n>> +              * may have been moved. So expect a bogus pointer to be read,\n>> +              * which materializes as error READ_GITFILE_ERR_NOT_A_REPO.\n>> +              */\n>> +             connect_work_tree_and_git_dir(path, real_new_git_dir);\n>\n> So connect_work_tree_and_git_dir() will update the .gitfile if it is\n> stale.\n>\n>> +             return;\n>> +     }\n>> +\n>> +     if (submodule_uses_worktrees(path))\n>> +             die(_(\"relocate_gitdir for submodule '%s' with \"\n>> +                   \"more than one worktree not supported\"), path);\n>\n> No current support for worktrees (yet!).\n>\n>> +\n>>       if (!prefix)\n>>               prefix = get_super_prefix();\n>>\n>> @@ -1437,22 +1448,14 @@ void absorb_git_dir_into_superproject(const char *prefix,\n>>                                     const char *path,\n>>                                     unsigned flags)\n>>  {\n>> -     const char *sub_git_dir, *v;\n>> -     char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;\n>>       struct strbuf gitdir = STRBUF_INIT;\n>> -\n>>       strbuf_addf(&gitdir, \"%s/.git\", path);\n>> -     sub_git_dir = resolve_gitdir(gitdir.buf);\n>>\n>>       /* Not populated? */\n>> -     if (!sub_git_dir)\n>> +     if (!file_exists(gitdir.buf))\n>>               goto out;\n>\n> There should be a is_submodule_populated() function now, maybe\n> we should start using it when performing population checks?\n\nYes I am aware of that, but the problem is we cannot use it here.\nis_submodule_populated[1], just like the code here, uses\nresolve_gitdir, which is\n\n    const char *resolve_gitdir(const char *suspect)\n    {\n        if (is_git_directory(suspect))\n           return suspect;\n        return read_gitfile(suspect);\n    }\n\nAnd there you see the problem: read_gitfile will die on error.\nwe'd have to have use read_gitfile_gently(old_git_dir, &err_code),\nand then allow READ_GITFILE_ERR_NOT_A_REPO to go through,\njust as above.\n\nAnd that is also the reason why we had to move submodule_uses_worktrees\ndown, as it also uses no gentle function to look for a git directory\n(read: it would die as well). When you have bogus content in your\n.git file, there is really nothing you can do to determine if the submodule\nis part of a worktree setup, so it is fine to postpone the check until after we\nfixed up the link.\n\nSo here is the bug you spotted: If it is a worktree already, then\nread_gitfile_gently would work fine, no need to \"fix\" it.\n\nI'll resend with logic as follows:\n\n    char *retvalue = read_gitfile_gently(old_git_dir, &err_code);\n    if (retvalue)\n        // return early; a worktree is fine here, no need to check\n        // because we do nothing\n\n    if (err_code == READ_GITFILE_ERR_NOT_A_REPO)\n        // connect; then check for worktree and return early;\n\n    // do the actual relocation.\n\n\n[1] as found e.g. at\nhttps://public-inbox.org/git/1481915002-162130-2-git-send-email-bmwill@google.com/\n\n>\n>>\n>> -     /* Is it already absorbed into the superprojects git dir? */\n>> -     real_sub_git_dir = real_pathdup(sub_git_dir);\n>> -     real_common_git_dir = real_pathdup(get_git_common_dir());\n>> -     if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n>> -             relocate_single_git_dir_into_superproject(prefix, path);\n>> +     relocate_single_git_dir_into_superproject(prefix, path);\n>\n> So the check was just pushed into the relocation function.\n\nThe check was pushed down, so we can use the\nconnect_work_tree_and_git_dir instead.\n\n>\n>>\n>>       if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n>>               struct child_process cp = CHILD_PROCESS_INIT;\n>> @@ -1481,6 +1484,4 @@ void absorb_git_dir_into_superproject(const char *prefix,\n>>\n>>  out:\n>>       strbuf_release(&gitdir);\n>> -     free(real_sub_git_dir);\n>> -     free(real_common_git_dir);\n>>  }\n>> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\n>> index 1c47780e2b..e2bbb449b6 100755\n>> --- a/t/t7412-submodule-absorbgitdirs.sh\n>> +++ b/t/t7412-submodule-absorbgitdirs.sh\n>> @@ -64,6 +64,33 @@ test_expect_success 'absorb the git dir in a nested submodule' '\n>>       test_cmp expect.2 actual.2\n>>  '\n>>\n>> +test_expect_success 're-setup nested submodule' '\n>> +     # un-absorb the direct submodule, to test if the nested submodule\n>> +     # is still correct (needs a rewrite of the gitfile only)\n>> +     rm -rf sub1/.git &&\n>> +     mv .git/modules/sub1 sub1/.git &&\n>> +     GIT_WORK_TREE=. git -C sub1 config --unset core.worktree &&\n>> +     # fixup the nested submodule\n>> +     echo \"gitdir: ../.git/modules/nested\" >sub1/nested/.git &&\n>> +     GIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \\\n>> +             core.worktree \"../../../nested\" &&\n>> +     # make sure this re-setup is correct\n>> +     git status --ignore-submodules=none\n>> +'\n>> +\n>> +test_expect_success 'absorb the git dir in a nested submodule' '\n>> +     git status >expect.1 &&\n>> +     git -C sub1/nested rev-parse HEAD >expect.2 &&\n>> +     git submodule absorbgitdirs &&\n>> +     test -f sub1/.git &&\n>> +     test -f sub1/nested/.git &&\n>> +     test -d .git/modules/sub1/modules/nested &&\n>> +     git status >actual.1 &&\n>> +     git -C sub1/nested rev-parse HEAD >actual.2 &&\n>> +     test_cmp expect.1 actual.1 &&\n>> +     test_cmp expect.2 actual.2\n>> +'\n>> +\n>>  test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n>>       git init sub2 &&\n>>       test_commit -C sub2 first &&\n>> --\n>> 2.11.0.486.g67830dbe1c\n>\n>\n> Aside from my one question the rest of this looks good to me.\n>\n> --\n> Brandon Williams\n"},{"id":"310123","messageId":"20170124235651.18749-1-sbeller@google.com","threadId":"44961","inReplyTo":"20170124221948.GB58021@google.com","subject":"[PATCHv2 0/3] fix recursive submodule absorbing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-24T23:56:48Z","receivedAt":"2017-01-24T23:57:06Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In this second iteration, only absorb_git_dir_into_superproject is touched,\nwhich does the check if connect_work_tree_and_git_dir is needed.\n\nInternally I also had a patch that converts is_submodule_populated\nto be gentle, but it is not needed here, so I dropped it before sending out.\n\nThanks,\nStefan\n\n\nStefan Beller (3):\n  Add gentle version of resolve_git_dir\n  cache.h: expose the dying procedure for reading gitlinks\n  submodule absorbing: fix worktree/gitdir pointers recursively for\n    non-moves\n\n cache.h                            |  5 +++-\n setup.c                            | 52 ++++++++++++++++++++------------------\n submodule.c                        | 43 ++++++++++++++++++++++++-------\n t/t7412-submodule-absorbgitdirs.sh | 27 ++++++++++++++++++++\n 4 files changed, 93 insertions(+), 34 deletions(-)\n\n-- \n2.11.0.495.g04f60290a0.dirty\n\n"},{"id":"310124","messageId":"20170124235651.18749-2-sbeller@google.com","threadId":"44961","inReplyTo":"20170124235651.18749-1-sbeller@google.com","subject":"[PATCHv2 1/3] Add gentle version of resolve_git_dir","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-24T23:56:49Z","receivedAt":"2017-01-24T23:57:09Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This follows a93bedada (setup: add gentle version of read_gitfile,\n2015-06-09), and assumes the same reasoning. resolve_git_dir is unsuited\nfor speculative calls, so we want to use the gentle version to find out\nabout potential errors.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n cache.h | 4 +++-\n setup.c | 4 ++--\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 00a029af36..cafa3d10ae 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -509,7 +509,9 @@ extern int is_nonbare_repository_dir(struct strbuf *path);\n #define READ_GITFILE_ERR_TOO_LARGE 8\n extern const char *read_gitfile_gently(const char *path, int *return_error_code);\n #define read_gitfile(path) read_gitfile_gently((path), NULL)\n-extern const char *resolve_gitdir(const char *suspect);\n+extern const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);\n+#define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)\n+\n extern void set_git_work_tree(const char *tree);\n \n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\ndiff --git a/setup.c b/setup.c\nindex 1b534a7508..4605fd3c3c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1017,11 +1017,11 @@ const char *setup_git_directory(void)\n \treturn setup_git_directory_gently(NULL);\n }\n \n-const char *resolve_gitdir(const char *suspect)\n+const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)\n {\n \tif (is_git_directory(suspect))\n \t\treturn suspect;\n-\treturn read_gitfile(suspect);\n+\treturn read_gitfile_gently(suspect, return_error_code);\n }\n \n /* if any standard file descriptor is missing open it to /dev/null */\n-- \n2.11.0.495.g04f60290a0.dirty\n\n"},{"id":"310125","messageId":"20170124235651.18749-3-sbeller@google.com","threadId":"44961","inReplyTo":"20170124235651.18749-1-sbeller@google.com","subject":"[PATCHv2 2/3] cache.h: expose the dying procedure for reading gitlinks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-24T23:56:50Z","receivedAt":"2017-01-24T23:57:12Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we want to react to only a subset of errors, defaulting\nthe rest to die as usual. Separate the block that takes care of dying\ninto its own function so we have easy access to it.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n cache.h |  1 +\n setup.c | 48 ++++++++++++++++++++++++++----------------------\n 2 files changed, 27 insertions(+), 22 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex cafa3d10ae..d55f5dccb1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -507,6 +507,7 @@ extern int is_nonbare_repository_dir(struct strbuf *path);\n #define READ_GITFILE_ERR_NO_PATH 6\n #define READ_GITFILE_ERR_NOT_A_REPO 7\n #define READ_GITFILE_ERR_TOO_LARGE 8\n+extern void read_gitfile_error_die(int error_code, const char *path, const char *dir);\n extern const char *read_gitfile_gently(const char *path, int *return_error_code);\n #define read_gitfile(path) read_gitfile_gently((path), NULL)\n extern const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);\ndiff --git a/setup.c b/setup.c\nindex 4605fd3c3c..967f289f1e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -486,6 +486,30 @@ int verify_repository_format(const struct repository_format *format,\n \treturn 0;\n }\n \n+void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n+{\n+\tswitch (error_code) {\n+\tcase READ_GITFILE_ERR_STAT_FAILED:\n+\tcase READ_GITFILE_ERR_NOT_A_FILE:\n+\t\t/* non-fatal; follow return path */\n+\t\tbreak;\n+\tcase READ_GITFILE_ERR_OPEN_FAILED:\n+\t\tdie_errno(\"Error opening '%s'\", path);\n+\tcase READ_GITFILE_ERR_TOO_LARGE:\n+\t\tdie(\"Too large to be a .git file: '%s'\", path);\n+\tcase READ_GITFILE_ERR_READ_FAILED:\n+\t\tdie(\"Error reading %s\", path);\n+\tcase READ_GITFILE_ERR_INVALID_FORMAT:\n+\t\tdie(\"Invalid gitfile format: %s\", path);\n+\tcase READ_GITFILE_ERR_NO_PATH:\n+\t\tdie(\"No path in gitfile: %s\", path);\n+\tcase READ_GITFILE_ERR_NOT_A_REPO:\n+\t\tdie(\"Not a git repository: %s\", dir);\n+\tdefault:\n+\t\tdie(\"BUG: unknown error code\");\n+\t}\n+}\n+\n /*\n  * Try to read the location of the git directory from the .git file,\n  * return path to git directory if found.\n@@ -559,28 +583,8 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n cleanup_return:\n \tif (return_error_code)\n \t\t*return_error_code = error_code;\n-\telse if (error_code) {\n-\t\tswitch (error_code) {\n-\t\tcase READ_GITFILE_ERR_STAT_FAILED:\n-\t\tcase READ_GITFILE_ERR_NOT_A_FILE:\n-\t\t\t/* non-fatal; follow return path */\n-\t\t\tbreak;\n-\t\tcase READ_GITFILE_ERR_OPEN_FAILED:\n-\t\t\tdie_errno(\"Error opening '%s'\", path);\n-\t\tcase READ_GITFILE_ERR_TOO_LARGE:\n-\t\t\tdie(\"Too large to be a .git file: '%s'\", path);\n-\t\tcase READ_GITFILE_ERR_READ_FAILED:\n-\t\t\tdie(\"Error reading %s\", path);\n-\t\tcase READ_GITFILE_ERR_INVALID_FORMAT:\n-\t\t\tdie(\"Invalid gitfile format: %s\", path);\n-\t\tcase READ_GITFILE_ERR_NO_PATH:\n-\t\t\tdie(\"No path in gitfile: %s\", path);\n-\t\tcase READ_GITFILE_ERR_NOT_A_REPO:\n-\t\t\tdie(\"Not a git repository: %s\", dir);\n-\t\tdefault:\n-\t\t\tassert(0);\n-\t\t}\n-\t}\n+\telse if (error_code)\n+\t\tread_gitfile_error_die(error_code, path, dir);\n \n \tfree(buf);\n \treturn error_code ? NULL : path;\n-- \n2.11.0.495.g04f60290a0.dirty\n\n"},{"id":"310126","messageId":"20170124235651.18749-4-sbeller@google.com","threadId":"44961","inReplyTo":"20170124235651.18749-1-sbeller@google.com","subject":"[PATCHv2 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-24T23:56:51Z","receivedAt":"2017-01-24T23:57:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.\nWhen nested is already absorbed into sub, but sub is not absorbed into\nits superproject, then we need to fixup the gitfile and core.worktree\nsetting for 'nested' when absorbing 'sub', but we do not need to move\nits git dir around.\n\nPreviously 'nested's gitfile contained \"gitdir: ../.git/modules/nested\";\nit has to be corrected to \"gitdir: ../../.git/modules/sub1/modules/nested\".\n\nAn alternative I considered to do this work lazily, i.e. when resolving\n\"../.git/modules/nested\", we would notice the \".git\" being a gitfile\nlinking to another path.  That seemed to be robuster by design, but harder\nto get the implementation right.  Maybe we have to do that anyway once we\ntry to have submodules and worktrees working nicely together, but for now\njust produce 'correct' (i.e. direct) pointers.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n submodule.c                        | 43 ++++++++++++++++++++++++++++++--------\n t/t7412-submodule-absorbgitdirs.sh | 27 ++++++++++++++++++++++++\n 2 files changed, 61 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 4c4f033e8a..95437105bf 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1437,22 +1437,47 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \t\t\t\t      const char *path,\n \t\t\t\t      unsigned flags)\n {\n+\tint err_code;\n \tconst char *sub_git_dir, *v;\n \tchar *real_sub_git_dir = NULL, *real_common_git_dir = NULL;\n \tstruct strbuf gitdir = STRBUF_INIT;\n-\n \tstrbuf_addf(&gitdir, \"%s/.git\", path);\n-\tsub_git_dir = resolve_gitdir(gitdir.buf);\n+\tsub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);\n \n \t/* Not populated? */\n-\tif (!sub_git_dir)\n-\t\tgoto out;\n+\tif (!sub_git_dir) {\n+\t\tchar *real_new_git_dir;\n+\t\tconst char *new_git_dir;\n+\t\tconst struct submodule *sub;\n+\n+\t\tif (err_code == READ_GITFILE_ERR_STAT_FAILED)\n+\t\t\tgoto out; /* unpopulated as expected */\n+\t\tif (err_code != READ_GITFILE_ERR_NOT_A_REPO)\n+\t\t\t/* We don't know what broke here. */\n+\t\t\tread_gitfile_error_die(err_code, path, NULL);\n \n-\t/* Is it already absorbed into the superprojects git dir? */\n-\treal_sub_git_dir = real_pathdup(sub_git_dir);\n-\treal_common_git_dir = real_pathdup(get_git_common_dir());\n-\tif (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n-\t\trelocate_single_git_dir_into_superproject(prefix, path);\n+\t\t/*\n+\t\t* Maybe populated, but no git directory was found?\n+\t\t* This can happen if the superproject is a submodule\n+\t\t* itself and was just absorbed. The absorption of the\n+\t\t* superproject did not rewrite the git file links yet,\n+\t\t* fix it now.\n+\t\t*/\n+\t\tsub = submodule_from_path(null_sha1, path);\n+\t\tif (!sub)\n+\t\t\tdie(_(\"could not lookup name for submodule '%s'\"), path);\n+\t\tnew_git_dir = git_path(\"modules/%s\", sub->name);\n+\t\tif (safe_create_leading_directories_const(new_git_dir) < 0)\n+\t\t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n+\t\treal_new_git_dir = real_pathdup(new_git_dir);\n+\t\tconnect_work_tree_and_git_dir(path, real_new_git_dir);\n+\t} else {\n+\t\t/* Is it already absorbed into the superprojects git dir? */\n+\t\treal_sub_git_dir = real_pathdup(sub_git_dir);\n+\t\treal_common_git_dir = real_pathdup(get_git_common_dir());\n+\t\tif (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n+\t\t\trelocate_single_git_dir_into_superproject(prefix, path);\n+\t}\n \n \tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\ndiff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\nindex 1c47780e2b..e2bbb449b6 100755\n--- a/t/t7412-submodule-absorbgitdirs.sh\n+++ b/t/t7412-submodule-absorbgitdirs.sh\n@@ -64,6 +64,33 @@ test_expect_success 'absorb the git dir in a nested submodule' '\n \ttest_cmp expect.2 actual.2\n '\n \n+test_expect_success 're-setup nested submodule' '\n+\t# un-absorb the direct submodule, to test if the nested submodule\n+\t# is still correct (needs a rewrite of the gitfile only)\n+\trm -rf sub1/.git &&\n+\tmv .git/modules/sub1 sub1/.git &&\n+\tGIT_WORK_TREE=. git -C sub1 config --unset core.worktree &&\n+\t# fixup the nested submodule\n+\techo \"gitdir: ../.git/modules/nested\" >sub1/nested/.git &&\n+\tGIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \\\n+\t\tcore.worktree \"../../../nested\" &&\n+\t# make sure this re-setup is correct\n+\tgit status --ignore-submodules=none\n+'\n+\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+\ttest -f sub1/.git &&\n+\ttest -f sub1/nested/.git &&\n+\ttest -d .git/modules/sub1/modules/nested &&\n+\tgit status >actual.1 &&\n+\tgit -C sub1/nested rev-parse HEAD >actual.2 &&\n+\ttest_cmp expect.1 actual.1 &&\n+\ttest_cmp expect.2 actual.2\n+'\n+\n test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n \tgit init sub2 &&\n \ttest_commit -C sub2 first &&\n-- \n2.11.0.495.g04f60290a0.dirty\n\n"},{"id":"310131","messageId":"20170125004610.GC58021@google.com","threadId":"44961","inReplyTo":"20170124235651.18749-4-sbeller@google.com","subject":"Re: [PATCHv2 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-01-25T00:46:10Z","receivedAt":"2017-01-25T00:46:51Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/24, Stefan Beller wrote:\n> Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.\n> When nested is already absorbed into sub, but sub is not absorbed into\n> its superproject, then we need to fixup the gitfile and core.worktree\n> setting for 'nested' when absorbing 'sub', but we do not need to move\n> its git dir around.\n> \n> Previously 'nested's gitfile contained \"gitdir: ../.git/modules/nested\";\n> it has to be corrected to \"gitdir: ../../.git/modules/sub1/modules/nested\".\n> \n> An alternative I considered to do this work lazily, i.e. when resolving\n> \"../.git/modules/nested\", we would notice the \".git\" being a gitfile\n> linking to another path.  That seemed to be robuster by design, but harder\n> to get the implementation right.  Maybe we have to do that anyway once we\n> try to have submodules and worktrees working nicely together, but for now\n> just produce 'correct' (i.e. direct) pointers.\n> \n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  submodule.c                        | 43 ++++++++++++++++++++++++++++++--------\n>  t/t7412-submodule-absorbgitdirs.sh | 27 ++++++++++++++++++++++++\n>  2 files changed, 61 insertions(+), 9 deletions(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index 4c4f033e8a..95437105bf 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1437,22 +1437,47 @@ void absorb_git_dir_into_superproject(const char *prefix,\n>  \t\t\t\t      const char *path,\n>  \t\t\t\t      unsigned flags)\n>  {\n> +\tint err_code;\n>  \tconst char *sub_git_dir, *v;\n>  \tchar *real_sub_git_dir = NULL, *real_common_git_dir = NULL;\n>  \tstruct strbuf gitdir = STRBUF_INIT;\n> -\n>  \tstrbuf_addf(&gitdir, \"%s/.git\", path);\n> -\tsub_git_dir = resolve_gitdir(gitdir.buf);\n> +\tsub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);\n>  \n>  \t/* Not populated? */\n> -\tif (!sub_git_dir)\n> -\t\tgoto out;\n> +\tif (!sub_git_dir) {\n> +\t\tchar *real_new_git_dir;\n> +\t\tconst char *new_git_dir;\n> +\t\tconst struct submodule *sub;\n> +\n> +\t\tif (err_code == READ_GITFILE_ERR_STAT_FAILED)\n> +\t\t\tgoto out; /* unpopulated as expected */\n> +\t\tif (err_code != READ_GITFILE_ERR_NOT_A_REPO)\n> +\t\t\t/* We don't know what broke here. */\n> +\t\t\tread_gitfile_error_die(err_code, path, NULL);\n>  \n> -\t/* Is it already absorbed into the superprojects git dir? */\n> -\treal_sub_git_dir = real_pathdup(sub_git_dir);\n> -\treal_common_git_dir = real_pathdup(get_git_common_dir());\n> -\tif (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n> -\t\trelocate_single_git_dir_into_superproject(prefix, path);\n> +\t\t/*\n> +\t\t* Maybe populated, but no git directory was found?\n> +\t\t* This can happen if the superproject is a submodule\n> +\t\t* itself and was just absorbed. The absorption of the\n> +\t\t* superproject did not rewrite the git file links yet,\n> +\t\t* fix it now.\n> +\t\t*/\n> +\t\tsub = submodule_from_path(null_sha1, path);\n> +\t\tif (!sub)\n> +\t\t\tdie(_(\"could not lookup name for submodule '%s'\"), path);\n> +\t\tnew_git_dir = git_path(\"modules/%s\", sub->name);\n> +\t\tif (safe_create_leading_directories_const(new_git_dir) < 0)\n> +\t\t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n> +\t\treal_new_git_dir = real_pathdup(new_git_dir);\n> +\t\tconnect_work_tree_and_git_dir(path, real_new_git_dir);\n\nMemory leak with 'real_new_git_dir'\n\n> +\t} else {\n> +\t\t/* Is it already absorbed into the superprojects git dir? */\n> +\t\treal_sub_git_dir = real_pathdup(sub_git_dir);\n> +\t\treal_common_git_dir = real_pathdup(get_git_common_dir());\n\nThe scope of 'real_sub_git_dir' and 'real_common_git_dir' variable can\nbe narrowed.  Move their declaration here and free it prior to exiting\nthe else block.\n\n> +\t\tif (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n\n'v' isn't ever used, just use starts_with() and get rid of 'v'\n\n> +\t\t\trelocate_single_git_dir_into_superproject(prefix, path);\n> +\t}\n>  \n\nThere's a label 'out' at the end of the function which can be removed as\nthere is no 'goto' statement using it.\n\n>  \tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n>  \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\n> index 1c47780e2b..e2bbb449b6 100755\n> --- a/t/t7412-submodule-absorbgitdirs.sh\n> +++ b/t/t7412-submodule-absorbgitdirs.sh\n> @@ -64,6 +64,33 @@ test_expect_success 'absorb the git dir in a nested submodule' '\n>  \ttest_cmp expect.2 actual.2\n>  '\n>  \n> +test_expect_success 're-setup nested submodule' '\n> +\t# un-absorb the direct submodule, to test if the nested submodule\n> +\t# is still correct (needs a rewrite of the gitfile only)\n> +\trm -rf sub1/.git &&\n> +\tmv .git/modules/sub1 sub1/.git &&\n> +\tGIT_WORK_TREE=. git -C sub1 config --unset core.worktree &&\n> +\t# fixup the nested submodule\n> +\techo \"gitdir: ../.git/modules/nested\" >sub1/nested/.git &&\n> +\tGIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \\\n> +\t\tcore.worktree \"../../../nested\" &&\n> +\t# make sure this re-setup is correct\n> +\tgit status --ignore-submodules=none\n> +'\n> +\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> +\ttest -f sub1/.git &&\n> +\ttest -f sub1/nested/.git &&\n> +\ttest -d .git/modules/sub1/modules/nested &&\n> +\tgit status >actual.1 &&\n> +\tgit -C sub1/nested rev-parse HEAD >actual.2 &&\n> +\ttest_cmp expect.1 actual.1 &&\n> +\ttest_cmp expect.2 actual.2\n> +'\n> +\n>  test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n>  \tgit init sub2 &&\n>  \ttest_commit -C sub2 first &&\n> -- \n> 2.11.0.495.g04f60290a0.dirty\n> \n\nYou can squash them into your patch, assuming you agree with the changes\n:)\n\n-- \nBrandon Williams\n\n--8<--\n\nFrom 0336c4bee60a85d8ad319ff55deea95debb3ceda Mon Sep 17 00:00:00 2001\nFrom: Brandon Williams <bmwill@google.com>\nDate: Tue, 24 Jan 2017 16:44:43 -0800\nSubject: [PATCH] SQUASH\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n submodule.c | 17 ++++++++++-------\n 1 file changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 95437105b..0d9c4bbbe 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1438,8 +1438,7 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \t\t\t\t      unsigned flags)\n {\n \tint err_code;\n-\tconst char *sub_git_dir, *v;\n-\tchar *real_sub_git_dir = NULL, *real_common_git_dir = NULL;\n+\tconst char *sub_git_dir;\n \tstruct strbuf gitdir = STRBUF_INIT;\n \tstrbuf_addf(&gitdir, \"%s/.git\", path);\n \tsub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);\n@@ -1471,12 +1470,18 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \t\t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n \t\treal_new_git_dir = real_pathdup(new_git_dir);\n \t\tconnect_work_tree_and_git_dir(path, real_new_git_dir);\n+\n+\t\tfree(real_new_git_dir);\n \t} else {\n \t\t/* Is it already absorbed into the superprojects git dir? */\n-\t\treal_sub_git_dir = real_pathdup(sub_git_dir);\n-\t\treal_common_git_dir = real_pathdup(get_git_common_dir());\n-\t\tif (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n+\t\tchar *real_sub_git_dir = real_pathdup(sub_git_dir);\n+\t\tchar *real_common_git_dir = real_pathdup(get_git_common_dir());\n+\n+\t\tif (!starts_with(real_sub_git_dir, real_common_git_dir))\n \t\t\trelocate_single_git_dir_into_superproject(prefix, path);\n+\n+\t\tfree(real_sub_git_dir);\n+\t\tfree(real_common_git_dir);\n \t}\n \n \tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n@@ -1506,6 +1511,4 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \n out:\n \tstrbuf_release(&gitdir);\n-\tfree(real_sub_git_dir);\n-\tfree(real_common_git_dir);\n }\n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"310251","messageId":"xmqq37g692da.fsf@gitster.mtv.corp.google.com","threadId":"44961","inReplyTo":"20170124235651.18749-4-sbeller@google.com","subject":"Re: [PATCHv2 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-25T22:54:57Z","receivedAt":"2017-01-25T22:55:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.\n> When nested is already absorbed into sub, but sub is not absorbed into\n> its superproject, then we need to fixup the gitfile and core.worktree\n> setting for 'nested' when absorbing 'sub', but we do not need to move\n> its git dir around.\n>\n> Previously 'nested's gitfile contained \"gitdir: ../.git/modules/nested\";\n> it has to be corrected to \"gitdir: ../../.git/modules/sub1/modules/nested\".\n\nThat sounds like a sensible way to make things consistent.\n\n"},{"id":"310253","messageId":"CAGZ79kaSQ8YE2S2MuaaOo9HeyHL7A66Ug9aO_j7F=o2e5RO6iQ@mail.gmail.com","threadId":"44961","inReplyTo":"20170125004610.GC58021@google.com","subject":"Re: [PATCHv2 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-25T23:04:08Z","receivedAt":"2017-01-25T23:04:19Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 24, 2017 at 4:46 PM, Brandon Williams <bmwill@google.com> wrote:\n> On 01/24, Stefan Beller wrote:\n>> +             connect_work_tree_and_git_dir(path, real_new_git_dir);\n>\n> Memory leak with 'real_new_git_dir'\n\nyup. :(\n\n\n> The scope of 'real_sub_git_dir' and 'real_common_git_dir' variable can\n> be narrowed.  Move their declaration here and free it prior to exiting\n> the else block.\n\nok.\n\n> 'v' isn't ever used, just use starts_with() and get rid of 'v'\n\nmakes sense.\n\n>\n>> +                     relocate_single_git_dir_into_superproject(prefix, path);\n>> +     }\n>>\n>\n> There's a label 'out' at the end of the function which can be removed as\n> there is no 'goto' statement using it.\n\nWe do use it in\n\n    if (err_code == READ_GITFILE_ERR_STAT_FAILED)\n        goto out; /* unpopulated as expected */\n\nI took your proposed SQUASH and removed the goto, see the followup email.\n"},{"id":"310255","messageId":"20170125230450.4393-1-sbeller@google.com","threadId":"44961","inReplyTo":"xmqq37g692da.fsf@gitster.mtv.corp.google.com","subject":"[PATCHv3 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-25T23:04:50Z","receivedAt":"2017-01-25T23:04:57Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.\nWhen nested is already absorbed into sub, but sub is not absorbed into\nits superproject, then we need to fixup the gitfile and core.worktree\nsetting for 'nested' when absorbing 'sub', but we do not need to move\nits git dir around.\n\nPreviously 'nested's gitfile contained \"gitdir: ../.git/modules/nested\";\nit has to be corrected to \"gitdir: ../../.git/modules/sub1/modules/nested\".\n\nAn alternative I considered to do this work lazily, i.e. when resolving\n\"../.git/modules/nested\", we would notice the \".git\" being a gitfile\nlinking to another path.  That seemed to be robuster by design, but harder\nto get the implementation right.  Maybe we have to do that anyway once we\ntry to have submodules and worktrees working nicely together, but for now\njust produce 'correct' (i.e. direct) pointers.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n This replaces the last patch of the series, containing Brandons SQUASH proposal\n as well as the removal of the goto.\n Thanks,\n Stefan\n\n submodule.c                        | 62 ++++++++++++++++++++++++++++----------\n t/t7412-submodule-absorbgitdirs.sh | 27 +++++++++++++++++\n 2 files changed, 73 insertions(+), 16 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 4c4f033e8a..3b98766a6b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1437,22 +1437,57 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \t\t\t\t      const char *path,\n \t\t\t\t      unsigned flags)\n {\n-\tconst char *sub_git_dir, *v;\n-\tchar *real_sub_git_dir = NULL, *real_common_git_dir = NULL;\n+\tint err_code;\n+\tconst char *sub_git_dir;\n \tstruct strbuf gitdir = STRBUF_INIT;\n-\n \tstrbuf_addf(&gitdir, \"%s/.git\", path);\n-\tsub_git_dir = resolve_gitdir(gitdir.buf);\n+\tsub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);\n \n \t/* Not populated? */\n-\tif (!sub_git_dir)\n-\t\tgoto out;\n+\tif (!sub_git_dir) {\n+\t\tchar *real_new_git_dir;\n+\t\tconst char *new_git_dir;\n+\t\tconst struct submodule *sub;\n+\n+\t\tif (err_code == READ_GITFILE_ERR_STAT_FAILED) {\n+\t\t\t/* unpopulated as expected */\n+\t\t\tstrbuf_release(&gitdir);\n+\t\t\treturn;\n+\t\t}\n+\n+\t\tif (err_code != READ_GITFILE_ERR_NOT_A_REPO)\n+\t\t\t/* We don't know what broke here. */\n+\t\t\tread_gitfile_error_die(err_code, path, NULL);\n+\n+\t\t/*\n+\t\t* Maybe populated, but no git directory was found?\n+\t\t* This can happen if the superproject is a submodule\n+\t\t* itself and was just absorbed. The absorption of the\n+\t\t* superproject did not rewrite the git file links yet,\n+\t\t* fix it now.\n+\t\t*/\n+\t\tsub = submodule_from_path(null_sha1, path);\n+\t\tif (!sub)\n+\t\t\tdie(_(\"could not lookup name for submodule '%s'\"), path);\n+\t\tnew_git_dir = git_path(\"modules/%s\", sub->name);\n+\t\tif (safe_create_leading_directories_const(new_git_dir) < 0)\n+\t\t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n+\t\treal_new_git_dir = real_pathdup(new_git_dir);\n+\t\tconnect_work_tree_and_git_dir(path, real_new_git_dir);\n+\n+\t\tfree(real_new_git_dir);\n+\t} else {\n+\t\t/* Is it already absorbed into the superprojects git dir? */\n+\t\tchar *real_sub_git_dir = real_pathdup(sub_git_dir);\n+\t\tchar *real_common_git_dir = real_pathdup(get_git_common_dir());\n \n-\t/* Is it already absorbed into the superprojects git dir? */\n-\treal_sub_git_dir = real_pathdup(sub_git_dir);\n-\treal_common_git_dir = real_pathdup(get_git_common_dir());\n-\tif (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n-\t\trelocate_single_git_dir_into_superproject(prefix, path);\n+\t\tif (!starts_with(real_sub_git_dir, real_common_git_dir))\n+\t\t\trelocate_single_git_dir_into_superproject(prefix, path);\n+\n+\t\tfree(real_sub_git_dir);\n+\t\tfree(real_common_git_dir);\n+\t}\n+\tstrbuf_release(&gitdir);\n \n \tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1478,9 +1513,4 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \n \t\tstrbuf_release(&sb);\n \t}\n-\n-out:\n-\tstrbuf_release(&gitdir);\n-\tfree(real_sub_git_dir);\n-\tfree(real_common_git_dir);\n }\ndiff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\nindex 1c47780e2b..e2bbb449b6 100755\n--- a/t/t7412-submodule-absorbgitdirs.sh\n+++ b/t/t7412-submodule-absorbgitdirs.sh\n@@ -64,6 +64,33 @@ test_expect_success 'absorb the git dir in a nested submodule' '\n \ttest_cmp expect.2 actual.2\n '\n \n+test_expect_success 're-setup nested submodule' '\n+\t# un-absorb the direct submodule, to test if the nested submodule\n+\t# is still correct (needs a rewrite of the gitfile only)\n+\trm -rf sub1/.git &&\n+\tmv .git/modules/sub1 sub1/.git &&\n+\tGIT_WORK_TREE=. git -C sub1 config --unset core.worktree &&\n+\t# fixup the nested submodule\n+\techo \"gitdir: ../.git/modules/nested\" >sub1/nested/.git &&\n+\tGIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \\\n+\t\tcore.worktree \"../../../nested\" &&\n+\t# make sure this re-setup is correct\n+\tgit status --ignore-submodules=none\n+'\n+\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+\ttest -f sub1/.git &&\n+\ttest -f sub1/nested/.git &&\n+\ttest -d .git/modules/sub1/modules/nested &&\n+\tgit status >actual.1 &&\n+\tgit -C sub1/nested rev-parse HEAD >actual.2 &&\n+\ttest_cmp expect.1 actual.1 &&\n+\ttest_cmp expect.2 actual.2\n+'\n+\n test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n \tgit init sub2 &&\n \ttest_commit -C sub2 first &&\n-- \n2.11.0.495.g04f60290a0.dirty\n\n"},{"id":"310264","messageId":"20170125233954.GD83343@google.com","threadId":"44961","inReplyTo":"20170125230450.4393-1-sbeller@google.com","subject":"Re: [PATCHv3 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-01-25T23:39:54Z","receivedAt":"2017-01-25T23:40:01Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/25, Stefan Beller wrote:\n> Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.\n> When nested is already absorbed into sub, but sub is not absorbed into\n> its superproject, then we need to fixup the gitfile and core.worktree\n> setting for 'nested' when absorbing 'sub', but we do not need to move\n> its git dir around.\n> \n> Previously 'nested's gitfile contained \"gitdir: ../.git/modules/nested\";\n> it has to be corrected to \"gitdir: ../../.git/modules/sub1/modules/nested\".\n> \n> An alternative I considered to do this work lazily, i.e. when resolving\n> \"../.git/modules/nested\", we would notice the \".git\" being a gitfile\n> linking to another path.  That seemed to be robuster by design, but harder\n> to get the implementation right.  Maybe we have to do that anyway once we\n> try to have submodules and worktrees working nicely together, but for now\n> just produce 'correct' (i.e. direct) pointers.\n> \n> Signed-off-by: Stefan Beller <sbeller@google.com>\n\nLooks good to me!\n\n-- \nBrandon Williams\n"}]}