[PATCH] blame.c: fix garbled error message

[PATCH] blame.c: fix garbled error message

[Lukas Fleischer]

The helper functions prepare_final() and prepare_initial() return a
pointer to a string that is a member of an object in the revs->pending
array. This array is later rebuilt when running prepare_revision_walk()
which potentially transforms the pointer target into a bogus string. Fix
this by maintaining a copy of the original string.

Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
---
The bug manifests when running `git blame HEAD^ -- nonexistent.file`.

Note that I could have reduced code churn a little by moving the
xstrdup() invocations to the call sites. However, I think that the
return value of these functions should not depend on the consistency of
a volatile data structure. On the other hand, you might also argue that
there currently are only two call sites and that the functions are
marked static, so if you prefer the less intrusive version, please let
me know.

 builtin/blame.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 303e217..34d6f4f 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2390,7 +2390,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	return commit;
 }
 
-static const char *prepare_final(struct scoreboard *sb)
+static char *prepare_final(struct scoreboard *sb)
 {
 	int i;
 	const char *final_commit_name = NULL;
@@ -2415,10 +2415,10 @@ static const char *prepare_final(struct scoreboard *sb)
 		sb->final = (struct commit *) obj;
 		final_commit_name = revs->pending.objects[i].name;
 	}
-	return final_commit_name;
+	return xstrdup(final_commit_name);
 }
 
-static const char *prepare_initial(struct scoreboard *sb)
+static char *prepare_initial(struct scoreboard *sb)
 {
 	int i;
 	const char *final_commit_name = NULL;
@@ -2445,7 +2445,7 @@ static const char *prepare_initial(struct scoreboard *sb)
 	}
 	if (!final_commit_name)
 		die("No commit to dig down to?");
-	return final_commit_name;
+	return xstrdup(final_commit_name);
 }
 
 static int blame_copy_callback(const struct option *option, const char *arg, int unset)
@@ -2489,7 +2489,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	struct origin *o;
 	struct blame_entry *ent = NULL;
 	long dashdash_pos, lno;
-	const char *final_commit_name = NULL;
+	char *final_commit_name = NULL;
 	enum object_type type;
 
 	static struct string_list range_list;
@@ -2786,6 +2786,8 @@ parse_done:
 
 	assign_blame(&sb, opt);
 
+	free(final_commit_name);
+
 	if (incremental)
 		return 0;
 
-- 
2.2.1

Re: [PATCH] blame.c: fix garbled error message

[Junio C Hamano]

Lukas Fleischer <git@cryptocrack.de> writes:

> The helper functions prepare_final() and prepare_initial() return a
> pointer to a string that is a member of an object in the revs->pending
> array. This array is later rebuilt when running prepare_revision_walk()
> which potentially transforms the pointer target into a bogus string. Fix
> this by maintaining a copy of the original string.
>
> Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
> ---
> The bug manifests when running `git blame HEAD^ -- nonexistent.file`.

Before 1da1e07c (clean up name allocation in prepare_revision_walk,
2014-10-15), these strings used to be non-volatile; they were instead
leaked more or less deliberately.  But these days, these strings are
cleared, so your patch is absolutely the right thing to do.

Thanks for catching and fixing.  This fix needs to go to the 2.2.x
maintenance track.

> Note that I could have reduced code churn a little by moving the
> xstrdup() invocations to the call sites. However, I think that the
> return value of these functions should not depend on the consistency of
> a volatile data structure. On the other hand, you might also argue that
> there currently are only two call sites and that the functions are
> marked static, so if you prefer the less intrusive version, please let
> me know.

FWIW, I agree that this can be argued either way.

If we had a common low-level API that is used by short-term users to
grab these names, it would be reasonable to make it the callers'
responsibility to strdup() the return values for safekeeping if they
want to keep using them long after the function returns.  But I
agree that prepare_final/prepare_initial are not such a low-level
common API functions.  I do not care too much about who does the
strdup(), either the callers of prepare_* or the callee.

