Odd "git diff" breakage

Odd "git diff" breakage

[Linus Torvalds]

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

Re: Odd "git diff" breakage

[Junio C Hamano]

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

Re: Odd "git diff" breakage

[Linus Torvalds]

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

Re: Odd "git diff" breakage

[Junio C Hamano]

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

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

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

[Junio C Hamano]

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

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

[Linus Torvalds]

On Mon, Mar 31, 2014 at 11:47 AM, Junio C Hamano <gitster@pobox.com> 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