Re: [PATCH] Ignore end-of-line style when computing similarity score for rename detection

I wonder if we have a path or the attribute cue at this point of
the operation.  Your patch may solve that special case of CRLF
mangled files, but diffcore-delta works both on text and binary
and I am not sure what damange/side-effect you are causing.

And you are not ignoring "\r\n vs \n difference" as the comment
claims, but are discarding '\r' unconditionally.  When we start
doing something like that, I would feel much better if we _know_
we are operating on a text file at least.

Signed-off-by: Steven Grimm <redacted>
---
Junio rightly points out that it would be a mistake to discard \r
characters from binary files when computing similarity scores. So now we
only do it if the file contents test as non-binary.

The file attributes aren't available at this level of the code, but they
could be propagated down from the higher levels if we don't trust
buffer_is_binary() to make an adequately accurate decision.


 diffcore-delta.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/diffcore-delta.c b/diffcore-delta.c
index 7338a40..52e648f 100644
--- a/diffcore-delta.c
+++ b/diffcore-delta.c

@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "diff.h"
 #include "diffcore.h"
+#include "xdiff-interface.h"
 
 /*
  * Idea here is very simple.

@@ -125,7 +126,8 @@ static struct spanhash_top *add_spanhash(struct spanhash_top *top,
 	}
 }
 
-static struct spanhash_top *hash_chars(unsigned char *buf, unsigned int sz)
+static struct spanhash_top *hash_chars(unsigned char *buf, unsigned int sz,
+				       int is_binary)
 {
 	int i, n;
 	unsigned int accum1, accum2, hashval;

@@ -143,9 +145,12 @@ static struct spanhash_top *hash_chars(unsigned char *buf, unsigned int sz)
 		unsigned int c = *buf++;
 		unsigned int old_1 = accum1;
 		sz--;
-		accum1 = (accum1 << 7) ^ (accum2 >> 25);
-		accum2 = (accum2 << 7) ^ (old_1 >> 25);
-		accum1 += c;
+		/* Ignore \r\n vs. \n when computing text file similarity. */
+		if (c != '\r' && ! is_binary) {
+			accum1 = (accum1 << 7) ^ (accum2 >> 25);
+			accum2 = (accum2 << 7) ^ (old_1 >> 25);
+			accum1 += c;
+		}
 		if (++n < 64 && c != '\n')
 			continue;
 		hashval = (accum1 + accum2 * 0x61) % HASHBASE;

@@ -172,14 +177,16 @@ int diffcore_count_changes(void *src, unsigned long src_size,
 	if (src_count_p)
 		src_count = *src_count_p;
 	if (!src_count) {
-		src_count = hash_chars(src, src_size);
+		int src_is_binary = buffer_is_binary(src, src_size);
+		src_count = hash_chars(src, src_size, src_is_binary);
 		if (src_count_p)
 			*src_count_p = src_count;
 	}
 	if (dst_count_p)
 		dst_count = *dst_count_p;
 	if (!dst_count) {
-		dst_count = hash_chars(dst, dst_size);
+		int dst_is_binary = buffer_is_binary(dst, dst_size);
+		dst_count = hash_chars(dst, dst_size, dst_is_binary);
 		if (dst_count_p)
 			*dst_count_p = dst_count;
 	}

-- 
1.5.2.2.571.ge134

Steven Grimm [off-list ref] wrote:

Junio rightly points out that it would be a mistake to discard \r
characters from binary files when computing similarity scores. So now we
only do it if the file contents test as non-binary.

The file attributes aren't available at this level of the code, but they
could be propagated down from the higher levels if we don't trust
buffer_is_binary() to make an adequately accurate decision.

Ick.  If we can get the attributes into diff_filespec this is
pretty easy, as you can do a crlf->lf conversion on both files if
both are considered to be text, but it doesn't look like it would
be very easy to get the attributes into the diff_filespec.

Actually even better if you can also run the in/out filter things.
I'm thinking of say an XML file that has had whitespace formatting
changes, but whose XSD and processors ignore unnecessary whitespace.
Be nice if the rename detection actually was able to canonicalize
both files before detecting the rename, assuming both files had a
canonicalizer input filter defined that does that...

Of course diff.c defines a nice diff_is_binary() at file scope that
does at least a "can we diff this" decision.  Might be good if that
could be reused for the rename detection.

OK, that's far more than I actually know about diffcore.  This is
one for Junio, Linus, you, and those who are less tired than I feel
right now...  ;-)

Personally I'd rather see us doing the right thing (use attributes
and fallback on guessing if no preference is stated either way)
over doing something half-a**ed (only guessing).

-- 
Shawn.

Hi,

On Wed, 27 Jun 2007, Steven Grimm wrote:

Junio rightly points out that it would be a mistake to discard \r
characters from binary files when computing similarity scores.

Somehow I think that this should be triggered by "--ignore-space-at-eol", 
_and_ be accompanied by a test case.

Ciao,
Dscho

Johannes Schindelin wrote:

Somehow I think that this should be triggered by "--ignore-space-at-eol", 
_and_ be accompanied by a test case.
  

Should --ignore-space-at-eol be an option to git-merge? Merges are where 
this functionality matters; for simple diffs, --ignore-space-at-eol 
actually already covers it. If we allow that option, should we also 
allow other git-diff options like --ignore-all-space and 
--ignore-space-change? What are the semantics of an autoresolved merge 
with those options in effect -- are they only used for rename detection, 
or do we, e.g., not flag conflicts with only whitespace changes? And if 
we don't, which version do we accept automatically?

-Steve

Hi,

On Thu, 28 Jun 2007, Steven Grimm wrote:

Johannes Schindelin wrote:

quoted

Somehow I think that this should be triggered by "--ignore-space-at-eol",
_and_ be accompanied by a test case.
  

Should --ignore-space-at-eol be an option to git-merge? Merges are where 
this functionality matters; for simple diffs, --ignore-space-at-eol 
actually already covers it.

Good point. However, I fail to see how the similarity detection should be 
so decoupled from the application. IOW what good is it if two files are 
rated similar if the merge cannot handle the CRLF/LF differences properly?

So two points here: since the merges are what you target, you definitely 
should mention that in the commit message. And you should make sure that 
all this trickery only kicks in when the merge has a chance to succeed.

Ciao,
Dscho