>
>  builtin/blame.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 303e217..34d6f4f 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2390,7 +2390,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
>  	return commit;
>  }
>  
> -static const char *prepare_final(struct scoreboard *sb)
> +static char *prepare_final(struct scoreboard *sb)
>  {
>  	int i;
>  	const char *final_commit_name = NULL;
> @@ -2415,10 +2415,10 @@ static const char *prepare_final(struct scoreboard *sb)
>  		sb->final = (struct commit *) obj;
>  		final_commit_name = revs->pending.objects[i].name;
>  	}
> -	return final_commit_name;
> +	return xstrdup(final_commit_name);
>  }
>  
> -static const char *prepare_initial(struct scoreboard *sb)
> +static char *prepare_initial(struct scoreboard *sb)
>  {
>  	int i;
>  	const char *final_commit_name = NULL;
> @@ -2445,7 +2445,7 @@ static const char *prepare_initial(struct scoreboard *sb)
>  	}
>  	if (!final_commit_name)
>  		die("No commit to dig down to?");
> -	return final_commit_name;
> +	return xstrdup(final_commit_name);
>  }
>  
>  static int blame_copy_callback(const struct option *option, const char *arg, int unset)
> @@ -2489,7 +2489,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  	struct origin *o;
>  	struct blame_entry *ent = NULL;
>  	long dashdash_pos, lno;
> -	const char *final_commit_name = NULL;
> +	char *final_commit_name = NULL;
>  	enum object_type type;
>  
>  	static struct string_list range_list;
> @@ -2786,6 +2786,8 @@ parse_done:
>  
>  	assign_blame(&sb, opt);
>  
> +	free(final_commit_name);
> +
>  	if (incremental)
>  		return 0;

Re: [PATCH] blame.c: fix garbled error message

[Jeff King]

On Mon, Jan 12, 2015 at 11:08:26AM -0800, Junio C Hamano wrote:

> Lukas Fleischer <git@cryptocrack.de> writes:
> 
> > The helper functions prepare_final() and prepare_initial() return a
> > pointer to a string that is a member of an object in the revs->pending
> > array. This array is later rebuilt when running prepare_revision_walk()
> > which potentially transforms the pointer target into a bogus string. Fix
> > this by maintaining a copy of the original string.
> >
> > Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
> > ---
> > The bug manifests when running `git blame HEAD^ -- nonexistent.file`.
> 
> Before 1da1e07c (clean up name allocation in prepare_revision_walk,
> 2014-10-15), these strings used to be non-volatile; they were instead
> leaked more or less deliberately.  But these days, these strings are
> cleared, so your patch is absolutely the right thing to do.
> 
> Thanks for catching and fixing.  This fix needs to go to the 2.2.x
> maintenance track.

Yeah, agreed. Sorry for not catching this as part of 1da1e07c.

I did a grep for 'pending.*name' to look at any other potential problem
sites. It looks like blame is the only one that tries to retain a
long-lived pointer to the name.

The other potentially interesting spot is that they are fed to the
object callbacks from traverse_commit_list for tags. However, none of
the callbacks saves it (and it would not make much sense to do so; they
also receive broken-down filenames in the same way, so if they want to
use it at all, they feed it through path_name() first, which makes a
copy).

So I think Lukas's patch fixes everything (and his positioning of the
strdup() calls is right where I would have put them).

-Peff

Re: [PATCH] blame.c: fix garbled error message

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Lukas Fleischer <git@cryptocrack.de> writes:
>
>> The helper functions prepare_final() and prepare_initial() return a
>> pointer to a string that is a member of an object in the revs->pending
>> array. This array is later rebuilt when running prepare_revision_walk()
>> which potentially transforms the pointer target into a bogus string. Fix
>> this by maintaining a copy of the original string.
>>
>> Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
>> ---
>> The bug manifests when running `git blame HEAD^ -- nonexistent.file`.
>
> Before 1da1e07c (clean up name allocation in prepare_revision_walk,
> 2014-10-15), these strings used to be non-volatile; they were instead
> leaked more or less deliberately.  But these days, these strings are
> cleared, so your patch is absolutely the right thing to do.
>
> Thanks for catching and fixing.  This fix needs to go to the 2.2.x
> maintenance track.

Sigh, but not so fast.

With the patch applied on top of 1da1e07c (or the result merged to
'next' for that matter), I see test breakages in many places "git
blame" is used, e.g. t7010.  Did you run the test suite?

This is because it is perfectly normal for prepare_final() to return
NULL.  Unconditionally running xstrdup() would of course fail.

