Re: [RFC] use typechange as rename source

From: Jeff King <peff@peff.net>
Date: Dec 1, 2007, 04:34 UTC
Message-ID: <20071201043407.GD30725@coredump.intra.peff.net>
In-Reply-To: <7vsl2n87jr.fsf@gitster.siamese.dyndns.org>

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

Show 7 quoted lines

> > 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