# [PATCHv3 0/2] Fix relative path issues in recursive submodules. 15 messages from 2016-04-01 to 2016-04-13. Participants: Stefan Beller, Ramsay Jones, Junio C Hamano, Eric Sunshine. Thread: https://gitlist.dev/t/41892 ## Stefan Beller, 2016-04-01 00:17 Subject: [PATCHv3 0/2] Fix relative path issues in recursive submodules. Message-ID: <1459469849-9643-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459469849-9643-1-git-send-email-sbeller%40google.com ``` Thanks Junio for review! v3: * This is a resend of the last two patches of the series, i.e. it replaces 44859d6626d4 and efdef1e2e in sb/submodule-helper-clone-regression-fix * use absolute_path for sm_gitdir * removed todo * we need to free the intermediate sm_gitdir, so rename that to sm_gitdir_rel and free it afterwards. * while we currently do not support an absolute `path`, we eventually might. If `path` is absolute it would be a pointer to `argv`, which would lead to a crash. Duplicate the path and the crash is prevented. (* I thought we could use it as well for `path`, but we cannot; as get_git_work_tree() != cwd) * diff to sb/submodule-helper-clone-regression-fix: diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 89cbbda..be7bf5f 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -153,12 +153,13 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url static int module_clone(int argc, const char **argv, const char *prefix) { - const char *path = NULL, *name = NULL, *url = NULL; + const char *name = NULL, *url = NULL; const char *reference = NULL, *depth = NULL; int quiet = 0; FILE *submodule_dot_git; - char *sm_gitdir, *p; - struct strbuf rel_path = STRBUF_INIT; /* for relative_path */ + char *sm_gitdir_rel, *p, *path = NULL; + const char *sm_gitdir; + struct strbuf rel_path = STRBUF_INIT; struct strbuf sb = STRBUF_INIT; struct option module_clone_options[] = { @@ -198,26 +199,14 @@ static int module_clone(int argc, const char **argv, const char *prefix) die(_("submodule--helper: unspecified or empty --path")); strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); - sm_gitdir = strbuf_detach(&sb, NULL); - - - if (!is_absolute_path(sm_gitdir)) { - char *cwd = xgetcwd(); - strbuf_addf(&sb, "%s/%s", cwd, sm_gitdir); - sm_gitdir = strbuf_detach(&sb, 0); - free(cwd); - } + sm_gitdir_rel = strbuf_detach(&sb, NULL); + sm_gitdir = absolute_path(sm_gitdir_rel); if (!is_absolute_path(path)) { - /* - * TODO: add prefix here once we allow calling from non root - * directory? - */ - strbuf_addf(&sb, "%s/%s", - get_git_work_tree(), - path); + strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path); path = strbuf_detach(&sb, 0); - } + } else + path = xstrdup(path); if (!file_exists(sm_gitdir)) { if (safe_create_leading_directories_const(sm_gitdir) < 0) @@ -240,6 +229,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) submodule_dot_git = fopen(sb.buf, "w"); if (!submodule_dot_git) die_errno(_("cannot open file '%s'"), sb.buf); + fprintf_or_die(submodule_dot_git, "gitdir: %s\n", relative_path(sm_gitdir, path, &rel_path)); if (fclose(submodule_dot_git)) @@ -255,8 +245,8 @@ static int module_clone(int argc, const char **argv, const char *prefix) relative_path(path, sm_gitdir, &rel_path)); strbuf_release(&sb); strbuf_release(&rel_path); - free(sm_gitdir); - + free(sm_gitdir_rel); + free(path); free(p); return 0; } v2: * reworded commit message for test (Thanks Junio!) * instead of "simplifying" the path handling in patch 2, we are prepared for anything the user throws at us (i.e. instead of segfault we die(_("submodule--helper: unspecified or empty --path")); (Thanks Eric, Jacob for pushing back here!) * reword commit message for patch 3 (safe_create_leading_directories_const is not a check, Thanks Junio!) * patch 4 (the fix): That got reworked completely. No flow controlling conditions in the write out phase! * patch 5 is a bonus! (replace fprintf by fprintf_or die) When implementing that I wondered if we also have close_or_die and open_or_die, but that doesn't seem to be the case. Thanks, Stefan v1: It took me longer than expected to fix this bug. It comes with a test and minor refactoring and applies on top of origin/sb/submodule-helper, such that it can be merged into 2.7, 2.8 as well as master. Patch 1 is a test which fails; it has a similar layout as the real failing repository Norio Nomura pointed out, but simplified to the bare essentials for reproduction of the bug. Patch 2&3 are not strictly necessary for fixing the isseu, but it removes stupid code I wrote, so the resulting code looks a little better. Patch 4 fixes the issue by giving more information to relative_path, such that a relative path can be found in all cases. Thanks, Stefan Stefan Beller (2): submodule--helper, module_clone: always operate on absolute paths submodule--helper, module_clone: catch fprintf failure builtin/submodule--helper.c | 32 ++++++++++++++------------------ t/t7400-submodule-basic.sh | 2 +- 2 files changed, 15 insertions(+), 19 deletions(-) -- 2.5.0.262.g0ef869b.dirty ``` ## Stefan Beller, 2016-04-01 00:17 Subject: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths Message-ID: <1459469849-9643-2-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459469849-9643-2-git-send-email-sbeller%40google.com In-Reply-To: <1459469849-9643-1-git-send-email-sbeller@google.com> ``` When giving relative paths to `relative_path` to compute a relative path from one directory to another, this may fail in `relative_path`. Make sure both arguments to `relative_path` are always absolute. Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 28 ++++++++++++++-------------- t/t7400-submodule-basic.sh | 2 +- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 0b9f546..b099817 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -153,11 +153,12 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url static int module_clone(int argc, const char **argv, const char *prefix) { - const char *path = NULL, *name = NULL, *url = NULL; + const char *name = NULL, *url = NULL; const char *reference = NULL, *depth = NULL; int quiet = 0; FILE *submodule_dot_git; - char *sm_gitdir, *cwd, *p; + char *sm_gitdir_rel, *p, *path = NULL; + const char *sm_gitdir; struct strbuf rel_path = STRBUF_INIT; struct strbuf sb = STRBUF_INIT; @@ -198,7 +199,14 @@ static int module_clone(int argc, const char **argv, const char *prefix) die(_("submodule--helper: unspecified or empty --path")); strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); - sm_gitdir = strbuf_detach(&sb, NULL); + sm_gitdir_rel = strbuf_detach(&sb, NULL); + sm_gitdir = absolute_path(sm_gitdir_rel); + + if (!is_absolute_path(path)) { + strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path); + path = strbuf_detach(&sb, 0); + } else + path = xstrdup(path); if (!file_exists(sm_gitdir)) { if (safe_create_leading_directories_const(sm_gitdir) < 0) @@ -229,24 +237,16 @@ static int module_clone(int argc, const char **argv, const char *prefix) strbuf_reset(&sb); strbuf_reset(&rel_path); - cwd = xgetcwd(); /* Redirect the worktree of the submodule in the superproject's config */ - if (!is_absolute_path(sm_gitdir)) { - strbuf_addf(&sb, "%s/%s", cwd, sm_gitdir); - free(sm_gitdir); - sm_gitdir = strbuf_detach(&sb, NULL); - } - - strbuf_addf(&sb, "%s/%s", cwd, path); p = git_pathdup_submodule(path, "config"); if (!p) die(_("could not get submodule directory for '%s'"), path); git_config_set_in_file(p, "core.worktree", - relative_path(sb.buf, sm_gitdir, &rel_path)); + relative_path(path, sm_gitdir, &rel_path)); strbuf_release(&sb); strbuf_release(&rel_path); - free(sm_gitdir); - free(cwd); + free(sm_gitdir_rel); + free(path); free(p); return 0; } diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index fc11809..ea3fabb 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -818,7 +818,7 @@ test_expect_success 'submodule add --name allows to replace a submodule with ano ) ' -test_expect_failure 'recursive relative submodules stay relative' ' +test_expect_success 'recursive relative submodules stay relative' ' test_when_finished "rm -rf super clone2 subsub sub3" && mkdir subsub && ( -- 2.5.0.264.gc776916.dirty ``` ## Stefan Beller, 2016-04-01 00:17 Subject: [PATCH 2/2] submodule--helper, module_clone: catch fprintf failure Message-ID: <1459469849-9643-3-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459469849-9643-3-git-send-email-sbeller%40google.com In-Reply-To: <1459469849-9643-1-git-send-email-sbeller@google.com> ``` The return value of fprintf is unchecked, which may lead to unreported errors. Use fprintf_or_die to report the error to the user. Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index b099817..be7bf5f 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -230,8 +230,8 @@ static int module_clone(int argc, const char **argv, const char *prefix) if (!submodule_dot_git) die_errno(_("cannot open file '%s'"), sb.buf); - fprintf(submodule_dot_git, "gitdir: %s\n", - relative_path(sm_gitdir, path, &rel_path)); + fprintf_or_die(submodule_dot_git, "gitdir: %s\n", + relative_path(sm_gitdir, path, &rel_path)); if (fclose(submodule_dot_git)) die(_("could not close file %s"), sb.buf); strbuf_reset(&sb); -- 2.5.0.264.gc776916.dirty ``` ## Ramsay Jones, 2016-04-01 14:41 Subject: Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. Message-ID: <56FE8898.20906@ramsayjones.plus.com> URL: https://gitlist.dev/e/56FE8898.20906%40ramsayjones.plus.com In-Reply-To: <1459469849-9643-1-git-send-email-sbeller@google.com> ``` On 01/04/16 01:17, Stefan Beller wrote: > Thanks Junio for review! > > v3: > * This is a resend of the last two patches of the series, i.e. it replaces > 44859d6626d4 and efdef1e2e in sb/submodule-helper-clone-regression-fix > > * use absolute_path for sm_gitdir Hi Stefan, In response to v1 of this series, I sent you a fixup patch to suppress a sparse warning (submodule: don't use an integer as a NULL pointer, 21-02-2016). In v2, you introduced a second identical warning (rather, for the same reason: using 0 as a NULL pointer as the second argument to strbuf_detach()). I was just about to send another patch, when you sent this out. As a result, you have suppressed the new warning, but the original remains. So, ... > * removed todo > * we need to free the intermediate sm_gitdir, so rename that to sm_gitdir_rel > and free it afterwards. > * while we currently do not support an absolute `path`, we eventually might. > If `path` is absolute it would be a pointer to `argv`, which would lead to a > crash. Duplicate the path and the crash is prevented. > (* I thought we could use it as well for `path`, but we cannot; as > get_git_work_tree() != cwd) > * diff to sb/submodule-helper-clone-regression-fix: > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 89cbbda..be7bf5f 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -153,12 +153,13 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url > > static int module_clone(int argc, const char **argv, const char *prefix) > { > - const char *path = NULL, *name = NULL, *url = NULL; > + const char *name = NULL, *url = NULL; > const char *reference = NULL, *depth = NULL; > int quiet = 0; > FILE *submodule_dot_git; > - char *sm_gitdir, *p; > - struct strbuf rel_path = STRBUF_INIT; /* for relative_path */ > + char *sm_gitdir_rel, *p, *path = NULL; > + const char *sm_gitdir; > + struct strbuf rel_path = STRBUF_INIT; > struct strbuf sb = STRBUF_INIT; > > struct option module_clone_options[] = { > @@ -198,26 +199,14 @@ static int module_clone(int argc, const char **argv, const char *prefix) > die(_("submodule--helper: unspecified or empty --path")); > > strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); > - sm_gitdir = strbuf_detach(&sb, NULL); > - > - > - if (!is_absolute_path(sm_gitdir)) { > - char *cwd = xgetcwd(); > - strbuf_addf(&sb, "%s/%s", cwd, sm_gitdir); > - sm_gitdir = strbuf_detach(&sb, 0); > - free(cwd); > - } > + sm_gitdir_rel = strbuf_detach(&sb, NULL); ... this is good, but ... > + sm_gitdir = absolute_path(sm_gitdir_rel); > > if (!is_absolute_path(path)) { > - /* > - * TODO: add prefix here once we allow calling from non root > - * directory? > - */ > - strbuf_addf(&sb, "%s/%s", > - get_git_work_tree(), > - path); > + strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path); > path = strbuf_detach(&sb, 0); ... can you please fix this up. Thanks! ATB, Ramsay Jones > - } > + } else > + path = xstrdup(path); > > if (!file_exists(sm_gitdir)) { > if (safe_create_leading_directories_const(sm_gitdir) < 0) > @@ -240,6 +229,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) > submodule_dot_git = fopen(sb.buf, "w"); > if (!submodule_dot_git) > die_errno(_("cannot open file '%s'"), sb.buf); > + > fprintf_or_die(submodule_dot_git, "gitdir: %s\n", > relative_path(sm_gitdir, path, &rel_path)); > if (fclose(submodule_dot_git)) > @@ -255,8 +245,8 @@ static int module_clone(int argc, const char **argv, const char *prefix) > relative_path(path, sm_gitdir, &rel_path)); > strbuf_release(&sb); > strbuf_release(&rel_path); > - free(sm_gitdir); > - > + free(sm_gitdir_rel); > + free(path); > free(p); > return 0; > } > > v2: > * reworded commit message for test (Thanks Junio!) > * instead of "simplifying" the path handling in patch 2, we are prepared > for anything the user throws at us (i.e. instead of segfault we > die(_("submodule--helper: unspecified or empty --path")); > (Thanks Eric, Jacob for pushing back here!) > * reword commit message for patch 3 (safe_create_leading_directories_const is > not a check, Thanks Junio!) > * patch 4 (the fix): That got reworked completely. No flow controlling > conditions in the write out phase! > * patch 5 is a bonus! (replace fprintf by fprintf_or die) When implementing > that I wondered if we also have close_or_die and open_or_die, but that doesn't > seem to be the case. > > Thanks, > Stefan > > v1: > > It took me longer than expected to fix this bug. > > It comes with a test and minor refactoring and applies on top of > origin/sb/submodule-helper, such that it can be merged into 2.7, 2.8 as well > as master. > > Patch 1 is a test which fails; it has a similar layout as the > real failing repository Norio Nomura pointed out, but simplified to the > bare essentials for reproduction of the bug. > > Patch 2&3 are not strictly necessary for fixing the isseu, but it removes > stupid code I wrote, so the resulting code looks a little better. > > Patch 4 fixes the issue by giving more information to relative_path, > such that a relative path can be found in all cases. > > Thanks, > Stefan > > Stefan Beller (2): > submodule--helper, module_clone: always operate on absolute paths > submodule--helper, module_clone: catch fprintf failure > > builtin/submodule--helper.c | 32 ++++++++++++++------------------ > t/t7400-submodule-basic.sh | 2 +- > 2 files changed, 15 insertions(+), 19 deletions(-) > ``` ## Junio C Hamano, 2016-04-01 19:18 Subject: Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths Message-ID: URL: https://gitlist.dev/e/xmqq7fghi1ds.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1459469849-9643-2-git-send-email-sbeller@google.com> ``` Stefan Beller writes: > + char *sm_gitdir_rel, *p, *path = NULL; > + const char *sm_gitdir; > struct strbuf rel_path = STRBUF_INIT; > struct strbuf sb = STRBUF_INIT; > > @@ -198,7 +199,14 @@ static int module_clone(int argc, const char **argv, const char *prefix) > die(_("submodule--helper: unspecified or empty --path")); > > strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); > - sm_gitdir = strbuf_detach(&sb, NULL); > + sm_gitdir_rel = strbuf_detach(&sb, NULL); > + sm_gitdir = absolute_path(sm_gitdir_rel); With this change, sm_gitdir will always be absolute, which means the parameter to clone_submodule() call (immedately after this hunk) will now be always absolute. I do not think "git clone" called from there will leave anything in the resulting repository that depends on the --separate-git-dir= being relative or absolute, so this change should not cause us new problems. By the way, this line is the last use of sm_gitdir_rel before it gets freed. I wonder sm_gitdir = absolute_path(sb.buf); would be sufficient. We can lose the variable, and free() on it at the end of this function, and an extra allocation if we can do so. > + if (!is_absolute_path(path)) { > + strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path); > + path = strbuf_detach(&sb, 0); > + } else > + path = xstrdup(path); > > if (!file_exists(sm_gitdir)) { > if (safe_create_leading_directories_const(sm_gitdir) < 0) Other than that, looks good to me. Thanks. ``` ## Junio C Hamano, 2016-04-01 19:30 Subject: Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths Message-ID: URL: https://gitlist.dev/e/xmqq37r5i0tv.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > By the way, this line is the last use of sm_gitdir_rel before it > gets freed. I wonder > > sm_gitdir = absolute_path(sb.buf); > > would be sufficient. We can lose the variable, and free() on it at > the end of this function, and an extra allocation if we can do so. > >> + if (!is_absolute_path(path)) { >> + strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path); >> + path = strbuf_detach(&sb, 0); >> + } else >> + path = xstrdup(path); >> >> if (!file_exists(sm_gitdir)) { >> if (safe_create_leading_directories_const(sm_gitdir) < 0) > > Other than that, looks good to me. Another thing I noticed is that it will not stay safe forever to borrow the result from absolute_path() for extended period of time. So how about this one (and Ramsay's "NULL must be spelled NULL, not 0") on top? -- >8 -- From: Junio C Hamano Date: Fri, 1 Apr 2016 12:23:16 -0700 Subject: [PATCH] submodule--helper: do not borrow absolute_path() result for too long absolute_path() is designed to allow its callers to take a brief peek of the result (typically, to be fed to functions like strbuf_add() and relative_path() as a parameter) without having to worry about freeing it, but the other side of the coin of that memory model is that the caller shouldn't rely too much on the result living forever--there may be a helper function the caller subsequently calls that makes its own call to absolute_path(), invalidating the earlier result. Use xstrdup() to make our own copy, and free(3) it when we are done. While at it, remove an unnecessary sm_gitdir_rel variable that was only used to as a parameter to call absolute_paht() and never used again. Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index b660a22..e69b340 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -157,8 +157,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) const char *reference = NULL, *depth = NULL; int quiet = 0; FILE *submodule_dot_git; - char *sm_gitdir_rel, *p, *path = NULL; - const char *sm_gitdir; + char *p, *path = NULL, *sm_gitdir; struct strbuf rel_path = STRBUF_INIT; struct strbuf sb = STRBUF_INIT; @@ -199,8 +198,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) die(_("submodule--helper: unspecified or empty --path")); strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); - sm_gitdir_rel = strbuf_detach(&sb, NULL); - sm_gitdir = absolute_path(sm_gitdir_rel); + sm_gitdir = xstrdup(absolute_path(sb.buf)); if (!is_absolute_path(path)) { strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path); @@ -245,7 +243,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) relative_path(path, sm_gitdir, &rel_path)); strbuf_release(&sb); strbuf_release(&rel_path); - free(sm_gitdir_rel); + free(sm_gitdir); free(path); free(p); return 0; -- 2.8.0-225-g297c00e ``` ## Eric Sunshine, 2016-04-01 20:09 Subject: Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSDNLvBYLUc1VvPoS%3DdQX9P3YCCiZVHcZ91MqCHZpf_dg%40mail.gmail.com In-Reply-To: ``` On Fri, Apr 1, 2016 at 3:30 PM, Junio C Hamano wrote: > Subject: [PATCH] submodule--helper: do not borrow absolute_path() result for too long > > absolute_path() is designed to allow its callers to take a brief > peek of the result (typically, to be fed to functions like > strbuf_add() and relative_path() as a parameter) without having to > worry about freeing it, but the other side of the coin of that > memory model is that the caller shouldn't rely too much on the > result living forever--there may be a helper function the caller > subsequently calls that makes its own call to absolute_path(), > invalidating the earlier result. > > Use xstrdup() to make our own copy, and free(3) it when we are done. > While at it, remove an unnecessary sm_gitdir_rel variable that was > only used to as a parameter to call absolute_paht() and never used s/absolute_paht/absolute_path/ > again. > > Signed-off-by: Junio C Hamano ``` ## Junio C Hamano, 2016-04-01 20:39 Subject: Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths Message-ID: URL: https://gitlist.dev/e/xmqqy48xgj2p.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > From: Junio C Hamano > Date: Fri, 1 Apr 2016 12:23:16 -0700 > Subject: [PATCH] submodule--helper: do not borrow absolute_path() result for too long > > absolute_path() is designed to allow its callers to take a brief > peek of the result (typically, to be fed to functions like > strbuf_add() and relative_path() as a parameter) without having to > ... > @@ -199,8 +198,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) > die(_("submodule--helper: unspecified or empty --path")); > > strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); > - sm_gitdir_rel = strbuf_detach(&sb, NULL); > - sm_gitdir = absolute_path(sm_gitdir_rel); > + sm_gitdir = xstrdup(absolute_path(sb.buf)); Just to prevent others from wasting time scratching their heads, we need strbuf_reset(&sb); here, as the strbuf will be reused later and is expected to be empty at this point. > > if (!is_absolute_path(path)) { > strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path); > @@ -245,7 +243,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) > relative_path(path, sm_gitdir, &rel_path)); > strbuf_release(&sb); > strbuf_release(&rel_path); > - free(sm_gitdir_rel); > + free(sm_gitdir); > free(path); > free(p); > return 0; ``` ## Stefan Beller, 2016-04-12 15:58 Subject: Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. Message-ID: URL: https://gitlist.dev/e/CAGZ79kYE3E7stNc3Xbzr-qvncLGt4qRPMbv897k5h0B50-Rogg%40mail.gmail.com In-Reply-To: <56FE8898.20906@ramsayjones.plus.com> ``` On Fri, Apr 1, 2016 at 7:41 AM, Ramsay Jones wrote: > > > On 01/04/16 01:17, Stefan Beller wrote: >> Thanks Junio for review! >> >> v3: >> * This is a resend of the last two patches of the series, i.e. it replaces >> 44859d6626d4 and efdef1e2e in sb/submodule-helper-clone-regression-fix >> >> * use absolute_path for sm_gitdir > > Hi Stefan, > > In response to v1 of this series, I sent you a fixup patch to suppress a > sparse warning (submodule: don't use an integer as a NULL pointer, 21-02-2016). > > In v2, you introduced a second identical warning (rather, for the same > reason: using 0 as a NULL pointer as the second argument to strbuf_detach()). > > I was just about to send another patch, when you sent this out. As a result, > you have suppressed the new warning, but the original remains. > > So, ... > >> * removed todo >> * we need to free the intermediate sm_gitdir, so rename that to sm_gitdir_rel >> and free it afterwards. >> * while we currently do not support an absolute `path`, we eventually might. >> If `path` is absolute it would be a pointer to `argv`, which would lead to a >> crash. Duplicate the path and the crash is prevented. >> (* I thought we could use it as well for `path`, but we cannot; as >> get_git_work_tree() != cwd) >> * diff to sb/submodule-helper-clone-regression-fix: >> >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c >> index 89cbbda..be7bf5f 100644 >> --- a/builtin/submodule--helper.c >> +++ b/builtin/submodule--helper.c >> @@ -153,12 +153,13 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url >> >> static int module_clone(int argc, const char **argv, const char *prefix) >> { >> - const char *path = NULL, *name = NULL, *url = NULL; >> + const char *name = NULL, *url = NULL; >> const char *reference = NULL, *depth = NULL; >> int quiet = 0; >> FILE *submodule_dot_git; >> - char *sm_gitdir, *p; >> - struct strbuf rel_path = STRBUF_INIT; /* for relative_path */ >> + char *sm_gitdir_rel, *p, *path = NULL; >> + const char *sm_gitdir; >> + struct strbuf rel_path = STRBUF_INIT; >> struct strbuf sb = STRBUF_INIT; >> >> struct option module_clone_options[] = { >> @@ -198,26 +199,14 @@ static int module_clone(int argc, const char **argv, const char *prefix) >> die(_("submodule--helper: unspecified or empty --path")); >> >> strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); >> - sm_gitdir = strbuf_detach(&sb, NULL); >> - >> - >> - if (!is_absolute_path(sm_gitdir)) { >> - char *cwd = xgetcwd(); >> - strbuf_addf(&sb, "%s/%s", cwd, sm_gitdir); >> - sm_gitdir = strbuf_detach(&sb, 0); >> - free(cwd); >> - } >> + sm_gitdir_rel = strbuf_detach(&sb, NULL); > > ... this is good, but ... > >> + sm_gitdir = absolute_path(sm_gitdir_rel); >> >> if (!is_absolute_path(path)) { >> - /* >> - * TODO: add prefix here once we allow calling from non root >> - * directory? >> - */ >> - strbuf_addf(&sb, "%s/%s", >> - get_git_work_tree(), >> - path); >> + strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path); >> path = strbuf_detach(&sb, 0); > > ... can you please fix this up. > > Thanks! > > ATB, > Ramsay Jones Looking at the current code of origin/sb/submodule-helper-clone-regression-fix we do not have this issue there, but I'll keep it in mind for a resend. > > >> - } >> + } else >> + path = xstrdup(path); >> >> if (!file_exists(sm_gitdir)) { >> if (safe_create_leading_directories_const(sm_gitdir) < 0) >> @@ -240,6 +229,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) >> submodule_dot_git = fopen(sb.buf, "w"); >> if (!submodule_dot_git) >> die_errno(_("cannot open file '%s'"), sb.buf); >> + >> fprintf_or_die(submodule_dot_git, "gitdir: %s\n", >> relative_path(sm_gitdir, path, &rel_path)); >> if (fclose(submodule_dot_git)) >> @@ -255,8 +245,8 @@ static int module_clone(int argc, const char **argv, const char *prefix) >> relative_path(path, sm_gitdir, &rel_path)); >> strbuf_release(&sb); >> strbuf_release(&rel_path); >> - free(sm_gitdir); >> - >> + free(sm_gitdir_rel); >> + free(path); >> free(p); >> return 0; >> } >> >> v2: >> * reworded commit message for test (Thanks Junio!) >> * instead of "simplifying" the path handling in patch 2, we are prepared >> for anything the user throws at us (i.e. instead of segfault we >> die(_("submodule--helper: unspecified or empty --path")); >> (Thanks Eric, Jacob for pushing back here!) >> * reword commit message for patch 3 (safe_create_leading_directories_const is >> not a check, Thanks Junio!) >> * patch 4 (the fix): That got reworked completely. No flow controlling >> conditions in the write out phase! >> * patch 5 is a bonus! (replace fprintf by fprintf_or die) When implementing >> that I wondered if we also have close_or_die and open_or_die, but that doesn't >> seem to be the case. >> >> Thanks, >> Stefan >> >> v1: >> >> It took me longer than expected to fix this bug. >> >> It comes with a test and minor refactoring and applies on top of >> origin/sb/submodule-helper, such that it can be merged into 2.7, 2.8 as well >> as master. >> >> Patch 1 is a test which fails; it has a similar layout as the >> real failing repository Norio Nomura pointed out, but simplified to the >> bare essentials for reproduction of the bug. >> >> Patch 2&3 are not strictly necessary for fixing the isseu, but it removes >> stupid code I wrote, so the resulting code looks a little better. >> >> Patch 4 fixes the issue by giving more information to relative_path, >> such that a relative path can be found in all cases. >> >> Thanks, >> Stefan >> >> Stefan Beller (2): >> submodule--helper, module_clone: always operate on absolute paths >> submodule--helper, module_clone: catch fprintf failure >> >> builtin/submodule--helper.c | 32 ++++++++++++++------------------ >> t/t7400-submodule-basic.sh | 2 +- >> 2 files changed, 15 insertions(+), 19 deletions(-) >> ``` ## Ramsay Jones, 2016-04-13 19:21 Subject: Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. Message-ID: <570E9C46.6080905@ramsayjones.plus.com> URL: https://gitlist.dev/e/570E9C46.6080905%40ramsayjones.plus.com In-Reply-To: ``` On 12/04/16 16:58, Stefan Beller wrote: > On Fri, Apr 1, 2016 at 7:41 AM, Ramsay Jones > wrote: >> [snip[ >>> - } >>> + sm_gitdir_rel = strbuf_detach(&sb, NULL); >> >> ... this is good, but ... >> >>> + sm_gitdir = absolute_path(sm_gitdir_rel); >>> >>> if (!is_absolute_path(path)) { >>> - /* >>> - * TODO: add prefix here once we allow calling from non root >>> - * directory? >>> - */ >>> - strbuf_addf(&sb, "%s/%s", >>> - get_git_work_tree(), >>> - path); >>> + strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path); >>> path = strbuf_detach(&sb, 0); >> >> ... can you please fix this up. >> >> Thanks! >> >> ATB, >> Ramsay Jones > > Looking at the current code of origin/sb/submodule-helper-clone-regression-fix > we do not have this issue there, but I'll keep it in mind for a resend. Hmm, actually, the above change wasn't the original culprit (as I thought), but a different instance of the same fault. :-D I've lost track of which version is now in 'pu' (currently @ 45a4edc "Merge branch 'sb/submodule-init' into pu"), but sparse is still warning: SP submodule.c submodule.c:256:43: warning: Using plain integer as NULL pointer So, the fix looks like: diff --git a/submodule.c b/submodule.c index 5d1238a..4cc1c27 100644 --- a/submodule.c +++ b/submodule.c @@ -253,7 +253,7 @@ const char *submodule_strategy_to_string(const struct submodule_update_strategy return NULL; case SM_UPDATE_COMMAND: strbuf_addf(&sb, "!%s", s->command); - return strbuf_detach(&sb, 0); + return strbuf_detach(&sb, NULL); } return NULL; } Also, I note that t7406-submodule-update.sh test #4 is failing. (looks like absolute vs relative paths) ATB, Ramsay Jones ``` ## Stefan Beller, 2016-04-13 20:34 Subject: Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. Message-ID: URL: https://gitlist.dev/e/CAGZ79kYj%2B4_6BsrcRRzsvdp_FcRG9VuiJ1tLjSo3D5_uWe2nfQ%40mail.gmail.com In-Reply-To: <570E9C46.6080905@ramsayjones.plus.com> ``` On Wed, Apr 13, 2016 at 12:21 PM, Ramsay Jones wrote: > > > On 12/04/16 16:58, Stefan Beller wrote: >> On Fri, Apr 1, 2016 at 7:41 AM, Ramsay Jones >> wrote: >>> > [snip[ > >>>> - } >>>> + sm_gitdir_rel = strbuf_detach(&sb, NULL); >>> >>> ... this is good, but ... >>> >>>> + sm_gitdir = absolute_path(sm_gitdir_rel); >>>> >>>> if (!is_absolute_path(path)) { >>>> - /* >>>> - * TODO: add prefix here once we allow calling from non root >>>> - * directory? >>>> - */ >>>> - strbuf_addf(&sb, "%s/%s", >>>> - get_git_work_tree(), >>>> - path); >>>> + strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path); >>>> path = strbuf_detach(&sb, 0); >>> >>> ... can you please fix this up. >>> >>> Thanks! >>> >>> ATB, >>> Ramsay Jones >> >> Looking at the current code of origin/sb/submodule-helper-clone-regression-fix >> we do not have this issue there, but I'll keep it in mind for a resend. > > Hmm, actually, the above change wasn't the original culprit (as I thought), but > a different instance of the same fault. :-D > > I've lost track of which version is now in 'pu' (currently @ 45a4edc "Merge branch > 'sb/submodule-init' into pu"), but sparse is still warning: > > SP submodule.c > submodule.c:256:43: warning: Using plain integer as NULL pointer > > So, the fix looks like: > > diff --git a/submodule.c b/submodule.c > index 5d1238a..4cc1c27 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -253,7 +253,7 @@ const char *submodule_strategy_to_string(const struct submodule_update_strategy > return NULL; > case SM_UPDATE_COMMAND: > strbuf_addf(&sb, "!%s", s->command); > - return strbuf_detach(&sb, 0); > + return strbuf_detach(&sb, NULL); > } > return NULL; > } > > Also, I note that t7406-submodule-update.sh test #4 is failing. > (looks like absolute vs relative paths) > > ATB, > Ramsay Jones > Ok fixed this instance here, too. I'll hunt down the path issue now. > > ``` ## Junio C Hamano, 2016-04-13 21:03 Subject: Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. Message-ID: URL: https://gitlist.dev/e/xmqqoa9djk60.fsf%40gitster.mtv.corp.google.com In-Reply-To: <570E9C46.6080905@ramsayjones.plus.com> ``` Ramsay Jones writes: > Also, I note that t7406-submodule-update.sh test #4 is failing. > (looks like absolute vs relative paths) I think that is $gmane/291363 http://thread.gmane.org/gmane.comp.version-control.git/291334/focus=291363 ``` ## Junio C Hamano, 2016-04-13 22:23 Subject: Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. Message-ID: URL: https://gitlist.dev/e/xmqqzisxi1w0.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Stefan Beller writes: > > Ok fixed this instance here, too. ... So... should I hold sb/submodule-helper-clone-regression-fix? It has been marked to be merged to 'next' for some time now. ``` ## Stefan Beller, 2016-04-13 22:30 Subject: Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. Message-ID: URL: https://gitlist.dev/e/CAGZ79kbh4q-w6yt6eHrvcGvH6mhmCJG1WdRSdO6bgpX%3DiLwARw%40mail.gmail.com In-Reply-To: ``` On Wed, Apr 13, 2016 at 3:23 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> >> Ok fixed this instance here, too. > > ... So... should I hold sb/submodule-helper-clone-regression-fix? > It has been marked to be merged to 'next' for some time now. (Both things Ramsay pointed at are in submodule.c, but sb/submodule-helper-clone-regression-fix never touches that file, so Ramsay talks about submodule-init here? I agree submodule-init is faulty and I am fixing/rewriting it now.) This series (sb/submodule-helper-clone-regression-fix), has no issues (on its own as well as playing together with any other submodule series in flight IIUC), so what is your concern for holding instead of merging? ``` ## Junio C Hamano, 2016-04-13 22:45 Subject: Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. Message-ID: URL: https://gitlist.dev/e/xmqqmvoxi0vo.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Stefan Beller writes: > ... so what is your concern for holding instead of merging? Nothing specific. Remember, I may be aware of all the discussion threads but I am certainly not reading every word in every thread. When the participants are trustworthy enough, instead of going back to the list archive (and risk overlooking a message that asks "please hold off merging this, I have another last-minute update") to see if everything known has been resolved in a satisfactory way myself, I'd just ask, which is more efficient _and_ reliable. ```