[PATCHv3 0/2] Fix relative path issues in recursive submodules.

[Stefan Beller <sbeller@google.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