{"thread":{"id":"41891","subject":"[PATCHv2 2/5] submodule--helper clone: simplify path check","startedAt":"2016-03-31T21:04:35Z","lastAt":"2016-03-31T23:08:02Z","messageCount":12,"participants":["Stefan Beller","Eric Sunshine","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"282392","messageId":"1459458280-17619-1-git-send-email-sbeller@google.com","threadId":"41891","inReplyTo":null,"subject":"[PATCHv2 0/5] Fix relative path issues in recursive submodules.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T21:04:35Z","receivedAt":"2016-03-31T21:04:35Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Thanks Junio, Eric and Jacob for review!\n\nv2:\n * reworded commit message for test (Thanks Junio!)\n * instead of \"simplifying\" the path handling in patch 2, we are prepared\n   for anything the user throws at us (i.e. instead of segfault we\n       die(_(\"submodule--helper: unspecified or empty --path\"));\n   (Thanks Eric, Jacob for pushing back here!)\n * reword commit message for patch 3 (safe_create_leading_directories_const is\n   not a check, Thanks Junio!)\n * patch 4 (the fix): That got reworked completely. No flow controlling\n   conditions in the write out phase!\n * patch 5 is a bonus! (replace fprintf by fprintf_or die) When implementing\n   that I wondered if we also have close_or_die and open_or_die, but that doesn't\n   seem to be the case.\n   \nThanks,\nStefan\n\nv1:\n\nIt 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 (5):\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, module_clone: always operate on absolute paths\n  submodule--helper, module_clone: catch fprintf failure\n\n builtin/submodule--helper.c | 52 +++++++++++++++++++++++++--------------------\n t/t7400-submodule-basic.sh  | 41 +++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+), 23 deletions(-)\n\n-- \n2.5.0.264.g39f00fe\n"},{"id":"282395","messageId":"1459458280-17619-2-git-send-email-sbeller@google.com","threadId":"41891","inReplyTo":"1459458280-17619-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 1/5] recursive submodules: test for relative paths","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T21:04:36Z","receivedAt":"2016-03-31T21:04:36Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"\"git submodule update --init --recursive\" uses full path to refer to\nthe true location of the repository in the \"gitdir:\" pointer for\nnested submodules; the command used to use relative paths.\n\nThis was reported by Norio Nomura in $gmane/290280.\n\nThe root cause for that bug is in using recursive submodules as\ntheir relative path handling was broken in ee8838d (2015-09-08,\nsubmodule: rewrite `module_clone` shell function in C).\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\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.g39f00fe\n"},{"id":"282390","messageId":"1459458280-17619-3-git-send-email-sbeller@google.com","threadId":"41891","inReplyTo":"1459458280-17619-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 2/5] submodule--helper clone: simplify path check","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T21:04:37Z","receivedAt":"2016-03-31T21:04:37Z","isPatch":false,"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.\nSide 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. That means the `else` part is dead code, so remove it.\n\nTo make the code futureproof, add a check for path being NULL or empty\nand report the error accordingly.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..4f0b002 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -194,6 +194,9 @@ 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+\tif (!path || !*path)\n+\t\tdie(_(\"submodule--helper: unspecified or empty --path\"));\n+\n \tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n \tsm_gitdir = strbuf_detach(&sb, NULL);\n \n@@ -215,10 +218,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.g39f00fe\n"},{"id":"282391","messageId":"1459458280-17619-4-git-send-email-sbeller@google.com","threadId":"41891","inReplyTo":"1459458280-17619-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 3/5] submodule--helper clone: remove double path checking","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T21:04:38Z","receivedAt":"2016-03-31T21:04:38Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We make sure that the parent directory of path exists (or create it\notherwise) and then do the same for path + \"/.git\".\n\nThat is equivalent to just making sure that the parent directory of\npath + \"/.git\" exists (or create it otherwise).\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\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 4f0b002..0b9f546 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -215,11 +215,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.g39f00fe\n"},{"id":"282394","messageId":"1459458280-17619-5-git-send-email-sbeller@google.com","threadId":"41891","inReplyTo":"1459458280-17619-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 4/5] submodule--helper, module_clone: always operate on absolute paths","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T21:04:39Z","receivedAt":"2016-03-31T21:04:39Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When giving relative paths to `relative_path` to compute a relative path\nfrom one directory to another, this may fail in `relative_path`.\nMake sure both arguments to `relative_path` are always absolute.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    Notice how the 2 relative path calls\n      relative_path(sm_gitdir, path, &rel_path)\n      relative_path(path, sm_gitdir, &rel_path)\n    now have the same arguments, just switched?\n    The symmetry there looks nice to read.\n    \n    This proposal is slightly more code than the previous patch,\n    but looking at the end result\n\n builtin/submodule--helper.c | 36 +++++++++++++++++++++++-------------\n t/t7400-submodule-basic.sh  |  2 +-\n 2 files changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 0b9f546..56c3033 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -157,8 +157,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tconst char *reference = NULL, *depth = NULL;\n \tint quiet = 0;\n \tFILE *submodule_dot_git;\n-\tchar *sm_gitdir, *cwd, *p;\n-\tstruct strbuf rel_path = STRBUF_INIT;\n+\tchar *sm_gitdir, *p;\n+\tstruct strbuf rel_path = STRBUF_INIT; /* for relative_path */\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tstruct option module_clone_options[] = {\n@@ -200,6 +200,25 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n \tsm_gitdir = strbuf_detach(&sb, NULL);\n \n+\n+\tif (!is_absolute_path(sm_gitdir)) {\n+\t\tchar *cwd = xgetcwd();\n+\t\tstrbuf_addf(&sb, \"%s/%s\", cwd, sm_gitdir);\n+\t\tsm_gitdir = strbuf_detach(&sb, 0);\n+\t\tfree(cwd);\n+\t}\n+\n+\tif (!is_absolute_path(path)) {\n+\t\t/*\n+\t\t * TODO: add prefix here once we allow calling from non root\n+\t\t * directory?\n+\t\t */\n+\t\tstrbuf_addf(&sb, \"%s/%s\",\n+\t\t\t    get_git_work_tree(),\n+\t\t\t    path);\n+\t\tpath = strbuf_detach(&sb, 0);\n+\t}\n+\n \tif (!file_exists(sm_gitdir)) {\n \t\tif (safe_create_leading_directories_const(sm_gitdir) < 0)\n \t\t\tdie(_(\"could not create directory '%s'\"), sm_gitdir);\n@@ -221,7 +240,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tsubmodule_dot_git = fopen(sb.buf, \"w\");\n \tif (!submodule_dot_git)\n \t\tdie_errno(_(\"cannot open file '%s'\"), sb.buf);\n-\n \tfprintf(submodule_dot_git, \"gitdir: %s\\n\",\n \t\trelative_path(sm_gitdir, path, &rel_path));\n \tif (fclose(submodule_dot_git))\n@@ -229,24 +247,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_reset(&sb);\n \tstrbuf_reset(&rel_path);\n \n-\tcwd = xgetcwd();\n \t/* Redirect the worktree of the submodule in the superproject's config */\n-\tif (!is_absolute_path(sm_gitdir)) {\n-\t\tstrbuf_addf(&sb, \"%s/%s\", cwd, sm_gitdir);\n-\t\tfree(sm_gitdir);\n-\t\tsm_gitdir = strbuf_detach(&sb, NULL);\n-\t}\n-\n-\tstrbuf_addf(&sb, \"%s/%s\", cwd, path);\n \tp = git_pathdup_submodule(path, \"config\");\n \tif (!p)\n \t\tdie(_(\"could not get submodule directory for '%s'\"), path);\n \tgit_config_set_in_file(p, \"core.worktree\",\n-\t\t\t       relative_path(sb.buf, sm_gitdir, &rel_path));\n+\t\t\t       relative_path(path, sm_gitdir, &rel_path));\n \tstrbuf_release(&sb);\n \tstrbuf_release(&rel_path);\n \tfree(sm_gitdir);\n-\tfree(cwd);\n+\n \tfree(p);\n \treturn 0;\n }\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.g39f00fe\n"},{"id":"282393","messageId":"1459458280-17619-6-git-send-email-sbeller@google.com","threadId":"41891","inReplyTo":"1459458280-17619-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 5/5] submodule--helper, module_clone: catch fprintf failure","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T21:04:40Z","receivedAt":"2016-03-31T21:04:40Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The return value of fprintf is unchecked, which may lead to\nunreported errors. Use fprintf_or_die to report the error to the user.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 56c3033..89cbbda 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -240,8 +240,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tsubmodule_dot_git = fopen(sb.buf, \"w\");\n \tif (!submodule_dot_git)\n \t\tdie_errno(_(\"cannot open file '%s'\"), sb.buf);\n-\tfprintf(submodule_dot_git, \"gitdir: %s\\n\",\n-\t\trelative_path(sm_gitdir, path, &rel_path));\n+\tfprintf_or_die(submodule_dot_git, \"gitdir: %s\\n\",\n+\t\t       relative_path(sm_gitdir, path, &rel_path));\n \tif (fclose(submodule_dot_git))\n \t\tdie(_(\"could not close file %s\"), sb.buf);\n \tstrbuf_reset(&sb);\n-- \n2.5.0.264.g39f00fe\n"},{"id":"282397","messageId":"CAPig+cSk2kDEs7nCt39LoeBjjmo-+7WH4rNNh0x6ULfeeAwQ_w@mail.gmail.com","threadId":"41891","inReplyTo":"1459458280-17619-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 2/5] submodule--helper clone: simplify path check","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-31T21:23:10Z","receivedAt":"2016-03-31T21:23:10Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 31, 2016 at 5:04 PM, Stefan Beller <sbeller@google.com> wrote:\n> submodule--helper clone: simplify path check\n\nI don't see anything in the patch which simplifies a path check.\nInstead, this version of the patch is now fixing a potential\nNULL-dereference.\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. That means the `else` part is dead code, so remove it.\n\nThe above description has very little if anything to do with what this\npatch is addressing considering that this is now a crash-fix patch.\nWhile it's true that the (current) shell code won't trigger this\ncrash, that's not particularly interesting or relevant.\n\n> To make the code futureproof, add a check for path being NULL or empty\n> and report the error accordingly.\n\nSelling this as a future-proofing change is misleading; it's a crash\nfix, plain and simple. I think the entire commit message could be\ncollapsed to something like this:\n\n    submodule--helper: fix potential NULL-dereference\n\n    Don't dereference NULL 'path' if it was never assigned. While\n    here also protect against empty --path argument.\n\nYou don't even need to mention removal of the conditional in the\nsecond hunk since, for anyone reading the patch, that's an obvious\nconsequence of adding the new check in the first hunk.\n\nThe patch itself looks fine.\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> @@ -194,6 +194,9 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n>         argc = parse_options(argc, argv, prefix, module_clone_options,\n>                              git_submodule_helper_usage, 0);\n>\n> +       if (!path || !*path)\n> +               die(_(\"submodule--helper: unspecified or empty --path\"));\n> +\n>         strbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n>         sm_gitdir = strbuf_detach(&sb, NULL);\n>\n> @@ -215,10 +218,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> 2.5.0.264.g39f00fe\n"},{"id":"282398","messageId":"CAPig+cToyvzc4RbBmAAQtGmJ3WpxO6Z+XzrxrXQqYVH2RJcb-g@mail.gmail.com","threadId":"41891","inReplyTo":"1459458280-17619-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 3/5] submodule--helper clone: remove double path checking","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-31T21:58:32Z","receivedAt":"2016-03-31T21:58:32Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 31, 2016 at 5:04 PM, Stefan Beller <sbeller@google.com> wrote:\n> submodule--helper clone: remove double path checking\n\nI think Junio mentioned in v1 that calling this \"path checking\" is misleading.\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\nThis part of the commit message is nicely improved.\n\nThe patch itself looks fine.\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> @@ -215,11 +215,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n>         }\n>\n>         /* Write a .git file in the submodule to redirect to the superproject. */\n> -       if (safe_create_leading_directories_const(path) < 0)\n> -               die(_(\"could not create directory '%s'\"), path);\n> -\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>         submodule_dot_git = fopen(sb.buf, \"w\");\n> --\n> 2.5.0.264.g39f00fe\n"},{"id":"282400","messageId":"xmqqk2kil255.fsf@gitster.mtv.corp.google.com","threadId":"41891","inReplyTo":"CAPig+cToyvzc4RbBmAAQtGmJ3WpxO6Z+XzrxrXQqYVH2RJcb-g@mail.gmail.com","subject":"Re: [PATCHv2 3/5] submodule--helper clone: remove double path checking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T22:21:26Z","receivedAt":"2016-03-31T22:21:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Thu, Mar 31, 2016 at 5:04 PM, Stefan Beller <sbeller@google.com> wrote:\n>> submodule--helper clone: remove double path checking\n>\n> I think Junio mentioned in v1 that calling this \"path checking\" is misleading.\n\nI'd tentatively use:\n\n    \"submodule--helper clone: create the submodule path just once\"\n"},{"id":"282402","messageId":"xmqqfuv6l1xf.fsf@gitster.mtv.corp.google.com","threadId":"41891","inReplyTo":"1459458280-17619-5-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 4/5] submodule--helper, module_clone: always operate on absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T22:26:04Z","receivedAt":"2016-03-31T22:26: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> +\tif (!is_absolute_path(sm_gitdir)) {\n> +\t\tchar *cwd = xgetcwd();\n> +\t\tstrbuf_addf(&sb, \"%s/%s\", cwd, sm_gitdir);\n> +\t\tsm_gitdir = strbuf_detach(&sb, 0);\n> +\t\tfree(cwd);\n\nIt would be surprising that this would be the first codepath that\nneeds to get an absolute pathname in the codebase that is more than\n10 years old, wouldn't it?  Don't we have a reasonable API helper\nfunction to do this kind of thing already?\n\n    ... goes and looks ...\n\nDoesn't absolute_path() work here?\n\n> @@ -221,7 +240,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n>  \tsubmodule_dot_git = fopen(sb.buf, \"w\");\n>  \tif (!submodule_dot_git)\n>  \t\tdie_errno(_(\"cannot open file '%s'\"), sb.buf);\n> -\n>  \tfprintf(submodule_dot_git, \"gitdir: %s\\n\",\n>  \t\trelative_path(sm_gitdir, path, &rel_path));\n>  \tif (fclose(submodule_dot_git))\n\nLooks like an unrelated change to me.\n\n> @@ -229,24 +247,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n>  \tstrbuf_reset(&sb);\n>  \tstrbuf_reset(&rel_path);\n>  \n> -\tcwd = xgetcwd();\n>  \t/* Redirect the worktree of the submodule in the superproject's config */\n> -\tif (!is_absolute_path(sm_gitdir)) {\n> -\t\tstrbuf_addf(&sb, \"%s/%s\", cwd, sm_gitdir);\n> -\t\tfree(sm_gitdir);\n> -\t\tsm_gitdir = strbuf_detach(&sb, NULL);\n> -\t}\n> -\n> -\tstrbuf_addf(&sb, \"%s/%s\", cwd, path);\n>  \tp = git_pathdup_submodule(path, \"config\");\n>  \tif (!p)\n>  \t\tdie(_(\"could not get submodule directory for '%s'\"), path);\n>  \tgit_config_set_in_file(p, \"core.worktree\",\n> -\t\t\t       relative_path(sb.buf, sm_gitdir, &rel_path));\n> +\t\t\t       relative_path(path, sm_gitdir, &rel_path));\n>  \tstrbuf_release(&sb);\n>  \tstrbuf_release(&rel_path);\n>  \tfree(sm_gitdir);\n> -\tfree(cwd);\n> +\n\nThis addition of blank, too.\n\n>  \tfree(p);\n>  \treturn 0;\n>  }\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 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"},{"id":"282409","messageId":"CAGZ79kaAs_ZwxfhLSTw3z8JY33vGToms2z6zYEiCmRpfS6SA+Q@mail.gmail.com","threadId":"41891","inReplyTo":"xmqqfuv6l1xf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2 4/5] submodule--helper, module_clone: always operate on absolute paths","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T22:59:29Z","receivedAt":"2016-03-31T22:59:29Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 31, 2016 at 3:26 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> +     if (!is_absolute_path(sm_gitdir)) {\n>> +             char *cwd = xgetcwd();\n>> +             strbuf_addf(&sb, \"%s/%s\", cwd, sm_gitdir);\n>> +             sm_gitdir = strbuf_detach(&sb, 0);\n>> +             free(cwd);\n>\n> It would be surprising that this would be the first codepath that\n> needs to get an absolute pathname in the codebase that is more than\n> 10 years old, wouldn't it?  Don't we have a reasonable API helper\n> function to do this kind of thing already?\n>\n>     ... goes and looks ...\n>\n> Doesn't absolute_path() work here?\n\n*reads absolute_path(...)*\n\n    Yes that is great. But why is it written the way it is?\n    I would have expected:\n\n const char *absolute_path(const char *path)\n {\n        static struct strbuf sb = STRBUF_INIT;\n-       strbuf_reset(&sb);\n        strbuf_add_absolute_path(&sb, path);\n-       return sb.buf;\n+       return strbuf_detach(&sb);\n }\n\nFurther checking reveals any caller uses it with\n\n    desired= xstrdup(absolute_path(my_argument));\n\nWe are loosing memory of the strbuf here. so if I we'd\ntake the diff above we can also get rid of all the xstrdups\nat the callers. For now I will adhere to all other callers and use\nxstrdup(absolute_path(...) here too.\n\nI'll remove the unrelated changes as well in a resend.\n\n>\n>> @@ -221,7 +240,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n>>       submodule_dot_git = fopen(sb.buf, \"w\");\n>>       if (!submodule_dot_git)\n>>               die_errno(_(\"cannot open file '%s'\"), sb.buf);\n>> -\n>>       fprintf(submodule_dot_git, \"gitdir: %s\\n\",\n>>               relative_path(sm_gitdir, path, &rel_path));\n>>       if (fclose(submodule_dot_git))\n>\n> Looks like an unrelated change to me.\n>\n>> @@ -229,24 +247,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n>>       strbuf_reset(&sb);\n>>       strbuf_reset(&rel_path);\n>>\n>> -     cwd = xgetcwd();\n>>       /* Redirect the worktree of the submodule in the superproject's config */\n>> -     if (!is_absolute_path(sm_gitdir)) {\n>> -             strbuf_addf(&sb, \"%s/%s\", cwd, sm_gitdir);\n>> -             free(sm_gitdir);\n>> -             sm_gitdir = strbuf_detach(&sb, NULL);\n>> -     }\n>> -\n>> -     strbuf_addf(&sb, \"%s/%s\", cwd, path);\n>>       p = git_pathdup_submodule(path, \"config\");\n>>       if (!p)\n>>               die(_(\"could not get submodule directory for '%s'\"), path);\n>>       git_config_set_in_file(p, \"core.worktree\",\n>> -                            relative_path(sb.buf, sm_gitdir, &rel_path));\n>> +                            relative_path(path, sm_gitdir, &rel_path));\n>>       strbuf_release(&sb);\n>>       strbuf_release(&rel_path);\n>>       free(sm_gitdir);\n>> -     free(cwd);\n>> +\n>\n> This addition of blank, too.\n>\n>>       free(p);\n>>       return 0;\n>>  }\n>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n>> index 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>>       )\n>>  '\n>>\n>> -test_expect_failure 'recursive relative submodules stay relative' '\n>> +test_expect_success 'recursive relative submodules stay relative' '\n>>       test_when_finished \"rm -rf super clone2 subsub sub3\" &&\n>>       mkdir subsub &&\n>>       (\n"},{"id":"282410","messageId":"CAGZ79kb4xOraKCODeLNqHCCdpdG=2aYofVsu0MRQteBozCsswQ@mail.gmail.com","threadId":"41891","inReplyTo":"CAGZ79kaAs_ZwxfhLSTw3z8JY33vGToms2z6zYEiCmRpfS6SA+Q@mail.gmail.com","subject":"Re: [PATCHv2 4/5] submodule--helper, module_clone: always operate on absolute paths","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T23:08:02Z","receivedAt":"2016-03-31T23:08:02Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 31, 2016 at 3:59 PM, Stefan Beller <sbeller@google.com> wrote:\n>\n> Further checking reveals any caller uses it with\n>\n>     desired= xstrdup(absolute_path(my_argument));\n>\n> We are loosing memory of the strbuf here. so if I we'd\n> take the diff above we can also get rid of all the xstrdups\n> at the callers. For now I will adhere to all other callers and use\n> xstrdup(absolute_path(...) here too.\n\nActually there is only two occurrences of xstrdup in builtin/clone.c\n"}]}