[PATCH] diff: fix 2 whitespace issues

[PATCH] diff: fix 2 whitespace issues

[Johannes Schindelin]

When whitespace or whitespace change was ignored, the function
xdl_recmatch() returned memcmp() style differences, which is wrong,
since it should return 0 on non-match.

Also, there were three horrible off-by-one bugs, even leading to wrong
hashes in the whitespace special handling.

The issue was noticed by Ray Lehtiniemi.

For good measure, this commit adds a test.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

	These bugs are embarassing. I have no idea how my tests succeeded 
	when I submitted the support for -b and -w...

	Note that "git diff --check" complains about six whitespaces
	at the end of line in the test, which are there on purpose.

 t/t4015-diff-whitespace.sh |  122 ++++++++++++++++++++++++++++++++++++++++++++
 xdiff/xutils.c             |   29 ++++------
 2 files changed, 134 insertions(+), 17 deletions(-)

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
new file mode 100755
index 0000000..c945085
--- /dev/null
+++ b/t/t4015-diff-whitespace.sh
@@ -0,0 +1,122 @@
+#!/bin/sh
+#
+# Copyright (c) 2006 Johannes E. Schindelin
+#
+
+test_description='Test special whitespace in diff engine.
+
+'
+. ./test-lib.sh
+. ../diff-lib.sh
+
+# Ray Lehtiniemi's example
+
+cat << EOF > x
+do {
+   nothing;
+} while (0);
+EOF
+
+git-update-index --add x
+
+cat << EOF > x
+do
+{
+   nothing;
+}
+while (0);
+EOF
+
+cat << EOF > expect
+diff --git a/x b/x
+index adf3937..6edc172 100644
+--- a/x
++++ b/x
+@@ -1,3 +1,5 @@
+-do {
++do
++{
+    nothing;
+-} while (0);
++}
++while (0);
+EOF
+
+git-diff > out
+test_expect_success "Ray's example without options" 'diff -u expect out'
+
+git-diff -w > out
+test_expect_success "Ray's example with -w" 'diff -u expect out'
+
+git-diff -b > out
+test_expect_success "Ray's example with -b" 'diff -u expect out'
+
+cat << EOF > x
+whitespace at beginning
+whitespace change
+whitespace in the middle
+whitespace at end
+unchanged line
+CR at end
+EOF
+
+git-update-index x
+
+cat << EOF > x
+	whitespace at beginning
+whitespace 	 change
+white space in the middle
+whitespace at end  
+unchanged line
+CR at end
+EOF
+
+cat << EOF > expect
+diff --git a/x b/x
+index d99af23..8b32fb5 100644
+--- a/x
++++ b/x
+@@ -1,6 +1,6 @@
+-whitespace at beginning
+-whitespace change
+-whitespace in the middle
+-whitespace at end
++	whitespace at beginning
++whitespace 	 change
++white space in the middle
++whitespace at end  
+ unchanged line
+-CR at end
++CR at end
+EOF
+git-diff > out
+test_expect_success 'another test, without options' 'diff -u expect out'
+
+cat << EOF > expect
+diff --git a/x b/x
+index d99af23..8b32fb5 100644
+EOF
+git-diff -w > out
+test_expect_success 'another test, with -w' 'diff -u expect out'
+
+cat << EOF > expect
+diff --git a/x b/x
+index d99af23..8b32fb5 100644
+--- a/x
++++ b/x
+@@ -1,6 +1,6 @@
+-whitespace at beginning
++	whitespace at beginning
+ whitespace change
+-whitespace in the middle
+-whitespace at end
++white space in the middle
++whitespace at end  
+ unchanged line
+-CR at end
++CR at end
+EOF
+git-diff -b > out
+test_expect_success 'another test, with -b' 'diff -u expect out'
+
+test_done
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index f7bdd39..9e4bb47 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -191,36 +191,30 @@ int xdl_recmatch(const char *l1, long s1
 	int i1, i2;
 
 	if (flags & XDF_IGNORE_WHITESPACE) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; i1++, i2++) {
+		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
 			if (isspace(l1[i1]))
 				while (isspace(l1[i1]) && i1 < s1)
 					i1++;
-			else if (isspace(l2[i2]))
+			if (isspace(l2[i2]))
 				while (isspace(l2[i2]) && i2 < s2)
 					i2++;
-			else if (l1[i1] != l2[i2])
-				return l2[i2] - l1[i1];
+			if (i1 < s1 && i2 < s2 && l1[i1++] != l2[i2++])
+				return 0;
 		}
