Re: [PATCH 3/3] merge-recursive: don't detect renames from empty files

[Jeff King <peff@peff.net>]

On Thu, Mar 22, 2012 at 02:18:51PM -0500, Jonathan Nieder wrote:

> > We could do the same thing for general diff rename
> > detection. However, the stakes are much less high there, as
> > we are explicitly reporting the rename to the user. It's
> > only the automatic nature of merge-recursive that makes the
> > result confusing. So there's not as much need for caution
> > when just showing a diff.
> 
> The stakes may be different, but doesn't the same justification apply
> anyway?  If "git diff -M" chooses a random pairing to describe a
> renaming of multiple empty files, that seems just as confusing as
> merge-recursive making the same mistake.

Maybe "stakes" was not the best word. My thinking was something like the
following. Matching empty files is a heuristic. So sometimes it will be
right, and sometimes it will be wrong. We want to make sure that the
confusion caused by being wrong is less than the goodness caused by
being right.  When we find renames for a merge, the badness in being
wrong is quite high. And the lack of goodness in failing to be right is
not all that high; you'll get a conflict which will bring the issue to
the user's attention, and they can fall back to using "git diff" to
investigate the situation.

Whereas with a regular diff, the badness of being wrong is not very
high. The user sees the diff and says "Really? Stupid git, that wasn't a
rename". And the lack of goodness in failing to be right is somewhat
worse, because there is no way to fall back and ask git "what renames
would you have found if you relaxed the heuristics a bit more?"

Of course, one could make that fallback an option. And given that we
are, by definition, talking about trivial empty files, it's not like the
rename detection somehow makes the diff a whole lot nicer. It just says
"rename X to Y" instead of "deleted Y, added X". The real value in the
rename detection is seeing the interdiff between X and Y, but it would
always be empty in this case anyway.

So I could go either way.

> If adding this check in diffcore is more complicated, doing it in
> merge-recursive for now seems fine and prudent, but if we are doing it
> at the merge-recursive level just to be conservative then that seems
> like the wrong layer.

It's not really more complicated, and based on Junio's response, I think
we want to do it there anyway. Doing it unconditionally for diff and
merge actually would make the code even simpler, then.

-Peff