>> Note that I could have reduced code churn a little by moving the
>> xstrdup() invocations to the call sites. However, I think that the
>> return value of these functions should not depend on the consistency of
>> a volatile data structure. On the other hand, you might also argue that
>> there currently are only two call sites and that the functions are
>> marked static, so if you prefer the less intrusive version, please let
>> me know.
>
> FWIW, I agree that this can be argued either way.
>
> If we had a common low-level API that is used by short-term users to
> grab these names, it would be reasonable to make it the callers'
> responsibility to strdup() the return values for safekeeping if they
> want to keep using them long after the function returns.  But I
> agree that prepare_final/prepare_initial are not such a low-level
> common API functions.  I do not care too much about who does the
> strdup(), either the callers of prepare_* or the callee.
>
>>
>>  builtin/blame.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> index 303e217..34d6f4f 100644
>> --- a/builtin/blame.c
>> +++ b/builtin/blame.c
>> @@ -2390,7 +2390,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
>>  	return commit;
>>  }
>>  
>> -static const char *prepare_final(struct scoreboard *sb)
>> +static char *prepare_final(struct scoreboard *sb)
>>  {
>>  	int i;
>>  	const char *final_commit_name = NULL;
>> @@ -2415,10 +2415,10 @@ static const char *prepare_final(struct scoreboard *sb)
>>  		sb->final = (struct commit *) obj;
>>  		final_commit_name = revs->pending.objects[i].name;
>>  	}
>> -	return final_commit_name;
>> +	return xstrdup(final_commit_name);
>>  }
>>  
>> -static const char *prepare_initial(struct scoreboard *sb)
>> +static char *prepare_initial(struct scoreboard *sb)
>>  {
>>  	int i;
>>  	const char *final_commit_name = NULL;
>> @@ -2445,7 +2445,7 @@ static const char *prepare_initial(struct scoreboard *sb)
>>  	}
>>  	if (!final_commit_name)
>>  		die("No commit to dig down to?");
>> -	return final_commit_name;
>> +	return xstrdup(final_commit_name);
>>  }
>>  
>>  static int blame_copy_callback(const struct option *option, const char *arg, int unset)
>> @@ -2489,7 +2489,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>>  	struct origin *o;
>>  	struct blame_entry *ent = NULL;
>>  	long dashdash_pos, lno;
>> -	const char *final_commit_name = NULL;
>> +	char *final_commit_name = NULL;
>>  	enum object_type type;
>>  
>>  	static struct string_list range_list;
>> @@ -2786,6 +2786,8 @@ parse_done:
>>  
>>  	assign_blame(&sb, opt);
>>  
>> +	free(final_commit_name);
>> +
>>  	if (incremental)
>>  		return 0;

Re: [PATCH] blame.c: fix garbled error message

[Jeff King]

On Mon, Jan 12, 2015 at 02:55:30PM -0800, Junio C Hamano wrote:

> With the patch applied on top of 1da1e07c (or the result merged to
> 'next' for that matter), I see test breakages in many places "git
> blame" is used, e.g. t7010.  Did you run the test suite?
> 
> This is because it is perfectly normal for prepare_final() to return
> NULL.  Unconditionally running xstrdup() would of course fail.

Eek. I even thought of this possibility and checked the prepare_initial
callsite, but not the prepare_final one.

As an aside, I have often been tempted to have xstrdup silently
propagate a NULL. It would have been the right thing to do here, but
maybe there are cases where the segfault is preferable for catching a
mistake early (otherwise you might store the NULL and then segfault much
later).

-Peff

Re: [PATCH] blame.c: fix garbled error message

[Lukas Fleischer]

