From: Anthony Foiani <anthony.foiani@gmail.com>
Date: Mon, 23 May 2011 04:05:36 GMT
Subject: Re: [PATCH] Improve errors from 'git diff --no-index'.
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!

> 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