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

[Junio C Hamano <gitster@pobox.com>]

Yurii Shevtsov <ungetch@gmail.com> writes:

>> ...  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.
> 
> So you want me to convert args ("diff D/ F" into "diff D/F F") before
> calling queue_diff()? But why?

Because it is wrong to do this inside queue_diff()?

Have you actually read what I wrote, tried the above sample
scenario, and thought what is happening in the codepath?

When the user asks to compare directory a/ and b/, the top-level
diff_no_index() would have paths[0]=="a" and paths[1]=="b", and
queue_diff() is called with these in name1 and name2.  Once it
learns that both of these are directories, it _recurses_ into itself
by appending the paths in these directories after these two names.
It finds that both of these directories have "sub" underneath, so it
makes a recursive call to itself to compare "a/sub" and "b/sub".

That call would notice that one is a directory and the other is
not.  That is where you are getting the "f/d conflict" error.

At that point, do you think it is sensible to rewrite that recursed
part of the diff into a comparison between "a/sub/sub" (which does
not exist, and which the user did not mean to compare with b/sub)
and "b/sub" (which is a file)?  I hope not.

> queue_diff() already check args' types and decides which
> comparison to do.

Yes, and I already hinted that that is an independent issue we may
want to fix, which I suspect is larger than GSoC Micro.  Also the
fix would be different.  Right now, it checks the types of paths and
then refuses to compare a directory and a file.  If we wanted to
change it to closer to what the rest of Git does, we would want it
to report that the directory and everything under it are removed and
then a new file is created (if the directory is on the left hand
side of the comparision and the file is on the right hand side) or
the other way around.  That will not involve "append the name of the
file at the end of the directory".

In short, "append the name of the file at the end of the directory"
logic has no place to live inside queue_diff() which handles the
recursion part of the operation.