On Mon, 12 Jan 2015 at 23:55:30, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Lukas Fleischer <git@cryptocrack.de> writes:
> >
> >> The helper functions prepare_final() and prepare_initial() return a
> >> pointer to a string that is a member of an object in the revs->pending
> >> array. This array is later rebuilt when running prepare_revision_walk()
> >> which potentially transforms the pointer target into a bogus string. Fix
> >> this by maintaining a copy of the original string.
> >>
> >> Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
> >> ---
> >> The bug manifests when running `git blame HEAD^ -- nonexistent.file`.
> >
> > Before 1da1e07c (clean up name allocation in prepare_revision_walk,
> > 2014-10-15), these strings used to be non-volatile; they were instead
> > leaked more or less deliberately.  But these days, these strings are
> > cleared, so your patch is absolutely the right thing to do.
> >
> > Thanks for catching and fixing.  This fix needs to go to the 2.2.x
> > maintenance track.
> 
> Sigh, but not so fast.
> 
> With the patch applied on top of 1da1e07c (or the result merged to
> 'next' for that matter), I see test breakages in many places "git
> blame" is used, e.g. t7010.  Did you run the test suite?
> 

No, I didn't.

> This is because it is perfectly normal for prepare_final() to return
> NULL.  Unconditionally running xstrdup() would of course fail.
> [...]

Something like

    return final_commit_name ? xstrdup(final_commit_name) : NULL;

should work, though, right? Calling free() with a null pointer is fine,
so there is nothing else to do here. You can either amend those two
lines or wait for me to resubmit v2 of the patch tomorrow :)

Re: [PATCH] blame.c: fix garbled error message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> As an aside, I have often been tempted to have xstrdup silently
> propagate a NULL. It would have been the right thing to do here, but
> maybe there are cases where the segfault is preferable for catching a
> mistake early (otherwise you might store the NULL and then segfault much
> later).

Great minds think alike.  The sentence after "but maybe ..." was
what I had in mind as a response in anticipation that somebody might
suggest that; a separate xstrdup_or_null() might be fine, but I'd
rather not to have xstrdup() that is _too_ magical.

Re: [PATCH] blame.c: fix garbled error message

[Jeff King]

On Mon, Jan 12, 2015 at 04:11:06PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > As an aside, I have often been tempted to have xstrdup silently
> > propagate a NULL. It would have been the right thing to do here, but
> > maybe there are cases where the segfault is preferable for catching a
> > mistake early (otherwise you might store the NULL and then segfault much
> > later).
> 
> Great minds think alike.  The sentence after "but maybe ..." was
> what I had in mind as a response in anticipation that somebody might
> suggest that; a separate xstrdup_or_null() might be fine, but I'd
> rather not to have xstrdup() that is _too_ magical.

Yeah. Of course, it is not _that_ many more characters to do a ternary
conditional. I guess the main benefit is that you do not have to repeat
the name of the variable (which lets you reuse a function result
directly, avoiding an explicit temporary).

