Yes, I have red what you wrote several times and tried your example.
I'm really sorry if I sound like I just ignored it. I just got a
little bit lost about which procedure needs patching. You're
absolutely right, queue_diff() is wrong place for it. So do you agree
that "append the name of the file at the end of the directory" logic
should be put to diff_no_index() which will also include calling
get_mode() for each path[] member? Sorry again for asking so much
questions
2015-03-16 19:14 GMT+02:00 Junio C Hamano <gitster@xxxxxxxxx>:
> Yurii Shevtsov <ungetch@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html