threads / discuss / 41891

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

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

## tl;dr

12 messages between Mar 31, 2016 and Mar 31, 2016.

replies: 11people: 3as markdown or json

Stefan Beller· Mar 31, 2016, 21:04 UTC · lore

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

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
Stefan Beller· Mar 31, 2016, 21:04 UTC · re: Stefan Beller · lore

[PATCHv2 1/5] 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 <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
Stefan Beller· Mar 31, 2016, 21:04 UTC · re: Stefan Beller · lore

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
Eric Sunshine· Mar 31, 2016, 21:23 UTC · re: Stefan Beller · lore

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

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.
Show 27 quoted lines
> 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
Stefan Beller· Mar 31, 2016, 21:04 UTC · re: Stefan Beller · lore

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

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
Eric Sunshine· Mar 31, 2016, 21:58 UTC · re: Stefan Beller · lore

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

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.
Show 5 quoted lines
> 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.
Show 17 quoted lines
> 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
Junio C Hamano· Mar 31, 2016, 22:21 UTC · re: Eric Sunshine · lore

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

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"
Stefan Beller· Mar 31, 2016, 21:04 UTC · re: Stefan Beller · lore

[PATCHv2 4/5] 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 <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
Junio C Hamano· Mar 31, 2016, 22:26 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 5 quoted lines
> +	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?
Show 8 quoted lines
> @@ -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.
Show 24 quoted lines
> @@ -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.
Show 16 quoted lines
>  	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 &&
>  	(
Stefan Beller· Mar 31, 2016, 22:59 UTC · re: Junio C Hamano · lore

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

On Thu, Mar 31, 2016 at 3:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
> 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.
Show 55 quoted lines
>
>> @@ -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 &&
>>       (
Stefan Beller· Mar 31, 2016, 23:08 UTC · re: Stefan Beller · lore

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

On Thu, Mar 31, 2016 at 3:59 PM, Stefan Beller <sbeller@google.com> wrote:
Show 9 quoted lines
>
> 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
Stefan Beller· Mar 31, 2016, 21:04 UTC · re: Stefan Beller · lore

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

← back to recent threads