{"thread":{"id":"34645","subject":"[PATCH] diff: remove another ternary expression always evaluating to true","startedAt":"2013-08-08T18:55:03Z","lastAt":"2013-08-08T22:17:49Z","messageCount":8,"participants":["Stefan Beller","Johannes Schindelin","Philip Oakley","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"224844","messageId":"1375988103-29947-1-git-send-email-stefanbeller@googlemail.com","threadId":"34645","inReplyTo":null,"subject":"[PATCH] diff: remove another ternary expression always evaluating to true","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-08T18:55:03Z","receivedAt":"2013-08-08T18:55:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The condition before the changed line dereferences 'one' to query the mode,\nso if the condition evaluates to true, the variable one must not be null.\nTherefore we do not need the ternary operator depending on one, giving\neither one->path or two->path. This always evaluates to one->path, so\nwe can remove the ternary operator.\n\nThe condition and the usage of the ternary operator have been introduced\nby the same commit (752c0c24, 2009-10-19, Add the --submodule option to\nthe diff option family). As that commit message refers to a GitTogether\nI'd assume that patch was crafted in a hurry, so maybe overlooking the\nneed for a ternary operator there.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 80f8439..f30b7e4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2252,8 +2252,7 @@ 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\tshow_submodule_summary(o->file, one->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-- \n1.8.4.rc1.25.gd121ba2\n"},{"id":"224854","messageId":"alpine.DEB.1.00.1308082257580.24252@s15462909.onlinehome-server.info","threadId":"34645","inReplyTo":"1375988103-29947-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] diff: remove another ternary expression always evaluating to true","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2013-08-08T21:01:03Z","receivedAt":"2013-08-08T21:01:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Thu, 8 Aug 2013, Stefan Beller wrote:\n\n> The condition before the changed line dereferences 'one' to query the mode,\n> so if the condition evaluates to true, the variable one must not be null.\n\nTo show this better, please use -U10 (or some other appropriate context\noption) in the future.\n\n> Therefore we do not need the ternary operator depending on one, giving\n> either one->path or two->path. This always evaluates to one->path, so we\n> can remove the ternary operator.\n> \n> The condition and the usage of the ternary operator have been introduced\n> by the same commit (752c0c24, 2009-10-19, Add the --submodule option to\n> the diff option family). As that commit message refers to a GitTogether\n> I'd assume that patch was crafted in a hurry, so maybe overlooking the\n> need for a ternary operator there.\n\nIf this is my code, I do not need a GitTogether to excuse my sloppiness.\nIn this particular case, I imagine the appropriate fix is to test for\none->path instead of removing the conditional, though.\n\nCiao,\nJohannes\n"},{"id":"224859","messageId":"52040C18.4030306@googlemail.com","threadId":"34645","inReplyTo":"alpine.DEB.1.00.1308082257580.24252@s15462909.onlinehome-server.info","subject":"Re: [PATCH] diff: remove another ternary expression always evaluating to true","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-08T21:22:32Z","receivedAt":"2013-08-08T21:22:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 08/08/2013 11:01 PM, Johannes Schindelin wrote:\n> Hi Stefan,\n> \n> On Thu, 8 Aug 2013, Stefan Beller wrote:\n> \n>> The condition before the changed line dereferences 'one' to query the mode,\n>> so if the condition evaluates to true, the variable one must not be null.\n> \n> To show this better, please use -U10 (or some other appropriate context\n> option) in the future.\n> \n>> Therefore we do not need the ternary operator depending on one, giving\n>> either one->path or two->path. This always evaluates to one->path, so we\n>> can remove the ternary operator.\n>>\n>> The condition and the usage of the ternary operator have been introduced\n>> by the same commit (752c0c24, 2009-10-19, Add the --submodule option to\n>> the diff option family). As that commit message refers to a GitTogether\n>> I'd assume that patch was crafted in a hurry, so maybe overlooking the\n>> need for a ternary operator there.\n> \n> If this is my code, I do not need a GitTogether to excuse my sloppiness.\n> In this particular case, I imagine the appropriate fix is to test for\n> one->path instead of removing the conditional, though.\n> \n> Ciao,\n> Johannes\n> \n\nOk, here is a resend with -U10\nI forgot about the -U10 as gitk shows a different number of lines \nof context around. Is there a way to configure gitk to show less lines\nof code or a default -U10 for send-email/format-patch?\n\nSo you rather propose to have \n-\t\tshow_submodule_summary(o->file, one ? one->path : two->path,\n+\t\tshow_submodule_summary(o->file, one->path ? one->path : two->path,\n\ninstead of the patch below?\n(The test suite run without any problem using that patch)\n\nStefan\n\n--8<--\nFrom 3a580c51f0bf70745f26eb5045d646dfead2afd3 Mon Sep 17 00:00:00 2001\nFrom: Stefan Beller \nDate: Thu, 8 Aug 2013 20:54:24 +0200\nSubject: [PATCH] diff: remove another ternary expression always evaluating to\n true\n\nThe condition before the changed line dereferences 'one' to query the mode,\nso if the condition evaluates to true, the variable one must not be null.\nTherefore we do not need the ternary operator depending on one, giving\neither one->path or two->path. This always evaluates to one->path, so\nwe can remove the ternary operator.\n\nThe condition and the usage of the ternary operator have been introduced\nby the same commit (752c0c24, 2009-10-19, Add the --submodule option to\nthe diff option family). As that commit message refers to a GitTogether\nI'd assume that patch was crafted in a hurry, so maybe overlooking the\nneed for a ternary operator there.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 80f8439..f30b7e4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2245,22 +2245,21 @@ static void builtin_diff(const char *name_a,\n \tstruct userdiff_driver *textconv_one = NULL;\n \tstruct userdiff_driver *textconv_two = NULL;\n \tstruct strbuf header = STRBUF_INIT;\n \tconst char *line_prefix = diff_line_prefix(o);\n \n \tif (DIFF_OPT_TST(o, SUBMODULE_LOG) &&\n \t\t\t(!one->mode || S_ISGITLINK(one->mode)) &&\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\tshow_submodule_summary(o->file, one->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 \t}\n \n \tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n \t\ttextconv_one = get_textconv(one);\n \t\ttextconv_two = get_textconv(two);\n \t}\n \n-- \n1.8.4.rc1.25.gd121ba2\n\n\n"},{"id":"224863","messageId":"alpine.DEB.1.00.1308082335050.24252@s15462909.onlinehome-server.info","threadId":"34645","inReplyTo":"52040C18.4030306@googlemail.com","subject":"Re: [PATCH] diff: remove another ternary expression always evaluating to true","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2013-08-08T21:36:28Z","receivedAt":"2013-08-08T21:36:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Thu, 8 Aug 2013, Stefan Beller wrote:\n\n> So you rather propose to have \n> -\t\tshow_submodule_summary(o->file, one ? one->path : two->path,\n> +\t\tshow_submodule_summary(o->file, one->path ? one->path : two->path,\n\nI do. The reason is that one->path could be NULL (but not both one->path\nand two->path) and the summary would not be as helpful as possible if it\nwrote \"(null)\" instead of the path of the submodule.\n\nCiao,\nJohannes\n"},{"id":"224862","messageId":"87241414EE67485DAC10602B3BB85F2D@PhilipOakley","threadId":"34645","inReplyTo":"1375988103-29947-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] diff: remove another ternary expression always evaluating to true","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-08-08T21:36:51Z","receivedAt":"2013-08-08T21:36:51Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Stefan Beller\" \nSent: Thursday, August 08, 2013 7:55 PM\nSubject: [PATCH] diff: remove another ternary expression always \nevaluating to true\n\nHave these issues (and the earlier expression simplifications patches \n$gmane/231916, $gmane/231912 ) been discovered with the \"STACK\" tool I'd \nnoted in $gmane/230542 which you were also interested in (I've not had \nchance to run the tool).\n\nIf so, it's probably worth referencing the tool and the paper which \nexplains the broader issues.\n\nPhilip\n\n\n> The condition before the changed line dereferences 'one' to query the \n> mode,\n> so if the condition evaluates to true, the variable one must not be \n> null.\n> Therefore we do not need the ternary operator depending on one, giving\n> either one->path or two->path. This always evaluates to one->path, so\n> we can remove the ternary operator.\n>\n> The condition and the usage of the ternary operator have been \n> introduced\n> by the same commit (752c0c24, 2009-10-19, Add the --submodule option \n> to\n> the diff option family). As that commit message refers to a \n> GitTogether\n> I'd assume that patch was crafted in a hurry, so maybe overlooking the\n> need for a ternary operator there.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 80f8439..f30b7e4 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2252,8 +2252,7 @@ static void builtin_diff(const char *name_a,\n> (!two->mode || S_ISGITLINK(two->mode))) {\n> const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);\n> const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);\n> - show_submodule_summary(o->file, one ? one->path : two->path,\n> - line_prefix,\n> + show_submodule_summary(o->file, one->path, line_prefix,\n> one->sha1, two->sha1, two->dirty_submodule,\n> meta, del, add, reset);\n> return;\n> -- \n> 1.8.4.rc1.25.gd121ba2\n>\n> --\n"},{"id":"224867","messageId":"7v8v0b96xe.fsf@alter.siamese.dyndns.org","threadId":"34645","inReplyTo":"alpine.DEB.1.00.1308082335050.24252@s15462909.onlinehome-server.info","subject":"Re: [PATCH] diff: remove another ternary expression always evaluating to true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-08T22:03:25Z","receivedAt":"2013-08-08T22:03:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Stefan,\n>\n> On Thu, 8 Aug 2013, Stefan Beller wrote:\n>\n>> So you rather propose to have \n>> -\t\tshow_submodule_summary(o->file, one ? one->path : two->path,\n>> +\t\tshow_submodule_summary(o->file, one->path ? one->path : two->path,\n>\n> I do. The reason is that one->path could be NULL (but not both one->path\n> and two->path) and the summary would not be as helpful as possible if it\n> wrote \"(null)\" instead of the path of the submodule.\n\nGood.\n\nAlso some C libraries would choke when asked to %s format a NULL,\ninstead of giving \"(null)\" to avoid segfaulting (which I think is a\nquirk in glibc).\n"},{"id":"224868","messageId":"1375999913-4912-1-git-send-email-stefanbeller@googlemail.com","threadId":"34645","inReplyTo":"7v8v0b96xe.fsf@alter.siamese.dyndns.org","subject":"[PATCH] diff: fix a possible null pointer dereference","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-08T22:11:53Z","receivedAt":"2013-08-08T22:11:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The condition in the ternary operator was wrong, hence the wrong char\npointer could be used as the parameter for show_submodule_summary.\none->path may be null, but we definitely need a non null path given\nto the function.\n\nSigned-off-by: Stefan Beller \nAcked-By: Johannes Schindelin \n---\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 80f8439..061694b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2252,7 +2252,7 @@ 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\tshow_submodule_summary(o->file, one->path ? one->path : two->path,\n \t\t\t\tline_prefix,\n \t\t\t\tone->sha1, two->sha1, two->dirty_submodule,\n \t\t\t\tmeta, del, add, reset);\n-- \n1.8.4.rc1.25.gd121ba2\n"},{"id":"224869","messageId":"5204190D.9040604@googlemail.com","threadId":"34645","inReplyTo":"87241414EE67485DAC10602B3BB85F2D@PhilipOakley","subject":"Re: [PATCH] diff: remove another ternary expression always evaluating to true","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-08T22:17:49Z","receivedAt":"2013-08-08T22:17:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 08/08/2013 11:36 PM, Philip Oakley wrote:\n> From: \"Stefan Beller\" \n> Sent: Thursday, August 08, 2013 7:55 PM\n> Subject: [PATCH] diff: remove another ternary expression always\n> evaluating to true\n> \n> Have these issues (and the earlier expression simplifications patches\n> $gmane/231916, $gmane/231912 ) been discovered with the \"STACK\" tool I'd\n> noted in $gmane/230542 which you were also interested in (I've not had\n> chance to run the tool).\n> \n> If so, it's probably worth referencing the tool and the paper which\n> explains the broader issues.\n> \n> Philip\n> \n\nYes, those 3 issues have been discovered using the STACK tool\nThe paper regarding that tool can be found at\nhttp://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf\n\nIt's definitely an interesting read! (At least for me it was ;)\nThe authors intend to make that tool available to broader public\nas open source in August this year, iirc.\n\nHowever I do not know if their repository address was\nalready announced publicly, so I did not announce to have used it.\n(Last time I announced using static code analysis tools for\nmy patches the review-process was much harder as well :D)\n\nStefan\n\n"}]}