Here's my attempt. Some cases are a little nicer, but overall, it does
not feel significantly more readable to me. I dunno. I could go either
way. I stuck Lukas's patch on top (modified to use xstrdup_or_null), if
we do want to go that route. Otherwise it needs the ?: treatment.

  [1/5]: git-compat-util: add xstrdup_or_null helper
  [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup
  [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup
  [4/5]: use xstrdup_or_null to replace ternary conditionals
  [5/5]: blame.c: fix garbled error message

[PATCH 1/5] git-compat-util: add xstrdup_or_null helper

[Jeff King]

It's a common idiom to duplicate a string if it is non-NULL,
or pass a literal NULL through. This is already a one-liner
in C, but you do have to repeat the name of the string
twice. So if there's a function call, you must write:

  const char *x = some_fun(...);
  return x ? xstrdup(x) : NULL;

instead of (with this patch) just:

  return xstrdup_or_null(some_fun(...));

Signed-off-by: Jeff King <peff@peff.net>
---
This example is the heart of the readability question to me. It is nice
to avoid the temporary, which makes the code more direct. But it also
sticks some_fun(...) inside another function, which is a little harder
to read.

 git-compat-util.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index dcecd85..8157eb2 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -675,6 +675,11 @@ extern char *xgetcwd(void);
 
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
 
+static inline char *xstrdup_or_null(const char *str)
+{
+	return str ? xstrdup(str) : NULL;
+}
+
 static inline size_t xsize_t(off_t len)
 {
 	if (len > (size_t) len)
-- 
2.2.1.425.g441bb3c

[PATCH 2/5] builtin/apply.c: use xstrdup_or_null instead of null_strdup

[Jeff King]

This file had its own identical helper that predates
xstrdup_or_null. Let's use the global one to avoid
repetition.

Signed-off-by: Jeff King <peff@peff.net>
---
You'll note that every one of these could just be a ?:-conditional
(there are no function calls).

 builtin/apply.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 0aad912..dfd7a34 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -657,11 +657,6 @@ static size_t diff_timestamp_len(const char *line, size_t len)
 	return line + len - end;
 }
 
-static char *null_strdup(const char *s)
-{
-	return s ? xstrdup(s) : NULL;
-}
-
 static char *find_name_common(const char *line, const char *def,
 			      int p_value, const char *end, int terminate)
 {
@@ -684,10 +679,10 @@ static char *find_name_common(const char *line, const char *def,
 			start = line;
 	}
 	if (!start)
-		return squash_slash(null_strdup(def));
+		return squash_slash(xstrdup_or_null(def));
 	len = line - start;
 	if (!len)
-		return squash_slash(null_strdup(def));
+		return squash_slash(xstrdup_or_null(def));
 
 	/*
 	 * Generally we prefer the shorter name, especially
@@ -909,7 +904,7 @@ static void parse_traditional_patch(const char *first, const char *second, struc
 			patch->old_name = name;
 		} else {
 			patch->old_name = name;
-			patch->new_name = null_strdup(name);
+			patch->new_name = xstrdup_or_null(name);
 		}
 	}
 	if (!name)
@@ -998,7 +993,7 @@ static int gitdiff_delete(const char *line, struct patch *patch)
 {
 	patch->is_delete = 1;
 	free(patch->old_name);
-	patch->old_name = null_strdup(patch->def_name);
+	patch->old_name = xstrdup_or_null(patch->def_name);
 	return gitdiff_oldmode(line, patch);
 }
 
@@ -1006,7 +1001,7 @@ static int gitdiff_newfile(const char *line, struct patch *patch)
 {
 	patch->is_new = 1;
 	free(patch->new_name);
-	patch->new_name = null_strdup(patch->def_name);
+	patch->new_name = xstrdup_or_null(patch->def_name);
 	return gitdiff_newmode(line, patch);
 }
 
-- 
2.2.1.425.g441bb3c

[PATCH 3/5] builtin/commit.c: use xstrdup_or_null instead of envdup

[Jeff King]

The only reason for envdup to be its own function is that we
have to save the result in a temporary string. With
xstrdup_or_null, we can feed the result of getenv()
directly.

Signed-off-by: Jeff King <peff@peff.net>
---
These ones need the temporary. Is the result more readable, or less?

 builtin/commit.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 7d90c35..5cd1478 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -559,20 +559,14 @@ static void set_ident_var(char **buf, char *val)
 	*buf = val;
 }
 
-static char *envdup(const char *var)
-{
-	const char *val = getenv(var);
-	return val ? xstrdup(val) : NULL;
-}
-
 static void determine_author_info(struct strbuf *author_ident)
 {
 	char *name, *email, *date;
 	struct ident_split author;
 
-	name = envdup("GIT_AUTHOR_NAME");
-	email = envdup("GIT_AUTHOR_EMAIL");
-	date = envdup("GIT_AUTHOR_DATE");
+	name = xstrdup_or_null(getenv("GIT_AUTHOR_NAME"));
+	email = xstrdup_or_null(getenv("GIT_AUTHOR_EMAIL"));
+	date = xstrdup_or_null(getenv("GIT_AUTHOR_DATE"));
 
 	if (author_message) {
 		struct ident_split ident;
-- 
2.2.1.425.g441bb3c

[PATCH 4/5] use xstrdup_or_null to replace ternary conditionals

[Jeff King]

This replaces "x ? xstrdup(x) : NULL" with xstrdup_or_null(x).
The change is fairly mechanical, with the exception of
resolve_refdup, which can eliminate a temporary variable.

There are still a few hits grepping for "?.*xstrdup", but
these are of slightly different forms and cannot be
converted (e.g., "x ? xstrdup(x->foo) : NULL").

Signed-off-by: Jeff King <peff@peff.net>
---
resolve_refdup is the interesting one to look at for readability here.

 config.c  | 2 +-
 grep.c    | 4 ++--
 notes.c   | 2 +-
 refs.c    | 3 +--
 remote.c  | 4 ++--
 shallow.c | 2 +-
 walker.c  | 2 +-
 7 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/config.c b/config.c
index 752e2e2..e5e64dc 100644
--- a/config.c
+++ b/config.c
@@ -1340,7 +1340,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
 		string_list_init(&e->value_list, 1);
 		hashmap_add(&cs->config_hash, e);
 	}
-	si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+	si = string_list_append_nodup(&e->value_list, xstrdup_or_null(value));
 
 	ALLOC_GROW(cs->list.items, cs->list.nr + 1, cs->list.alloc);
 	l_item = &cs->list.items[cs->list.nr++];
diff --git a/grep.c b/grep.c
index 6e085f8..b58c7c6 100644
--- a/grep.c
+++ b/grep.c
@@ -1661,8 +1661,8 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
 		      const void *identifier)
 {
 	gs->type = type;
-	gs->name = name ? xstrdup(name) : NULL;
-	gs->path = path ? xstrdup(path) : NULL;
+	gs->name = xstrdup_or_null(name);
+	gs->path = xstrdup_or_null(path);
 	gs->buf = NULL;
 	gs->size = 0;
 	gs->driver = NULL;
diff --git a/notes.c b/notes.c
index c763a21..2be4d7f 100644
--- a/notes.c
+++ b/notes.c
@@ -1006,7 +1006,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	t->root = (struct int_node *) xcalloc(1, sizeof(struct int_node));
 	t->first_non_note = NULL;
 	t->prev_non_note = NULL;
-	t->ref = notes_ref ? xstrdup(notes_ref) : NULL;
+	t->ref = xstrdup_or_null(notes_ref);
 	t->combine_notes = combine_notes;
 	t->initialized = 1;
 	t->dirty = 0;
diff --git a/refs.c b/refs.c
index 5fcacc6..78fec5c 100644
--- a/refs.c
+++ b/refs.c
@@ -1618,8 +1618,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned
 
 char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags)
 {
-	const char *ret = resolve_ref_unsafe(ref, resolve_flags, sha1, flags);
-	return ret ? xstrdup(ret) : NULL;
+	return xstrdup_or_null(resolve_ref_unsafe(ref, resolve_flags, sha1, flags));
 }
 
 /* The argument to filter_refs */
diff --git a/remote.c b/remote.c
index 5b9c693..7b71ebf 100644
--- a/remote.c
+++ b/remote.c
@@ -975,8 +975,8 @@ struct ref *copy_ref(const struct ref *ref)
 	cpy = xmalloc(sizeof(struct ref) + len + 1);
 	memcpy(cpy, ref, sizeof(struct ref) + len + 1);
 	cpy->next = NULL;
-	cpy->symref = ref->symref ? xstrdup(ref->symref) : NULL;
-	cpy->remote_status = ref->remote_status ? xstrdup(ref->remote_status) : NULL;
+	cpy->symref = xstrdup_or_null(ref->symref);
+	cpy->remote_status = xstrdup_or_null(ref->remote_status);
 	cpy->peer_ref = copy_ref(ref->peer_ref);
 	return cpy;
 }
diff --git a/shallow.c b/shallow.c
index cdd0775..f5e6720 100644
--- a/shallow.c
+++ b/shallow.c
@@ -22,7 +22,7 @@ void set_alternate_shallow_file(const char *path, int override)
 	if (alternate_shallow_file && !override)
 		return;
 	free(alternate_shallow_file);
-	alternate_shallow_file = path ? xstrdup(path) : NULL;
+	alternate_shallow_file = xstrdup_or_null(path);
 }
 
 int register_shallow(const unsigned char *sha1)
diff --git a/walker.c b/walker.c
index f149371..483da4e 100644
--- a/walker.c
+++ b/walker.c
@@ -232,7 +232,7 @@ int walker_targets_stdin(char ***target, const char ***write_ref)
 			REALLOC_ARRAY(*write_ref, targets_alloc);
 		}
 		(*target)[targets] = xstrdup(tg_one);
