[PATCH 0/4] Fix relative path issues in recursive submodules.

[PATCH 0/4] Fix relative path issues in recursive submodules.

[Stefan Beller]

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

[PATCH 1/4] recursive submodules: test for relative paths

[Stefan Beller]

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 <sbeller@google.com>
---
 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

Re: [PATCH 1/4] recursive submodules: test for relative paths

[Junio C Hamano]

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

Re: [PATCH 1/4] recursive submodules: test for relative paths

[Stefan Beller]

On Thu, Mar 31, 2016 at 9:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> 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.

[PATCH 2/4] submodule--helper clone: simplify path check

[Stefan Beller]

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 <sbeller@google.com>
---
 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

Re: [PATCH 2/4] submodule--helper clone: simplify path check

[Jacob Keller]

On Wed, Mar 30, 2016 at 5:17 PM, Stefan Beller <sbeller@google.com> 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

Re: [PATCH 2/4] submodule--helper clone: simplify path check

[Eric Sunshine]

On Wed, Mar 30, 2016 at 8:17 PM, Stefan Beller <sbeller@google.com> 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 <sbeller@google.com>
> ---
> 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);
> --

Re: [PATCH 2/4] submodule--helper clone: simplify path check

[Stefan Beller]

On Thu, Mar 31, 2016 at 12:31 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Mar 30, 2016 at 8:17 PM, Stefan Beller <sbeller@google.com> 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.

Re: [PATCH 2/4] submodule--helper clone: simplify path check

[Junio C Hamano]

Stefan Beller <sbeller@google.com> 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);
 

Re: [PATCH 2/4] submodule--helper clone: simplify path check

[Eric Sunshine]

On Thu, Mar 31, 2016 at 12:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> 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"));

[PATCH 3/4] submodule--helper clone: remove double path checking

[Stefan Beller]

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 <sbeller@google.com>
---
 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

Re: [PATCH 3/4] submodule--helper clone: remove double path checking

[Junio C Hamano]

Stefan Beller <sbeller@google.com> 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 <sbeller@google.com>
> ---
>  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");

[PATCH 4/4] submodule--helper: use relative path if possible

[Stefan Beller]

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 <sbeller@google.com>
---
 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

Re: [PATCH 4/4] submodule--helper: use relative path if possible

[Junio C Hamano]

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

Re: [PATCH 4/4] submodule--helper: use relative path if possible

[Stefan Beller]

On Thu, Mar 31, 2016 at 9:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> 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 <sbeller@google.com>
>> ---
>>  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".
>

Re: [PATCH 0/4] Fix relative path issues in recursive submodules.

[Junio C Hamano]

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

Re: [PATCH 0/4] Fix relative path issues in recursive submodules.

[Stefan Beller]

On Thu, Mar 31, 2016 at 10:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> 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.