[PATCHv2 0/5] Fix relative path issues in recursive submodules.

[PATCHv2 0/5] Fix relative path issues in recursive submodules.

[Stefan Beller]

Thanks Junio, Eric and Jacob for review!

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 (5):
  recursive submodules: test for relative paths
  submodule--helper clone: simplify path check
  submodule--helper clone: remove double path checking
  submodule--helper, module_clone: always operate on absolute paths
  submodule--helper, module_clone: catch fprintf failure

 builtin/submodule--helper.c | 52 +++++++++++++++++++++++++--------------------
 t/t7400-submodule-basic.sh  | 41 +++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 23 deletions(-)

-- 
2.5.0.264.g39f00fe

[PATCHv2 1/5] recursive submodules: test for relative paths

[Stefan Beller]

"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 <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.g39f00fe

[PATCHv2 2/5] 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. That means the `else` part is dead code, so remove it.

To make the code futureproof, add a check for path being NULL or empty
and report the error accordingly.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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 f4c3eff..4f0b002 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);
-- 
2.5.0.264.g39f00fe

Re: [PATCHv2 2/5] submodule--helper clone: simplify path check

[Eric Sunshine]

On Thu, Mar 31, 2016 at 5:04 PM, Stefan Beller <sbeller@google.com> wrote:
> submodule--helper clone: simplify path check

I don't see anything in the patch which simplifies a path check.
Instead, this version of the patch is now fixing a potential
NULL-dereference.

> 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. That means the `else` part is dead code, so remove it.

The above description has very little if anything to do with what this
patch is addressing considering that this is now a crash-fix patch.
While it's true that the (current) shell code won't trigger this
crash, that's not particularly interesting or relevant.

> To make the code futureproof, add a check for path being NULL or empty
> and report the error accordingly.

Selling this as a future-proofing change is misleading; it's a crash
fix, plain and simple. I think the entire commit message could be
collapsed to something like this:

    submodule--helper: fix potential NULL-dereference

    Don't dereference NULL 'path' if it was never assigned. While
    here also protect against empty --path argument.

You don't even need to mention removal of the conditional in the
second hunk since, for anyone reading the patch, that's an obvious
consequence of adding the new check in the first hunk.

The patch itself looks fine.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git 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);
> --
> 2.5.0.264.g39f00fe

[PATCHv2 3/5] submodule--helper clone: remove double path checking

[Stefan Beller]

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 <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 4f0b002..0b9f546 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");
-- 
2.5.0.264.g39f00fe

Re: [PATCHv2 3/5] submodule--helper clone: remove double path checking

[Eric Sunshine]

On Thu, Mar 31, 2016 at 5:04 PM, Stefan Beller <sbeller@google.com> wrote:
> submodule--helper clone: remove double path checking

I think Junio mentioned in v1 that calling this "path checking" is misleading.

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

This part of the commit message is nicely improved.

The patch itself looks fine.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git 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");
> --
> 2.5.0.264.g39f00fe

Re: [PATCHv2 3/5] submodule--helper clone: remove double path checking

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Mar 31, 2016 at 5:04 PM, Stefan Beller <sbeller@google.com> wrote:
>> submodule--helper clone: remove double path checking
>
> I think Junio mentioned in v1 that calling this "path checking" is misleading.

I'd tentatively use:

    "submodule--helper clone: create the submodule path just once"

[PATCHv2 4/5] 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>
---