-		(*write_ref)[targets] = rf_one ? xstrdup(rf_one) : NULL;
+		(*write_ref)[targets] = xstrdup_or_null(rf_one);
 		targets++;
 	}
 	strbuf_release(&buf);
-- 
2.2.1.425.g441bb3c

[PATCH 5/5] blame.c: fix garbled error message

[Jeff King]

From: Lukas Fleischer <git@cryptocrack.de>

The helper functions prepare_final() and prepare_initial() return a
pointer to a string that is a member of an object in the revs->pending
array. This array is later rebuilt when running prepare_revision_walk()
which potentially transforms the pointer target into a bogus string. Fix
this by maintaining a copy of the original string.

Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/blame.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 303e217..0374fe8 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2390,7 +2390,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	return commit;
 }
 
-static const char *prepare_final(struct scoreboard *sb)
+static char *prepare_final(struct scoreboard *sb)
 {
 	int i;
 	const char *final_commit_name = NULL;
@@ -2415,10 +2415,10 @@ static const char *prepare_final(struct scoreboard *sb)
 		sb->final = (struct commit *) obj;
 		final_commit_name = revs->pending.objects[i].name;
 	}
-	return final_commit_name;
+	return xstrdup_or_null(final_commit_name);
 }
 
-static const char *prepare_initial(struct scoreboard *sb)
+static char *prepare_initial(struct scoreboard *sb)
 {
 	int i;
 	const char *final_commit_name = NULL;
@@ -2445,7 +2445,7 @@ static const char *prepare_initial(struct scoreboard *sb)
 	}
 	if (!final_commit_name)
 		die("No commit to dig down to?");
