# Odd "git diff" breakage 6 messages from 2014-03-31 to 2014-03-31. Participants: Linus Torvalds, Junio C Hamano. Thread: https://gitlist.dev/t/36321 ## Linus Torvalds, 2014-03-31 18:05 Subject: Odd "git diff" breakage Message-ID: URL: https://gitlist.dev/e/CA%2B55aFxYBDXs8mGQ3weR2PSOdMgOzMXPT%3DuWstL4c4BKnykkdA%40mail.gmail.com ``` I hit this oddity when not remembering the right syntax for --color-words.. Try this (outside of a git repository): touch a b git diff -u --color=words a b and watch it scroll (infinitely) printing out error: option `color' expects "always", "auto", or "never" forever. I haven't tried to root-cause it, since I'm supposed to be merging stuff.. Linus ``` ## Junio C Hamano, 2014-03-31 18:30 Subject: Re: Odd "git diff" breakage Message-ID: URL: https://gitlist.dev/e/xmqq7g7a6x0g.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Linus Torvalds writes: > I hit this oddity when not remembering the right syntax for --color-words.. > > Try this (outside of a git repository): > > touch a b > git diff -u --color=words a b > > and watch it scroll (infinitely) printing out > > error: option `color' expects "always", "auto", or "never" > > forever. > > I haven't tried to root-cause it, since I'm supposed to be merging stuff.. > > Linus Hmph, interesting. "outside a repository" is the key, it seems. And I can see it all the way down to 1.7.3 (at least). ``` ## Linus Torvalds, 2014-03-31 18:35 Subject: Re: Odd "git diff" breakage Message-ID: URL: https://gitlist.dev/e/CA%2B55aFzjqsMzQxAJAK0DUM0DepE_qQSzSXyN%3Di9juUyzySM8oQ%40mail.gmail.com In-Reply-To: ``` On Mon, Mar 31, 2014 at 11:30 AM, Junio C Hamano wrote: > > Hmph, interesting. "outside a repository" is the key, it seems. Well, you can do it inside a repository too, but then you need to use the "--no-index" flag to get the "diff two files" behavior. It will result in the same infinite error messages. Linus ``` ## Junio C Hamano, 2014-03-31 18:38 Subject: Re: Odd "git diff" breakage Message-ID: URL: https://gitlist.dev/e/xmqq38hy6wnk.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > Linus Torvalds writes: > >> I hit this oddity when not remembering the right syntax for --color-words.. >> >> Try this (outside of a git repository): >> >> touch a b >> git diff -u --color=words a b >> >> and watch it scroll (infinitely) printing out >> >> error: option `color' expects "always", "auto", or "never" >> >> forever. >> >> I haven't tried to root-cause it, since I'm supposed to be merging stuff.. >> >> Linus > > Hmph, interesting. "outside a repository" is the key, it seems. > And I can see it all the way down to 1.7.3 (at least). diff_opt_parse() returns the number of options parsed, or often returns error() which is defined to return -1. Yes, return value of 0 is "I don't understand that option", which should cause the caller to say that, but negative return should not be forgotten. diff-no-index.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/diff-no-index.c b/diff-no-index.c index c554691..265709b 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -198,7 +198,7 @@ void diff_no_index(struct rev_info *revs, i++; else { j = diff_opt_parse(&revs->diffopt, argv + i, argc - i); - if (!j) + if (j <= 0) die("invalid diff option/value: %s", argv[i]); i += j; } ``` ## Junio C Hamano, 2014-03-31 18:47 Subject: [PATCH] diff-no-index: correctly diagnose error return from diff_opt_parse() Message-ID: URL: https://gitlist.dev/e/xmqqy4zq5hoq.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` diff_opt_parse() returns the number of options parsed, or often returns error() which is defined to return -1. Yes, return value of 0 is "I did not process that option at all", which should cause the caller to say that, but negative return should not be forgotten. This bug the caused it to infinitely show the same error message because the returned value was used to decrement the loop control variable, e.g. $ git diff --no-index --color=words a b error: option `color' expects "always", "auto", or "never" error: option `color' expects "always", "auto", or "never" ... Instead, make it act like so: $ git diff --no-index --color=words a b error: option `color' expects "always", "auto", or "never" fatal: invalid diff option/value: --color=words Reported-by: Linus Torvalds Signed-off-by: Junio C Hamano --- * The other callchain to this function comes from rev-list.c, and the caller does handle it correctly. diff-no-index.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/diff-no-index.c b/diff-no-index.c index 00a8eef..ecf9293 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -244,7 +244,7 @@ void diff_no_index(struct rev_info *revs, i++; else { j = diff_opt_parse(&revs->diffopt, argv + i, argc - i); - if (!j) + if (j <= 0) die("invalid diff option/value: %s", argv[i]); i += j; } -- 1.9.1-513-gce53123 ``` ## Linus Torvalds, 2014-03-31 19:14 Subject: Re: [PATCH] diff-no-index: correctly diagnose error return from diff_opt_parse() Message-ID: URL: https://gitlist.dev/e/CA%2B55aFyLu88bjKuSOrWs5fNX%3DQsFNo9F1T6wRvp_Ogy%2BPpX5CQ%40mail.gmail.com In-Reply-To: ``` On Mon, Mar 31, 2014 at 11:47 AM, Junio C Hamano wrote: > > Instead, make it act like so: > > $ git diff --no-index --color=words a b > error: option `color' expects "always", "auto", or "never" > fatal: invalid diff option/value: --color=words Thanks, Linus ```