{"thread":{"id":"38335","subject":"[PATCH] blame.c: fix garbled error message","startedAt":"2015-01-10T21:33:21Z","lastAt":"2015-01-14T21:54:26Z","messageCount":19,"participants":["Lukas Fleischer","Junio C Hamano","Jeff King","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"254540","messageId":"1420925601-21615-1-git-send-email-git@cryptocrack.de","threadId":"38335","inReplyTo":null,"subject":"[PATCH] blame.c: fix garbled error message","fromName":"Lukas Fleischer","fromEmail":"git@cryptocrack.de","sentAt":"2015-01-10T21:33:21Z","receivedAt":"2015-01-10T21:33:21Z","isPatch":true,"sender":{"key":"git@cryptocrack.de","avatar":null},"body":"The helper functions prepare_final() and prepare_initial() return a\npointer to a string that is a member of an object in the revs->pending\narray. This array is later rebuilt when running prepare_revision_walk()\nwhich potentially transforms the pointer target into a bogus string. Fix\nthis by maintaining a copy of the original string.\n\nSigned-off-by: Lukas Fleischer \n---\nThe bug manifests when running `git blame HEAD^ -- nonexistent.file`.\n\nNote that I could have reduced code churn a little by moving the\nxstrdup() invocations to the call sites. However, I think that the\nreturn value of these functions should not depend on the consistency of\na volatile data structure. On the other hand, you might also argue that\nthere currently are only two call sites and that the functions are\nmarked static, so if you prefer the less intrusive version, please let\nme know.\n\n builtin/blame.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 303e217..34d6f4f 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2390,7 +2390,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \treturn commit;\n }\n \n-static const char *prepare_final(struct scoreboard *sb)\n+static char *prepare_final(struct scoreboard *sb)\n {\n \tint i;\n \tconst char *final_commit_name = NULL;\n@@ -2415,10 +2415,10 @@ static const char *prepare_final(struct scoreboard *sb)\n \t\tsb->final = (struct commit *) obj;\n \t\tfinal_commit_name = revs->pending.objects[i].name;\n \t}\n-\treturn final_commit_name;\n+\treturn xstrdup(final_commit_name);\n }\n \n-static const char *prepare_initial(struct scoreboard *sb)\n+static char *prepare_initial(struct scoreboard *sb)\n {\n \tint i;\n \tconst char *final_commit_name = NULL;\n@@ -2445,7 +2445,7 @@ static const char *prepare_initial(struct scoreboard *sb)\n \t}\n \tif (!final_commit_name)\n \t\tdie(\"No commit to dig down to?\");\n-\treturn final_commit_name;\n+\treturn xstrdup(final_commit_name);\n }\n \n static int blame_copy_callback(const struct option *option, const char *arg, int unset)\n@@ -2489,7 +2489,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tstruct origin *o;\n \tstruct blame_entry *ent = NULL;\n \tlong dashdash_pos, lno;\n-\tconst char *final_commit_name = NULL;\n+\tchar *final_commit_name = NULL;\n \tenum object_type type;\n \n \tstatic struct string_list range_list;\n@@ -2786,6 +2786,8 @@ parse_done:\n \n \tassign_blame(&sb, opt);\n \n+\tfree(final_commit_name);\n+\n \tif (incremental)\n \t\treturn 0;\n \n-- \n2.2.1\n"},{"id":"254556","messageId":"xmqq4mrv95qt.fsf@gitster.dls.corp.google.com","threadId":"38335","inReplyTo":"1420925601-21615-1-git-send-email-git@cryptocrack.de","subject":"Re: [PATCH] blame.c: fix garbled error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-12T19:08:26Z","receivedAt":"2015-01-12T19:08:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lukas Fleischer writes:\n\n> The helper functions prepare_final() and prepare_initial() return a\n> pointer to a string that is a member of an object in the revs->pending\n> array. This array is later rebuilt when running prepare_revision_walk()\n> which potentially transforms the pointer target into a bogus string. Fix\n> this by maintaining a copy of the original string.\n>\n> Signed-off-by: Lukas Fleischer \n> ---\n> The bug manifests when running `git blame HEAD^ -- nonexistent.file`.\n\nBefore 1da1e07c (clean up name allocation in prepare_revision_walk,\n2014-10-15), these strings used to be non-volatile; they were instead\nleaked more or less deliberately. But these days, these strings are\ncleared, so your patch is absolutely the right thing to do.\n\nThanks for catching and fixing. This fix needs to go to the 2.2.x\nmaintenance track.\n\n> Note that I could have reduced code churn a little by moving the\n> xstrdup() invocations to the call sites. However, I think that the\n> return value of these functions should not depend on the consistency of\n> a volatile data structure. On the other hand, you might also argue that\n> there currently are only two call sites and that the functions are\n> marked static, so if you prefer the less intrusive version, please let\n> me know.\n\nFWIW, I agree that this can be argued either way.\n\nIf we had a common low-level API that is used by short-term users to\ngrab these names, it would be reasonable to make it the callers'\nresponsibility to strdup() the return values for safekeeping if they\nwant to keep using them long after the function returns. But I\nagree that prepare_final/prepare_initial are not such a low-level\ncommon API functions. I do not care too much about who does the\nstrdup(), either the callers of prepare_* or the callee.\n\n>\n> builtin/blame.c | 12 +++++++-----\n> 1 file changed, 7 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 303e217..34d6f4f 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2390,7 +2390,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n> \treturn commit;\n> }\n> \n> -static const char *prepare_final(struct scoreboard *sb)\n> +static char *prepare_final(struct scoreboard *sb)\n> {\n> \tint i;\n> \tconst char *final_commit_name = NULL;\n> @@ -2415,10 +2415,10 @@ static const char *prepare_final(struct scoreboard *sb)\n> \t\tsb->final = (struct commit *) obj;\n> \t\tfinal_commit_name = revs->pending.objects[i].name;\n> \t}\n> -\treturn final_commit_name;\n> +\treturn xstrdup(final_commit_name);\n> }\n> \n> -static const char *prepare_initial(struct scoreboard *sb)\n> +static char *prepare_initial(struct scoreboard *sb)\n> {\n> \tint i;\n> \tconst char *final_commit_name = NULL;\n> @@ -2445,7 +2445,7 @@ static const char *prepare_initial(struct scoreboard *sb)\n> \t}\n> \tif (!final_commit_name)\n> \t\tdie(\"No commit to dig down to?\");\n> -\treturn final_commit_name;\n> +\treturn xstrdup(final_commit_name);\n> }\n> \n> static int blame_copy_callback(const struct option *option, const char *arg, int unset)\n> @@ -2489,7 +2489,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n> \tstruct origin *o;\n> \tstruct blame_entry *ent = NULL;\n> \tlong dashdash_pos, lno;\n> -\tconst char *final_commit_name = NULL;\n> +\tchar *final_commit_name = NULL;\n> \tenum object_type type;\n> \n> \tstatic struct string_list range_list;\n> @@ -2786,6 +2786,8 @@ parse_done:\n> \n> \tassign_blame(&sb, opt);\n> \n> +\tfree(final_commit_name);\n> +\n> \tif (incremental)\n> \t\treturn 0;\n"},{"id":"254558","messageId":"20150112204025.GB9686@peff.net","threadId":"38335","inReplyTo":"xmqq4mrv95qt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] blame.c: fix garbled error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-12T20:40:26Z","receivedAt":"2015-01-12T20:40:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 12, 2015 at 11:08:26AM -0800, Junio C Hamano wrote:\n\n> Lukas Fleischer writes:\n> \n> > The helper functions prepare_final() and prepare_initial() return a\n> > pointer to a string that is a member of an object in the revs->pending\n> > array. This array is later rebuilt when running prepare_revision_walk()\n> > which potentially transforms the pointer target into a bogus string. Fix\n> > this by maintaining a copy of the original string.\n> >\n> > Signed-off-by: Lukas Fleischer \n> > ---\n> > The bug manifests when running `git blame HEAD^ -- nonexistent.file`.\n> \n> Before 1da1e07c (clean up name allocation in prepare_revision_walk,\n> 2014-10-15), these strings used to be non-volatile; they were instead\n> leaked more or less deliberately. But these days, these strings are\n> cleared, so your patch is absolutely the right thing to do.\n> \n> Thanks for catching and fixing. This fix needs to go to the 2.2.x\n> maintenance track.\n\nYeah, agreed. Sorry for not catching this as part of 1da1e07c.\n\nI did a grep for 'pending.*name' to look at any other potential problem\nsites. It looks like blame is the only one that tries to retain a\nlong-lived pointer to the name.\n\nThe other potentially interesting spot is that they are fed to the\nobject callbacks from traverse_commit_list for tags. However, none of\nthe callbacks saves it (and it would not make much sense to do so; they\nalso receive broken-down filenames in the same way, so if they want to\nuse it at all, they feed it through path_name() first, which makes a\ncopy).\n\nSo I think Lukas's patch fixes everything (and his positioning of the\nstrdup() calls is right where I would have put them).\n\n-Peff\n"},{"id":"254566","messageId":"xmqqzj9n623h.fsf@gitster.dls.corp.google.com","threadId":"38335","inReplyTo":"xmqq4mrv95qt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] blame.c: fix garbled error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-12T22:55:30Z","receivedAt":"2015-01-12T22:55:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Lukas Fleischer writes:\n>\n>> The helper functions prepare_final() and prepare_initial() return a\n>> pointer to a string that is a member of an object in the revs->pending\n>> array. This array is later rebuilt when running prepare_revision_walk()\n>> which potentially transforms the pointer target into a bogus string. Fix\n>> this by maintaining a copy of the original string.\n>>\n>> Signed-off-by: Lukas Fleischer \n>> ---\n>> The bug manifests when running `git blame HEAD^ -- nonexistent.file`.\n>\n> Before 1da1e07c (clean up name allocation in prepare_revision_walk,\n> 2014-10-15), these strings used to be non-volatile; they were instead\n> leaked more or less deliberately. But these days, these strings are\n> cleared, so your patch is absolutely the right thing to do.\n>\n> Thanks for catching and fixing. This fix needs to go to the 2.2.x\n> maintenance track.\n\nSigh, but not so fast.\n\nWith the patch applied on top of 1da1e07c (or the result merged to\n'next' for that matter), I see test breakages in many places \"git\nblame\" is used, e.g. t7010. Did you run the test suite?\n\nThis is because it is perfectly normal for prepare_final() to return\nNULL. Unconditionally running xstrdup() would of course fail.\n\n>> Note that I could have reduced code churn a little by moving the\n>> xstrdup() invocations to the call sites. However, I think that the\n>> return value of these functions should not depend on the consistency of\n>> a volatile data structure. On the other hand, you might also argue that\n>> there currently are only two call sites and that the functions are\n>> marked static, so if you prefer the less intrusive version, please let\n>> me know.\n>\n> FWIW, I agree that this can be argued either way.\n>\n> If we had a common low-level API that is used by short-term users to\n> grab these names, it would be reasonable to make it the callers'\n> responsibility to strdup() the return values for safekeeping if they\n> want to keep using them long after the function returns. But I\n> agree that prepare_final/prepare_initial are not such a low-level\n> common API functions. I do not care too much about who does the\n> strdup(), either the callers of prepare_* or the callee.\n>\n>>\n>> builtin/blame.c | 12 +++++++-----\n>> 1 file changed, 7 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/builtin/blame.c b/builtin/blame.c\n>> index 303e217..34d6f4f 100644\n>> --- a/builtin/blame.c\n>> +++ b/builtin/blame.c\n>> @@ -2390,7 +2390,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n>> \treturn commit;\n>> }\n>> \n>> -static const char *prepare_final(struct scoreboard *sb)\n>> +static char *prepare_final(struct scoreboard *sb)\n>> {\n>> \tint i;\n>> \tconst char *final_commit_name = NULL;\n>> @@ -2415,10 +2415,10 @@ static const char *prepare_final(struct scoreboard *sb)\n>> \t\tsb->final = (struct commit *) obj;\n>> \t\tfinal_commit_name = revs->pending.objects[i].name;\n>> \t}\n>> -\treturn final_commit_name;\n>> +\treturn xstrdup(final_commit_name);\n>> }\n>> \n>> -static const char *prepare_initial(struct scoreboard *sb)\n>> +static char *prepare_initial(struct scoreboard *sb)\n>> {\n>> \tint i;\n>> \tconst char *final_commit_name = NULL;\n>> @@ -2445,7 +2445,7 @@ static const char *prepare_initial(struct scoreboard *sb)\n>> \t}\n>> \tif (!final_commit_name)\n>> \t\tdie(\"No commit to dig down to?\");\n>> -\treturn final_commit_name;\n>> +\treturn xstrdup(final_commit_name);\n>> }\n>> \n>> static int blame_copy_callback(const struct option *option, const char *arg, int unset)\n>> @@ -2489,7 +2489,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n>> \tstruct origin *o;\n>> \tstruct blame_entry *ent = NULL;\n>> \tlong dashdash_pos, lno;\n>> -\tconst char *final_commit_name = NULL;\n>> +\tchar *final_commit_name = NULL;\n>> \tenum object_type type;\n>> \n>> \tstatic struct string_list range_list;\n>> @@ -2786,6 +2786,8 @@ parse_done:\n>> \n>> \tassign_blame(&sb, opt);\n>> \n>> +\tfree(final_commit_name);\n>> +\n>> \tif (incremental)\n>> \t\treturn 0;\n"},{"id":"254567","messageId":"20150112231231.GA4023@peff.net","threadId":"38335","inReplyTo":"xmqqzj9n623h.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] blame.c: fix garbled error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-12T23:12:31Z","receivedAt":"2015-01-12T23:12:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 12, 2015 at 02:55:30PM -0800, Junio C Hamano wrote:\n\n> With the patch applied on top of 1da1e07c (or the result merged to\n> 'next' for that matter), I see test breakages in many places \"git\n> blame\" is used, e.g. t7010. Did you run the test suite?\n> \n> This is because it is perfectly normal for prepare_final() to return\n> NULL. Unconditionally running xstrdup() would of course fail.\n\nEek. I even thought of this possibility and checked the prepare_initial\ncallsite, but not the prepare_final one.\n\nAs an aside, I have often been tempted to have xstrdup silently\npropagate a NULL. It would have been the right thing to do here, but\nmaybe there are cases where the segfault is preferable for catching a\nmistake early (otherwise you might store the NULL and then segfault much\nlater).\n\n-Peff\n"},{"id":"254568","messageId":"20150112231849.4992.72982@typhoon.lan","threadId":"38335","inReplyTo":"xmqqzj9n623h.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] blame.c: fix garbled error message","fromName":"Lukas Fleischer","fromEmail":"git@cryptocrack.de","sentAt":"2015-01-12T23:18:49Z","receivedAt":"2015-01-12T23:18:49Z","isPatch":true,"sender":{"key":"git@cryptocrack.de","avatar":null},"body":"On Mon, 12 Jan 2015 at 23:55:30, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Lukas Fleischer writes:\n> >\n> >> The helper functions prepare_final() and prepare_initial() return a\n> >> pointer to a string that is a member of an object in the revs->pending\n> >> array. This array is later rebuilt when running prepare_revision_walk()\n> >> which potentially transforms the pointer target into a bogus string. Fix\n> >> this by maintaining a copy of the original string.\n> >>\n> >> Signed-off-by: Lukas Fleischer \n> >> ---\n> >> The bug manifests when running `git blame HEAD^ -- nonexistent.file`.\n> >\n> > Before 1da1e07c (clean up name allocation in prepare_revision_walk,\n> > 2014-10-15), these strings used to be non-volatile; they were instead\n> > leaked more or less deliberately. But these days, these strings are\n> > cleared, so your patch is absolutely the right thing to do.\n> >\n> > Thanks for catching and fixing. This fix needs to go to the 2.2.x\n> > maintenance track.\n> \n> Sigh, but not so fast.\n> \n> With the patch applied on top of 1da1e07c (or the result merged to\n> 'next' for that matter), I see test breakages in many places \"git\n> blame\" is used, e.g. t7010. Did you run the test suite?\n> \n\nNo, I didn't.\n\n> This is because it is perfectly normal for prepare_final() to return\n> NULL. Unconditionally running xstrdup() would of course fail.\n> [...]\n\nSomething like\n\n return final_commit_name ? xstrdup(final_commit_name) : NULL;\n\nshould work, though, right? Calling free() with a null pointer is fine,\nso there is nothing else to do here. You can either amend those two\nlines or wait for me to resubmit v2 of the patch tomorrow :)\n"},{"id":"254576","messageId":"xmqqzj9n4k11.fsf@gitster.dls.corp.google.com","threadId":"38335","inReplyTo":"20150112231231.GA4023@peff.net","subject":"Re: [PATCH] blame.c: fix garbled error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-13T00:11:06Z","receivedAt":"2015-01-13T00:11:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> As an aside, I have often been tempted to have xstrdup silently\n> propagate a NULL. It would have been the right thing to do here, but\n> maybe there are cases where the segfault is preferable for catching a\n> mistake early (otherwise you might store the NULL and then segfault much\n> later).\n\nGreat minds think alike. The sentence after \"but maybe ...\" was\nwhat I had in mind as a response in anticipation that somebody might\nsuggest that; a separate xstrdup_or_null() might be fine, but I'd\nrather not to have xstrdup() that is _too_ magical.\n"},{"id":"254582","messageId":"20150113015427.GA5497@peff.net","threadId":"38335","inReplyTo":"xmqqzj9n4k11.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] blame.c: fix garbled error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-13T01:54:27Z","receivedAt":"2015-01-13T01:54:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 12, 2015 at 04:11:06PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > As an aside, I have often been tempted to have xstrdup silently\n> > propagate a NULL. It would have been the right thing to do here, but\n> > maybe there are cases where the segfault is preferable for catching a\n> > mistake early (otherwise you might store the NULL and then segfault much\n> > later).\n> \n> Great minds think alike. The sentence after \"but maybe ...\" was\n> what I had in mind as a response in anticipation that somebody might\n> suggest that; a separate xstrdup_or_null() might be fine, but I'd\n> rather not to have xstrdup() that is _too_ magical.\n\nYeah. Of course, it is not _that_ many more characters to do a ternary\nconditional. I guess the main benefit is that you do not have to repeat\nthe name of the variable (which lets you reuse a function result\ndirectly, avoiding an explicit temporary).\n\nHere's my attempt. Some cases are a little nicer, but overall, it does\nnot feel significantly more readable to me. I dunno. I could go either\nway. I stuck Lukas's patch on top (modified to use xstrdup_or_null), if\nwe do want to go that route. Otherwise it needs the ?: treatment.\n\n [1/5]: git-compat-util: add xstrdup_or_null helper\n [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup\n [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup\n [4/5]: use xstrdup_or_null to replace ternary conditionals\n [5/5]: blame.c: fix garbled error message\n"},{"id":"254583","messageId":"20150113015736.GA18986@peff.net","threadId":"38335","inReplyTo":"20150113015427.GA5497@peff.net","subject":"[PATCH 1/5] git-compat-util: add xstrdup_or_null helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-13T01:57:37Z","receivedAt":"2015-01-13T01:57:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's a common idiom to duplicate a string if it is non-NULL,\nor pass a literal NULL through. This is already a one-liner\nin C, but you do have to repeat the name of the string\ntwice. So if there's a function call, you must write:\n\n const char *x = some_fun(...);\n return x ? xstrdup(x) : NULL;\n\ninstead of (with this patch) just:\n\n return xstrdup_or_null(some_fun(...));\n\nSigned-off-by: Jeff King \n---\nThis example is the heart of the readability question to me. It is nice\nto avoid the temporary, which makes the code more direct. But it also\nsticks some_fun(...) inside another function, which is a little harder\nto read.\n\n git-compat-util.h | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex dcecd85..8157eb2 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -675,6 +675,11 @@ extern char *xgetcwd(void);\n \n #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))\n \n+static inline char *xstrdup_or_null(const char *str)\n+{\n+\treturn str ? xstrdup(str) : NULL;\n+}\n+\n static inline size_t xsize_t(off_t len)\n {\n \tif (len > (size_t) len)\n-- \n2.2.1.425.g441bb3c\n"},{"id":"254584","messageId":"20150113015815.GB18986@peff.net","threadId":"38335","inReplyTo":"20150113015427.GA5497@peff.net","subject":"[PATCH 2/5] builtin/apply.c: use xstrdup_or_null instead of null_strdup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-13T01:58:15Z","receivedAt":"2015-01-13T01:58:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This file had its own identical helper that predates\nxstrdup_or_null. Let's use the global one to avoid\nrepetition.\n\nSigned-off-by: Jeff King \n---\nYou'll note that every one of these could just be a ?:-conditional\n(there are no function calls).\n\n builtin/apply.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 0aad912..dfd7a34 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -657,11 +657,6 @@ static size_t diff_timestamp_len(const char *line, size_t len)\n \treturn line + len - end;\n }\n \n-static char *null_strdup(const char *s)\n-{\n-\treturn s ? xstrdup(s) : NULL;\n-}\n-\n static char *find_name_common(const char *line, const char *def,\n \t\t\t int p_value, const char *end, int terminate)\n {\n@@ -684,10 +679,10 @@ static char *find_name_common(const char *line, const char *def,\n \t\t\tstart = line;\n \t}\n \tif (!start)\n-\t\treturn squash_slash(null_strdup(def));\n+\t\treturn squash_slash(xstrdup_or_null(def));\n \tlen = line - start;\n \tif (!len)\n-\t\treturn squash_slash(null_strdup(def));\n+\t\treturn squash_slash(xstrdup_or_null(def));\n \n \t/*\n \t * Generally we prefer the shorter name, especially\n@@ -909,7 +904,7 @@ static void parse_traditional_patch(const char *first, const char *second, struc\n \t\t\tpatch->old_name = name;\n \t\t} else {\n \t\t\tpatch->old_name = name;\n-\t\t\tpatch->new_name = null_strdup(name);\n+\t\t\tpatch->new_name = xstrdup_or_null(name);\n \t\t}\n \t}\n \tif (!name)\n@@ -998,7 +993,7 @@ static int gitdiff_delete(const char *line, struct patch *patch)\n {\n \tpatch->is_delete = 1;\n \tfree(patch->old_name);\n-\tpatch->old_name = null_strdup(patch->def_name);\n+\tpatch->old_name = xstrdup_or_null(patch->def_name);\n \treturn gitdiff_oldmode(line, patch);\n }\n \n@@ -1006,7 +1001,7 @@ static int gitdiff_newfile(const char *line, struct patch *patch)\n {\n \tpatch->is_new = 1;\n \tfree(patch->new_name);\n-\tpatch->new_name = null_strdup(patch->def_name);\n+\tpatch->new_name = xstrdup_or_null(patch->def_name);\n \treturn gitdiff_newmode(line, patch);\n }\n \n-- \n2.2.1.425.g441bb3c\n"},{"id":"254585","messageId":"20150113015832.GC18986@peff.net","threadId":"38335","inReplyTo":"20150113015427.GA5497@peff.net","subject":"[PATCH 3/5] builtin/commit.c: use xstrdup_or_null instead of envdup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-13T01:58:33Z","receivedAt":"2015-01-13T01:58:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The only reason for envdup to be its own function is that we\nhave to save the result in a temporary string. With\nxstrdup_or_null, we can feed the result of getenv()\ndirectly.\n\nSigned-off-by: Jeff King \n---\nThese ones need the temporary. Is the result more readable, or less?\n\n builtin/commit.c | 12 +++---------\n 1 file changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 7d90c35..5cd1478 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -559,20 +559,14 @@ static void set_ident_var(char **buf, char *val)\n \t*buf = val;\n }\n \n-static char *envdup(const char *var)\n-{\n-\tconst char *val = getenv(var);\n-\treturn val ? xstrdup(val) : NULL;\n-}\n-\n static void determine_author_info(struct strbuf *author_ident)\n {\n \tchar *name, *email, *date;\n \tstruct ident_split author;\n \n-\tname = envdup(\"GIT_AUTHOR_NAME\");\n-\temail = envdup(\"GIT_AUTHOR_EMAIL\");\n-\tdate = envdup(\"GIT_AUTHOR_DATE\");\n+\tname = xstrdup_or_null(getenv(\"GIT_AUTHOR_NAME\"));\n+\temail = xstrdup_or_null(getenv(\"GIT_AUTHOR_EMAIL\"));\n+\tdate = xstrdup_or_null(getenv(\"GIT_AUTHOR_DATE\"));\n \n \tif (author_message) {\n \t\tstruct ident_split ident;\n-- \n2.2.1.425.g441bb3c\n"},{"id":"254586","messageId":"20150113015909.GD18986@peff.net","threadId":"38335","inReplyTo":"20150113015427.GA5497@peff.net","subject":"[PATCH 4/5] use xstrdup_or_null to replace ternary conditionals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-13T01:59:09Z","receivedAt":"2015-01-13T01:59:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This replaces \"x ? xstrdup(x) : NULL\" with xstrdup_or_null(x).\nThe change is fairly mechanical, with the exception of\nresolve_refdup, which can eliminate a temporary variable.\n\nThere are still a few hits grepping for \"?.*xstrdup\", but\nthese are of slightly different forms and cannot be\nconverted (e.g., \"x ? xstrdup(x->foo) : NULL\").\n\nSigned-off-by: Jeff King \n---\nresolve_refdup is the interesting one to look at for readability here.\n\n config.c | 2 +-\n grep.c | 4 ++--\n notes.c | 2 +-\n refs.c | 3 +--\n remote.c | 4 ++--\n shallow.c | 2 +-\n walker.c | 2 +-\n 7 files changed, 9 insertions(+), 10 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 752e2e2..e5e64dc 100644\n--- a/config.c\n+++ b/config.c\n@@ -1340,7 +1340,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n \t\tstring_list_init(&e->value_list, 1);\n \t\thashmap_add(&cs->config_hash, e);\n \t}\n-\tsi = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);\n+\tsi = string_list_append_nodup(&e->value_list, xstrdup_or_null(value));\n \n \tALLOC_GROW(cs->list.items, cs->list.nr + 1, cs->list.alloc);\n \tl_item = &cs->list.items[cs->list.nr++];\ndiff --git a/grep.c b/grep.c\nindex 6e085f8..b58c7c6 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1661,8 +1661,8 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n \t\t const void *identifier)\n {\n \tgs->type = type;\n-\tgs->name = name ? xstrdup(name) : NULL;\n-\tgs->path = path ? xstrdup(path) : NULL;\n+\tgs->name = xstrdup_or_null(name);\n+\tgs->path = xstrdup_or_null(path);\n \tgs->buf = NULL;\n \tgs->size = 0;\n \tgs->driver = NULL;\ndiff --git a/notes.c b/notes.c\nindex c763a21..2be4d7f 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1006,7 +1006,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tt->root = (struct int_node *) xcalloc(1, sizeof(struct int_node));\n \tt->first_non_note = NULL;\n \tt->prev_non_note = NULL;\n-\tt->ref = notes_ref ? xstrdup(notes_ref) : NULL;\n+\tt->ref = xstrdup_or_null(notes_ref);\n \tt->combine_notes = combine_notes;\n \tt->initialized = 1;\n \tt->dirty = 0;\ndiff --git a/refs.c b/refs.c\nindex 5fcacc6..78fec5c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1618,8 +1618,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned\n \n char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags)\n {\n-\tconst char *ret = resolve_ref_unsafe(ref, resolve_flags, sha1, flags);\n-\treturn ret ? xstrdup(ret) : NULL;\n+\treturn xstrdup_or_null(resolve_ref_unsafe(ref, resolve_flags, sha1, flags));\n }\n \n /* The argument to filter_refs */\ndiff --git a/remote.c b/remote.c\nindex 5b9c693..7b71ebf 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -975,8 +975,8 @@ struct ref *copy_ref(const struct ref *ref)\n \tcpy = xmalloc(sizeof(struct ref) + len + 1);\n \tmemcpy(cpy, ref, sizeof(struct ref) + len + 1);\n \tcpy->next = NULL;\n-\tcpy->symref = ref->symref ? xstrdup(ref->symref) : NULL;\n-\tcpy->remote_status = ref->remote_status ? xstrdup(ref->remote_status) : NULL;\n+\tcpy->symref = xstrdup_or_null(ref->symref);\n+\tcpy->remote_status = xstrdup_or_null(ref->remote_status);\n \tcpy->peer_ref = copy_ref(ref->peer_ref);\n \treturn cpy;\n }\ndiff --git a/shallow.c b/shallow.c\nindex cdd0775..f5e6720 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -22,7 +22,7 @@ void set_alternate_shallow_file(const char *path, int override)\n \tif (alternate_shallow_file && !override)\n \t\treturn;\n \tfree(alternate_shallow_file);\n-\talternate_shallow_file = path ? xstrdup(path) : NULL;\n+\talternate_shallow_file = xstrdup_or_null(path);\n }\n \n int register_shallow(const unsigned char *sha1)\ndiff --git a/walker.c b/walker.c\nindex f149371..483da4e 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -232,7 +232,7 @@ int walker_targets_stdin(char ***target, const char ***write_ref)\n \t\t\tREALLOC_ARRAY(*write_ref, targets_alloc);\n \t\t}\n \t\t(*target)[targets] = xstrdup(tg_one);\n-\t\t(*write_ref)[targets] = rf_one ? xstrdup(rf_one) : NULL;\n+\t\t(*write_ref)[targets] = xstrdup_or_null(rf_one);\n \t\ttargets++;\n \t}\n \tstrbuf_release(&buf);\n-- \n2.2.1.425.g441bb3c\n"},{"id":"254587","messageId":"20150113015925.GE18986@peff.net","threadId":"38335","inReplyTo":"20150113015427.GA5497@peff.net","subject":"[PATCH 5/5] blame.c: fix garbled error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-13T01:59:26Z","receivedAt":"2015-01-13T01:59:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Lukas Fleischer \n\nThe helper functions prepare_final() and prepare_initial() return a\npointer to a string that is a member of an object in the revs->pending\narray. This array is later rebuilt when running prepare_revision_walk()\nwhich potentially transforms the pointer target into a bogus string. Fix\nthis by maintaining a copy of the original string.\n\nSigned-off-by: Lukas Fleischer \nSigned-off-by: Jeff King \n---\n builtin/blame.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 303e217..0374fe8 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2390,7 +2390,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \treturn commit;\n }\n \n-static const char *prepare_final(struct scoreboard *sb)\n+static char *prepare_final(struct scoreboard *sb)\n {\n \tint i;\n \tconst char *final_commit_name = NULL;\n@@ -2415,10 +2415,10 @@ static const char *prepare_final(struct scoreboard *sb)\n \t\tsb->final = (struct commit *) obj;\n \t\tfinal_commit_name = revs->pending.objects[i].name;\n \t}\n-\treturn final_commit_name;\n+\treturn xstrdup_or_null(final_commit_name);\n }\n \n-static const char *prepare_initial(struct scoreboard *sb)\n+static char *prepare_initial(struct scoreboard *sb)\n {\n \tint i;\n \tconst char *final_commit_name = NULL;\n@@ -2445,7 +2445,7 @@ static const char *prepare_initial(struct scoreboard *sb)\n \t}\n \tif (!final_commit_name)\n \t\tdie(\"No commit to dig down to?\");\n-\treturn final_commit_name;\n+\treturn xstrdup(final_commit_name);\n }\n \n static int blame_copy_callback(const struct option *option, const char *arg, int unset)\n@@ -2489,7 +2489,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tstruct origin *o;\n \tstruct blame_entry *ent = NULL;\n \tlong dashdash_pos, lno;\n-\tconst char *final_commit_name = NULL;\n+\tchar *final_commit_name = NULL;\n \tenum object_type type;\n \n \tstatic struct string_list range_list;\n@@ -2786,6 +2786,8 @@ parse_done:\n \n \tassign_blame(&sb, opt);\n \n+\tfree(final_commit_name);\n+\n \tif (incremental)\n \t\treturn 0;\n \n-- \n2.2.1.425.g441bb3c\n"},{"id":"254588","messageId":"20150113022119.GA29365@google.com","threadId":"38335","inReplyTo":"20150113015736.GA18986@peff.net","subject":"Re: [PATCH 1/5] git-compat-util: add xstrdup_or_null helper","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2015-01-13T02:21:19Z","receivedAt":"2015-01-13T02:21:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -675,6 +675,11 @@ extern char *xgetcwd(void);\n> \n> #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))\n> \n> +static inline char *xstrdup_or_null(const char *str)\n> +{\n> +\treturn str ? xstrdup(str) : NULL;\n> +}\n\nWould it make sense for xstrdup to always include the NULL check,\navoiding the need for the more verbose xstrdup_or_null?\n\nJonathan\n\ndiff --git i/wrapper.c w/wrapper.c\nindex 007ec0d..5a835e8 100644\n--- i/wrapper.c\n+++ w/wrapper.c\n@@ -40,7 +40,11 @@ try_to_free_t set_try_to_free_routine(try_to_free_t routine)\n \n char *xstrdup(const char *str)\n {\n-\tchar *ret = strdup(str);\n+\tchar *ret;\n+\n+\tif (!str)\n+\t\treturn NULL;\n+\tret = strdup(str);\n \tif (!ret) {\n \t\ttry_to_free_routine(strlen(str) + 1);\n \t\tret = strdup(str);\n@@ -125,7 +129,11 @@ void *xmemdupz(const void *data, size_t len)\n \n char *xstrndup(const char *str, size_t len)\n {\n-\tchar *p = memchr(str, '\\0', len);\n+\tchar *p;\n+\n+\tif (!str)\n+\t\treturn NULL;\n+\tp = memchr(str, '\\0', len);\n \treturn xmemdupz(str, p ? p - str : len);\n }\n \n"},{"id":"254589","messageId":"20150113022336.GA3927@peff.net","threadId":"38335","inReplyTo":"20150113022119.GA29365@google.com","subject":"Re: [PATCH 1/5] git-compat-util: add xstrdup_or_null helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-13T02:23:36Z","receivedAt":"2015-01-13T02:23:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 12, 2015 at 06:21:19PM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > --- a/git-compat-util.h\n> > +++ b/git-compat-util.h\n> > @@ -675,6 +675,11 @@ extern char *xgetcwd(void);\n> > \n> > #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))\n> > \n> > +static inline char *xstrdup_or_null(const char *str)\n> > +{\n> > +\treturn str ? xstrdup(str) : NULL;\n> > +}\n> \n> Would it make sense for xstrdup to always include the NULL check,\n> avoiding the need for the more verbose xstrdup_or_null?\n\nRead the rest of the thread. :)\n\n-Peff\n"},{"id":"254653","messageId":"20150114142146.16390.54937@typhoon.lan","threadId":"38335","inReplyTo":"20150113015427.GA5497@peff.net","subject":"Re: [PATCH] blame.c: fix garbled error message","fromName":"Lukas Fleischer","fromEmail":"git@cryptocrack.de","sentAt":"2015-01-14T14:21:46Z","receivedAt":"2015-01-14T14:21:46Z","isPatch":true,"sender":{"key":"git@cryptocrack.de","avatar":null},"body":"On Tue, 13 Jan 2015 at 02:54:27, Jeff King wrote:\n> On Mon, Jan 12, 2015 at 04:11:06PM -0800, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > As an aside, I have often been tempted to have xstrdup silently\n> > > propagate a NULL. It would have been the right thing to do here, but\n> > > maybe there are cases where the segfault is preferable for catching a\n> > > mistake early (otherwise you might store the NULL and then segfault much\n> > > later).\n> > \n> > Great minds think alike. The sentence after \"but maybe ...\" was\n> > what I had in mind as a response in anticipation that somebody might\n> > suggest that; a separate xstrdup_or_null() might be fine, but I'd\n> > rather not to have xstrdup() that is _too_ magical.\n> \n> Yeah. Of course, it is not _that_ many more characters to do a ternary\n> conditional. I guess the main benefit is that you do not have to repeat\n> the name of the variable (which lets you reuse a function result\n> directly, avoiding an explicit temporary).\n> \n> Here's my attempt. Some cases are a little nicer, but overall, it does\n> not feel significantly more readable to me. I dunno. I could go either\n> way. I stuck Lukas's patch on top (modified to use xstrdup_or_null), if\n> we do want to go that route. Otherwise it needs the ?: treatment.\n> \n> [1/5]: git-compat-util: add xstrdup_or_null helper\n> [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup\n> [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup\n> [4/5]: use xstrdup_or_null to replace ternary conditionals\n> [5/5]: blame.c: fix garbled error message\n> \n\nLooks good to me! I am not sure whether those patches should be built on\ntop of (a fixed version of) my patch, though, which would make\nbackporting the fix to the maintenance branch straightforward. Junio?\n"},{"id":"254658","messageId":"xmqqr3uxb7kj.fsf@gitster.dls.corp.google.com","threadId":"38335","inReplyTo":"20150114142146.16390.54937@typhoon.lan","subject":"Re: [PATCH] blame.c: fix garbled error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-14T17:22:52Z","receivedAt":"2015-01-14T17:22:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lukas Fleischer writes:\n\n>> [1/5]: git-compat-util: add xstrdup_or_null helper\n>> [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup\n>> [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup\n>> [4/5]: use xstrdup_or_null to replace ternary conditionals\n>> [5/5]: blame.c: fix garbled error message\n>> \n>\n> Looks good to me! I am not sure whether those patches should be built on\n> top of (a fixed version of) my patch, though, which would make\n> backporting the fix to the maintenance branch straightforward. Junio?\n\nWe can queue these five on top of 1da1e07c (clean up name allocation\nin prepare_revision_walk, 2014-10-15), which changed the rule of the\ngame to break this code, that only is in v2.2 and later.\n\nAnd the result should merge just fine to 'maint'.\n"},{"id":"254689","messageId":"20150114204926.GA1155@peff.net","threadId":"38335","inReplyTo":"xmqqr3uxb7kj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] blame.c: fix garbled error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-14T20:49:26Z","receivedAt":"2015-01-14T20:49:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 14, 2015 at 09:22:52AM -0800, Junio C Hamano wrote:\n\n> Lukas Fleischer writes:\n> \n> >> [1/5]: git-compat-util: add xstrdup_or_null helper\n> >> [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup\n> >> [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup\n> >> [4/5]: use xstrdup_or_null to replace ternary conditionals\n> >> [5/5]: blame.c: fix garbled error message\n> >> \n> >\n> > Looks good to me! I am not sure whether those patches should be built on\n> > top of (a fixed version of) my patch, though, which would make\n> > backporting the fix to the maintenance branch straightforward. Junio?\n> \n> We can queue these five on top of 1da1e07c (clean up name allocation\n> in prepare_revision_walk, 2014-10-15), which changed the rule of the\n> game to break this code, that only is in v2.2 and later.\n> \n> And the result should merge just fine to 'maint'.\n\nAre we in agreement then that the resulting code with the helper is\nactually easier to read? I think replacing the straight ?: lines is, but\nI am on the fence on whether:\n\n const char *x = some_fun(...);\n return xstrdup_or_null(x);\n\nis better or worse than:\n\n return xstrdup_or_null(some_fun(....));\n\n-Peff\n"},{"id":"254700","messageId":"xmqqsifd81v1.fsf@gitster.dls.corp.google.com","threadId":"38335","inReplyTo":"20150114204926.GA1155@peff.net","subject":"Re: [PATCH] blame.c: fix garbled error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-14T21:54:26Z","receivedAt":"2015-01-14T21:54:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jan 14, 2015 at 09:22:52AM -0800, Junio C Hamano wrote:\n> ...\n>> And the result should merge just fine to 'maint'.\n>\n> Are we in agreement then that the resulting code with the helper is\n> actually easier to read? I think replacing the straight ?: lines is, but\n> I am on the fence on whether:\n>\n> const char *x = some_fun(...);\n> return xstrdup_or_null(x);\n>\n> is better or worse than:\n>\n> return xstrdup_or_null(some_fun(....));\n\nI think the latter is fine as long as some_fun(...) invocation does\nnot get overly long, and even the longest I saw in refs.c, i.e.\n\n return xstrdup_or_null(resolve_ref_unsafe(ref, resolve_flags, sha1, flags));\n\ndid not bother me too much.\n"}]}