Strange effect merging empty file

Hi,

I found a "strange effect" when merging from a branch containing a change of a previously empty file. The change is added to another empty file in the current branch by the merge.

I guess git sees it as a renamed file which is logical from a content-perspective.

Not sure what to do with this, I would not say it is a bug really...

Reproduce as follows (should be cut-n-paste friendly):

# Start with a new repository git init echo 'Readme file' > README git add README git commit -m 'Initial commit'

# Setup the branch to be merged git checkout -b import touch empty.txt echo hello world > hello.txt git add empty.txt hello.txt git commit -m 'Import 1.0' git tag IMPORT_1.0 echo This file is no longer empty > empty.txt git commit -m 'Import 1.1' empty.txt git tag IMPORT_1.1

# Setup master branch git checkout master mkdir static touch static/.gitignore git add static/.gitignore git commit -m 'Static web content'

# Merge import 1.0 and remove the empty file git merge IMPORT_1.0 git rm empty.txt git commit -m 'Remove empty file' empty.txt

# Merge import 1.1 and watch empty.txt contents show up in .gitignore git merge IMPORT_1.1 cat static/.gitignore

regards, Ralf

On 03/21/2012 11:28 AM, Ralf Nyren wrote:

It does seem like a bug. An unrelated file is clobbered.

Here's a slightly simplified version that is actually cut-n-paste friendly after recent changes to always edit merge comments...

# Start with a new repository git init echo Readme file >README git add README git commit -m 'Initial commit'

# Setup the branch to be merged git checkout -b import touch empty.txt echo hello world >hello.txt git add empty.txt hello.txt git commit -m 'Import 1.0' echo This file is no longer empty >empty.txt git commit -m 'Import 1.1' empty.txt

# Setup master branch git checkout master touch .gitignore git add .gitignore git commit -m 'Static web content'

# Merge import 1.0 and remove the empty file git merge --no-edit import^ git rm empty.txt git commit -m 'Remove empty file'

# Merge import 1.1 and watch empty.txt contents show up in .gitignore git merge --no-edit import cat .gitignore

Regards, Zbyszek

Zbigniew Jędrzejewski-Szmek  <zbyszek@in.waw.pl> writes:

Given what merge-recursive does, I am not very surprised, even though I agree that it feels like a very surprising end user experience ;-)

Zbigniew> touch .gitignore

If you're doing this to make sure git makes an empty directory, always put the directory name in there as a comment... that makes it (a) not an empty file (so merge doesn't get confused) and (b) unique, so when you change it, merge won't try to change other similar empty files.

On 22/03/12 13:17, Randal L. Schwartz wrote:

Good point.

Renaming directories makes the comment out-of-date but since uniqueness is what matters it should not be a problem.

regards, Ralf

On 03/22/2012 01:17 PM, Randal L. Schwartz wrote:

Yes, this will indeed fix this particular problem. But in general, empty files can be used for various reasons, and it can be a pretty nasty surprise if they sprout random content as a result of a merge.

Maybe merge-recursive could special-case empty files?

Zbyszek

On Thu, Mar 22, 2012 at 01:47:04PM +0100, Zbigniew Jędrzejewski-Szmek wrote:

I was thinking the same thing. And it seems this came up once before, and the list seemed to favor special-casing merge-recursive (but not diffcore):

  http://thread.gmane.org/gmane.comp.version-control.git/116917/focus=117082

-Peff

Jeff King <peff@peff.net> writes:

Yeah, thanks for digging up the old thread. I was looking at the patch to merge-recursive from Dscho on that thread and I think it identified the place that needs patching correctly. I was on a tablet, without the access to the surrounding code outside the patch context, so I do not know if the logic to detect the pure-rename of an empty file in the patch was correct, or the patch still applies to the current codebase, though.

On Thu, Mar 22, 2012 at 10:03:02AM -0700, Junio C Hamano wrote:

It's easy to apply the patch manually, and I have written a test. However, it seems to cause lots of other parts of t6022 to fail. I'll try to dig up the cause.

-Peff

On Thu, Mar 22, 2012 at 01:59:53PM -0400, Jeff King wrote:

Found it. The diff code is very smart about doing as little work as possible. For a raw diff (i.e., not patch), we can often get away with not loading the blob at all, and therefore have no idea what the size is. The inexact rename code may load it, of course, but any file which is an exact rename will have a "0" size, also.

