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

From: Jeff King <peff@peff.net>
Date: Mar 22, 2012, 21:53 UTC
Message-ID: <20120322215355.GA750@sigill.intra.peff.net>
In-Reply-To: <20120322191851.GA23293@burratino>

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

Show 11 quoted lines

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