Re: Strange effect merging empty file

Jeff King [off-list ref] writes:

That is a slight layering violation, in that we are making assumptions
about how the diffcore-rename subsystem works.

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:

Jeff King [off-list ref] writes:

quoted

That is a slight layering violation, in that we are making assumptions
about how the diffcore-rename subsystem works.

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.

Signed-off-by: Jeff King <redacted>
---
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(-)

diff --git a/diff.c b/diff.c
index 377ec1e..0b70aad 100644
--- a/diff.c
+++ b/diff.c

@@ -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=")) {

diff --git a/diff.h b/diff.h
index cb68743..dd48eca 100644
--- a/diff.h
+++ b/diff.h

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

diff --git a/diffcore-rename.c b/diffcore-rename.c
index f639601..216a7a4 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c

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

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

diff --git a/merge-recursive.c b/merge-recursive.c
index 318d32e..0fb1743 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

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

diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index 9d8584e..1104249 100755
--- a/t/t6022-merge-rename.sh
+++ b/t/t6022-merge-rename.sh

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