We can get around it by just checking for the empty-blob sha1. The patch below should do the right thing, and passes the whole test suite.

---

On Thu, Mar 22, 2012 at 02:25:33PM -0400, Jeff King wrote:

Here it is broken up properly into commits:

  [1/3]: drop casts from users EMPTY_TREE_SHA1_BIN
  [2/3]: make is_empty_blob_sha1 available everywhere
  [3/3]: merge-recursive: don't detect renames from empty files

The first one is just a cleanup I noticed while adding EMPTY_BLOB_SHA1_BIN, and is not strictly related. The second one is the refactoring bits, and the third one is the real change.

-Peff

This macro already evaluates to the correct type, as it casts the string literal to "unsigned char *" itself (and callers who want the literal can use the _LITERAL form).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/diff.c    |    2 +-
 merge-recursive.c |    2 +-
 sequencer.c       |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

The read-cache implementation defines this static function, but it is a generally useful concept in git. Let's give the empty blob the same treatment as the empty tree, providing both hex and binary forms of the sha1.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h      |   13 +++++++++++++
 read-cache.c |   10 ----------
 2 files changed, 13 insertions(+), 10 deletions(-)

Merge-recursive detects renames so that if one side modifies "foo" and the other side moves it to "bar", the modification is applied to "bar". However, our rename detection is based on content analysis, it can be wrong (i.e., two files were not intended as a rename, but just happen to have the same or similar content).

This is quite rare if the files actually contain content, since two unrelated files are unlikely to have exactly the same content. However, empty files present a problem, in that there is nothing to analyze. An uninteresting placeholder file with zero bytes may or may not be related to a placeholder file with another name.

The result is that adding content to an empty file may cause confusion if the other side of a merge removed it; your content may end up in another random placeholder file that was added.

Let's err on the side of caution and not consider empty files as renames. This will cause a modify/delete conflict on the merge, which will let the user sort it out themselves.

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-recursive.c       |    2 +-
 t/t6022-merge-rename.sh |   16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

Jeff King wrote:

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.

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.

Thanks for a clean and pleasant patch. Jonathan

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

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.

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

Jeff King <peff@peff.net> writes:

Thanks.

The "I do not know if the logic is correct" reservation pays off ;-)

I still wonder why checking only the preimage side is sufficient, though. Shouldn't we check both sides?

On Thu, Mar 22, 2012 at 11:52:54AM -0700, Junio C Hamano wrote:

If the source is an empty file, then the other side must be an empty file, too, no? Otherwise it would not be a rename. It could not be exact, for obvious reasons. And it could not be inexact, because there is no content to analyze on the source side.

Ditto for the destination side. How could something be renamed to an empty file, but not be empty on the source side?

That is a slight layering violation, in that we are making assumptions about how the diffcore-rename subsystem works. In theory diffcore-rename could learn to read rename annotations from the commit message or something (bleh!). But then, we'd probably want to update merge-recursive in that instance to accept those "yes, it's definitely a rename" markers, even for an empty file.

I think it would be slightly cleaner to tell diffcore-rename "be conservative, and don't use empty files as sources" via an option. It's not as trivial as this one-liner, but it shouldn't be much more than the small patch you posted in the earlier thread.

-Peff

Jeff King <peff@peff.net> writes:

I do not think I have to say any more than that. The special case we want to have is for the "empty to empty" case and nothing else, and I do not want to see people having to remember to look at the merge-recursive code if/when rename detection starts to treat "empty to small" as "rename with minor modification."

On Thu, Mar 22, 2012 at 12:12:06PM -0700, Junio C Hamano wrote:

Here's a 2-patch series to replace the old 3/3 (they go on top of the first two cleanups from the previous iteration).

  [1/2]: teach diffcore-rename to optionally ignore empty content
  [2/2]: merge-recursive: don't detect renames of empty files

Thinking on this more, it is actually a more generic problem than just empty files. It is really a problem of having generic placeholder files with the same content. So a fully general solution would be something like a gitattribute for "don't do renames on this". However, in practice, these placeholder files are empty (since any non-empty file is likely to actually have different content). So I think just dropping the empty files as rename candidates is a pretty good heuristic, and it's nice and simple.

After responding to Jonathan, I'm on the fence about whether diff should follow the same heuristic. I left the diff behavior unchanged, but a 3/2 that turns it off by default would be a trivial one-liner.

