correct git merge behavior or corner case?

I have stumbled upon the following blog post via one of the news aggegrators and wondered whether the behavior is correct and expected or he's expecting something wrong or doing something wrong.

I cannot see a wrong usage pattern from what he has written.

http://blog.teksol.info/2009/04/15/beware-of-gits-content-tracking.html

as the author hasn't posted here after a couple of days I decided to take his question here for at least understanding what behavior he is experiencing.

Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:

Well, the author of the blog post gave us *no* information about what he did, or what he expected. He's just showing us a diff, which we're not even sure how he created, and then complaining about Git not doing something he wanted.

Hi,

On Mon, 20 Apr 2009, Tuncer Ayaz wrote:

First, a blog posting is a lousy place to post complaints. That just decreased my confidence level in the competence of Francois.

Second, he does not state what he expects to see instead. And for the love of God, I cannot see what should be wrong in the output he gets. Another few notches down.

Ciao, Dscho

Johannes Schindelin wrote:

I think that I managed to recreate what he is describing.

#!/bin/bash

cd $(mktemp -d repo.XXXXX)

git init

touch date git add date git commit -memptydate

git branch parallel

touch LICENSE git add LICENSE git commit -mLICENSE

git checkout parallel date >date git add date git commit -mdate

git checkout master git rm date git commit -mnodate

git merge parallel

cat LICENSE

On Mon, Apr 20, 2009 at 12:10:35PM +0200, Anders Melchiorsen wrote:

So basically one branch removes a file and adds an identical file under a different name, while the other branch modifies the original file. Git detects it as a rename, and applies the change from the second branch to the newly added file instead of generating a conflict.

This is _exactly_ what git's rename detection is designed to do. Yes, it seems horribly confusing in this toy example, but that is because it is a toy example: both 'date' and 'LICENSE' are empty files. But with real files, if a source file has actual content but is deleted, there is a new filename with the identical or near-identical content, and the patch applies to the new content without conflicts, then applying it there is probably exactly what you want.

The only complaint I have in that example is that there is nothing indicating to the user that the patch was applied to a renamed version. The output I get is:

  $ git merge parallel
  Merge made by recursive.
   LICENSE |    1 +
   1 files changed, 1 insertions(+), 0 deletions(-)

Perhaps a note indicating that it applied changes for "date" to "LICENSE" would be helpful.

-Peff

On Mon, Apr 20, 2009 at 10:44:33PM -0400, Jeff King wrote:

Looking back over the blog post, it seems that the original question was not about a toy example, but what looks like some boilerplate that involved empty files.

Maybe git should refuse to detect exact renames between empty files. That is easy enough to special-case, and would help people who have these sorts of boilerplate hierarchies. It would mean that we fail to automatically resolve something like:

  $ touch foo && git add foo && git commit -m boilerplate
  $ git branch other
  $ echo content >foo && git commit -m 'fill in boilerplate'
  $ git checkout other
  $ git mv foo bar && git commit -m reorganize
  $ git merge master

But the failure case is actually quite reasonable. We just mark it as a conflict, which is of course trivial for the user to resolve because the ancestor, by definition, had nothing in it.

-Peff

Jeff King <peff@peff.net> writes:

I had to briefly wonder what the fallout would be if we begin special- casing empty blobs excluded even from exact renames. We effectively do not consider fuzzy renames for blobs smaller than certain threshold, and sane projects would not have an empty file tracked anyway, so...

A much lessor impact change would be to keep the diffcore-rename as-is, so that it does detect exact renames between a pair of empty files, but special case it in merge-recursive. I think I like the latter approach better.

In any case, here is what the damage would look like...

 diffcore-rename.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

Heya,

On Tue, Apr 21, 2009 at 05:09, Junio C Hamano <gitster@pobox.com> wrote:

Except python projects that are full of empty __init__.py files.... no?

Hi,

On Tue, 21 Apr 2009, Sverre Rabbelier wrote:

But they would be a good example why we do _not_ want rename detection there.

I actually agree with Junio, though, that we want this special handling of empty files only in merge-recursive.

Ciao, Dscho

Heya,

On Tue, Apr 21, 2009 at 10:56, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Yes, I would agree that they are indeed a good example. The other day I moved a directory in a python project (containing several sub-directories and as such several empty __init__.py files), and it renamed them rather wrongly :P.

The diff looked something like: Renamed root/old/a/__init__.py to root/new/b/__init__.py Renamed root/old/b/__init__.py to root/new/c/__init__.py Renamed root/old/c/__init__.py to root/new/a/__init__.py

While of course, a better result would have been: Renamed root/old/a/__init__.py to root/new/a/__init__.py Renamed root/old/b/__init__.py to root/new/b/__init__.py Renamed root/old/c/__init__.py to root/new/c/__init__.py

Hi,

On Tue, 21 Apr 2009, Sverre Rabbelier wrote:

Heh, at some point I had a patch in my tree (dunno if it still there) to use Levenshtein for rename detection, that should have helped...

Ciao, Dscho

Hi,

On Tue, 21 Apr 2009, Johannes Schindelin wrote:

And this _might_ be enough (not even compile-tested due to lack of time; the OP did not provide the test as a proper patch):

---

 merge-recursive.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

This doesn't work for me (actually, it segfaults, "re" has just been declared). However, removing "re->" solves the problem.

---

 merge-recursive.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

And here is a test case:

---

 t/t6035-merge-suspected-rename.sh |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

On Tue, Apr 21, 2009 at 07:54:34PM +0200, Michał Kiedrowicz wrote:

I thought the point was about _empty_ renames. This is a small but non-zero rename.

-Peff

Jeff King <peff@peff.net> wrote:

Yes, you are right. But change these echos to touch and you'll see that this bug happens if date and LICENSE are the same, not necessary empty.

Michał Kiedrowicz

On Tue, Apr 21, 2009 at 08:47:01PM +0200, Michał Kiedrowicz wrote:

Right, because it is not exactly a bug, as I explained here:

  http://article.gmane.org/gmane.comp.version-control.git/117078

I say "not exactly a bug", because it is working as intended by the software authors, but because the intended behavior is to make a heuristic guess (some content moved from one filename to another, so we guess that a patch against the original content should actually go against the new location), the guess may not follow the user's expectations.

It is easy to see how the heuristic can be fooled in a toy example. But what we really care about is whether and how the heuristic is fooled in the real world. In the real world, there seems to be some non-trivial probability of removing an empty file, adding a new one elsewhere, and then merging with somebody who touched the empty file. So it may be worth improving the heuristic for this special case, especially because the harm done in a false negative is relatively small.

But what is the probability of doing the same thing to a file that has non-trivial contents? I would guess it is much less likely, and by special-casing it as a conflict, you have a much higher chance of bothering users who were relying on actual rename detection for their non-trivial case[1]. Of course, I don't have actual numbers, so I'm just guessing.

So my point is that while both are perhaps a failing of the heuristic, only one is going to be worth tweaking the heuristic for. So that is the one that should be included in the test case, since it is how somebody implementing a proposed tweak can test their tweak.

-Peff

[1] On a side note, this got me thinking about how git handles rename detection during merging. One of the things I like about git is that it tries to be very stupid: if something is questionable during a merge, it calls attention to it and makes it _easy_ for the user to access the versions and resolve (and I love mergetool for this). But renames are not like this: either they happen during auto-conflict-resolution, or they don't. I wonder if it might be a better strategy to barf on conflicts due removed files that could be resolved by questionable renames, but have a post-merge "renametool" that shows the user potential renames and lets them interactively specify the resolution. But maybe that would just be annoying, since 99% of the time, the rename detection gets it right.