Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 16, 2015, 03:50 UTC
Message-ID: <xmqqr3spsir2.fsf@gitster.dls.corp.google.com>
In-Reply-To: <vpq1tkq5fsw.fsf@anie.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 13 quoted lines

>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -97,8 +97,25 @@ static int queue_diff(struct diff_options *o,
>>         if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
>>                 return -1;
>>
>> -       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))
>> -               return error("file/directory conflict: %s, %s", name1, name2);
>
> I'm surprised to see this error message totally go away. The idea of the
> microproject was to DWIM (do what I mean) better, but the dwim should
> apply only when $directory/$file actually exists. Otherwise, the error
> message should actually be raised.

I actually think this check, when we really fixed "diff --no-index" to work sensibly, should go away and be replaced with something sensible. As it stands now, even before we think about dwimming "diff D/ F" into "diff D/F F", a simple formulation like this will error out.

    $ mkdir -p a/sub b
    $ touch a/file b/file b/sub a/sub/file
    $ git diff --no-index a b
    error: file/directory conflict: a/sub, b/sub

Admittedly, that is how ordinary "diff -r" works, but I am not sure if we want to emulate that aspect of GNU diff. If the old tree a has a directory 'sub' with 'file' under it (i.e. a/sub/file) where the new tree has a file at 'sub', then the recursive diff can show the removal of sub/file and creation of sub, no? That is what we show for normal "git diff".

But I _think_ fixing that is way outside the scope of GSoC Micro.

And patching this function, because it is recursively called from within it, to "dwim" is simply wrong. When we see a/sub that is a directory and b/sub that is a file, we do *NOT* want to rewrite the comparison to comparison between a/sub/sub and b/sub.

What needs to be fixed for the Micro is the top-level call to queue_diff() that is made blindly between paths[0] and paths[1] without checking what kind of things these are.