{"thread":{"id":"25499","subject":"[PATCH] diff: report bogus input to -C/-M/-B","startedAt":"2010-10-21T14:49:47Z","lastAt":"2010-10-21T20:37:29Z","messageCount":4,"participants":["Jeff King","Jonathan Nieder","Kevin Ballard"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"153968","messageId":"20101021144947.GA18210@sigill.intra.peff.net","threadId":"25499","inReplyTo":null,"subject":"[PATCH] diff: report bogus input to -C/-M/-B","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-10-21T14:49:47Z","receivedAt":"2010-10-21T14:49:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We already detect invalid input to these functions, but we\nsimply exit with an error code, never saying anything as\nsimple as \"your input was wrong\". Let's fix that.\n\nBefore:\n\n  $ git diff -CM\n  $ echo $?\n  128\n\nAfter:\n\n  $ git diff -CM\n  error: invalid argument to -C: M\n  $ echo $?\n  128\n\nThere should be no problems with having diff_opt_parse print\nto stderr, as there is already precedent in complaining\nabout bogus --color and --output arguments.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nMore old patch potpourri. Original thread:\n\n  http://article.gmane.org/gmane.comp.version-control.git/146570\n\nI think this one just got overlooked.\n\n diff.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 71efa8e..8487643 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3142,18 +3142,18 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t/* renames options */\n \telse if (!prefixcmp(arg, \"-B\")) {\n \t\tif ((options->break_opt = diff_scoreopt_parse(arg)) == -1)\n-\t\t\treturn -1;\n+\t\t\treturn error(\"invalid argument to -B: %s\", arg+2);\n \t}\n \telse if (!prefixcmp(arg, \"-M\")) {\n \t\tif ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n-\t\t\treturn -1;\n+\t\t\treturn error(\"invalid argument to -M: %s\", arg+2);\n \t\toptions->detect_rename = DIFF_DETECT_RENAME;\n \t}\n \telse if (!prefixcmp(arg, \"-C\")) {\n \t\tif (options->detect_rename == DIFF_DETECT_COPY)\n \t\t\tDIFF_OPT_SET(options, FIND_COPIES_HARDER);\n \t\tif ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n-\t\t\treturn -1;\n+\t\t\treturn error(\"invalid argument to -C: %s\", arg+2);\n \t\toptions->detect_rename = DIFF_DETECT_COPY;\n \t}\n \telse if (!strcmp(arg, \"--no-renames\"))\n-- \n1.7.3.1.235.gdd6c0\n"},{"id":"153981","messageId":"20101021165340.GE3732@burratino","threadId":"25499","inReplyTo":"20101021144947.GA18210@sigill.intra.peff.net","subject":"Re: [PATCH] diff: report bogus input to -C/-M/-B","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-21T16:53:40Z","receivedAt":"2010-10-21T16:53:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Before:\n> \n>   $ git diff -CM\n>   $ echo $?\n>   128\n> \n> After:\n> \n>   $ git diff -CM\n>   error: invalid argument to -C: M\n>   $ echo $?\n>   128\n\nYes, please.\n\nWho is it that exits(128) in this code path?  Are there other\nfunctions it calls that might return an error without reporting it to\nstderr?\n"},{"id":"153983","messageId":"20101021170744.GB15667@sigill.intra.peff.net","threadId":"25499","inReplyTo":"20101021165340.GE3732@burratino","subject":"Re: [PATCH] diff: report bogus input to -C/-M/-B","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-10-21T17:07:44Z","receivedAt":"2010-10-21T17:07:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 21, 2010 at 11:53:40AM -0500, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Before:\n> > \n> >   $ git diff -CM\n> >   $ echo $?\n> >   128\n> > \n> > After:\n> > \n> >   $ git diff -CM\n> >   error: invalid argument to -C: M\n> >   $ echo $?\n> >   128\n> \n> Yes, please.\n> \n> Who is it that exits(128) in this code path?  Are there other\n> functions it calls that might return an error without reporting it to\n> stderr?\n\nIt's setup_revisions(). It only happens when calling\nhandle_revision_opt(), which either succeeds, calls error() for its\nerror cases, or chains to diff_opt_parse(). The latter uses error()\nappropriately except for the -B/-M/-C cases.\n\nSo I think we're good after this patch is applied.\n\n-Peff\n"},{"id":"154018","messageId":"9E2CE0E8-40A4-4909-9317-B3DE9006474E@sb.org","threadId":"25499","inReplyTo":"20101021144947.GA18210@sigill.intra.peff.net","subject":"Re: [PATCH] diff: report bogus input to -C/-M/-B","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2010-10-21T20:37:29Z","receivedAt":"2010-10-21T20:37:29Z","isPatch":true,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"On Oct 21, 2010, at 7:49 AM, Jeff King wrote:\n\n> We already detect invalid input to these functions, but we\n> simply exit with an error code, never saying anything as\n> simple as \"your input was wrong\". Let's fix that.\n> \n> Before:\n> \n>  $ git diff -CM\n>  $ echo $?\n>  128\n> \n> After:\n> \n>  $ git diff -CM\n>  error: invalid argument to -C: M\n>  $ echo $?\n>  128\n> \n> There should be no problems with having diff_opt_parse print\n> to stderr, as there is already precedent in complaining\n> about bogus --color and --output arguments.\n\nThank you. I've had a note to myself to fix this for the past month, and still haven't gotten around to it. Glad to see that procrastinating pays off! ;)\n\n-Kevin Ballard"}]}