Notes:
    Notice how the 2 relative path calls
      relative_path(sm_gitdir, path, &rel_path)
      relative_path(path, sm_gitdir, &rel_path)
    now have the same arguments, just switched?
    The symmetry there looks nice to read.
    
    This proposal is slightly more code than the previous patch,
    but looking at the end result

 builtin/submodule--helper.c | 36 +++++++++++++++++++++++-------------
 t/t7400-submodule-basic.sh  |  2 +-
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0b9f546..56c3033 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -157,8 +157,8 @@ 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, *cwd, *p;
-	struct strbuf rel_path = STRBUF_INIT;
+	char *sm_gitdir, *p;
+	struct strbuf rel_path = STRBUF_INIT; /* for relative_path */
 	struct strbuf sb = STRBUF_INIT;
 
 	struct option module_clone_options[] = {
@@ -200,6 +200,25 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	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);
+	}
+
+	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);
+		path = strbuf_detach(&sb, 0);
+	}
+
 	if (!file_exists(sm_gitdir)) {
 		if (safe_create_leading_directories_const(sm_gitdir) < 0)
 			die(_("could not create directory '%s'"), sm_gitdir);
@@ -221,7 +240,6 @@ 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(submodule_dot_git, "gitdir: %s\n",
 		relative_path(sm_gitdir, path, &rel_path));
 	if (fclose(submodule_dot_git))
@@ -229,24 +247,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(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.g39f00fe

Re: [PATCHv2 4/5] submodule--helper, module_clone: always operate on absolute paths

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +	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);

It would be surprising that this would be the first codepath that
needs to get an absolute pathname in the codebase that is more than
10 years old, wouldn't it?  Don't we have a reasonable API helper
function to do this kind of thing already?

    ... goes and looks ...

Doesn't absolute_path() work here?

> @@ -221,7 +240,6 @@ 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(submodule_dot_git, "gitdir: %s\n",
>  		relative_path(sm_gitdir, path, &rel_path));
>  	if (fclose(submodule_dot_git))

Looks like an unrelated change to me.

> @@ -229,24 +247,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);
> +

This addition of blank, too.

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

Re: [PATCHv2 4/5] submodule--helper, module_clone: always operate on absolute paths

[Stefan Beller]

On Thu, Mar 31, 2016 at 3:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +     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);
>
> It would be surprising that this would be the first codepath that
> needs to get an absolute pathname in the codebase that is more than
> 10 years old, wouldn't it?  Don't we have a reasonable API helper
> function to do this kind of thing already?
>
>     ... goes and looks ...
>
> Doesn't absolute_path() work here?

*reads absolute_path(...)*

    Yes that is great. But why is it written the way it is?
    I would have expected:

 const char *absolute_path(const char *path)
 {
        static struct strbuf sb = STRBUF_INIT;
-       strbuf_reset(&sb);
        strbuf_add_absolute_path(&sb, path);
-       return sb.buf;
+       return strbuf_detach(&sb);
 }

Further checking reveals any caller uses it with

    desired= xstrdup(absolute_path(my_argument));

We are loosing memory of the strbuf here. so if I we'd
take the diff above we can also get rid of all the xstrdups
at the callers. For now I will adhere to all other callers and use
xstrdup(absolute_path(...) here too.

I'll remove the unrelated changes as well in a resend.

>
>> @@ -221,7 +240,6 @@ 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(submodule_dot_git, "gitdir: %s\n",
>>               relative_path(sm_gitdir, path, &rel_path));
>>       if (fclose(submodule_dot_git))
>
> Looks like an unrelated change to me.
>
>> @@ -229,24 +247,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);
>> +
>
> This addition of blank, too.
>
>>       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 &&
>>       (

Re: [PATCHv2 4/5] submodule--helper, module_clone: always operate on absolute paths

[Stefan Beller]

On Thu, Mar 31, 2016 at 3:59 PM, Stefan Beller <sbeller@google.com> wrote:
>
> Further checking reveals any caller uses it with
>
>     desired= xstrdup(absolute_path(my_argument));
>
> We are loosing memory of the strbuf here. so if I we'd
> take the diff above we can also get rid of all the xstrdups
> at the callers. For now I will adhere to all other callers and use
> xstrdup(absolute_path(...) here too.

Actually there is only two occurrences of xstrdup in builtin/clone.c

[PATCHv2 5/5] 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 56c3033..89cbbda 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -240,8 +240,8 @@ 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(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.g39f00fe