threads / discuss / 36321

Odd "git diff" breakage

Subject: Odd "git diff" breakage

## tl;dr

6 messages between Mar 31, 2014 and Mar 31, 2014.

replies: 5people: 2as markdown or json

Linus Torvalds· Mar 31, 2014, 18:05 UTC · lore

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· Mar 31, 2014, 18:30 UTC · re: Linus Torvalds · lore

Re: Odd "git diff" breakage

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 16 quoted lines

> 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· Mar 31, 2014, 18:35 UTC · re: Junio C Hamano · lore

Re: Odd "git diff" breakage

On Mon, Mar 31, 2014 at 11:30 AM, Junio C Hamano <gitster@pobox.com> 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· Mar 31, 2014, 18:38 UTC · re: Junio C Hamano · lore

Re: Odd "git diff" breakage

Junio C Hamano <gitster@pobox.com> writes:

Show 21 quoted lines

> Linus Torvalds <torvalds@linux-foundation.org> 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· Mar 31, 2014, 18:47 UTC · re: Linus Torvalds · lore

[PATCH] diff-no-index: correctly diagnose error return from diff_opt_parse()

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 <torvalds@linux-foundation.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * 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· Mar 31, 2014, 19:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] diff-no-index: correctly diagnose error return from diff_opt_parse()

On Mon, Mar 31, 2014 at 11:47 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

>
> 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