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

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

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.

Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<[email protected]> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.posterous.com/ for Smalltalk discussion

Good point.

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

regards, Ralf

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

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

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.

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

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.

---

index e5e1aa4..61671b6 100644
@@ -708,6 +708,8 @@ static inline void hashclr(unsigned char *hash)
 #define EMPTY_TREE_SHA1_BIN \
 	 ((const unsigned char *) EMPTY_TREE_SHA1_BIN_LITERAL)
 
+int is_empty_blob_sha1(const unsigned char *sha1);
+
 int git_mkstemp(char *path, size_t n, const char *template);
 
 int git_mkstemps(char *path, size_t n, const char *template, int suffix_len);

index 6479a60..ed4ff16 100644
@@ -502,7 +502,7 @@ static struct string_list *get_renames(struct merge_options *o,
 		struct string_list_item *item;
 		struct rename *re;
 		struct diff_filepair *pair = diff_queued_diff.queue[i];
-		if (pair->status != 'R') {
+		if (pair->status != 'R' || is_empty_blob_sha1(pair->one->sha1)) {
 			diff_free_filepair(pair);
 			continue;
 		}

index 274e54b..dfabad0 100644
@@ -157,7 +157,7 @@ static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)
 	return 0;
 }
 
-static int is_empty_blob_sha1(const unsigned char *sha1)
+int is_empty_blob_sha1(const unsigned char *sha1)
 {
 	static const unsigned char empty_blob_sha1[20] = {
 		0xe6,0x9d,0xe2,0x9b,0xb2,0xd1,0xd6,0x43,0x4b,0x8b,

index 9d8584e..1104249 100755
@@ -884,4 +884,20 @@ test_expect_success 'no spurious "refusing to lose untracked" message' '
 	! grep "refusing to lose untracked file" errors.txt
 '
 
+test_expect_success 'do not follow renames for empty files' '
+	git checkout -f -b empty-base &&
+	>empty1 &&
+	git add empty1 &&
+	git commit -m base &&
+	echo content >empty1 &&
+	git add empty1 &&
+	git commit -m fill &&
+	git checkout -b empty-topic HEAD^ &&
+	git mv empty1 empty2 &&
+	git commit -m rename &&
+	test_must_fail git merge empty-base &&
+	>expect &&
+	test_cmp expect empty2
+'
+
 test_done

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

---
 builtin/diff.c    |    2 +-
 merge-recursive.c |    2 +-
 sequencer.c       |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

index 424c815..9069dc4 100644
@@ -327,7 +327,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 				add_head_to_pending(&rev);
 				if (!rev.pending.nr) {
 					struct tree *tree;
-					tree = lookup_tree((const unsigned char*)EMPTY_TREE_SHA1_BIN);
+					tree = lookup_tree(EMPTY_TREE_SHA1_BIN);
 					add_pending_object(&rev, &tree->object, "HEAD");
 				}
 				break;

index 6479a60..318d32e 100644
@@ -1914,7 +1914,7 @@ int merge_recursive(struct merge_options *o,
 		/* if there is no common ancestor, use an empty tree */
 		struct tree *tree;
 
-		tree = lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
+		tree = lookup_tree(EMPTY_TREE_SHA1_BIN);
 		merged_common_ancestors = make_virtual_commit(tree, "ancestor");
 	}
 

index a37846a..4307364 100644
@@ -164,7 +164,7 @@ static void write_message(struct strbuf *msgbuf, const char *filename)
 
 static struct tree *empty_tree(void)
 {
-	return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
+	return lookup_tree(EMPTY_TREE_SHA1_BIN);
 }
 
 static int error_dirty_index(struct replay_opts *opts)

1.7.10.rc0.9.gdcbe9

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.

---
 cache.h      |   13 +++++++++++++
 read-cache.c |   10 ----------
 2 files changed, 13 insertions(+), 10 deletions(-)

index e5e1aa4..5280574 100644
@@ -708,6 +708,19 @@ static inline void hashclr(unsigned char *hash)
 #define EMPTY_TREE_SHA1_BIN \
 	 ((const unsigned char *) EMPTY_TREE_SHA1_BIN_LITERAL)
 
+#define EMPTY_BLOB_SHA1_HEX \
+	"e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"
+#define EMPTY_BLOB_SHA1_BIN_LITERAL \
+	"\xe6\x9d\xe2\x9b\xb2\xd1\xd6\x43\x4b\x8b" \
+	"\x29\xae\x77\x5a\xd8\xc2\xe4\x8c\x53\x91"
+#define EMPTY_BLOB_SHA1_BIN \
+	((const unsigned char *) EMPTY_BLOB_SHA1_BIN_LITERAL)
+
+static inline int is_empty_blob_sha1(const unsigned char *sha1)
+{
+	return !hashcmp(sha1, EMPTY_BLOB_SHA1_BIN);
+}
+
 int git_mkstemp(char *path, size_t n, const char *template);
 
 int git_mkstemps(char *path, size_t n, const char *template, int suffix_len);

index 274e54b..6c8f395 100644
@@ -157,16 +157,6 @@ static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)
 	return 0;
 }
 
