From 3fea121df31c108ee9c52fd888a90d68a3bbb2b2 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Thu, 31 Mar 2016 14:04:36 -0700 Subject: [PATCH 1/6] recursive submodules: test for relative paths "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). Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- 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 540771ca41..fc1180999e 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 && From 3c0663e16630ddfd9f4c2e46e52c9984a03b888d Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Thu, 31 Mar 2016 14:04:37 -0700 Subject: [PATCH 2/6] submodule--helper: fix potential NULL-dereference Don't dereference NULL 'path' if it was never assigned. Also protect against an empty --path argument. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index f4c3eff179..4f0b0022ee 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -194,6 +194,9 @@ 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); + if (!path || !*path) + die(_("submodule--helper: unspecified or empty --path")); + strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); sm_gitdir = strbuf_detach(&sb, NULL); @@ -215,10 +218,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); From 47d5d6487902753f9483dc340de12551f90d4bdb Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Thu, 31 Mar 2016 14:04:38 -0700 Subject: [PATCH 3/6] submodule--helper clone: create the submodule path just once 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). Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 4f0b0022ee..0b9f546de5 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -215,11 +215,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"); From f8eaa0ba98b3bd9cb9035eba184a2d9806d30b27 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Thu, 31 Mar 2016 17:17:28 -0700 Subject: [PATCH 4/6] submodule--helper, module_clone: always operate on absolute paths 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 Signed-off-by: Junio C Hamano --- 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 0b9f546de5..b660a22cbe 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, NULL); + } 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 fc1180999e..ea3fabbed8 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 && ( From 1ea4d9b7c8008f5172e9995d87e2cf2d594a7ac7 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 1 Apr 2016 12:23:16 -0700 Subject: [PATCH 5/6] 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_path() and never used again. Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index b660a22cbe..b59c66f011 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,8 @@ 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)); + strbuf_reset(&sb); if (!is_absolute_path(path)) { strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path); @@ -245,7 +244,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; From 1f15ba1f3c370acbe85d451fe1520bffe0b2cb6f Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Thu, 31 Mar 2016 17:17:29 -0700 Subject: [PATCH 6/6] submodule--helper, module_clone: catch fprintf failure 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 Signed-off-by: Junio C Hamano --- 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 b59c66f011..b3a60f56c3 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -229,8 +229,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);