Re: [RFC] use typechange as rename source

On Fri, Nov 30, 2007 at 06:36:56PM -0800, Junio C Hamano wrote:

quoted

I have always been a bit confused about diffcore-break, so I am probably
misunderstanding what you mean. But are you saying that
diffcore-break.c:should_break should return 1 for typechanges?

What I had in mind was to do something like that in spirit, but instead
break such a filepair inside diffcore-rename (iow, even when the user
did not say -B) early on.

Ah, I see. BTW, I totally screwed up the tests I did earlier. Returning
1 from should_break _does_ produce the same results for my simple case
(copy + typechange).

But after re-reading your patch and the surrounding code, that is
more or less what you are doing (without actually recording the extra
broken pair to be merged back later).

I don't think we need to, because they are never actually "broken"; we
simply consider the source a candidate for renaming, but keep the pair
together to note the typechange.

which is essentially doing the same thing but only for the "remove the
regular file" half.  One has to wonder how the lack of handling the
other half affects the outcome and still produce a result more intuitive
than the current code.

AIUI, because we never broke the pair in the first place, we don't need
to look for a source for that dest (the "add a new symlink" half). It's
already part of the same filepair.

Whether this is by design or simply a happy accident that we record both
renames and typechanges in diff_filepairs, I'm not sure. Or perhaps I'm
totally misunderstanding how the breaking works.

In your test case, the "new" symlink won't have any similar symlink that
is removed from the preimage, so registering it as a rename destination
would not make a difference (it will say "no match found, so create this
as usual"), but I am not convinced if that would work well in general.

I don't know that it makes a difference. We are impacting only a
'typechange', which implies that we have a filepair in which both p->one
and p->two are valid; thus, the current code doesn't use it as a rename
dst at all.

-Peff