{"thread":{"id":"23735","subject":"git diff flags","startedAt":"2010-05-07T19:41:56Z","lastAt":"2010-05-08T04:24:26Z","messageCount":3,"participants":["Eli Barzilay","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"141158","messageId":"19428.27908.870639.422432@winooski.ccs.neu.edu","threadId":"23735","inReplyTo":null,"subject":"git diff flags","fromName":"Eli Barzilay","fromEmail":"eli@barzilay.org","sentAt":"2010-05-07T19:41:56Z","receivedAt":"2010-05-07T19:41:56Z","isPatch":false,"sender":{"key":"eli@barzilay.org","avatar":"https://avatars.githubusercontent.com/u/185905?v=4"},"body":"There's something strange with how `git diff' parses its flags:\n\n  git diff --some-bogus-flag\n  --> complains as expected\n\n  git diff --follow\n  --> works but there's no mention of this in the man page, and it\n      doesn't look like it's doing anything\n\n  git diff -C -M\n  --> works as expected too\n\n  git diff -CM\n  --> doesn't say anything (but it does exit with an error code)\n\n  git diff -CM --I-can-write-anything here!\n  --> does the same\n\n-- \n          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:\n                    http://barzilay.org/                   Maze is Life!\n"},{"id":"141221","messageId":"20100508040602.GA14998@coredump.intra.peff.net","threadId":"23735","inReplyTo":"19428.27908.870639.422432@winooski.ccs.neu.edu","subject":"Re: git diff flags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-08T04:06:02Z","receivedAt":"2010-05-08T04:06:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 07, 2010 at 03:41:56PM -0400, Eli Barzilay wrote:\n\n> There's something strange with how `git diff' parses its flags:\n\nYep.\n\n>   git diff --some-bogus-flag\n>   --> complains as expected\n\nYep.\n\n>   git diff --follow\n>   --> works but there's no mention of this in the man page, and it\n>       doesn't look like it's doing anything\n\n--follow is a \"diff option\" even though it is about revision\ntraversal. This is an artifact of the implementation, where\nrename-following happens at a low level where we have only diff_options.\nIt should be possible to fix, though (parse the option at the revision\nlayer, but have it set the diff_options flag).\n\n>   git diff -C -M\n>   --> works as expected too\n\nYep.\n\n>   git diff -CM\n>   --> doesn't say anything (but it does exit with an error code)\n\nThe revision and diff parsers do not use parse_options, and thus don't\nunderstand things like bundling. Even if they did, -C takes an optional\nargument. In this case, the argument format is bogus, so it aborts, but\nfor some reason there is no error message (it is the same with -B, -M,\nand -C).\n\n>   git diff -CM --I-can-write-anything here!\n>   --> does the same\n\nBecause we barfed at -CM already. It only looks funny because we didn't\nbother to print a message.\n\nPatches welcome.\n\n-Peff\n"},{"id":"141222","messageId":"20100508042426.GB14998@coredump.intra.peff.net","threadId":"23735","inReplyTo":"20100508040602.GA14998@coredump.intra.peff.net","subject":"Re: git diff flags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-08T04:24:26Z","receivedAt":"2010-05-08T04:24:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 08, 2010 at 12:06:02AM -0400, Jeff King wrote:\n\n> >   git diff --follow\n> >   --> works but there's no mention of this in the man page, and it\n> >       doesn't look like it's doing anything\n> \n> --follow is a \"diff option\" even though it is about revision\n> traversal. This is an artifact of the implementation, where\n> rename-following happens at a low level where we have only diff_options.\n> It should be possible to fix, though (parse the option at the revision\n> layer, but have it set the diff_options flag).\n\nActually, that doesn't help. \"git diff\" actually uses setup_revisions,\nso many revision traversal options are parsed and ignored. E.g.:\n\n  $ git diff --pretty=short\n\nwill happily run and do nothing with the --pretty=short bit, as we are\nnot actually showing commits.\n\n> >   git diff -CM\n> >   --> doesn't say anything (but it does exit with an error code)\n> \n> The revision and diff parsers do not use parse_options, and thus don't\n> understand things like bundling. Even if they did, -C takes an optional\n> argument. In this case, the argument format is bogus, so it aborts, but\n> for some reason there is no error message (it is the same with -B, -M,\n> and -C).\n> \n> >   git diff -CM --I-can-write-anything here!\n> >   --> does the same\n> \n> Because we barfed at -CM already. It only looks funny because we didn't\n> bother to print a message.\n> \n> Patches welcome.\n\nActually, this patch is really quite trivial. Here it is.\n\n-- >8 --\nSubject: [PATCH] diff: report bogus input to -C/-M/-B\n\nWe 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---\nActually, --output seems to call die(). We could do that, too, and it\nwould give us the slightly nicer:\n\n  fatal: invalid argument to -C: M\n\nThough I guess that is slightly less libified, setup_revisions just\nexits anyway.\n\n diff.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e40c127..0c955be 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2783,18 +2783,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.1.176.gcff095.dirty\n"}]}