{"thread":{"id":"41880","subject":"[PATCH 0/4] Fix relative path issues in recursive submodules.","startedAt":"2016-03-31T00:17:33Z","lastAt":"2016-03-31T19:22:05Z","messageCount":17,"participants":["Stefan Beller","Jacob Keller","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"282281","messageId":"1459383457-6848-1-git-send-email-sbeller@google.com","threadId":"41880","inReplyTo":null,"subject":"[PATCH 0/4] Fix relative path issues in recursive submodules.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T00:17:33Z","receivedAt":"2016-03-31T00:17:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"It took me longer than expected to fix this bug.\n\nIt comes with a test and minor refactoring and applies on top of\norigin/sb/submodule-helper, such that it can be merged into 2.7, 2.8 as well\nas master.\n\nPatch 1 is a test which fails; it has a similar layout as the\nreal failing repository Norio Nomura pointed out, but simplified to the\nbare essentials for reproduction of the bug.\n\nPatch 2&3 are not strictly necessary for fixing the isseu, but it removes\nstupid code I wrote, so the resulting code looks a little better.\n\nPatch 4 fixes the issue by giving more information to relative_path,\nsuch that a relative path can be found in all cases.\n\nThanks,\nStefan\n\nStefan Beller (4):\n recursive submodules: test for relative paths\n submodule--helper clone: simplify path check\n submodule--helper clone: remove double path checking\n submodule--helper: use relative path if possible\n\n builtin/submodule--helper.c | 16 ++++++++--------\n t/t7400-submodule-basic.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 49 insertions(+), 8 deletions(-)\n\n-- \n2.5.0.264.g4004fdc.dirty\n"},{"id":"282282","messageId":"1459383457-6848-2-git-send-email-sbeller@google.com","threadId":"41880","inReplyTo":"1459383457-6848-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/4] recursive submodules: test for relative paths","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T00:17:34Z","receivedAt":"2016-03-31T00:17:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This was reported as a regression at $gmane/290280. The root cause for\nthat bug is in using recursive submodules as their relative path handling\nseems to be broken in ee8838d (2015-09-08, submodule: rewrite\n`module_clone` shell function in C).\n\nSigned-off-by: Stefan Beller \n---\n t/t7400-submodule-basic.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 41 insertions(+)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 540771c..fc11809 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -818,6 +818,47 @@ test_expect_success 'submodule add --name allows to replace a submodule with ano\n \t)\n '\n \n+test_expect_failure 'recursive relative submodules stay relative' '\n+\ttest_when_finished \"rm -rf super clone2 subsub sub3\" &&\n+\tmkdir subsub &&\n+\t(\n+\t\tcd subsub &&\n+\t\tgit init &&\n+\t\t>t &&\n+\t\tgit add t &&\n+\t\tgit commit -m \"initial commit\"\n+\t) &&\n+\tmkdir sub3 &&\n+\t(\n+\t\tcd sub3 &&\n+\t\tgit init &&\n+\t\t>t &&\n+\t\tgit add t &&\n+\t\tgit commit -m \"initial commit\" &&\n+\t\tgit submodule add ../subsub dirdir/subsub &&\n+\t\tgit commit -m \"add submodule subsub\"\n+\t) &&\n+\tmkdir super &&\n+\t(\n+\t\tcd super &&\n+\t\tgit init &&\n+\t\t>t &&\n+\t\tgit add t &&\n+\t\tgit commit -m \"initial commit\" &&\n+\t\tgit submodule add ../sub3 &&\n+\t\tgit commit -m \"add submodule sub\"\n+\t) &&\n+\tgit clone super clone2 &&\n+\t(\n+\t\tcd clone2 &&\n+\t\tgit submodule update --init --recursive &&\n+\t\techo \"gitdir: ../.git/modules/sub3\" >./sub3/.git_expect &&\n+\t\techo \"gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub\" >./sub3/dirdir/subsub/.git_expect\n+\t) &&\n+\ttest_cmp clone2/sub3/.git_expect clone2/sub3/.git &&\n+\ttest_cmp clone2/sub3/dirdir/subsub/.git_expect clone2/sub3/dirdir/subsub/.git\n+'\n+\n test_expect_success 'submodule add with an existing name fails unless forced' '\n \t(\n \t\tcd addtest2 &&\n-- \n2.5.0.264.g4004fdc.dirty\n"},{"id":"282283","messageId":"1459383457-6848-3-git-send-email-sbeller@google.com","threadId":"41880","inReplyTo":"1459383457-6848-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/4] submodule--helper clone: simplify path check","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T00:17:35Z","receivedAt":"2016-03-31T00:17:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The calling shell code makes sure that `path` is non null and non empty.\n(side note: it cannot be null as just three lines before it is passed\nto safe_create_leading_directories_const which would crash as you feed\nit null).\n\nSigned-off-by: Stefan Beller \n---\n builtin/submodule--helper.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..88002ca 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -215,10 +215,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tif (safe_create_leading_directories_const(path) < 0)\n \t\tdie(_(\"could not create directory '%s'\"), path);\n \n-\tif (path && *path)\n-\t\tstrbuf_addf(&sb, \"%s/.git\", path);\n-\telse\n-\t\tstrbuf_addstr(&sb, \".git\");\n+\tstrbuf_addf(&sb, \"%s/.git\", path);\n \n \tif (safe_create_leading_directories_const(sb.buf) < 0)\n \t\tdie(_(\"could not create leading directories of '%s'\"), sb.buf);\n-- \n2.5.0.264.g4004fdc.dirty\n"},{"id":"282284","messageId":"1459383457-6848-4-git-send-email-sbeller@google.com","threadId":"41880","inReplyTo":"1459383457-6848-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/4] submodule--helper clone: remove double path checking","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T00:17:36Z","receivedAt":"2016-03-31T00:17:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Just a few lines after the deleted code we call\n\n safe_create_leading_directories_const(path + \"/.git\")\n\nso the check is done twice without action in between.\nRemove the first check.\n\nSigned-off-by: Stefan Beller \n---\n builtin/submodule--helper.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 88002ca..914e561 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -212,11 +212,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \t/* Write a .git file in the submodule to redirect to the superproject. */\n-\tif (safe_create_leading_directories_const(path) < 0)\n-\t\tdie(_(\"could not create directory '%s'\"), path);\n-\n \tstrbuf_addf(&sb, \"%s/.git\", path);\n-\n \tif (safe_create_leading_directories_const(sb.buf) < 0)\n \t\tdie(_(\"could not create leading directories of '%s'\"), sb.buf);\n \tsubmodule_dot_git = fopen(sb.buf, \"w\");\n-- \n2.5.0.264.g4004fdc.dirty\n"},{"id":"282285","messageId":"1459383457-6848-5-git-send-email-sbeller@google.com","threadId":"41880","inReplyTo":"1459383457-6848-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/4] submodule--helper: use relative path if possible","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T00:17:37Z","receivedAt":"2016-03-31T00:17:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As submodules have working directory and their git directory far apart\nrelative_path(gitdir, work_dir) will not produce a relative path when\ngit_dir is absolute. When the gitdir is absolute, we need to convert\nthe workdir to an absolute path as well to compute the relative path.\n\n(e.g. gitdir=/home/user/project/.git/modules/submodule,\nworkdir=submodule/, relative_dir doesn't take the current working directory\ninto account, so there is no way for it to know that the correct answer\nis indeed \"../.git/modules/submodule\", if the workdir was given as\n/home/user/project/submodule, the answer is obvious.)\n\nSigned-off-by: Stefan Beller \n---\n builtin/submodule--helper.c | 7 +++++++\n t/t7400-submodule-basic.sh | 2 +-\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 914e561..0b0fad3 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -159,6 +159,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tFILE *submodule_dot_git;\n \tchar *sm_gitdir, *cwd, *p;\n \tstruct strbuf rel_path = STRBUF_INIT;\n+\tstruct strbuf abs_path = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tstruct option module_clone_options[] = {\n@@ -219,7 +220,12 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tif (!submodule_dot_git)\n \t\tdie_errno(_(\"cannot open file '%s'\"), sb.buf);\n \n+\tstrbuf_addf(&abs_path, \"%s/%s\",\n+\t\t get_git_work_tree(),\n+\t\t path);\n \tfprintf(submodule_dot_git, \"gitdir: %s\\n\",\n+\t\tis_absolute_path(sm_gitdir) ?\n+\t\trelative_path(sm_gitdir, abs_path.buf, &rel_path) :\n \t\trelative_path(sm_gitdir, path, &rel_path));\n \tif (fclose(submodule_dot_git))\n \t\tdie(_(\"could not close file %s\"), sb.buf);\n@@ -242,6 +248,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\t\t relative_path(sb.buf, sm_gitdir, &rel_path));\n \tstrbuf_release(&sb);\n \tstrbuf_release(&rel_path);\n+\tstrbuf_release(&abs_path);\n \tfree(sm_gitdir);\n \tfree(cwd);\n \tfree(p);\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex fc11809..ea3fabb 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -818,7 +818,7 @@ test_expect_success 'submodule add --name allows to replace a submodule with ano\n \t)\n '\n \n-test_expect_failure 'recursive relative submodules stay relative' '\n+test_expect_success 'recursive relative submodules stay relative' '\n \ttest_when_finished \"rm -rf super clone2 subsub sub3\" &&\n \tmkdir subsub &&\n \t(\n-- \n2.5.0.264.g4004fdc.dirty\n"},{"id":"282286","messageId":"CA+P7+xq0w+dyhpT7VeURjj5wEa6++p_N2ASLYzjeQy+eOgLQLQ@mail.gmail.com","threadId":"41880","inReplyTo":"1459383457-6848-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/4] submodule--helper clone: simplify path check","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-03-31T00:32:16Z","receivedAt":"2016-03-31T00:32:16Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Mar 30, 2016 at 5:17 PM, Stefan Beller wrote:\n> The calling shell code makes sure that `path` is non null and non empty.\n> (side note: it cannot be null as just three lines before it is passed\n> to safe_create_leading_directories_const which would crash as you feed\n> it null).\n>\n\nSo we're going to assume that all callers of submodule--helper will\nprovide a non-null non-empty path? Hmm, since we expect only our shell\ncode to really do this... that's probably ok I think. I don't think it\ncan do any real harm should someone outside git call it with a bad\npath.\n\nThanks,\nJake\n"},{"id":"282298","messageId":"CAPig+cTz3fTyXFM4oPNO3Tv6dP0hN152jf9_JZOrsAZad1+O6Q@mail.gmail.com","threadId":"41880","inReplyTo":"1459383457-6848-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/4] submodule--helper clone: simplify path check","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-31T07:31:11Z","receivedAt":"2016-03-31T07:31:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 30, 2016 at 8:17 PM, Stefan Beller wrote:\n> The calling shell code makes sure that `path` is non null and non empty.\n> (side note: it cannot be null as just three lines before it is passed\n> to safe_create_leading_directories_const which would crash as you feed\n> it null).\n\nI'm confused by this. So, you're saying that it's okay (and desirable)\nfor git-submodule--helper to segfault when the user does this:\n\n % git submodule--helper clone\n Segmentation fault: 11\n\nrather than, say, printing a useful error message, such as:\n\n submodule--helper: missing or empty --path\n\n?\n\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> @@ -215,10 +215,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n> if (safe_create_leading_directories_const(path) < 0)\n> die(_(\"could not create directory '%s'\"), path);\n>\n> - if (path && *path)\n> - strbuf_addf(&sb, \"%s/.git\", path);\n> - else\n> - strbuf_addstr(&sb, \".git\");\n> + strbuf_addf(&sb, \"%s/.git\", path);\n>\n> if (safe_create_leading_directories_const(sb.buf) < 0)\n> die(_(\"could not create leading directories of '%s'\"), sb.buf);\n> --\n"},{"id":"282336","messageId":"xmqqpouappxp.fsf@gitster.mtv.corp.google.com","threadId":"41880","inReplyTo":"1459383457-6848-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/4] recursive submodules: test for relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T16:33:54Z","receivedAt":"2016-03-31T16:33:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This was reported as a regression at $gmane/290280. The root cause for\n> that bug is in using recursive submodules as their relative path handling\n> seems to be broken in ee8838d (2015-09-08, submodule: rewrite\n> `module_clone` shell function in C).\n\nI've reworded the above like so while queuing.\n\n \"git submodule update --init --recursive\" uses full path to refer to\n the true location of the repository in the \"gitdir:\" pointer for\n nested submodules; the command used to use relative paths.\n\n This was reported by Norio Nomura in $gmane/290280.\n\n The root cause for that bug is in using recursive submodules as\n their relative path handling was broken in ee8838d (2015-09-08,\n submodule: rewrite `module_clone` shell function in C).\n\nThanks.\n\n\n> Signed-off-by: Stefan Beller \n> ---\n> t/t7400-submodule-basic.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 41 insertions(+)\n>\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 540771c..fc11809 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -818,6 +818,47 @@ test_expect_success 'submodule add --name allows to replace a submodule with ano\n> \t)\n> '\n> \n> +test_expect_failure 'recursive relative submodules stay relative' '\n> +\ttest_when_finished \"rm -rf super clone2 subsub sub3\" &&\n> +\tmkdir subsub &&\n> +\t(\n> +\t\tcd subsub &&\n> +\t\tgit init &&\n> +\t\t>t &&\n> +\t\tgit add t &&\n> +\t\tgit commit -m \"initial commit\"\n> +\t) &&\n> +\tmkdir sub3 &&\n> +\t(\n> +\t\tcd sub3 &&\n> +\t\tgit init &&\n> +\t\t>t &&\n> +\t\tgit add t &&\n> +\t\tgit commit -m \"initial commit\" &&\n> +\t\tgit submodule add ../subsub dirdir/subsub &&\n> +\t\tgit commit -m \"add submodule subsub\"\n> +\t) &&\n> +\tmkdir super &&\n> +\t(\n> +\t\tcd super &&\n> +\t\tgit init &&\n> +\t\t>t &&\n> +\t\tgit add t &&\n> +\t\tgit commit -m \"initial commit\" &&\n> +\t\tgit submodule add ../sub3 &&\n> +\t\tgit commit -m \"add submodule sub\"\n> +\t) &&\n> +\tgit clone super clone2 &&\n> +\t(\n> +\t\tcd clone2 &&\n> +\t\tgit submodule update --init --recursive &&\n> +\t\techo \"gitdir: ../.git/modules/sub3\" >./sub3/.git_expect &&\n> +\t\techo \"gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub\" >./sub3/dirdir/subsub/.git_expect\n> +\t) &&\n> +\ttest_cmp clone2/sub3/.git_expect clone2/sub3/.git &&\n> +\ttest_cmp clone2/sub3/dirdir/subsub/.git_expect clone2/sub3/dirdir/subsub/.git\n> +'\n> +\n> test_expect_success 'submodule add with an existing name fails unless forced' '\n> \t(\n> \t\tcd addtest2 &&\n"},{"id":"282337","messageId":"xmqqlh4yppsq.fsf@gitster.mtv.corp.google.com","threadId":"41880","inReplyTo":"1459383457-6848-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/4] submodule--helper clone: simplify path check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T16:36:53Z","receivedAt":"2016-03-31T16:36:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The calling shell code makes sure that `path` is non null and non empty.\n> (side note: it cannot be null as just three lines before it is passed\n> to safe_create_leading_directories_const which would crash as you feed\n> it null).\n\nThis is not Java so let's spell that thing NULL.\n\nPerhaps it would be cheap-enough prudence to do this on top?\n\n builtin/submodule--helper.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 88002ca..f11796a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -194,6 +194,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, module_clone_options,\n \t\t\t git_submodule_helper_usage, 0);\n \n+\tassert(path);\n+\n \tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n \tsm_gitdir = strbuf_detach(&sb, NULL);\n \n"},{"id":"282339","messageId":"xmqqh9fmppg6.fsf@gitster.mtv.corp.google.com","threadId":"41880","inReplyTo":"1459383457-6848-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/4] submodule--helper clone: remove double path checking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T16:44:25Z","receivedAt":"2016-03-31T16:44:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Just a few lines after the deleted code we call\n>\n> safe_create_leading_directories_const(path + \"/.git\")\n>\n> so the check is done twice without action in between.\n> Remove the first check.\n\nI am hesitant to call the call to this function a \"check\". If you\ndo not yet have the leading directories, they get created.\n\n We make sure that the parent directory of path exists (or create it\n otherwise) and then do the same for path + \"/.git\".\n\n That is equivalent to just making sure that the parent directory of\n path + \"/.git\" exists (or create it otherwise).\n\nPerhaps?\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/submodule--helper.c | 4 ----\n> 1 file changed, 4 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 88002ca..914e561 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -212,11 +212,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \t/* Write a .git file in the submodule to redirect to the superproject. */\n> -\tif (safe_create_leading_directories_const(path) < 0)\n> -\t\tdie(_(\"could not create directory '%s'\"), path);\n> -\n> \tstrbuf_addf(&sb, \"%s/.git\", path);\n> -\n> \tif (safe_create_leading_directories_const(sb.buf) < 0)\n> \t\tdie(_(\"could not create leading directories of '%s'\"), sb.buf);\n> \tsubmodule_dot_git = fopen(sb.buf, \"w\");\n"},{"id":"282340","messageId":"CAGZ79kYiCetyhDJwBnJ9MSufZMkW_WZepESK+U_yAOJEgfzMXg@mail.gmail.com","threadId":"41880","inReplyTo":"xmqqpouappxp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/4] recursive submodules: test for relative paths","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T16:47:22Z","receivedAt":"2016-03-31T16:47:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 31, 2016 at 9:33 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> This was reported as a regression at $gmane/290280. The root cause for\n>> that bug is in using recursive submodules as their relative path handling\n>> seems to be broken in ee8838d (2015-09-08, submodule: rewrite\n>> `module_clone` shell function in C).\n>\n> I've reworded the above like so while queuing.\n>\n> \"git submodule update --init --recursive\" uses full path to refer to\n> the true location of the repository in the \"gitdir:\" pointer for\n> nested submodules; the command used to use relative paths.\n>\n> This was reported by Norio Nomura in $gmane/290280.\n>\n> The root cause for that bug is in using recursive submodules as\n> their relative path handling was broken in ee8838d (2015-09-08,\n> submodule: rewrite `module_clone` shell function in C).\n>\n> Thanks.\n>\n\nThanks!\n\nI'll pickup the reworded version and resend the series as there seems\nto be discussion\non the other patches which requires some work by me.\n"},{"id":"282342","messageId":"xmqqd1qapore.fsf@gitster.mtv.corp.google.com","threadId":"41880","inReplyTo":"1459383457-6848-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 4/4] submodule--helper: use relative path if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T16:59:17Z","receivedAt":"2016-03-31T16:59:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> As submodules have working directory and their git directory far apart\n> relative_path(gitdir, work_dir) will not produce a relative path when\n> git_dir is absolute. When the gitdir is absolute, we need to convert\n> the workdir to an absolute path as well to compute the relative path.\n>\n> (e.g. gitdir=/home/user/project/.git/modules/submodule,\n> workdir=submodule/, relative_dir doesn't take the current working directory\n> into account, so there is no way for it to know that the correct answer\n> is indeed \"../.git/modules/submodule\", if the workdir was given as\n> /home/user/project/submodule, the answer is obvious.)\n>\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/submodule--helper.c | 7 +++++++\n> t/t7400-submodule-basic.sh | 2 +-\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 914e561..0b0fad3 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -159,6 +159,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n> \tFILE *submodule_dot_git;\n> \tchar *sm_gitdir, *cwd, *p;\n> \tstruct strbuf rel_path = STRBUF_INIT;\n> +\tstruct strbuf abs_path = STRBUF_INIT;\n> \tstruct strbuf sb = STRBUF_INIT;\n> \n> \tstruct option module_clone_options[] = {\n> @@ -219,7 +220,12 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n> \tif (!submodule_dot_git)\n> \t\tdie_errno(_(\"cannot open file '%s'\"), sb.buf);\n> \n> +\tstrbuf_addf(&abs_path, \"%s/%s\",\n> +\t\t get_git_work_tree(),\n> +\t\t path);\n\nThe \"path\" is assumed to be _always_ relative to work tree?\n\nI am wondering if it would be prudent to have an assert for that\nbefore doing this, just like I suggested assert(path) for [2/4]\nearlier [*1*].\n\nOn the other hand, if we allow path to be absolute, this would need\nto become something like:\n\n\tif (is_absolute_path(path))\n \tstrbuf_addstr(&abs_path, path);\n\telse\n \tstrbuf_addf(&abs_path, \"%s/%s\", get_git_work_tree(), path);\n\n> \tfprintf(submodule_dot_git, \"gitdir: %s\\n\",\n> +\t\tis_absolute_path(sm_gitdir) ?\n> +\t\trelative_path(sm_gitdir, abs_path.buf, &rel_path) :\n> \t\trelative_path(sm_gitdir, path, &rel_path));\n\nIt seems that the abs_path computation is not needed at all if\nsm_gitdir is relative to begin with. I wonder if the code gets\neasier to read and can avoid unnecessary strbuf manipulation\nif this entire hunk is structured more like so:\n\n\tif (is_absolute_path(sm_gitdir)) {\n\t\t...\n\t} else {\n \t...\n\t}\n\tfprintf(submodule_dot_git, \"gitdir: %s\\n\",\n \trelative_path(sm_gitdir, base_path, &rel_path));\n\n> \tif (fclose(submodule_dot_git))\n> \t\tdie(_(\"could not close file %s\"), sb.buf);\n\n[Footnote]\n\n*1* BTW, I tightened the assert for 2/4 to \"assert(path && *path)\"\nto match the assumption in its log message, i.e. \"The calling shell\ncode makes sure that path is non-NULL and non empty\".\n"},{"id":"282343","messageId":"xmqq7fgipojj.fsf@gitster.mtv.corp.google.com","threadId":"41880","inReplyTo":"1459383457-6848-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 0/4] Fix relative path issues in recursive submodules.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T17:04:00Z","receivedAt":"2016-03-31T17:04:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> It took me longer than expected to fix this bug.\n>\n> It comes with a test and minor refactoring and applies on top of\n> origin/sb/submodule-helper, such that it can be merged into 2.7, 2.8 as well\n> as master.\n>\n> Patch 1 is a test which fails; it has a similar layout as the\n> real failing repository Norio Nomura pointed out, but simplified to the\n> bare essentials for reproduction of the bug.\n>\n> Patch 2&3 are not strictly necessary for fixing the isseu, but it removes\n> stupid code I wrote, so the resulting code looks a little better.\n>\n> Patch 4 fixes the issue by giving more information to relative_path,\n> such that a relative path can be found in all cases.\n\nThere were some minor nits, but I saw nothing glaringly wrong to\nbreak the topic. Thanks for working on this.\n"},{"id":"282344","messageId":"CAPig+cRq3CoE1mH9=TeKnujQYVb0+tURpYNSsRKtnK_FTDyBAQ@mail.gmail.com","threadId":"41880","inReplyTo":"xmqqlh4yppsq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/4] submodule--helper clone: simplify path check","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-31T17:04:32Z","receivedAt":"2016-03-31T17:04:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 31, 2016 at 12:36 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> The calling shell code makes sure that `path` is non null and non empty.\n>> (side note: it cannot be null as just three lines before it is passed\n>> to safe_create_leading_directories_const which would crash as you feed\n>> it null).\n>\n> This is not Java so let's spell that thing NULL.\n>\n> Perhaps it would be cheap-enough prudence to do this on top?\n>\n> argc = parse_options(argc, argv, prefix, module_clone_options,\n> git_submodule_helper_usage, 0);\n>\n> + assert(path);\n\nHmm, from the user perspective, this is still a \"crash\", just as the\nexisting segfault is a crash. While one can argue that this is an\ninternal command only invoked in a controlled fashion, it's not hard\nto imagine someone running it manually to understand its behavior or\nto debug some problem. This function already presents proper error\nmessages for other problems it encounters, so it seems reasonable for\nit do so for this problem, as well.\n\n if (!path || !*path)\n die(_(\"submodule--helper: unspecified or empty --path\"));\n"},{"id":"282346","messageId":"CAGZ79kYbX5dggD7cs1PRHpkUS2gRTp1fAmt3mg1n1Og3ZB3fRw@mail.gmail.com","threadId":"41880","inReplyTo":"CAPig+cTz3fTyXFM4oPNO3Tv6dP0hN152jf9_JZOrsAZad1+O6Q@mail.gmail.com","subject":"Re: [PATCH 2/4] submodule--helper clone: simplify path check","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T17:17:46Z","receivedAt":"2016-03-31T17:17:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 31, 2016 at 12:31 AM, Eric Sunshine wrote:\n> On Wed, Mar 30, 2016 at 8:17 PM, Stefan Beller wrote:\n>> The calling shell code makes sure that `path` is non null and non empty.\n>> (side note: it cannot be null as just three lines before it is passed\n>> to safe_create_leading_directories_const which would crash as you feed\n>> it null).\n>\n> I'm confused by this. So, you're saying that it's okay (and desirable)\n> for git-submodule--helper to segfault when the user does this:\n>\n> % git submodule--helper clone\n> Segmentation fault: 11\n>\n> rather than, say, printing a useful error message, such as:\n>\n> submodule--helper: missing or empty --path\n\nI think I was rather saying,\n* that you see the segfault behavior not at all when channeling\n the command through the proper frontend command\n* that it already crashes (we access *path before this check,\n so this check is pointless)\n\n\n> While one can argue that this is an\n> internal command only invoked in a controlled fashion, it's not hard\n> to imagine someone running it manually to understand its behavior or\n> to debug some problem.\n\n(additionally:)\nAnd later we may want to reuse this code when replacing the\nfrontend command to be written in C completely.\n\n> if (!path || !*path)\n> die(_(\"submodule--helper: unspecified or empty --path\"));\n\nThat sounds good to me. I'll pick it up.\n"},{"id":"282347","messageId":"CAGZ79kYyVJcOsWvkK=Qm0rE0hd-xTq=TPi8groynJ4Vokft74g@mail.gmail.com","threadId":"41880","inReplyTo":"xmqq7fgipojj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/4] Fix relative path issues in recursive submodules.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T17:20:21Z","receivedAt":"2016-03-31T17:20:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 31, 2016 at 10:04 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> It took me longer than expected to fix this bug.\n>>\n>> It comes with a test and minor refactoring and applies on top of\n>> origin/sb/submodule-helper, such that it can be merged into 2.7, 2.8 as well\n>> as master.\n>>\n>> Patch 1 is a test which fails; it has a similar layout as the\n>> real failing repository Norio Nomura pointed out, but simplified to the\n>> bare essentials for reproduction of the bug.\n>>\n>> Patch 2&3 are not strictly necessary for fixing the isseu, but it removes\n>> stupid code I wrote, so the resulting code looks a little better.\n>>\n>> Patch 4 fixes the issue by giving more information to relative_path,\n>> such that a relative path can be found in all cases.\n>\n> There were some minor nits, but I saw nothing glaringly wrong to\n> break the topic. Thanks for working on this.\n\nThanks for review and discussion, I plan on resending this series.\n\nCurrently I have the opinion to drop 2&3 (the path assumption and\ndouble safe_create_leading_dir) and send patch 1 and 4 combined as\na bugfix only as that is more the spirit what we want to see for\nan eventual merge to maint?\n\nThe refactoring patches 2&3 can be sent later as separate patches\nfor master, I would think.\n"},{"id":"282374","messageId":"CAGZ79kb9sRDuCvJw=S8_BLy7hej_YqE9Y-NbnQhQmafsZuEpwg@mail.gmail.com","threadId":"41880","inReplyTo":"xmqqd1qapore.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4] submodule--helper: use relative path if possible","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T19:22:05Z","receivedAt":"2016-03-31T19:22:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 31, 2016 at 9:59 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> As submodules have working directory and their git directory far apart\n>> relative_path(gitdir, work_dir) will not produce a relative path when\n>> git_dir is absolute. When the gitdir is absolute, we need to convert\n>> the workdir to an absolute path as well to compute the relative path.\n>>\n>> (e.g. gitdir=/home/user/project/.git/modules/submodule,\n>> workdir=submodule/, relative_dir doesn't take the current working directory\n>> into account, so there is no way for it to know that the correct answer\n>> is indeed \"../.git/modules/submodule\", if the workdir was given as\n>> /home/user/project/submodule, the answer is obvious.)\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> builtin/submodule--helper.c | 7 +++++++\n>> t/t7400-submodule-basic.sh | 2 +-\n>> 2 files changed, 8 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index 914e561..0b0fad3 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -159,6 +159,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n>> FILE *submodule_dot_git;\n>> char *sm_gitdir, *cwd, *p;\n>> struct strbuf rel_path = STRBUF_INIT;\n>> + struct strbuf abs_path = STRBUF_INIT;\n>> struct strbuf sb = STRBUF_INIT;\n>>\n>> struct option module_clone_options[] = {\n>> @@ -219,7 +220,12 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n>> if (!submodule_dot_git)\n>> die_errno(_(\"cannot open file '%s'\"), sb.buf);\n>>\n>> + strbuf_addf(&abs_path, \"%s/%s\",\n>> + get_git_work_tree(),\n>> + path);\n>\n> The \"path\" is assumed to be _always_ relative to work tree?\n\nIn the code prior to this patch, that was assumed, yes.\n(e.g. later in the code:\n\n /* Redirect the worktree of the submodule in the superproject's config */\n cwd = xgetcwd();\n strbuf_addf(&sb, \"%s/%s\", cwd, path);\n ....\n relative_path(sb.buf, ...\n)\n\n>\n> I am wondering if it would be prudent to have an assert for that\n> before doing this, just like I suggested assert(path) for [2/4]\n> earlier [*1*].\n>\n> On the other hand, if we allow path to be absolute, this would need\n> to become something like:\n>\n> if (is_absolute_path(path))\n> strbuf_addstr(&abs_path, path);\n> else\n> strbuf_addf(&abs_path, \"%s/%s\", get_git_work_tree(), path);\n>\n>> fprintf(submodule_dot_git, \"gitdir: %s\\n\",\n>> + is_absolute_path(sm_gitdir) ?\n>> + relative_path(sm_gitdir, abs_path.buf, &rel_path) :\n>> relative_path(sm_gitdir, path, &rel_path));\n>\n> It seems that the abs_path computation is not needed at all if\n> sm_gitdir is relative to begin with. I wonder if the code gets\n> easier to read and can avoid unnecessary strbuf manipulation\n> if this entire hunk is structured more like so:\n>\n> if (is_absolute_path(sm_gitdir)) {\n> ...\n> } else {\n> ...\n> }\n> fprintf(submodule_dot_git, \"gitdir: %s\\n\",\n> relative_path(sm_gitdir, base_path, &rel_path));\n>\n>> if (fclose(submodule_dot_git))\n>> die(_(\"could not close file %s\"), sb.buf);\n>\n> [Footnote]\n\nI just simplified the code to be\n\n if (!is_absolute(path))\n path=make_absolute(...);\n if (!is_absolute(sm_gitdir))\n sm_gitdir=make_absolute(...);\n ...\n /* rest operates under absolute path only, no\n conditions any more! */\n\n\nAnd I'd think that is cleanest to read and understand.\n\nHaving absolute path for both path and gitdir all the time\nleaves no exceptions for relative_path to spew errors because\none of both is relative and not connected to the other absolute.\n\n>\n> *1* BTW, I tightened the assert for 2/4 to \"assert(path && *path)\"\n> to match the assumption in its log message, i.e. \"The calling shell\n> code makes sure that path is non-NULL and non empty\".\n>\n"}]}