-	return final_commit_name;
+	return xstrdup(final_commit_name);
 }
 
 static int blame_copy_callback(const struct option *option, const char *arg, int unset)
@@ -2489,7 +2489,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	struct origin *o;
 	struct blame_entry *ent = NULL;
 	long dashdash_pos, lno;
-	const char *final_commit_name = NULL;
+	char *final_commit_name = NULL;
 	enum object_type type;
 
 	static struct string_list range_list;
@@ -2786,6 +2786,8 @@ parse_done:
 
 	assign_blame(&sb, opt);
 
+	free(final_commit_name);
+
 	if (incremental)
 		return 0;
 
-- 
2.2.1.425.g441bb3c

Re: [PATCH 1/5] git-compat-util: add xstrdup_or_null helper

[Jonathan Nieder]

Jeff King wrote:

> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -675,6 +675,11 @@ extern char *xgetcwd(void);
>  
>  #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
>  
> +static inline char *xstrdup_or_null(const char *str)
> +{
> +	return str ? xstrdup(str) : NULL;
> +}

Would it make sense for xstrdup to always include the NULL check,
avoiding the need for the more verbose xstrdup_or_null?

Jonathan

diff --git i/wrapper.c w/wrapper.c
index 007ec0d..5a835e8 100644
--- i/wrapper.c
+++ w/wrapper.c
@@ -40,7 +40,11 @@ try_to_free_t set_try_to_free_routine(try_to_free_t routine)
 
 char *xstrdup(const char *str)
 {
-	char *ret = strdup(str);
+	char *ret;
+
+	if (!str)
+		return NULL;
+	ret = strdup(str);
 	if (!ret) {
 		try_to_free_routine(strlen(str) + 1);
 		ret = strdup(str);
@@ -125,7 +129,11 @@ void *xmemdupz(const void *data, size_t len)
 
 char *xstrndup(const char *str, size_t len)
 {
-	char *p = memchr(str, '\0', len);
+	char *p;
+
+	if (!str)
+		return NULL;
+	p = memchr(str, '\0', len);
 	return xmemdupz(str, p ? p - str : len);
 }
 

Re: [PATCH 1/5] git-compat-util: add xstrdup_or_null helper

[Jeff King]

On Mon, Jan 12, 2015 at 06:21:19PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -675,6 +675,11 @@ extern char *xgetcwd(void);
> >  
> >  #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
> >  
> > +static inline char *xstrdup_or_null(const char *str)
> > +{
> > +	return str ? xstrdup(str) : NULL;
> > +}
> 
> Would it make sense for xstrdup to always include the NULL check,
> avoiding the need for the more verbose xstrdup_or_null?

Read the rest of the thread. :)

-Peff

Re: [PATCH] blame.c: fix garbled error message

[Lukas Fleischer]

On Tue, 13 Jan 2015 at 02:54:27, Jeff King wrote:
> On Mon, Jan 12, 2015 at 04:11:06PM -0800, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > As an aside, I have often been tempted to have xstrdup silently
> > > propagate a NULL. It would have been the right thing to do here, but
> > > maybe there are cases where the segfault is preferable for catching a
> > > mistake early (otherwise you might store the NULL and then segfault much
> > > later).
> > 
> > Great minds think alike.  The sentence after "but maybe ..." was
> > what I had in mind as a response in anticipation that somebody might
> > suggest that; a separate xstrdup_or_null() might be fine, but I'd
> > rather not to have xstrdup() that is _too_ magical.
> 
> Yeah. Of course, it is not _that_ many more characters to do a ternary
> conditional. I guess the main benefit is that you do not have to repeat
> the name of the variable (which lets you reuse a function result
> directly, avoiding an explicit temporary).
> 
> Here's my attempt. Some cases are a little nicer, but overall, it does
> not feel significantly more readable to me. I dunno. I could go either
> way. I stuck Lukas's patch on top (modified to use xstrdup_or_null), if
> we do want to go that route. Otherwise it needs the ?: treatment.
> 
>   [1/5]: git-compat-util: add xstrdup_or_null helper
>   [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup
>   [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup
>   [4/5]: use xstrdup_or_null to replace ternary conditionals
>   [5/5]: blame.c: fix garbled error message
> 

Looks good to me! I am not sure whether those patches should be built on
top of (a fixed version of) my patch, though, which would make
backporting the fix to the maintenance branch straightforward. Junio?

Re: [PATCH] blame.c: fix garbled error message

[Junio C Hamano]

Lukas Fleischer <git@cryptocrack.de> writes:

>>   [1/5]: git-compat-util: add xstrdup_or_null helper
>>   [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup
>>   [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup
>>   [4/5]: use xstrdup_or_null to replace ternary conditionals
>>   [5/5]: blame.c: fix garbled error message
>> 
>
> Looks good to me! I am not sure whether those patches should be built on
> top of (a fixed version of) my patch, though, which would make
> backporting the fix to the maintenance branch straightforward. Junio?

We can queue these five on top of 1da1e07c (clean up name allocation
in prepare_revision_walk, 2014-10-15), which changed the rule of the
game to break this code, that only is in v2.2 and later.

And the result should merge just fine to 'maint'.

Re: [PATCH] blame.c: fix garbled error message

[Jeff King]

On Wed, Jan 14, 2015 at 09:22:52AM -0800, Junio C Hamano wrote:

> Lukas Fleischer <git@cryptocrack.de> writes:
> 
> >>   [1/5]: git-compat-util: add xstrdup_or_null helper
> >>   [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup
> >>   [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup
> >>   [4/5]: use xstrdup_or_null to replace ternary conditionals
> >>   [5/5]: blame.c: fix garbled error message
> >> 
> >
> > Looks good to me! I am not sure whether those patches should be built on
> > top of (a fixed version of) my patch, though, which would make
> > backporting the fix to the maintenance branch straightforward. Junio?
> 
> We can queue these five on top of 1da1e07c (clean up name allocation
> in prepare_revision_walk, 2014-10-15), which changed the rule of the
> game to break this code, that only is in v2.2 and later.
> 
> And the result should merge just fine to 'maint'.

Are we in agreement then that the resulting code with the helper is
actually easier to read? I think replacing the straight ?: lines is, but
I am on the fence on whether:

  const char *x = some_fun(...);
  return xstrdup_or_null(x);

is better or worse than:

  return xstrdup_or_null(some_fun(....));

-Peff

Re: [PATCH] blame.c: fix garbled error message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 14, 2015 at 09:22:52AM -0800, Junio C Hamano wrote:
> ...
>> And the result should merge just fine to 'maint'.
>
> Are we in agreement then that the resulting code with the helper is
> actually easier to read? I think replacing the straight ?: lines is, but
> I am on the fence on whether:
>
>   const char *x = some_fun(...);
>   return xstrdup_or_null(x);
>
> is better or worse than:
>
>   return xstrdup_or_null(some_fun(....));

I think the latter is fine as long as some_fun(...) invocation does
not get overly long, and even the longest I saw in refs.c, i.e.

        return xstrdup_or_null(resolve_ref_unsafe(ref, resolve_flags, sha1, flags));

did not bother me too much.