-		if (i1 >= s1)
-			return 1;
-		else if (i2 >= s2)
-			return -1;
+		return (i1 >= s1 && i2 >= s2);
 	} else if (flags & XDF_IGNORE_WHITESPACE_CHANGE) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; i1++, i2++) {
+		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
 			if (isspace(l1[i1])) {
 				if (!isspace(l2[i2]))
-					return -1;
+					return 0;
 				while (isspace(l1[i1]) && i1 < s1)
 					i1++;
 				while (isspace(l2[i2]) && i2 < s2)
 					i2++;
-			} else if (l1[i1] != l2[i2])
-				return l2[i2] - l1[i1];
+			} else if (l1[i1++] != l2[i2++])
+				return 0;
 		}
-		if (i1 >= s1)
-			return 1;
-		else if (i2 >= s2)
-			return -1;
+		return (i1 >= s1 && i2 >= s2);
 	} else
 		return s1 == s2 && !memcmp(l1, l2, s1);
 
@@ -233,7 +227,8 @@ unsigned long xdl_hash_record(char const
 
 	for (; ptr < top && *ptr != '\n'; ptr++) {
 		if (isspace(*ptr) && (flags & XDF_WHITESPACE_FLAGS)) {
-			while (ptr < top && isspace(*ptr) && ptr[1] != '\n')
+			while (ptr + 1 < top && isspace(ptr[1])
+					&& ptr[1] != '\n')
 				ptr++;
 			if (flags & XDF_IGNORE_WHITESPACE_CHANGE) {
 				ha += (ha << 5);
-- 
1.4.3.rc2.g4e05

Re: [PATCH] diff: fix 2 whitespace issues

[Junio C Hamano]

I noticed this breakage sometime last week and it was sitting at
near the bottom of my stack of things to look at.  Ray's
bugreport and your fix were quite timely.  Thanks.

You mentioned six whitespace problems but I counted only three
and the test failed on "CR at the end"; the test vector was easy
to hand-fix thanks to the "index" line.

This patch is an example that we do not want to transmit files
that has CRs in e-mail.  These CRs appear in format-patch
output, so either the user needs to do --attach (and perhaps the
option needs to do base64 or qp in such a case) or format-patch
needs to treat a blob with CR as binary and emit binary diff?
The latter is not appropriate since patches apply just fine with
CR in them.

Re: [PATCH] diff: fix 2 whitespace issues

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1781 bytes --]

Hi,

On Thu, 12 Oct 2006, Junio C Hamano wrote:

> I noticed this breakage sometime last week and it was sitting at near 
> the bottom of my stack of things to look at.

I actually enjoyed tracking your todo "branch", although lately, there is 
substantially less traffic there. Maybe git is finished ;-)

> You mentioned six whitespace problems but I counted only three
> and the test failed on "CR at the end"; the test vector was easy
> to hand-fix thanks to the "index" line.
> 
> This patch is an example that we do not want to transmit files
> that has CRs in e-mail.  These CRs appear in format-patch
> output, so either the user needs to do --attach (and perhaps the
> option needs to do base64 or qp in such a case) or format-patch
> needs to treat a blob with CR as binary and emit binary diff?
> The latter is not appropriate since patches apply just fine with
> CR in them.

The problem is more likely my (strange) workflow. I never use 
git-send-email. Not only because I am a bit wary of the Perl stuff, but 
also because I cannot use sendmail directly (some stoopid "firewall" 
pretending to fix spamming from %&/%&/ users with their %&"§ infected 
Windows machines).

Thus, I used ^R in my venerable patched pine to insert the file, and (just 
a guess) pine -- in its infinite wisdom -- decided I'd probably not want 
the carriage return, when I put it there on purpose, using my l33t sk1llz.

In hindsight, it might be not _that_ important to test for a carriage 
return, but testing _any_ whitespace should do (which I put in also, for 
good measure). However, carriage returns from my beloved friends using the 
Most Stupid operating system were the reason I hacked in the whitespace 
options, and therefore I wanted to test this case specifically.

Ciao,
Dscho