-static int is_empty_blob_sha1(const unsigned char *sha1)
-{
-	static const unsigned char empty_blob_sha1[20] = {
-		0xe6,0x9d,0xe2,0x9b,0xb2,0xd1,0xd6,0x43,0x4b,0x8b,
-		0x29,0xae,0x77,0x5a,0xd8,0xc2,0xe4,0x8c,0x53,0x91
-	};
-
-	return !hashcmp(sha1, empty_blob_sha1);
-}
-
 static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 {
 	unsigned int changed = 0;

1.7.10.rc0.9.gdcbe9

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.

---
 merge-recursive.c       |    2 +-
 t/t6022-merge-rename.sh |   16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

index 318d32e..0255f50 100644
@@ -502,7 +502,7 @@ static struct string_list *get_renames(struct merge_options *o,
 		struct string_list_item *item;
 		struct rename *re;
 		struct diff_filepair *pair = diff_queued_diff.queue[i];
-		if (pair->status != 'R') {
+		if (pair->status != 'R' || is_empty_blob_sha1(pair->one->sha1)) {
 			diff_free_filepair(pair);
 			continue;
 		}

index 9d8584e..1104249 100755
@@ -884,4 +884,20 @@ test_expect_success 'no spurious "refusing to lose untracked" message' '
 	! grep "refusing to lose untracked file" errors.txt
 '
 
+test_expect_success 'do not follow renames for empty files' '
+	git checkout -f -b empty-base &&
+	>empty1 &&
+	git add empty1 &&
+	git commit -m base &&
+	echo content >empty1 &&
+	git add empty1 &&
+	git commit -m fill &&
+	git checkout -b empty-topic HEAD^ &&
+	git mv empty1 empty2 &&
+	git commit -m rename &&
+	test_must_fail git merge empty-base &&
+	>expect &&
+	test_cmp expect empty2
+'
+
 test_done

1.7.10.rc0.9.gdcbe9

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

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

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?

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

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

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.

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

index 377ec1e..0b70aad 100644
@@ -3136,6 +3136,7 @@ void diff_setup(struct diff_options *options)
 	options->rename_limit = -1;
 	options->dirstat_permille = diff_dirstat_permille_default;
 	options->context = 3;
+	DIFF_OPT_SET(options, RENAME_EMPTY);
 
 	options->change = diff_change;
 	options->add_remove = diff_addremove;

@@ -3506,6 +3507,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	}
 	else if (!strcmp(arg, "--no-renames"))
 		options->detect_rename = 0;
+	else if (!strcmp(arg, "--rename-empty"))
+		DIFF_OPT_SET(options, RENAME_EMPTY);
+	else if (!strcmp(arg, "--no-rename-empty"))
+		DIFF_OPT_CLR(options, RENAME_EMPTY);
 	else if (!strcmp(arg, "--relative"))
 		DIFF_OPT_SET(options, RELATIVE_NAME);
 	else if (!prefixcmp(arg, "--relative=")) {

index cb68743..dd48eca 100644
@@ -60,7 +60,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_SILENT_ON_REMOVE    (1 <<  5)
 #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
 #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)
-/* (1 <<  8) unused */
+#define DIFF_OPT_RENAME_EMPTY        (1 <<  8)
 /* (1 <<  9) unused */
 #define DIFF_OPT_HAS_CHANGES         (1 << 10)
 #define DIFF_OPT_QUICK               (1 << 11)

index f639601..216a7a4 100644
@@ -512,9 +512,15 @@ void diffcore_rename(struct diff_options *options)
 			else if (options->single_follow &&
 				 strcmp(options->single_follow, p->two->path))
 				continue; /* not interested */
+			else if (!DIFF_OPT_TST(options, RENAME_EMPTY) &&
+				 is_empty_blob_sha1(p->two->sha1))
+				continue;
 			else
 				locate_rename_dst(p->two, 1);
 		}
+		else if (!DIFF_OPT_TST(options, RENAME_EMPTY) &&
+			 is_empty_blob_sha1(p->one->sha1))
+			continue;
 		else if (!DIFF_PAIR_UNMERGED(p) && !DIFF_FILE_VALID(p->two)) {
 			/*
 			 * If the source is a broken "delete", and

1.7.10.rc0.9.gdcbe9

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.

---
 merge-recursive.c       |    1 +
 t/t6022-merge-rename.sh |   16 ++++++++++++++++
 2 files changed, 17 insertions(+)

index 318d32e..0fb1743 100644
@@ -485,6 +485,7 @@ static struct string_list *get_renames(struct merge_options *o,
 	renames = xcalloc(1, sizeof(struct string_list));
 	diff_setup(&opts);
 	DIFF_OPT_SET(&opts, RECURSIVE);
+	DIFF_OPT_CLR(&opts, RENAME_EMPTY);
 	opts.detect_rename = DIFF_DETECT_RENAME;
 	opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :
 			    o->diff_rename_limit >= 0 ? o->diff_rename_limit :

index 9d8584e..1104249 100755
@@ -884,4 +884,20 @@ test_expect_success 'no spurious "refusing to lose untracked" message' '
 	! grep "refusing to lose untracked file" errors.txt
 '
 
+test_expect_success 'do not follow renames for empty files' '
+	git checkout -f -b empty-base &&
+	>empty1 &&
+	git add empty1 &&
+	git commit -m base &&
+	echo content >empty1 &&
+	git add empty1 &&
+	git commit -m fill &&
+	git checkout -b empty-topic HEAD^ &&
+	git mv empty1 empty2 &&
+	git commit -m rename &&
+	test_must_fail git merge empty-base &&
+	>expect &&
+	test_cmp expect empty2
+'
+
 test_done

1.7.10.rc0.9.gdcbe9

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.

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

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