[PATCH] diff: fix 2 whitespace issues

Johannes Schindelin· Oct 12, 2006, 12:22 UTC · lore

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

Show changes to 2 files +131 −17

t/t4015-diff-whitespace.sh, xdiff/xutils.c

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

Junio C Hamano· Oct 12, 2006, 16:40 UTC · re: Johannes Schindelin · lore

Re: [PATCH] diff: fix 2 whitespace issues

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.

Johannes Schindelin· Oct 12, 2006, 21:08 UTC · re: Junio C Hamano · lore

Re: [PATCH] diff: fix 2 whitespace issues

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

Show 11 quoted lines

> 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

patchdiff: fix 2 whitespace issues