{"thread":{"id":"36321","subject":"Odd \"git diff\" breakage","startedAt":"2014-03-31T18:05:06Z","lastAt":"2014-03-31T19:14:24Z","messageCount":6,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"238120","messageId":"CA+55aFxYBDXs8mGQ3weR2PSOdMgOzMXPT=uWstL4c4BKnykkdA@mail.gmail.com","threadId":"36321","inReplyTo":null,"subject":"Odd \"git diff\" breakage","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-03-31T18:05:06Z","receivedAt":"2014-03-31T18:05:06Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"I hit this oddity when not remembering the right syntax for --color-words..\n\nTry this (outside of a git repository):\n\n touch a b\n git diff -u --color=words a b\n\nand watch it scroll (infinitely) printing out\n\n error: option `color' expects \"always\", \"auto\", or \"never\"\n\nforever.\n\nI haven't tried to root-cause it, since I'm supposed to be merging stuff..\n\n Linus\n"},{"id":"238122","messageId":"xmqq7g7a6x0g.fsf@gitster.dls.corp.google.com","threadId":"36321","inReplyTo":"CA+55aFxYBDXs8mGQ3weR2PSOdMgOzMXPT=uWstL4c4BKnykkdA@mail.gmail.com","subject":"Re: Odd \"git diff\" breakage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T18:30:55Z","receivedAt":"2014-03-31T18:30:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> I hit this oddity when not remembering the right syntax for --color-words..\n>\n> Try this (outside of a git repository):\n>\n> touch a b\n> git diff -u --color=words a b\n>\n> and watch it scroll (infinitely) printing out\n>\n> error: option `color' expects \"always\", \"auto\", or \"never\"\n>\n> forever.\n>\n> I haven't tried to root-cause it, since I'm supposed to be merging stuff..\n>\n> Linus\n\nHmph, interesting. \"outside a repository\" is the key, it seems.\nAnd I can see it all the way down to 1.7.3 (at least).\n"},{"id":"238123","messageId":"CA+55aFzjqsMzQxAJAK0DUM0DepE_qQSzSXyN=i9juUyzySM8oQ@mail.gmail.com","threadId":"36321","inReplyTo":"xmqq7g7a6x0g.fsf@gitster.dls.corp.google.com","subject":"Re: Odd \"git diff\" breakage","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-03-31T18:35:13Z","receivedAt":"2014-03-31T18:35:13Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Mar 31, 2014 at 11:30 AM, Junio C Hamano wrote:\n>\n> Hmph, interesting. \"outside a repository\" is the key, it seems.\n\nWell, you can do it inside a repository too, but then you need to use\nthe \"--no-index\" flag to get the \"diff two files\" behavior. It will\nresult in the same infinite error messages.\n\n Linus\n"},{"id":"238124","messageId":"xmqq38hy6wnk.fsf@gitster.dls.corp.google.com","threadId":"36321","inReplyTo":"xmqq7g7a6x0g.fsf@gitster.dls.corp.google.com","subject":"Re: Odd \"git diff\" breakage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T18:38:39Z","receivedAt":"2014-03-31T18:38:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Linus Torvalds writes:\n>\n>> I hit this oddity when not remembering the right syntax for --color-words..\n>>\n>> Try this (outside of a git repository):\n>>\n>> touch a b\n>> git diff -u --color=words a b\n>>\n>> and watch it scroll (infinitely) printing out\n>>\n>> error: option `color' expects \"always\", \"auto\", or \"never\"\n>>\n>> forever.\n>>\n>> I haven't tried to root-cause it, since I'm supposed to be merging stuff..\n>>\n>> Linus\n>\n> Hmph, interesting. \"outside a repository\" is the key, it seems.\n> And I can see it all the way down to 1.7.3 (at least).\n\ndiff_opt_parse() returns the number of options parsed, or often\nreturns error() which is defined to return -1. Yes, return value\nof 0 is \"I don't understand that option\", which should cause the\ncaller to say that, but negative return should not be forgotten.\n\n\n\n diff-no-index.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex c554691..265709b 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -198,7 +198,7 @@ void diff_no_index(struct rev_info *revs,\n \t\t\ti++;\n \t\telse {\n \t\t\tj = diff_opt_parse(&revs->diffopt, argv + i, argc - i);\n-\t\t\tif (!j)\n+\t\t\tif (j <= 0)\n \t\t\t\tdie(\"invalid diff option/value: %s\", argv[i]);\n \t\t\ti += j;\n \t\t}\n"},{"id":"238125","messageId":"xmqqy4zq5hoq.fsf@gitster.dls.corp.google.com","threadId":"36321","inReplyTo":"CA+55aFxYBDXs8mGQ3weR2PSOdMgOzMXPT=uWstL4c4BKnykkdA@mail.gmail.com","subject":"[PATCH] diff-no-index: correctly diagnose error return from diff_opt_parse()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T18:47:17Z","receivedAt":"2014-03-31T18:47:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"diff_opt_parse() returns the number of options parsed, or often\nreturns error() which is defined to return -1. Yes, return value of\n0 is \"I did not process that option at all\", which should cause the\ncaller to say that, but negative return should not be forgotten.\n\nThis bug the caused it to infinitely show the same error message\nbecause the returned value was used to decrement the loop control\nvariable, e.g.\n\n $ git diff --no-index --color=words a b\n error: option `color' expects \"always\", \"auto\", or \"never\"\n error: option `color' expects \"always\", \"auto\", or \"never\"\n ...\n\nInstead, make it act like so:\n\n $ git diff --no-index --color=words a b\n error: option `color' expects \"always\", \"auto\", or \"never\"\n fatal: invalid diff option/value: --color=words\n\nReported-by: Linus Torvalds \nSigned-off-by: Junio C Hamano \n---\n\n * The other callchain to this function comes from rev-list.c, and\n the caller does handle it correctly.\n\n diff-no-index.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 00a8eef..ecf9293 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -244,7 +244,7 @@ void diff_no_index(struct rev_info *revs,\n \t\t\ti++;\n \t\telse {\n \t\t\tj = diff_opt_parse(&revs->diffopt, argv + i, argc - i);\n-\t\t\tif (!j)\n+\t\t\tif (j <= 0)\n \t\t\t\tdie(\"invalid diff option/value: %s\", argv[i]);\n \t\t\ti += j;\n \t\t}\n-- \n1.9.1-513-gce53123\n"},{"id":"238130","messageId":"CA+55aFyLu88bjKuSOrWs5fNX=QsFNo9F1T6wRvp_Ogy+PpX5CQ@mail.gmail.com","threadId":"36321","inReplyTo":"xmqqy4zq5hoq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff-no-index: correctly diagnose error return from diff_opt_parse()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-03-31T19:14:24Z","receivedAt":"2014-03-31T19:14:24Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Mar 31, 2014 at 11:47 AM, Junio C Hamano wrote:\n>\n> Instead, make it act like so:\n>\n> $ git diff --no-index --color=words a b\n> error: option `color' expects \"always\", \"auto\", or \"never\"\n> fatal: invalid diff option/value: --color=words\n\nThanks,\n\n Linus\n"}]}