{"thread":{"id":"37331","subject":"[PATCH] mailsplit.c: remove dead code","startedAt":"2014-08-11T21:11:21Z","lastAt":"2014-08-17T08:14:45Z","messageCount":9,"participants":["Stefan Beller","René Scharfe","Matthieu Moy","Junio C Hamano","Jonathan Nieder","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"247598","messageId":"1407791481-17410-1-git-send-email-stefanbeller@gmail.com","threadId":"37331","inReplyTo":null,"subject":"[PATCH] mailsplit.c: remove dead code","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-11T21:11:21Z","receivedAt":"2014-08-11T21:11:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This was found by coverity. (Id: 290001)\n\nthe variable 'output' is only assigned to a value inequal to NUL,\nafter all gotos to the corrupt label.\nTherefore we can conclude the two removed lines are actually dead code.\n\nSigned-off-by: Stefan Beller \n---\n builtin/mailsplit.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 06296d4..b499014 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -93,8 +93,6 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)\n \treturn status;\n \n corrupt:\n-\tif (output)\n-\t\tfclose(output);\n \tunlink(name);\n \tfprintf(stderr, \"corrupt mailbox\\n\");\n \texit(1);\n-- \n2.1.0.rc2\n"},{"id":"247613","messageId":"53EA430E.8050905@web.de","threadId":"37331","inReplyTo":"1407791481-17410-1-git-send-email-stefanbeller@gmail.com","subject":"Re: [PATCH] mailsplit.c: remove dead code","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-08-12T16:38:38Z","receivedAt":"2014-08-12T16:38:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.08.2014 um 23:11 schrieb Stefan Beller:\n> This was found by coverity. (Id: 290001)\n>\n> the variable 'output' is only assigned to a value inequal to NUL,\n> after all gotos to the corrupt label.\n> Therefore we can conclude the two removed lines are actually dead code.\n\nAfter reading the above for the first time I thought it meant the \nopposite of what's actually going on. Perhaps it's the placement of \n\"only\", the comma or a flawed understanding of grammar on my part?\n\nIn any case, there is only one way to reach the label named corrupt, and \nthe variable named output is always NULL if that branch is taken. That \nmeans the removed code was a no-op. With those two lines gone you also \ndon't need to initialize output anymore, by the way.\n\nAnd since there is only a single goto, you could move the three \nremaining error handling lines up to the if statement. Keeping \ncondition and dependent code together would be an improvement, I think.\n\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/mailsplit.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\n> index 06296d4..b499014 100644\n> --- a/builtin/mailsplit.c\n> +++ b/builtin/mailsplit.c\n> @@ -93,8 +93,6 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)\n> \treturn status;\n>\n> corrupt:\n> -\tif (output)\n> -\t\tfclose(output);\n> \tunlink(name);\n> \tfprintf(stderr, \"corrupt mailbox\\n\");\n> \texit(1);\n>\n"},{"id":"247624","messageId":"1407878487-23530-1-git-send-email-stefanbeller@gmail.com","threadId":"37331","inReplyTo":"53EA430E.8050905@web.de","subject":"[PATCH] mailsplit.c: remove dead code","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-12T21:21:27Z","receivedAt":"2014-08-12T21:21:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This was found by coverity. (Id: 290001)\n\nThe variable 'output' is assigned to a value\nafter all gotos to the corrupt label.\n\nRemove the goto by moving the errorhandling code to the\ncondition, which detects the error.\n\nSigned-off-by: Stefan Beller \nHelped-by: René Scharfe \n---\n builtin/mailsplit.c | 17 ++++++-----------\n 1 file changed, 6 insertions(+), 11 deletions(-)\n\nIf I understood correctly, this is what you had in mind?\n\nThanks,\nStefan\n\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 06296d4..763cda0 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -53,14 +53,16 @@ static int keep_cr;\n */\n static int split_one(FILE *mbox, const char *name, int allow_bare)\n {\n-\tFILE *output = NULL;\n+\tFILE *output;\n \tint fd;\n \tint status = 0;\n \tint is_bare = !is_from_line(buf.buf, buf.len);\n \n-\tif (is_bare && !allow_bare)\n-\t\tgoto corrupt;\n-\n+\tif (is_bare && !allow_bare) {\n+\t\tunlink(name);\n+\t\tfprintf(stderr, \"corrupt mailbox\\n\");\n+\t\texit(1);\n+\t}\n \tfd = open(name, O_WRONLY | O_CREAT | O_EXCL, 0666);\n \tif (fd < 0)\n \t\tdie_errno(\"cannot open output file '%s'\", name);\n@@ -91,13 +93,6 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)\n \t}\n \tfclose(output);\n \treturn status;\n-\n- corrupt:\n-\tif (output)\n-\t\tfclose(output);\n-\tunlink(name);\n-\tfprintf(stderr, \"corrupt mailbox\\n\");\n-\texit(1);\n }\n \n static int populate_maildir_list(struct string_list *list, const char *path)\n-- \n2.1.0.rc2\n"},{"id":"247625","messageId":"vpqy4utz8x5.fsf@anie.imag.fr","threadId":"37331","inReplyTo":"1407878487-23530-1-git-send-email-stefanbeller@gmail.com","subject":"Re: [PATCH] mailsplit.c: remove dead code","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-12T21:37:26Z","receivedAt":"2014-08-12T21:37:26Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Stefan Beller writes:\n\n> +\tif (is_bare && !allow_bare) {\n> +\t\tunlink(name);\n> +\t\tfprintf(stderr, \"corrupt mailbox\\n\");\n> +\t\texit(1);\n> +\t}\n\nNot directly related to your patch, but shouldn't this be using\n\ndie(_(\"corrupt mailbox\"));\n\ninstead?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247626","messageId":"xmqqvbpx4cd6.fsf@gitster.dls.corp.google.com","threadId":"37331","inReplyTo":"vpqy4utz8x5.fsf@anie.imag.fr","subject":"Re: [PATCH] mailsplit.c: remove dead code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-12T21:38:45Z","receivedAt":"2014-08-12T21:38:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Stefan Beller writes:\n>\n>> +\tif (is_bare && !allow_bare) {\n>> +\t\tunlink(name);\n>> +\t\tfprintf(stderr, \"corrupt mailbox\\n\");\n>> +\t\texit(1);\n>> +\t}\n>\n> Not directly related to your patch, but shouldn't this be using\n>\n> die(_(\"corrupt mailbox\"));\n>\n> instead?\n\nDoesn't the exit status of mailsplit matter to its existing\ninvokers?\n"},{"id":"247627","messageId":"CAPc5daVuO9_wNCSzTi2PFb5JGKSw_awdsUaFNZS_S7v89fAZHg@mail.gmail.com","threadId":"37331","inReplyTo":"xmqqvbpx4cd6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] mailsplit.c: remove dead code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-12T21:50:54Z","receivedAt":"2014-08-12T21:50:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"... answering to myself, the only invoker does not seem to care,\nso I do not mind if the fprintf/exit gets turned into die() in a\nfollow-up patch.\n\nThanks.\n\nOn Tue, Aug 12, 2014 at 2:38 PM, Junio C Hamano wrote:\n> Matthieu Moy writes:\n>\n>> Stefan Beller writes:\n>>\n>>> + if (is_bare && !allow_bare) {\n>>> + unlink(name);\n>>> + fprintf(stderr, \"corrupt mailbox\\n\");\n>>> + exit(1);\n>>> + }\n>>\n>> Not directly related to your patch, but shouldn't this be using\n>>\n>> die(_(\"corrupt mailbox\"));\n>>\n>> instead?\n>\n> Doesn't the exit status of mailsplit matter to its existing\n> invokers?\n"},{"id":"247628","messageId":"vpqd2c5z81w.fsf@anie.imag.fr","threadId":"37331","inReplyTo":"xmqqvbpx4cd6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] mailsplit.c: remove dead code","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-12T21:56:11Z","receivedAt":"2014-08-12T21:56:11Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> Stefan Beller writes:\n>>\n>>> +\tif (is_bare && !allow_bare) {\n>>> +\t\tunlink(name);\n>>> +\t\tfprintf(stderr, \"corrupt mailbox\\n\");\n>>> +\t\texit(1);\n>>> +\t}\n>>\n>> Not directly related to your patch, but shouldn't this be using\n>>\n>> die(_(\"corrupt mailbox\"));\n>>\n>> instead?\n>\n> Doesn't the exit status of mailsplit matter to its existing\n> invokers?\n\nNot within Git. I doubt anybody checks if the exit status is 1 and\nrelies on that for corrupt mailboxes, but OTOH, changing the code may\nnot be worth the trouble.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247632","messageId":"20140812225826.GC24621@google.com","threadId":"37331","inReplyTo":"1407878487-23530-1-git-send-email-stefanbeller@gmail.com","subject":"Re: [PATCH] mailsplit.c: remove dead code","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-08-12T22:58:26Z","receivedAt":"2014-08-12T22:58:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> This was found by coverity. (Id: 290001)\n[...]\n> Signed-off-by: Stefan Beller \n> Helped-by: René Scharfe \n> ---\n> builtin/mailsplit.c | 17 ++++++-----------\n> 1 file changed, 6 insertions(+), 11 deletions(-)\n\nThe idea is to simplify error handling (removing cleanup of the\n'output' var) by making it more obvious that there is only one kind of\nerror, right?\n\nFor what it's worth, it looks good to me (and much clearer than the\nlast version). It's always possible to reintroduce goto-based error\nhandling later if another kind of error is introduced, and in the\nmeantime this is simpler and does not change behavior.\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"247809","messageId":"20140817081444.GI23808@peff.net","threadId":"37331","inReplyTo":"53EA430E.8050905@web.de","subject":"Re: [PATCH] mailsplit.c: remove dead code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-17T08:14:45Z","receivedAt":"2014-08-17T08:14:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 12, 2014 at 06:38:38PM +0200, René Scharfe wrote:\n\n> Am 11.08.2014 um 23:11 schrieb Stefan Beller:\n> >This was found by coverity. (Id: 290001)\n> >\n> >the variable 'output' is only assigned to a value inequal to NUL,\n> >after all gotos to the corrupt label.\n> >Therefore we can conclude the two removed lines are actually dead code.\n> \n> After reading the above for the first time I thought it meant the opposite\n> of what's actually going on. Perhaps it's the placement of \"only\", the\n> comma or a flawed understanding of grammar on my part?\n> \n> In any case, there is only one way to reach the label named corrupt, and the\n> variable named output is always NULL if that branch is taken. That means\n> the removed code was a no-op. With those two lines gone you also don't need\n> to initialize output anymore, by the way.\n> \n> And since there is only a single goto, you could move the three remaining\n> error handling lines up to the if statement. Keeping condition and\n> dependent code together would be an improvement, I think.\n\nI think that would be a correct refactoring of the current code, but I\nhave to wonder why the other die cases are not using \"goto corrupt\" in\nthe first place.\n\nThe other thing this code path does is unlink the file \"name\". In the\ncurrent code, this is _also_ a noop. We \"goto corrupt\" before we\nactually open the output file. So like the fclose(output), it is\ncleaning up an operation that was never started. It can just go away.\n\nBut the bigger question is: should the other code paths be cleaning up\nthe file? It probably doesn't matter, as mailsplit is typically run in\na temporary directory in the first place, so it is up to the caller to\nclean up any half-formed cruft. And if we did want to clean up cruft, we\nshould probably do it with an atexit/signal handler to catch more cases.\n\nGiven that we are not cleaning up now and nobody has complained, I'd be\ninclined to say we should not. And the unlink can just go away, and all\nerrors can just call die().\n\n-Peff\n"}]}