Re: [PATCH] Improve errors from 'git diff --no-index'.

From: Anthony Foiani <anthony.foiani@gmail.com>
Date: May 23, 2011, 04:05 UTC
Message-ID: <BANLkTi=Uf8X+Bkd+CZ9qz0wXhHn8wwYzrA@mail.gmail.com>
In-Reply-To: <7vhb8mw1e6.fsf@alter.siamese.dyndns.org>

Junio --

On Sun, May 22, 2011 at 9:18 PM, Junio C Hamano <gitster@pobox.com> wrote:

> No, it was only your 'I tried to use "git diff"' that was confusing to me.

Apologies!

Show 28 quoted lines

> With pathspecs or any other options, it is understandable that you were
> confused, for example from an output like this:
>
>> $ git diff -b main.c
>> usage: git diff [--no-index] <path> <path>
>
> At that point we are committed to no-index codepath, so probably a good
> thing to do is to update that confusing usage message a bit better.
>
> It already does change the behaviour when --no-index is not given by the
> end user, so perhaps it would be sufficient to change this part:
>
>        if (argc != i + 2)
>                usagef("git diff %s <path> <path>",
>                        no_index ? "--no-index" : "[--no-index]");
>
> to something like (caution: I am not typing this in my MUA):
>
>        if (argc != i + 2) {
>                if (no_index)
>                        warning("Assuming '--no-index'; you are in no repository");
>                usagef("git diff %s <path> <path>",
>                        no_index ? "--no-index" : "[--no-index]");
>        }
>
> without changing anything else.
>
> That would be just a patch with 1 line removed, 4 lines added, no?

The "no_index" variable purely indicates whether or not "--no-index" was given on the command line; for this particular warning, you want "nongit" instead.

Alternately, you could simply do this above the "if (argc..." condition:

if (!no_index)
        warning("Assuming '--no-index'");

It wouldn't explain *why*, but would at least provide some sort of a hint to the user that they're maybe not doing what they thought they were doing.

If that's more to your taste, then I'd rather see this than nothing.  :)

You might also want to account for the fact that, even within a git repo, if "git diff" is invoked on two paths that are both outside the repo, then --no-index is forced. E.g.:

$ cd linux-2.6-stable/
$ git log -1 --pretty=short
commit 831d52bc153971b70e64eccfbed2b232394f22f8
Author: Suresh Siddha <suresh.b.siddha@intel.com>

    x86, mm: avoid possible bogus tlb entries by clearing prev
mm_cpumask after switching mm

$ ls -al /tmp/{foo,bar} -rw-rw-r--. 1 tony tony 0 May 22 10:09 /tmp/bar -rw-rw-r--. 1 tony tony 0 May 22 10:00 /tmp/foo

$ # current git tip: $ git diff /tmp/{foo,bar}

$ # with my patch: $ ../git/git-diff /tmp/{foo,bar} warning: neither '/tmp/foo' nor '/tmp/bar' are tracked, forcing --no-index

Although I apparently did break something...

$ touch bar $ git diff bar /tmp/foo $ ../git/git-diff bar /tmp/foo fatal: '/tmp/foo' is outside repository

Given that I broke things, and that a +100/-25 patch is a bit excessive, consider it retracted.

Thanks once again!

Best regards, Tony