{"thread":{"id":"34644","subject":"[PATCH] diff: remove ternary operator evaluating always to true","startedAt":"2013-08-08T18:31:44Z","lastAt":"2013-08-12T17:15:48Z","messageCount":6,"participants":["Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"224840","messageId":"1375986704-11441-1-git-send-email-stefanbeller@googlemail.com","threadId":"34644","inReplyTo":null,"subject":"[PATCH] diff: remove ternary operator evaluating always to true","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-08T18:31:44Z","receivedAt":"2013-08-08T18:31:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The line being changed is deep inside the function builtin_diff.\nThe variable name_b, which is used to evaluate the ternary expression\nmust evaluate to true at that position, hence the replacement with\njust name_b.\n\nThe name_b variable only occurs a few times in that lengthy function:\nAs a parameter to the function itself:\n\tstatic void builtin_diff(const char *name_a,\n\t\t\t\t const char *name_b,\n\t\t\t\t...\nThe next occurrences are at:\n\t/* Never use a non-valid filename anywhere if at all possible */\n\tname_a = DIFF_FILE_VALID(one) ? name_a : name_b;\n\tname_b = DIFF_FILE_VALID(two) ? name_b : name_a;\n\n\ta_one = quote_two(a_prefix, name_a + (*name_a == '/'));\n\tb_two = quote_two(b_prefix, name_b + (*name_b == '/'));\n\nIn the last line of this block 'name_b' is dereferenced and compared\nto '/'. This would crash if name_b was NULL. Hence in the following code\nwe can assume name_b being non-null.\n\nThe next occurrence is just as a function argument, which doesn't change\nthe memory, which name_b points to, so the assumption name_b being not\nnull still holds:\n\temit_rewrite_diff(name_a, name_b, one, two,\n\t\t\t\ttextconv_one, textconv_two, o);\n\nThe next occurrence would be the line of this patch. As name_b still must\nbe not null, we can remove the ternary operator.\n\n\nInside the emit_rewrite_diff function there is a also a line\n\tecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);\nwhich was also simplified as there is also a dereference before the\nternary operator.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 266112c..80f8439 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -669,7 +669,7 @@ static void emit_rewrite_diff(const char *name_a,\n \tmemset(&ecbdata, 0, sizeof(ecbdata));\n \tecbdata.color_diff = want_color(o->use_color);\n \tecbdata.found_changesp = &o->found_changes;\n-\tecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);\n+\tecbdata.ws_rule = whitespace_rule(name_b);\n \tecbdata.opt = o;\n \tif (ecbdata.ws_rule & WS_BLANK_AT_EOF) {\n \t\tmmfile_t mf1, mf2;\n@@ -2372,7 +2372,7 @@ static void builtin_diff(const char *name_a,\n \t\tecbdata.label_path = lbl;\n \t\tecbdata.color_diff = want_color(o->use_color);\n \t\tecbdata.found_changesp = &o->found_changes;\n-\t\tecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);\n+\t\tecbdata.ws_rule = whitespace_rule(name_b);\n \t\tif (ecbdata.ws_rule & WS_BLANK_AT_EOF)\n \t\t\tcheck_blank_at_eof(&mf1, &mf2, &ecbdata);\n \t\tecbdata.opt = o;\n-- \n1.8.4.rc1.25.gd121ba2\n"},{"id":"224998","messageId":"20130810072114.GD30185@sigill.intra.peff.net","threadId":"34644","inReplyTo":"1375986704-11441-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] diff: remove ternary operator evaluating always to true","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-10T07:21:14Z","receivedAt":"2013-08-10T07:21:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 08, 2013 at 08:31:44PM +0200, Stefan Beller wrote:\n\n> The next occurrences are at:\n> \t/* Never use a non-valid filename anywhere if at all possible */\n> \tname_a = DIFF_FILE_VALID(one) ? name_a : name_b;\n> \tname_b = DIFF_FILE_VALID(two) ? name_b : name_a;\n> \n> \ta_one = quote_two(a_prefix, name_a + (*name_a == '/'));\n> \tb_two = quote_two(b_prefix, name_b + (*name_b == '/'));\n> \n> In the last line of this block 'name_b' is dereferenced and compared\n> to '/'. This would crash if name_b was NULL. Hence in the following code\n> we can assume name_b being non-null.\n\nI think your change is correct, but I find the reasoning above a little\nsuspect. It assumes that the second chunk of code (accessing name_a and\nname_b) is correct, and pins the correctness of the code you are\nchanging to it. If the second chunk is buggy, then you are actually\nmaking the code worse.\n\nThe interesting part is the top chunk, which aliases the names if one of\nthem is NULL. And there is an implicit assumption there that we will\nnever get _two_ NULL names in this function. And that is why the second\nchunk does not ever crash (and why your change is the right thing to\ndo).\n\nSorry if this seems nit-picky. It is just that seemingly trivial\ncleanups can sometimes end up revealing larger bugs, and I think it is\nworth making sure we understand the root cause to be sure that our\ncleanup really is trivial.\n\nI wonder if the implicit expectation of the function to take at least\none non-NULL name would be more obvious if the first few lines were\nwritten as:\n\n if (DIFF_FILE_VALID(one)) {\n if (!DIFF_FILE_VALID(two))\n name_b = name_a;\n } else if (DIFF_FILE_VALID(two))\n name_a = name_b;\n else\n die(\"BUG: two invalid files to diff\");\n\nThat covers all of the cases explicitly, though it is IMHO uglier to\nread (and there is still an implicit assumption that the name is\nnon-NULL if DIFF_FILE_VALID() is true).\n\n-Peff\n"},{"id":"225072","messageId":"7vli471mxy.fsf@alter.siamese.dyndns.org","threadId":"34644","inReplyTo":"20130810072114.GD30185@sigill.intra.peff.net","subject":"Re: [PATCH] diff: remove ternary operator evaluating always to true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-12T05:46:01Z","receivedAt":"2013-08-12T05:46:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Aug 08, 2013 at 08:31:44PM +0200, Stefan Beller wrote:\n>\n>> The next occurrences are at:\n>> \t/* Never use a non-valid filename anywhere if at all possible */\n>> \tname_a = DIFF_FILE_VALID(one) ? name_a : name_b;\n>> \tname_b = DIFF_FILE_VALID(two) ? name_b : name_a;\n>> \n>> \ta_one = quote_two(a_prefix, name_a + (*name_a == '/'));\n>> \tb_two = quote_two(b_prefix, name_b + (*name_b == '/'));\n>> \n>> In the last line of this block 'name_b' is dereferenced and compared\n>> to '/'. This would crash if name_b was NULL. Hence in the following code\n>> we can assume name_b being non-null.\n>\n> I think your change is correct, but I find the reasoning above a little\n> suspect. It assumes that the second chunk of code (accessing name_a and\n> name_b) is correct, and pins the correctness of the code you are\n> changing to it. If the second chunk is buggy, then you are actually\n> making the code worse.\n\nTrue. I think the original code structure design is name_a should\nalways exist but name_b may not (the caller of run_diff_cmd() that\neventually calls this call these \"name\" and \"other\", and the intent\nis renaming filepair is what needs \"other\").\n\n> I wonder if the implicit expectation of the function to take at least\n> one non-NULL name would be more obvious if the first few lines were\n> written as:\n>\n> if (DIFF_FILE_VALID(one)) {\n> if (!DIFF_FILE_VALID(two))\n> name_b = name_a;\n> } else if (DIFF_FILE_VALID(two))\n> name_a = name_b;\n> else\n> die(\"BUG: two invalid files to diff\");\n>\n> That covers all of the cases explicitly, though it is IMHO uglier to\n> read (and there is still an implicit assumption that the name is\n> non-NULL if DIFF_FILE_VALID() is true).\n\nI think that is an overall improvement, especially if we also update\nthe checks of {one,two}->mode made for the block that deals with\nsubmodules to use DIFF_FILE_VALID().\n\nThanks.\n"},{"id":"225095","messageId":"52089D81.5010506@googlemail.com","threadId":"34644","inReplyTo":"7vli471mxy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: remove ternary operator evaluating always to true","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-12T08:32:01Z","receivedAt":"2013-08-12T08:32:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 08/12/2013 07:46 AM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> On Thu, Aug 08, 2013 at 08:31:44PM +0200, Stefan Beller wrote:\n>>\n>>> The next occurrences are at:\n>>> \t/* Never use a non-valid filename anywhere if at all possible */\n>>> \tname_a = DIFF_FILE_VALID(one) ? name_a : name_b;\n>>> \tname_b = DIFF_FILE_VALID(two) ? name_b : name_a;\n>>>\n>>> \ta_one = quote_two(a_prefix, name_a + (*name_a == '/'));\n>>> \tb_two = quote_two(b_prefix, name_b + (*name_b == '/'));\n>>>\n>>> In the last line of this block 'name_b' is dereferenced and compared\n>>> to '/'. This would crash if name_b was NULL. Hence in the following code\n>>> we can assume name_b being non-null.\n>>\n>> I think your change is correct, but I find the reasoning above a little\n>> suspect. It assumes that the second chunk of code (accessing name_a and\n>> name_b) is correct, and pins the correctness of the code you are\n>> changing to it. If the second chunk is buggy, then you are actually\n>> making the code worse.\n> \n> True. I think the original code structure design is name_a should\n> always exist but name_b may not (the caller of run_diff_cmd() that\n> eventually calls this call these \"name\" and \"other\", and the intent\n> is renaming filepair is what needs \"other\").\n> \n>> I wonder if the implicit expectation of the function to take at least\n>> one non-NULL name would be more obvious if the first few lines were\n>> written as:\n>>\n>> if (DIFF_FILE_VALID(one)) {\n>> if (!DIFF_FILE_VALID(two))\n>> name_b = name_a;\n>> } else if (DIFF_FILE_VALID(two))\n>> name_a = name_b;\n>> else\n>> die(\"BUG: two invalid files to diff\");\n>>\n>> That covers all of the cases explicitly, though it is IMHO uglier to\n>> read (and there is still an implicit assumption that the name is\n>> non-NULL if DIFF_FILE_VALID() is true).\n> \n> I think that is an overall improvement, especially if we also update\n> the checks of {one,two}->mode made for the block that deals with\n> submodules to use DIFF_FILE_VALID().\n> \n> Thanks.\n> \n\nSo, do I understand your reasoning, when proposing this patch?\n(This may break whitespaces as it's copied into my MUA, will resend with\ngit send-mail if you think this is the right thing.)\n\nThis patch just covers your discussion and not the previous patches.\n\nStefan\n\n--8<--\nFrom 701bab4f15598ba230552af7f1d5719187f1b2e8 Mon Sep 17 00:00:00 2001\nFrom: Stefan Beller \nDate: Mon, 12 Aug 2013 10:29:07 +0200\nSubject: [PATCH] diff: Additional error checking for input parameters\n\nThis makes the diff function error out instead of segfaulting if the\nparameters are bad.\n\nHelped-by: Jeff King \nHelped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n diff.c | 16 ++++++++++++----\n 1 file changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e53ddad..de21971 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2254,8 +2254,11 @@ static void builtin_diff(const char *name_a,\n \t\t\t(!two->mode || S_ISGITLINK(two->mode))) {\n \t\tconst char *del = diff_get_color_opt(o, DIFF_FILE_OLD);\n \t\tconst char *add = diff_get_color_opt(o, DIFF_FILE_NEW);\n-\t\tshow_submodule_summary(o->file, one ? one->path : two->path,\n-\t\t\t\tline_prefix,\n+\t\tstruct diff_filespec *spec = one && DIFF_FILE_VALID(one) ? one : two;\n+\t\tif (!spec && !DIFF_FILE_VALID(spec))\n+\t\t\tdie(\"BUG: two invalid diff_filespec structs in diff\");\n+\n+\t\tshow_submodule_summary(o->file, spec->path, line_prefix,\n \t\t\t\tone->sha1, two->sha1, two->dirty_submodule,\n \t\t\t\tmeta, del, add, reset);\n \t\treturn;\n@@ -2276,8 +2279,13 @@ static void builtin_diff(const char *name_a,\n \t}\n \n \t/* Never use a non-valid filename anywhere if at all possible */\n-\tname_a = DIFF_FILE_VALID(one) ? name_a : name_b;\n-\tname_b = DIFF_FILE_VALID(two) ? name_b : name_a;\n+\tif (DIFF_FILE_VALID(one)) {\n+\t\tif (!DIFF_FILE_VALID(two))\n+\t\t\tname_b = name_a;\n+\t} else if (DIFF_FILE_VALID(two))\n+\t\tname_a = name_b;\n+\telse\n+\t\tdie(\"BUG: two invalid files to diff\");\n \n \ta_one = quote_two(a_prefix, name_a + (*name_a == '/'));\n \tb_two = quote_two(b_prefix, name_b + (*name_b == '/'));\n-- \n1.8.4.rc2\n\n\n"},{"id":"225097","messageId":"52089F22.6010106@googlemail.com","threadId":"34644","inReplyTo":"52089D81.5010506@googlemail.com","subject":"Re: [PATCH] diff: remove ternary operator evaluating always to true","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-12T08:38:58Z","receivedAt":"2013-08-12T08:38:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 08/12/2013 10:32 AM, Stefan Beller wrote:\n> \n> diff --git a/diff.c b/diff.c\n> index e53ddad..de21971 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2254,8 +2254,11 @@ static void builtin_diff(const char *name_a,\n> \t\t\t(!two->mode || S_ISGITLINK(two->mode))) {\n> \t\tconst char *del = diff_get_color_opt(o, DIFF_FILE_OLD);\n> \t\tconst char *add = diff_get_color_opt(o, DIFF_FILE_NEW);\n> -\t\tshow_submodule_summary(o->file, one ? one->path : two->path,\n> -\t\t\t\tline_prefix,\n> +\t\tstruct diff_filespec *spec = one && DIFF_FILE_VALID(one) ? one : two;\n> +\t\tif (!spec && !DIFF_FILE_VALID(spec))\n> +\t\t\tdie(\"BUG: two invalid diff_filespec structs in diff\");\n> +\n> +\t\tshow_submodule_summary(o->file, spec->path, line_prefix,\n> \t\t\t\tone->sha1, two->sha1, two->dirty_submodule,\n> \t\t\t\tmeta, del, add, reset);\n> \t\treturn;\n\nThis doesn't make sense, as we're definitely dereferencing both \none and two with ->sha1, so we do not need the \"one &&\" and the \n\"!spec &&\". \nI think the originally sent patches are correct, but the commit \nmessage may need rewriting to comply with Jeffs nitpicking.\n\n\n\n"},{"id":"225118","messageId":"7vzjsmzv7f.fsf@alter.siamese.dyndns.org","threadId":"34644","inReplyTo":"52089D81.5010506@googlemail.com","subject":"Re: [PATCH] diff: remove ternary operator evaluating always to true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-12T17:15:48Z","receivedAt":"2013-08-12T17:15:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On 08/12/2013 07:46 AM, Junio C Hamano wrote:\n>> I think that is an overall improvement, especially if we also update\n>> the checks of {one,two}->mode made for the block that deals with\n>> submodules to use DIFF_FILE_VALID().\n>> \n>> Thanks.\n>> \n>\n> So, do I understand your reasoning, when proposing this patch?\n\nI meant the condition in the \"if\" statement, not the parameters to\nshow_submodule_summary(). There are checks on ->mode fields that\nbypass the DIFF_FILE_VALID() macro, hurting readability.\n"}]}