-Peff

Our rename detection is a heuristic, matching pairs of removed and added files with similar or identical content. It's unlikely to be wrong when there is actual content to compare, and we already take care not to do inexact rename detection when there is not enough content to produce good results.

However, we always do exact rename detection, even when the blob is tiny or empty. It's easy to get false positives with an empty blob, simply because it is an obvious content to use as a boilerplate (e.g., when telling git that an empty directory is worth tracking via an empty .gitignore).

This patch lets callers specify whether or not they are interested in using empty files as rename sources and destinations. The default is "yes", keeping the original behavior. It works by detecting the empty-blob sha1 for rename sources and destinations.

One more flexible alternative would be to allow the caller to specify a minimum size for a blob to be "interesting" for rename detection. But that would catch small boilerplate files, not large ones (e.g., if you had the GPL COPYING file in many directories).

A better alternative would be to allow a "-rename" gitattribute to allow boilerplate files to be marked as such. I'll leave the complexity of that solution until such time as somebody actually wants it. The complaints we've seen so far revolve around empty files, so let's start with the simple thing.

Signed-off-by: Jeff King <peff@peff.net>
---
From the previous discussion, we know we could get away with just
dropping empty files from the rename_src list. However, doing it for
both the src and dst lists is a little more obvious and robust. And
since some of the rename detection is O(src*dst), keeping the lists
as small as possible is a good thing.

I added command-line triggers mostly for testing and debugging, and didn't bother to advertise them in the documentation. Obviously if we decide that diff should just have this behavior, this patch can be even smaller.

 diff.c            |    5 +++++
 diff.h            |    2 +-
 diffcore-rename.c |    6 ++++++
 3 files changed, 12 insertions(+), 1 deletion(-)

Merge-recursive detects renames so that if one side modifies "foo" and the other side moves it to "bar", the modification is applied to "bar". However, our rename detection is based on content analysis, it can be wrong (i.e., two files were not intended as a rename, but just happen to have the same or similar content).

This is quite rare if the files actually contain content, since two unrelated files are unlikely to have exactly the same content. However, empty files present a problem, in that there is nothing to analyze. An uninteresting placeholder file with zero bytes may or may not be related to a placeholder file with another name.

The result is that adding content to an empty file may cause confusion if the other side of a merge removed it; your content may end up in another random placeholder file that was added.

Let's err on the side of caution and not consider empty files as renames. This will cause a modify/delete conflict on the merge, which will let the user sort it out themselves.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-recursive.c       |    1 +
 t/t6022-merge-rename.sh |   16 ++++++++++++++++
 2 files changed, 17 insertions(+)

Jeff King <peff@peff.net> writes:

Hrm. As this is probably useful for older maintenance track, I would have preferred not to see the first one that touches sequencer.c that did not exist in the 1.7.7.x maintenance track, as the change is purely "cosmetic" and does not have anything to do with fixing the over-agressive merge.

I thought that our recommendation for keeping an otherwise empty directories around was to have .gitignore file with two entries in it, namely:

	.*
        *

So these files will be everywhere and without being empty, no?

But I tend to prefer the simplicity of limiting this to empty files anyway.

I am also torn, but somewhat in favor of avoiding random permutations of empty files.

I can think of only one situation somebody _could_ argue that detecting empty-to-empty rename is halfway sensible: when there is only one empty file that was removed, and one new empty file that was added. Even in such a case, the user could have done "rm this; >that", and depending on the nature of these two files, "mv this that" may not have made _any_ sense even if they are both empty, and that is why I said "halfway" sensible.

On Thu, Mar 22, 2012 at 04:37:05PM -0700, Junio C Hamano wrote:

I considered that, but assumed this was not a maint fix, but rather a new "feature" to improve the heuristics. That being said, the first patch is entirely unrelated. The others don't depend on it textually or semantically, and it does not need to be part of the series (I just happened to notice it while in the area).

I dunno. I have never recommended that, but maybe it is something people do.

Yeah. We might want to do a gitattributes thing on top, but I'd much rather have this in the meantime, as it seems like it handles the bulk of the complaints we have actually seen on the list.

-Peff

Jeff King <peff@peff.net> writes:

Ok. Perhaps I over-reacted to the issue, which sounded like a grave mismerge that can go unnoticed.