* [PATCHv3 0/2] Fix relative path issues in recursive submodules. @ 2016-04-01 0:17 Stefan Beller 2016-04-01 0:17 ` [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths Stefan Beller ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Stefan Beller @ 2016-04-01 0:17 UTC (permalink / raw) To: gitster, git, sunshine, jacob.keller; +Cc: norio.nomura, Stefan Beller 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths 2016-04-01 0:17 [PATCHv3 0/2] Fix relative path issues in recursive submodules Stefan Beller @ 2016-04-01 0:17 ` Stefan Beller 2016-04-01 19:18 ` Junio C Hamano 2016-04-01 0:17 ` [PATCH 2/2] submodule--helper, module_clone: catch fprintf failure Stefan Beller 2016-04-01 14:41 ` [PATCHv3 0/2] Fix relative path issues in recursive submodules Ramsay Jones 2 siblings, 1 reply; 15+ messages in thread From: Stefan Beller @ 2016-04-01 0:17 UTC (permalink / raw) To: gitster, git, sunshine, jacob.keller; +Cc: norio.nomura, Stefan Beller 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 <sbeller@google.com> --- 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths 2016-04-01 0:17 ` [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths Stefan Beller @ 2016-04-01 19:18 ` Junio C Hamano 2016-04-01 19:30 ` Junio C Hamano 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2016-04-01 19:18 UTC (permalink / raw) To: Stefan Beller; +Cc: git, sunshine, jacob.keller, norio.nomura Stefan Beller <sbeller@google.com> 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=<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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths 2016-04-01 19:18 ` Junio C Hamano @ 2016-04-01 19:30 ` Junio C Hamano 2016-04-01 20:09 ` Eric Sunshine 2016-04-01 20:39 ` Junio C Hamano 0 siblings, 2 replies; 15+ messages in thread From: Junio C Hamano @ 2016-04-01 19:30 UTC (permalink / raw) To: Stefan Beller; +Cc: git, sunshine, jacob.keller, norio.nomura Junio C Hamano <gitster@pobox.com> 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 <gitster@pobox.com> 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 <gitster@pobox.com> --- 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths 2016-04-01 19:30 ` Junio C Hamano @ 2016-04-01 20:09 ` Eric Sunshine 2016-04-01 20:39 ` Junio C Hamano 1 sibling, 0 replies; 15+ messages in thread From: Eric Sunshine @ 2016-04-01 20:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stefan Beller, Git List, Jacob Keller, Norio Nomura On Fri, Apr 1, 2016 at 3:30 PM, Junio C Hamano <gitster@pobox.com> 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 <gitster@pobox.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths 2016-04-01 19:30 ` Junio C Hamano 2016-04-01 20:09 ` Eric Sunshine @ 2016-04-01 20:39 ` Junio C Hamano 1 sibling, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2016-04-01 20:39 UTC (permalink / raw) To: Stefan Beller; +Cc: git, sunshine, jacob.keller, norio.nomura Junio C Hamano <gitster@pobox.com> writes: > From: Junio C Hamano <gitster@pobox.com> > 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; ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] submodule--helper, module_clone: catch fprintf failure 2016-04-01 0:17 [PATCHv3 0/2] Fix relative path issues in recursive submodules Stefan Beller 2016-04-01 0:17 ` [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths Stefan Beller @ 2016-04-01 0:17 ` Stefan Beller 2016-04-01 14:41 ` [PATCHv3 0/2] Fix relative path issues in recursive submodules Ramsay Jones 2 siblings, 0 replies; 15+ messages in thread From: Stefan Beller @ 2016-04-01 0:17 UTC (permalink / raw) To: gitster, git, sunshine, jacob.keller; +Cc: norio.nomura, Stefan Beller 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 <sbeller@google.com> --- 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. 2016-04-01 0:17 [PATCHv3 0/2] Fix relative path issues in recursive submodules Stefan Beller 2016-04-01 0:17 ` [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths Stefan Beller 2016-04-01 0:17 ` [PATCH 2/2] submodule--helper, module_clone: catch fprintf failure Stefan Beller @ 2016-04-01 14:41 ` Ramsay Jones 2016-04-12 15:58 ` Stefan Beller 2 siblings, 1 reply; 15+ messages in thread From: Ramsay Jones @ 2016-04-01 14:41 UTC (permalink / raw) To: Stefan Beller, gitster, git, sunshine, jacob.keller; +Cc: norio.nomura 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(-) > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. 2016-04-01 14:41 ` [PATCHv3 0/2] Fix relative path issues in recursive submodules Ramsay Jones @ 2016-04-12 15:58 ` Stefan Beller 2016-04-13 19:21 ` Ramsay Jones 0 siblings, 1 reply; 15+ messages in thread From: Stefan Beller @ 2016-04-12 15:58 UTC (permalink / raw) To: Ramsay Jones Cc: Junio C Hamano, git@vger.kernel.org, Eric Sunshine, Jacob Keller, Norio Nomura On Fri, Apr 1, 2016 at 7:41 AM, Ramsay Jones <ramsay@ramsayjones.plus.com> 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(-) >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. 2016-04-12 15:58 ` Stefan Beller @ 2016-04-13 19:21 ` Ramsay Jones 2016-04-13 20:34 ` Stefan Beller 2016-04-13 21:03 ` Junio C Hamano 0 siblings, 2 replies; 15+ messages in thread From: Ramsay Jones @ 2016-04-13 19:21 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, git@vger.kernel.org, Eric Sunshine, Jacob Keller, Norio Nomura On 12/04/16 16:58, Stefan Beller wrote: > On Fri, Apr 1, 2016 at 7:41 AM, Ramsay Jones > <ramsay@ramsayjones.plus.com> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. 2016-04-13 19:21 ` Ramsay Jones @ 2016-04-13 20:34 ` Stefan Beller 2016-04-13 22:23 ` Junio C Hamano 2016-04-13 21:03 ` Junio C Hamano 1 sibling, 1 reply; 15+ messages in thread From: Stefan Beller @ 2016-04-13 20:34 UTC (permalink / raw) To: Ramsay Jones Cc: Junio C Hamano, git@vger.kernel.org, Eric Sunshine, Jacob Keller, Norio Nomura On Wed, Apr 13, 2016 at 12:21 PM, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: > > > On 12/04/16 16:58, Stefan Beller wrote: >> On Fri, Apr 1, 2016 at 7:41 AM, Ramsay Jones >> <ramsay@ramsayjones.plus.com> 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. > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. 2016-04-13 20:34 ` Stefan Beller @ 2016-04-13 22:23 ` Junio C Hamano 2016-04-13 22:30 ` Stefan Beller 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2016-04-13 22:23 UTC (permalink / raw) To: Stefan Beller Cc: Ramsay Jones, git@vger.kernel.org, Eric Sunshine, Jacob Keller, Norio Nomura Stefan Beller <sbeller@google.com> 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. 2016-04-13 22:23 ` Junio C Hamano @ 2016-04-13 22:30 ` Stefan Beller 2016-04-13 22:45 ` Junio C Hamano 0 siblings, 1 reply; 15+ messages in thread From: Stefan Beller @ 2016-04-13 22:30 UTC (permalink / raw) To: Junio C Hamano Cc: Ramsay Jones, git@vger.kernel.org, Eric Sunshine, Jacob Keller, Norio Nomura On Wed, Apr 13, 2016 at 3:23 PM, Junio C Hamano <gitster@pobox.com> wrote: > Stefan Beller <sbeller@google.com> 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. 2016-04-13 22:30 ` Stefan Beller @ 2016-04-13 22:45 ` Junio C Hamano 0 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2016-04-13 22:45 UTC (permalink / raw) To: Stefan Beller Cc: Ramsay Jones, git@vger.kernel.org, Eric Sunshine, Jacob Keller, Norio Nomura Stefan Beller <sbeller@google.com> 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules. 2016-04-13 19:21 ` Ramsay Jones 2016-04-13 20:34 ` Stefan Beller @ 2016-04-13 21:03 ` Junio C Hamano 1 sibling, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2016-04-13 21:03 UTC (permalink / raw) To: Ramsay Jones Cc: Stefan Beller, git@vger.kernel.org, Eric Sunshine, Jacob Keller, Norio Nomura Ramsay Jones <ramsay@ramsayjones.plus.com> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-04-13 22:45 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-01 0:17 [PATCHv3 0/2] Fix relative path issues in recursive submodules Stefan Beller 2016-04-01 0:17 ` [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths Stefan Beller 2016-04-01 19:18 ` Junio C Hamano 2016-04-01 19:30 ` Junio C Hamano 2016-04-01 20:09 ` Eric Sunshine 2016-04-01 20:39 ` Junio C Hamano 2016-04-01 0:17 ` [PATCH 2/2] submodule--helper, module_clone: catch fprintf failure Stefan Beller 2016-04-01 14:41 ` [PATCHv3 0/2] Fix relative path issues in recursive submodules Ramsay Jones 2016-04-12 15:58 ` Stefan Beller 2016-04-13 19:21 ` Ramsay Jones 2016-04-13 20:34 ` Stefan Beller 2016-04-13 22:23 ` Junio C Hamano 2016-04-13 22:30 ` Stefan Beller 2016-04-13 22:45 ` Junio C Hamano 2016-04-13 21:03 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).