Re: [RFC] use typechange as rename source

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

Jeff King <peff@peff.net> writes:

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

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

If we did the "automatic break of typechange" early, instead of your
patch, when we come to the register_rename_src() loop, one half of the
broken pair (i.e. "create a new symlink here") will be processed
in this part of the loop:

		if (!DIFF_FILE_VALID(p->one)) {
			if (!DIFF_FILE_VALID(p->two))
				continue; /* unmerged */
			else if (options->single_follow &&
				 strcmp(options->single_follow, p->two->path))
				continue; /* not interested */
			else
				locate_rename_dst(p->two, 1);
		}

and rename_dst is registered here.  The other half (i.e. "remove the
regular file") will be caught by this part in the loop:

		else if (!DIFF_FILE_VALID(p->two)) {
			/*
			 * If the source is a broken "delete", and
			 * they did not really want to get broken,
			 * that means the source actually stays.
			 * So we increment the "rename_used" score
			 * by one, to indicate ourselves as a user
			 */
			if (p->broken_pair && !p->score)
				p->one->rename_used++;
			register_rename_src(p->one, p->score);
		}

to register a source candidate.

Instead your patch does that with a single:

+		else if (DIFF_PAIR_TYPE_CHANGED(p)) {
+			p->one->rename_used++;
+			register_rename_src(p->one, p->score);
+		}

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.

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.