{"thread":{"id":"5900","subject":"[PATCH] diff: fix 2 whitespace issues","startedAt":"2006-10-12T12:22:14Z","lastAt":"2006-10-12T21:08:03Z","messageCount":3,"participants":["Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"28661","messageId":"Pine.LNX.4.63.0610121418180.14200@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"5900","inReplyTo":null,"subject":"[PATCH] diff: fix 2 whitespace issues","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-10-12T12:22:14Z","receivedAt":"2006-10-12T12:22:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nWhen whitespace or whitespace change was ignored, the function\nxdl_recmatch() returned memcmp() style differences, which is wrong,\nsince it should return 0 on non-match.\n\nAlso, there were three horrible off-by-one bugs, even leading to wrong\nhashes in the whitespace special handling.\n\nThe issue was noticed by Ray Lehtiniemi.\n\nFor good measure, this commit adds a test.\n\nSigned-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n\n---\n\n\tThese bugs are embarassing. I have no idea how my tests succeeded \n\twhen I submitted the support for -b and -w...\n\n\tNote that \"git diff --check\" complains about six whitespaces\n\tat the end of line in the test, which are there on purpose.\n\n t/t4015-diff-whitespace.sh |  122 ++++++++++++++++++++++++++++++++++++++++++++\n xdiff/xutils.c             |   29 ++++------\n 2 files changed, 134 insertions(+), 17 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nnew file mode 100755\nindex 0000000..c945085\n--- /dev/null\n+++ b/t/t4015-diff-whitespace.sh\n@@ -0,0 +1,122 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2006 Johannes E. Schindelin\n+#\n+\n+test_description='Test special whitespace in diff engine.\n+\n+'\n+. ./test-lib.sh\n+. ../diff-lib.sh\n+\n+# Ray Lehtiniemi's example\n+\n+cat << EOF > x\n+do {\n+   nothing;\n+} while (0);\n+EOF\n+\n+git-update-index --add x\n+\n+cat << EOF > x\n+do\n+{\n+   nothing;\n+}\n+while (0);\n+EOF\n+\n+cat << EOF > expect\n+diff --git a/x b/x\n+index adf3937..6edc172 100644\n+--- a/x\n++++ b/x\n+@@ -1,3 +1,5 @@\n+-do {\n++do\n++{\n+    nothing;\n+-} while (0);\n++}\n++while (0);\n+EOF\n+\n+git-diff > out\n+test_expect_success \"Ray's example without options\" 'diff -u expect out'\n+\n+git-diff -w > out\n+test_expect_success \"Ray's example with -w\" 'diff -u expect out'\n+\n+git-diff -b > out\n+test_expect_success \"Ray's example with -b\" 'diff -u expect out'\n+\n+cat << EOF > x\n+whitespace at beginning\n+whitespace change\n+whitespace in the middle\n+whitespace at end\n+unchanged line\n+CR at end\n+EOF\n+\n+git-update-index x\n+\n+cat << EOF > x\n+\twhitespace at beginning\n+whitespace \t change\n+white space in the middle\n+whitespace at end  \n+unchanged line\n+CR at end\n+EOF\n+\n+cat << EOF > expect\n+diff --git a/x b/x\n+index d99af23..8b32fb5 100644\n+--- a/x\n++++ b/x\n+@@ -1,6 +1,6 @@\n+-whitespace at beginning\n+-whitespace change\n+-whitespace in the middle\n+-whitespace at end\n++\twhitespace at beginning\n++whitespace \t change\n++white space in the middle\n++whitespace at end  \n+ unchanged line\n+-CR at end\n++CR at end\n+EOF\n+git-diff > out\n+test_expect_success 'another test, without options' 'diff -u expect out'\n+\n+cat << EOF > expect\n+diff --git a/x b/x\n+index d99af23..8b32fb5 100644\n+EOF\n+git-diff -w > out\n+test_expect_success 'another test, with -w' 'diff -u expect out'\n+\n+cat << EOF > expect\n+diff --git a/x b/x\n+index d99af23..8b32fb5 100644\n+--- a/x\n++++ b/x\n+@@ -1,6 +1,6 @@\n+-whitespace at beginning\n++\twhitespace at beginning\n+ whitespace change\n+-whitespace in the middle\n+-whitespace at end\n++white space in the middle\n++whitespace at end  \n+ unchanged line\n+-CR at end\n++CR at end\n+EOF\n+git-diff -b > out\n+test_expect_success 'another test, with -b' 'diff -u expect out'\n+\n+test_done\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex f7bdd39..9e4bb47 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -191,36 +191,30 @@ int xdl_recmatch(const char *l1, long s1\n \tint i1, i2;\n \n \tif (flags & XDF_IGNORE_WHITESPACE) {\n-\t\tfor (i1 = i2 = 0; i1 < s1 && i2 < s2; i1++, i2++) {\n+\t\tfor (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {\n \t\t\tif (isspace(l1[i1]))\n \t\t\t\twhile (isspace(l1[i1]) && i1 < s1)\n \t\t\t\t\ti1++;\n-\t\t\telse if (isspace(l2[i2]))\n+\t\t\tif (isspace(l2[i2]))\n \t\t\t\twhile (isspace(l2[i2]) && i2 < s2)\n \t\t\t\t\ti2++;\n-\t\t\telse if (l1[i1] != l2[i2])\n-\t\t\t\treturn l2[i2] - l1[i1];\n+\t\t\tif (i1 < s1 && i2 < s2 && l1[i1++] != l2[i2++])\n+\t\t\t\treturn 0;\n \t\t}\n-\t\tif (i1 >= s1)\n-\t\t\treturn 1;\n-\t\telse if (i2 >= s2)\n-\t\t\treturn -1;\n+\t\treturn (i1 >= s1 && i2 >= s2);\n \t} else if (flags & XDF_IGNORE_WHITESPACE_CHANGE) {\n-\t\tfor (i1 = i2 = 0; i1 < s1 && i2 < s2; i1++, i2++) {\n+\t\tfor (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {\n \t\t\tif (isspace(l1[i1])) {\n \t\t\t\tif (!isspace(l2[i2]))\n-\t\t\t\t\treturn -1;\n+\t\t\t\t\treturn 0;\n \t\t\t\twhile (isspace(l1[i1]) && i1 < s1)\n \t\t\t\t\ti1++;\n \t\t\t\twhile (isspace(l2[i2]) && i2 < s2)\n \t\t\t\t\ti2++;\n-\t\t\t} else if (l1[i1] != l2[i2])\n-\t\t\t\treturn l2[i2] - l1[i1];\n+\t\t\t} else if (l1[i1++] != l2[i2++])\n+\t\t\t\treturn 0;\n \t\t}\n-\t\tif (i1 >= s1)\n-\t\t\treturn 1;\n-\t\telse if (i2 >= s2)\n-\t\t\treturn -1;\n+\t\treturn (i1 >= s1 && i2 >= s2);\n \t} else\n \t\treturn s1 == s2 && !memcmp(l1, l2, s1);\n \n@@ -233,7 +227,8 @@ unsigned long xdl_hash_record(char const\n \n \tfor (; ptr < top && *ptr != '\\n'; ptr++) {\n \t\tif (isspace(*ptr) && (flags & XDF_WHITESPACE_FLAGS)) {\n-\t\t\twhile (ptr < top && isspace(*ptr) && ptr[1] != '\\n')\n+\t\t\twhile (ptr + 1 < top && isspace(ptr[1])\n+\t\t\t\t\t&& ptr[1] != '\\n')\n \t\t\t\tptr++;\n \t\t\tif (flags & XDF_IGNORE_WHITESPACE_CHANGE) {\n \t\t\t\tha += (ha << 5);\n-- \n1.4.3.rc2.g4e05\n"},{"id":"28672","messageId":"7v4pu9pjwe.fsf@assigned-by-dhcp.cox.net","threadId":"5900","inReplyTo":"Pine.LNX.4.63.0610121418180.14200@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] diff: fix 2 whitespace issues","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-12T16:40:33Z","receivedAt":"2006-10-12T16:40:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I noticed this breakage sometime last week and it was sitting at\nnear the bottom of my stack of things to look at.  Ray's\nbugreport and your fix were quite timely.  Thanks.\n\nYou mentioned six whitespace problems but I counted only three\nand the test failed on \"CR at the end\"; the test vector was easy\nto hand-fix thanks to the \"index\" line.\n\nThis patch is an example that we do not want to transmit files\nthat has CRs in e-mail.  These CRs appear in format-patch\noutput, so either the user needs to do --attach (and perhaps the\noption needs to do base64 or qp in such a case) or format-patch\nneeds to treat a blob with CR as binary and emit binary diff?\nThe latter is not appropriate since patches apply just fine with\nCR in them.\n"},{"id":"28683","messageId":"Pine.LNX.4.63.0610122254350.14200@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"5900","inReplyTo":"7v4pu9pjwe.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] diff: fix 2 whitespace issues","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-10-12T21:08:03Z","receivedAt":"2006-10-12T21:08:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 12 Oct 2006, Junio C Hamano wrote:\n\n> I noticed this breakage sometime last week and it was sitting at near \n> the bottom of my stack of things to look at.\n\nI actually enjoyed tracking your todo \"branch\", although lately, there is \nsubstantially less traffic there. Maybe git is finished ;-)\n\n> You mentioned six whitespace problems but I counted only three\n> and the test failed on \"CR at the end\"; the test vector was easy\n> to hand-fix thanks to the \"index\" line.\n> \n> This patch is an example that we do not want to transmit files\n> that has CRs in e-mail.  These CRs appear in format-patch\n> output, so either the user needs to do --attach (and perhaps the\n> option needs to do base64 or qp in such a case) or format-patch\n> needs to treat a blob with CR as binary and emit binary diff?\n> The latter is not appropriate since patches apply just fine with\n> CR in them.\n\nThe problem is more likely my (strange) workflow. I never use \ngit-send-email. Not only because I am a bit wary of the Perl stuff, but \nalso because I cannot use sendmail directly (some stoopid \"firewall\" \npretending to fix spamming from %&/%&/ users with their %&\"§ infected \nWindows machines).\n\nThus, I used ^R in my venerable patched pine to insert the file, and (just \na guess) pine -- in its infinite wisdom -- decided I'd probably not want \nthe carriage return, when I put it there on purpose, using my l33t sk1llz.\n\nIn hindsight, it might be not _that_ important to test for a carriage \nreturn, but testing _any_ whitespace should do (which I put in also, for \ngood measure). However, carriage returns from my beloved friends using the \nMost Stupid operating system were the reason I hacked in the whitespace \noptions, and therefore I wanted to test this case specifically.\n\nCiao,\nDscho\n"}]}