# [PATCH 0/4] Fix relative path issues in recursive submodules. 17 messages from 2016-03-31 to 2016-03-31. Participants: Stefan Beller, Jacob Keller, Eric Sunshine, Junio C Hamano. Thread: https://gitlist.dev/t/41880 ## Stefan Beller, 2016-03-31 00:17 Subject: [PATCH 0/4] Fix relative path issues in recursive submodules. Message-ID: <1459383457-6848-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459383457-6848-1-git-send-email-sbeller%40google.com ``` 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 (4): recursive submodules: test for relative paths submodule--helper clone: simplify path check submodule--helper clone: remove double path checking submodule--helper: use relative path if possible builtin/submodule--helper.c | 16 ++++++++-------- t/t7400-submodule-basic.sh | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 8 deletions(-) -- 2.5.0.264.g4004fdc.dirty ``` ## Stefan Beller, 2016-03-31 00:17 Subject: [PATCH 1/4] recursive submodules: test for relative paths Message-ID: <1459383457-6848-2-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459383457-6848-2-git-send-email-sbeller%40google.com In-Reply-To: <1459383457-6848-1-git-send-email-sbeller@google.com> ``` This was reported as a regression at $gmane/290280. The root cause for that bug is in using recursive submodules as their relative path handling seems to be broken in ee8838d (2015-09-08, submodule: rewrite `module_clone` shell function in C). Signed-off-by: Stefan Beller --- t/t7400-submodule-basic.sh | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 540771c..fc11809 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -818,6 +818,47 @@ test_expect_success 'submodule add --name allows to replace a submodule with ano ) ' +test_expect_failure 'recursive relative submodules stay relative' ' + test_when_finished "rm -rf super clone2 subsub sub3" && + mkdir subsub && + ( + cd subsub && + git init && + >t && + git add t && + git commit -m "initial commit" + ) && + mkdir sub3 && + ( + cd sub3 && + git init && + >t && + git add t && + git commit -m "initial commit" && + git submodule add ../subsub dirdir/subsub && + git commit -m "add submodule subsub" + ) && + mkdir super && + ( + cd super && + git init && + >t && + git add t && + git commit -m "initial commit" && + git submodule add ../sub3 && + git commit -m "add submodule sub" + ) && + git clone super clone2 && + ( + cd clone2 && + git submodule update --init --recursive && + echo "gitdir: ../.git/modules/sub3" >./sub3/.git_expect && + echo "gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub" >./sub3/dirdir/subsub/.git_expect + ) && + test_cmp clone2/sub3/.git_expect clone2/sub3/.git && + test_cmp clone2/sub3/dirdir/subsub/.git_expect clone2/sub3/dirdir/subsub/.git +' + test_expect_success 'submodule add with an existing name fails unless forced' ' ( cd addtest2 && -- 2.5.0.264.g4004fdc.dirty ``` ## Stefan Beller, 2016-03-31 00:17 Subject: [PATCH 2/4] submodule--helper clone: simplify path check Message-ID: <1459383457-6848-3-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459383457-6848-3-git-send-email-sbeller%40google.com In-Reply-To: <1459383457-6848-1-git-send-email-sbeller@google.com> ``` The calling shell code makes sure that `path` is non null and non empty. (side note: it cannot be null as just three lines before it is passed to safe_create_leading_directories_const which would crash as you feed it null). Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index f4c3eff..88002ca 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -215,10 +215,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) if (safe_create_leading_directories_const(path) < 0) die(_("could not create directory '%s'"), path); - if (path && *path) - strbuf_addf(&sb, "%s/.git", path); - else - strbuf_addstr(&sb, ".git"); + strbuf_addf(&sb, "%s/.git", path); if (safe_create_leading_directories_const(sb.buf) < 0) die(_("could not create leading directories of '%s'"), sb.buf); -- 2.5.0.264.g4004fdc.dirty ``` ## Stefan Beller, 2016-03-31 00:17 Subject: [PATCH 3/4] submodule--helper clone: remove double path checking Message-ID: <1459383457-6848-4-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459383457-6848-4-git-send-email-sbeller%40google.com In-Reply-To: <1459383457-6848-1-git-send-email-sbeller@google.com> ``` Just a few lines after the deleted code we call safe_create_leading_directories_const(path + "/.git") so the check is done twice without action in between. Remove the first check. Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 88002ca..914e561 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -212,11 +212,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) } /* Write a .git file in the submodule to redirect to the superproject. */ - if (safe_create_leading_directories_const(path) < 0) - die(_("could not create directory '%s'"), path); - strbuf_addf(&sb, "%s/.git", path); - if (safe_create_leading_directories_const(sb.buf) < 0) die(_("could not create leading directories of '%s'"), sb.buf); submodule_dot_git = fopen(sb.buf, "w"); -- 2.5.0.264.g4004fdc.dirty ``` ## Stefan Beller, 2016-03-31 00:17 Subject: [PATCH 4/4] submodule--helper: use relative path if possible Message-ID: <1459383457-6848-5-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459383457-6848-5-git-send-email-sbeller%40google.com In-Reply-To: <1459383457-6848-1-git-send-email-sbeller@google.com> ``` As submodules have working directory and their git directory far apart relative_path(gitdir, work_dir) will not produce a relative path when git_dir is absolute. When the gitdir is absolute, we need to convert the workdir to an absolute path as well to compute the relative path. (e.g. gitdir=/home/user/project/.git/modules/submodule, workdir=submodule/, relative_dir doesn't take the current working directory into account, so there is no way for it to know that the correct answer is indeed "../.git/modules/submodule", if the workdir was given as /home/user/project/submodule, the answer is obvious.) Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 7 +++++++ t/t7400-submodule-basic.sh | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 914e561..0b0fad3 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -159,6 +159,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) FILE *submodule_dot_git; char *sm_gitdir, *cwd, *p; struct strbuf rel_path = STRBUF_INIT; + struct strbuf abs_path = STRBUF_INIT; struct strbuf sb = STRBUF_INIT; struct option module_clone_options[] = { @@ -219,7 +220,12 @@ static int module_clone(int argc, const char **argv, const char *prefix) if (!submodule_dot_git) die_errno(_("cannot open file '%s'"), sb.buf); + strbuf_addf(&abs_path, "%s/%s", + get_git_work_tree(), + path); fprintf(submodule_dot_git, "gitdir: %s\n", + is_absolute_path(sm_gitdir) ? + relative_path(sm_gitdir, abs_path.buf, &rel_path) : relative_path(sm_gitdir, path, &rel_path)); if (fclose(submodule_dot_git)) die(_("could not close file %s"), sb.buf); @@ -242,6 +248,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) relative_path(sb.buf, sm_gitdir, &rel_path)); strbuf_release(&sb); strbuf_release(&rel_path); + strbuf_release(&abs_path); free(sm_gitdir); free(cwd); free(p); 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.g4004fdc.dirty ``` ## Jacob Keller, 2016-03-31 00:32 Subject: Re: [PATCH 2/4] submodule--helper clone: simplify path check Message-ID: URL: https://gitlist.dev/e/CA%2BP7%2Bxq0w%2BdyhpT7VeURjj5wEa6%2B%2Bp_N2ASLYzjeQy%2BeOgLQLQ%40mail.gmail.com In-Reply-To: <1459383457-6848-3-git-send-email-sbeller@google.com> ``` On Wed, Mar 30, 2016 at 5:17 PM, Stefan Beller wrote: > The calling shell code makes sure that `path` is non null and non empty. > (side note: it cannot be null as just three lines before it is passed > to safe_create_leading_directories_const which would crash as you feed > it null). > So we're going to assume that all callers of submodule--helper will provide a non-null non-empty path? Hmm, since we expect only our shell code to really do this... that's probably ok I think. I don't think it can do any real harm should someone outside git call it with a bad path. Thanks, Jake ``` ## Eric Sunshine, 2016-03-31 07:31 Subject: Re: [PATCH 2/4] submodule--helper clone: simplify path check Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTz3fTyXFM4oPNO3Tv6dP0hN152jf9_JZOrsAZad1%2BO6Q%40mail.gmail.com In-Reply-To: <1459383457-6848-3-git-send-email-sbeller@google.com> ``` On Wed, Mar 30, 2016 at 8:17 PM, Stefan Beller wrote: > The calling shell code makes sure that `path` is non null and non empty. > (side note: it cannot be null as just three lines before it is passed > to safe_create_leading_directories_const which would crash as you feed > it null). I'm confused by this. So, you're saying that it's okay (and desirable) for git-submodule--helper to segfault when the user does this: % git submodule--helper clone Segmentation fault: 11 rather than, say, printing a useful error message, such as: submodule--helper: missing or empty --path ? > Signed-off-by: Stefan Beller > --- > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > @@ -215,10 +215,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) > if (safe_create_leading_directories_const(path) < 0) > die(_("could not create directory '%s'"), path); > > - if (path && *path) > - strbuf_addf(&sb, "%s/.git", path); > - else > - strbuf_addstr(&sb, ".git"); > + strbuf_addf(&sb, "%s/.git", path); > > if (safe_create_leading_directories_const(sb.buf) < 0) > die(_("could not create leading directories of '%s'"), sb.buf); > -- ``` ## Junio C Hamano, 2016-03-31 16:33 Subject: Re: [PATCH 1/4] recursive submodules: test for relative paths Message-ID: URL: https://gitlist.dev/e/xmqqpouappxp.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1459383457-6848-2-git-send-email-sbeller@google.com> ``` Stefan Beller writes: > This was reported as a regression at $gmane/290280. The root cause for > that bug is in using recursive submodules as their relative path handling > seems to be broken in ee8838d (2015-09-08, submodule: rewrite > `module_clone` shell function in C). I've reworded the above like so while queuing. "git submodule update --init --recursive" uses full path to refer to the true location of the repository in the "gitdir:" pointer for nested submodules; the command used to use relative paths. This was reported by Norio Nomura in $gmane/290280. The root cause for that bug is in using recursive submodules as their relative path handling was broken in ee8838d (2015-09-08, submodule: rewrite `module_clone` shell function in C). Thanks. > Signed-off-by: Stefan Beller > --- > t/t7400-submodule-basic.sh | 41 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index 540771c..fc11809 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -818,6 +818,47 @@ test_expect_success 'submodule add --name allows to replace a submodule with ano > ) > ' > > +test_expect_failure 'recursive relative submodules stay relative' ' > + test_when_finished "rm -rf super clone2 subsub sub3" && > + mkdir subsub && > + ( > + cd subsub && > + git init && > + >t && > + git add t && > + git commit -m "initial commit" > + ) && > + mkdir sub3 && > + ( > + cd sub3 && > + git init && > + >t && > + git add t && > + git commit -m "initial commit" && > + git submodule add ../subsub dirdir/subsub && > + git commit -m "add submodule subsub" > + ) && > + mkdir super && > + ( > + cd super && > + git init && > + >t && > + git add t && > + git commit -m "initial commit" && > + git submodule add ../sub3 && > + git commit -m "add submodule sub" > + ) && > + git clone super clone2 && > + ( > + cd clone2 && > + git submodule update --init --recursive && > + echo "gitdir: ../.git/modules/sub3" >./sub3/.git_expect && > + echo "gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub" >./sub3/dirdir/subsub/.git_expect > + ) && > + test_cmp clone2/sub3/.git_expect clone2/sub3/.git && > + test_cmp clone2/sub3/dirdir/subsub/.git_expect clone2/sub3/dirdir/subsub/.git > +' > + > test_expect_success 'submodule add with an existing name fails unless forced' ' > ( > cd addtest2 && ``` ## Junio C Hamano, 2016-03-31 16:36 Subject: Re: [PATCH 2/4] submodule--helper clone: simplify path check Message-ID: URL: https://gitlist.dev/e/xmqqlh4yppsq.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1459383457-6848-3-git-send-email-sbeller@google.com> ``` Stefan Beller writes: > The calling shell code makes sure that `path` is non null and non empty. > (side note: it cannot be null as just three lines before it is passed > to safe_create_leading_directories_const which would crash as you feed > it null). This is not Java so let's spell that thing NULL. Perhaps it would be cheap-enough prudence to do this on top? builtin/submodule--helper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 88002ca..f11796a 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -194,6 +194,8 @@ static int module_clone(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, module_clone_options, git_submodule_helper_usage, 0); + assert(path); + strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); sm_gitdir = strbuf_detach(&sb, NULL); ``` ## Junio C Hamano, 2016-03-31 16:44 Subject: Re: [PATCH 3/4] submodule--helper clone: remove double path checking Message-ID: URL: https://gitlist.dev/e/xmqqh9fmppg6.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1459383457-6848-4-git-send-email-sbeller@google.com> ``` Stefan Beller writes: > Just a few lines after the deleted code we call > > safe_create_leading_directories_const(path + "/.git") > > so the check is done twice without action in between. > Remove the first check. I am hesitant to call the call to this function a "check". If you do not yet have the leading directories, they get created. We make sure that the parent directory of path exists (or create it otherwise) and then do the same for path + "/.git". That is equivalent to just making sure that the parent directory of path + "/.git" exists (or create it otherwise). Perhaps? > > Signed-off-by: Stefan Beller > --- > builtin/submodule--helper.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 88002ca..914e561 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -212,11 +212,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) > } > > /* Write a .git file in the submodule to redirect to the superproject. */ > - if (safe_create_leading_directories_const(path) < 0) > - die(_("could not create directory '%s'"), path); > - > strbuf_addf(&sb, "%s/.git", path); > - > if (safe_create_leading_directories_const(sb.buf) < 0) > die(_("could not create leading directories of '%s'"), sb.buf); > submodule_dot_git = fopen(sb.buf, "w"); ``` ## Stefan Beller, 2016-03-31 16:47 Subject: Re: [PATCH 1/4] recursive submodules: test for relative paths Message-ID: URL: https://gitlist.dev/e/CAGZ79kYiCetyhDJwBnJ9MSufZMkW_WZepESK%2BU_yAOJEgfzMXg%40mail.gmail.com In-Reply-To: ``` On Thu, Mar 31, 2016 at 9:33 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> This was reported as a regression at $gmane/290280. The root cause for >> that bug is in using recursive submodules as their relative path handling >> seems to be broken in ee8838d (2015-09-08, submodule: rewrite >> `module_clone` shell function in C). > > I've reworded the above like so while queuing. > > "git submodule update --init --recursive" uses full path to refer to > the true location of the repository in the "gitdir:" pointer for > nested submodules; the command used to use relative paths. > > This was reported by Norio Nomura in $gmane/290280. > > The root cause for that bug is in using recursive submodules as > their relative path handling was broken in ee8838d (2015-09-08, > submodule: rewrite `module_clone` shell function in C). > > Thanks. > Thanks! I'll pickup the reworded version and resend the series as there seems to be discussion on the other patches which requires some work by me. ``` ## Junio C Hamano, 2016-03-31 16:59 Subject: Re: [PATCH 4/4] submodule--helper: use relative path if possible Message-ID: URL: https://gitlist.dev/e/xmqqd1qapore.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1459383457-6848-5-git-send-email-sbeller@google.com> ``` Stefan Beller writes: > As submodules have working directory and their git directory far apart > relative_path(gitdir, work_dir) will not produce a relative path when > git_dir is absolute. When the gitdir is absolute, we need to convert > the workdir to an absolute path as well to compute the relative path. > > (e.g. gitdir=/home/user/project/.git/modules/submodule, > workdir=submodule/, relative_dir doesn't take the current working directory > into account, so there is no way for it to know that the correct answer > is indeed "../.git/modules/submodule", if the workdir was given as > /home/user/project/submodule, the answer is obvious.) > > Signed-off-by: Stefan Beller > --- > builtin/submodule--helper.c | 7 +++++++ > t/t7400-submodule-basic.sh | 2 +- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 914e561..0b0fad3 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -159,6 +159,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) > FILE *submodule_dot_git; > char *sm_gitdir, *cwd, *p; > struct strbuf rel_path = STRBUF_INIT; > + struct strbuf abs_path = STRBUF_INIT; > struct strbuf sb = STRBUF_INIT; > > struct option module_clone_options[] = { > @@ -219,7 +220,12 @@ static int module_clone(int argc, const char **argv, const char *prefix) > if (!submodule_dot_git) > die_errno(_("cannot open file '%s'"), sb.buf); > > + strbuf_addf(&abs_path, "%s/%s", > + get_git_work_tree(), > + path); The "path" is assumed to be _always_ relative to work tree? I am wondering if it would be prudent to have an assert for that before doing this, just like I suggested assert(path) for [2/4] earlier [*1*]. On the other hand, if we allow path to be absolute, this would need to become something like: if (is_absolute_path(path)) strbuf_addstr(&abs_path, path); else strbuf_addf(&abs_path, "%s/%s", get_git_work_tree(), path); > fprintf(submodule_dot_git, "gitdir: %s\n", > + is_absolute_path(sm_gitdir) ? > + relative_path(sm_gitdir, abs_path.buf, &rel_path) : > relative_path(sm_gitdir, path, &rel_path)); It seems that the abs_path computation is not needed at all if sm_gitdir is relative to begin with. I wonder if the code gets easier to read and can avoid unnecessary strbuf manipulation if this entire hunk is structured more like so: if (is_absolute_path(sm_gitdir)) { ... } else { ... } fprintf(submodule_dot_git, "gitdir: %s\n", relative_path(sm_gitdir, base_path, &rel_path)); > if (fclose(submodule_dot_git)) > die(_("could not close file %s"), sb.buf); [Footnote] *1* BTW, I tightened the assert for 2/4 to "assert(path && *path)" to match the assumption in its log message, i.e. "The calling shell code makes sure that path is non-NULL and non empty". ``` ## Junio C Hamano, 2016-03-31 17:04 Subject: Re: [PATCH 0/4] Fix relative path issues in recursive submodules. Message-ID: URL: https://gitlist.dev/e/xmqq7fgipojj.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1459383457-6848-1-git-send-email-sbeller@google.com> ``` Stefan Beller writes: > 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. There were some minor nits, but I saw nothing glaringly wrong to break the topic. Thanks for working on this. ``` ## Eric Sunshine, 2016-03-31 17:04 Subject: Re: [PATCH 2/4] submodule--helper clone: simplify path check Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRq3CoE1mH9%3DTeKnujQYVb0%2BtURpYNSsRKtnK_FTDyBAQ%40mail.gmail.com In-Reply-To: ``` On Thu, Mar 31, 2016 at 12:36 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> The calling shell code makes sure that `path` is non null and non empty. >> (side note: it cannot be null as just three lines before it is passed >> to safe_create_leading_directories_const which would crash as you feed >> it null). > > This is not Java so let's spell that thing NULL. > > Perhaps it would be cheap-enough prudence to do this on top? > > argc = parse_options(argc, argv, prefix, module_clone_options, > git_submodule_helper_usage, 0); > > + assert(path); Hmm, from the user perspective, this is still a "crash", just as the existing segfault is a crash. While one can argue that this is an internal command only invoked in a controlled fashion, it's not hard to imagine someone running it manually to understand its behavior or to debug some problem. This function already presents proper error messages for other problems it encounters, so it seems reasonable for it do so for this problem, as well. if (!path || !*path) die(_("submodule--helper: unspecified or empty --path")); ``` ## Stefan Beller, 2016-03-31 17:17 Subject: Re: [PATCH 2/4] submodule--helper clone: simplify path check Message-ID: URL: https://gitlist.dev/e/CAGZ79kYbX5dggD7cs1PRHpkUS2gRTp1fAmt3mg1n1Og3ZB3fRw%40mail.gmail.com In-Reply-To: ``` On Thu, Mar 31, 2016 at 12:31 AM, Eric Sunshine wrote: > On Wed, Mar 30, 2016 at 8:17 PM, Stefan Beller wrote: >> The calling shell code makes sure that `path` is non null and non empty. >> (side note: it cannot be null as just three lines before it is passed >> to safe_create_leading_directories_const which would crash as you feed >> it null). > > I'm confused by this. So, you're saying that it's okay (and desirable) > for git-submodule--helper to segfault when the user does this: > > % git submodule--helper clone > Segmentation fault: 11 > > rather than, say, printing a useful error message, such as: > > submodule--helper: missing or empty --path I think I was rather saying, * that you see the segfault behavior not at all when channeling the command through the proper frontend command * that it already crashes (we access *path before this check, so this check is pointless) > While one can argue that this is an > internal command only invoked in a controlled fashion, it's not hard > to imagine someone running it manually to understand its behavior or > to debug some problem. (additionally:) And later we may want to reuse this code when replacing the frontend command to be written in C completely. > if (!path || !*path) > die(_("submodule--helper: unspecified or empty --path")); That sounds good to me. I'll pick it up. ``` ## Stefan Beller, 2016-03-31 17:20 Subject: Re: [PATCH 0/4] Fix relative path issues in recursive submodules. Message-ID: URL: https://gitlist.dev/e/CAGZ79kYyVJcOsWvkK%3DQm0rE0hd-xTq%3DTPi8groynJ4Vokft74g%40mail.gmail.com In-Reply-To: ``` On Thu, Mar 31, 2016 at 10:04 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> 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. > > There were some minor nits, but I saw nothing glaringly wrong to > break the topic. Thanks for working on this. Thanks for review and discussion, I plan on resending this series. Currently I have the opinion to drop 2&3 (the path assumption and double safe_create_leading_dir) and send patch 1 and 4 combined as a bugfix only as that is more the spirit what we want to see for an eventual merge to maint? The refactoring patches 2&3 can be sent later as separate patches for master, I would think. ``` ## Stefan Beller, 2016-03-31 19:22 Subject: Re: [PATCH 4/4] submodule--helper: use relative path if possible Message-ID: URL: https://gitlist.dev/e/CAGZ79kb9sRDuCvJw%3DS8_BLy7hej_YqE9Y-NbnQhQmafsZuEpwg%40mail.gmail.com In-Reply-To: ``` On Thu, Mar 31, 2016 at 9:59 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> As submodules have working directory and their git directory far apart >> relative_path(gitdir, work_dir) will not produce a relative path when >> git_dir is absolute. When the gitdir is absolute, we need to convert >> the workdir to an absolute path as well to compute the relative path. >> >> (e.g. gitdir=/home/user/project/.git/modules/submodule, >> workdir=submodule/, relative_dir doesn't take the current working directory >> into account, so there is no way for it to know that the correct answer >> is indeed "../.git/modules/submodule", if the workdir was given as >> /home/user/project/submodule, the answer is obvious.) >> >> Signed-off-by: Stefan Beller >> --- >> builtin/submodule--helper.c | 7 +++++++ >> t/t7400-submodule-basic.sh | 2 +- >> 2 files changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c >> index 914e561..0b0fad3 100644 >> --- a/builtin/submodule--helper.c >> +++ b/builtin/submodule--helper.c >> @@ -159,6 +159,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) >> FILE *submodule_dot_git; >> char *sm_gitdir, *cwd, *p; >> struct strbuf rel_path = STRBUF_INIT; >> + struct strbuf abs_path = STRBUF_INIT; >> struct strbuf sb = STRBUF_INIT; >> >> struct option module_clone_options[] = { >> @@ -219,7 +220,12 @@ static int module_clone(int argc, const char **argv, const char *prefix) >> if (!submodule_dot_git) >> die_errno(_("cannot open file '%s'"), sb.buf); >> >> + strbuf_addf(&abs_path, "%s/%s", >> + get_git_work_tree(), >> + path); > > The "path" is assumed to be _always_ relative to work tree? In the code prior to this patch, that was assumed, yes. (e.g. later in the code: /* Redirect the worktree of the submodule in the superproject's config */ cwd = xgetcwd(); strbuf_addf(&sb, "%s/%s", cwd, path); .... relative_path(sb.buf, ... ) > > I am wondering if it would be prudent to have an assert for that > before doing this, just like I suggested assert(path) for [2/4] > earlier [*1*]. > > On the other hand, if we allow path to be absolute, this would need > to become something like: > > if (is_absolute_path(path)) > strbuf_addstr(&abs_path, path); > else > strbuf_addf(&abs_path, "%s/%s", get_git_work_tree(), path); > >> fprintf(submodule_dot_git, "gitdir: %s\n", >> + is_absolute_path(sm_gitdir) ? >> + relative_path(sm_gitdir, abs_path.buf, &rel_path) : >> relative_path(sm_gitdir, path, &rel_path)); > > It seems that the abs_path computation is not needed at all if > sm_gitdir is relative to begin with. I wonder if the code gets > easier to read and can avoid unnecessary strbuf manipulation > if this entire hunk is structured more like so: > > if (is_absolute_path(sm_gitdir)) { > ... > } else { > ... > } > fprintf(submodule_dot_git, "gitdir: %s\n", > relative_path(sm_gitdir, base_path, &rel_path)); > >> if (fclose(submodule_dot_git)) >> die(_("could not close file %s"), sb.buf); > > [Footnote] I just simplified the code to be if (!is_absolute(path)) path=make_absolute(...); if (!is_absolute(sm_gitdir)) sm_gitdir=make_absolute(...); ... /* rest operates under absolute path only, no conditions any more! */ And I'd think that is cleanest to read and understand. Having absolute path for both path and gitdir all the time leaves no exceptions for relative_path to spew errors because one of both is relative and not connected to the other absolute. > > *1* BTW, I tightened the assert for 2/4 to "assert(path && *path)" > to match the assumption in its log message, i.e. "The calling shell > code makes sure that path is non-NULL and non empty". > ```