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

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

[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

[PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths

[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

[PATCH 2/2] submodule--helper, module_clone: catch fprintf failure

[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

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

[Ramsay Jones]

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(-)
> 

Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths

[Junio C Hamano]

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.

Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths

[Junio C Hamano]

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

Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths

[Eric Sunshine]

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>

Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths

[Junio C Hamano]

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;

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

[Stefan Beller]

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(-)
>>

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

[Ramsay Jones]

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

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

[Stefan Beller]

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.

>
>

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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.

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

[Stefan Beller]

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?

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

[Junio C Hamano]

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.