{"thread":{"id":"25484","subject":"[PATCH] diff: handle lines containing only whitespace better","startedAt":"2010-10-20T04:46:18Z","lastAt":"2010-10-21T00:11:44Z","messageCount":9,"participants":["Kevin Ballard","Junio C Hamano","Nazri Ramliy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"153849","messageId":"1287549978-54280-1-git-send-email-kevin@sb.org","threadId":"25484","inReplyTo":null,"subject":"[PATCH] diff: handle lines containing only whitespace better","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2010-10-20T04:46:18Z","receivedAt":"2010-10-20T04:46:18Z","isPatch":true,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"When a line contains nothing but whitespace and the core.whitespace config\noption contains blank-at-eol, the whitespace on the line is being printed\ntwice, once unhighlighted (unless otherwise matched by one of the other\ncore.whitespace values), and a second time highlighted for blank-at-eol.\n\nUpdate the leading indentation check to stop checking when it reaches\nthe trailing whitespace.\n\nSigned-off-by: Kevin Ballard \n---\n ws.c | 7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/ws.c b/ws.c\nindex d7b8c33..7302f8f 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -174,8 +174,11 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,\n \t\t}\n \t}\n \n+\tif (trailing_whitespace == -1)\n+\t\ttrailing_whitespace = len;\n+\n \t/* Check indentation */\n-\tfor (i = 0; i < len; i++) {\n+\tfor (i = 0; i < trailing_whitespace; i++) {\n \t\tif (line[i] == ' ')\n \t\t\tcontinue;\n \t\tif (line[i] != '\\t')\n@@ -218,8 +221,6 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,\n \t\t * Now the rest of the line starts at \"written\".\n \t\t * The non-highlighted part ends at \"trailing_whitespace\".\n \t\t */\n-\t\tif (trailing_whitespace == -1)\n-\t\t\ttrailing_whitespace = len;\n \n \t\t/* Emit non-highlighted (middle) segment. */\n \t\tif (trailing_whitespace - written > 0) {\n-- \n1.7.3.1.211.g81fee.dirty\n"},{"id":"153852","messageId":"7vzku9wfrp.fsf@alter.siamese.dyndns.org","threadId":"25484","inReplyTo":"1287549978-54280-1-git-send-email-kevin@sb.org","subject":"Re: [PATCH] diff: handle lines containing only whitespace better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-20T06:16:10Z","receivedAt":"2010-10-20T06:16:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hmm, tests?\n"},{"id":"153855","messageId":"780B144B-03E0-4ED5-8E92-D4EB3CBBBF71@sb.org","threadId":"25484","inReplyTo":"7vzku9wfrp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: handle lines containing only whitespace better","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2010-10-20T06:38:41Z","receivedAt":"2010-10-20T06:38:41Z","isPatch":true,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"On Oct 19, 2010, at 11:16 PM, Junio C Hamano wrote:\n\n> Hmm, tests?\n\nI checked, there seem to be no existing tests for the whitespace highlighting output. All the tests just use `git diff --check` to see if it was caught. And given that the problem only occurs when it's emitting the colored highlighting, I wasn't sure how to go about adding tests for this as I'd need to create an expect file that contains all the same ansi color codes, and I thought that might be a bit fragile or hard to do correctly.\n\nIncidentally, I just realized the description of the patch is slightly wrong. The problem only occurs when the line contains at least one tab. Should I resend the patch with an updated description? I can also attempt to write tests if you can give me some guidance on how to deal with the need for ansi color codes.\n\n-Kevin Ballard\n"},{"id":"153857","messageId":"AANLkTi=XhsJyi0Tb5j8j1WKUNTbuy4Y6ugaAifYGBzB_@mail.gmail.com","threadId":"25484","inReplyTo":"780B144B-03E0-4ED5-8E92-D4EB3CBBBF71@sb.org","subject":"Re: [PATCH] diff: handle lines containing only whitespace better","fromName":"Nazri Ramliy","fromEmail":"ayiehere@gmail.com","sentAt":"2010-10-20T07:51:12Z","receivedAt":"2010-10-20T07:51:12Z","isPatch":true,"sender":{"key":"ayiehere@gmail.com","avatar":"https://avatars.githubusercontent.com/u/164756?v=4"},"body":"On Wed, Oct 20, 2010 at 2:38 PM, Kevin Ballard wrote:\n> ... I can also attempt to write\n> tests if you can give me some guidance on how to deal with the need for ansi\n> color codes.\n\nHave a look at t/t4207-log-decoration-colors.sh for inspiration.\n\nnazri\n"},{"id":"153872","messageId":"7v4ocgx2we.fsf@alter.siamese.dyndns.org","threadId":"25484","inReplyTo":"780B144B-03E0-4ED5-8E92-D4EB3CBBBF71@sb.org","subject":"Re: [PATCH] diff: handle lines containing only whitespace better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-20T16:08:49Z","receivedAt":"2010-10-20T16:08:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Ballard writes:\n\n> Incidentally, I just realized the description of the patch is slightly\n> wrong. The problem only occurs when the line contains at least one\n> tab...\n\nThat exactly is why I asked for tests, as I couldn't reproduce it from the\ndescription at all.\n\nBut I see it now.\n\n\t$ HT=' '\n\t$ mv Makefile Makefile+\n $ sed -e \"3s/^.*/$HT/\" Makefile+ >Makefile\n $ git diff --color\n\n> ...Should I resend the patch with an updated description? I can also\n> attempt to write tests if you can give me some guidance on how to deal\n> with the need for ansi color codes.\n\nThis may show us a good starting point.\n\n $ git grep RED t/\n"},{"id":"153899","messageId":"1287613046-61804-1-git-send-email-kevin@sb.org","threadId":"25484","inReplyTo":"7v4ocgx2we.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2 1/2] test-lib: extend test_decode_color to handle more color codes","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2010-10-20T22:17:25Z","receivedAt":"2010-10-20T22:17:25Z","isPatch":true,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"Enhance the test_decode_color function to handle all common color codes,\nincluding background colors and escapes that contain multiple codes.\nThis change necessitates changing to , so update t4034\nas well.\n\nThis change is necessary for the next commit in order to test\nbackground colors properly.\n\nSigned-off-by: Kevin Ballard \n---\nI turned sed into awk. Looks like awk is already used elsehwere in git,\nso I'm assuming this is safe, but please let me know if it's not.\n\n t/t4034-diff-words.sh | 72 ++++++++++++++++++++++++------------------------\n t/test-lib.sh | 49 ++++++++++++++++++++++++++++-----\n 2 files changed, 77 insertions(+), 44 deletions(-)\n\ndiff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh\nindex 6f7548c..3f3c757 100755\n--- a/t/t4034-diff-words.sh\n+++ b/t/t4034-diff-words.sh\n@@ -35,10 +35,10 @@ aeff = aeff * ( aaa )\n EOF\n \n cat > expect <<\\EOF\n-diff --git a/pre b/post\n-index 330b04f..5ed8eff 100644\n---- a/pre\n-+++ b/post\n+diff --git a/pre b/post\n+index 330b04f..5ed8eff 100644\n+--- a/pre\n++++ b/post\n @@ -1,3 +1,7 @@\n h(4)h(4),hh[44]\n \n@@ -122,10 +122,10 @@ test_expect_success '--word-diff=plain --no-color' '\n '\n \n cat > expect <diff --git a/pre b/post\n-index 330b04f..5ed8eff 100644\n---- a/pre\n-+++ b/post\n+diff --git a/pre b/post\n+index 330b04f..5ed8eff 100644\n+--- a/pre\n++++ b/post\n @@ -1,3 +1,7 @@\n [-h(4)-]{+h(4),hh[44]+}\n \n@@ -143,10 +143,10 @@ test_expect_success '--word-diff=plain --color' '\n '\n \n cat > expect <<\\EOF\n-diff --git a/pre b/post\n-index 330b04f..5ed8eff 100644\n---- a/pre\n-+++ b/post\n+diff --git a/pre b/post\n+index 330b04f..5ed8eff 100644\n+--- a/pre\n++++ b/post\n @@ -1 +1 @@\n h(4)h(4),hh[44]\n @@ -3,0 +4,4 @@ a = b + c\n@@ -163,10 +163,10 @@ test_expect_success 'word diff without context' '\n '\n \n cat > expect <<\\EOF\n-diff --git a/pre b/post\n-index 330b04f..5ed8eff 100644\n---- a/pre\n-+++ b/post\n+diff --git a/pre b/post\n+index 330b04f..5ed8eff 100644\n+--- a/pre\n++++ b/post\n @@ -1,3 +1,7 @@\n h(4),hh[44]\n \n@@ -199,10 +199,10 @@ test_expect_success 'option overrides .gitattributes' '\n '\n \n cat > expect <<\\EOF\n-diff --git a/pre b/post\n-index 330b04f..5ed8eff 100644\n---- a/pre\n-+++ b/post\n+diff --git a/pre b/post\n+index 330b04f..5ed8eff 100644\n+--- a/pre\n++++ b/post\n @@ -1,3 +1,7 @@\n h(4),hh[44]\n \n@@ -231,10 +231,10 @@ test_expect_success 'command-line overrides config' '\n '\n \n cat > expect <<\\EOF\n-diff --git a/pre b/post\n-index 330b04f..5ed8eff 100644\n---- a/pre\n-+++ b/post\n+diff --git a/pre b/post\n+index 330b04f..5ed8eff 100644\n+--- a/pre\n++++ b/post\n @@ -1,3 +1,7 @@\n h(4),{+hh+}[44]\n \n@@ -260,10 +260,10 @@ test_expect_success 'remove diff driver regex' '\n '\n \n cat > expect <<\\EOF\n-diff --git a/pre b/post\n-index 330b04f..5ed8eff 100644\n---- a/pre\n-+++ b/post\n+diff --git a/pre b/post\n+index 330b04f..5ed8eff 100644\n+--- a/pre\n++++ b/post\n @@ -1,3 +1,7 @@\n h(4),hh[44]\n \n@@ -282,10 +282,10 @@ echo 'aaa (aaa)' > pre\n echo 'aaa (aaa) aaa' > post\n \n cat > expect <<\\EOF\n-diff --git a/pre b/post\n-index c29453b..be22f37 100644\n---- a/pre\n-+++ b/post\n+diff --git a/pre b/post\n+index c29453b..be22f37 100644\n+--- a/pre\n++++ b/post\n @@ -1 +1 @@\n aaa (aaa) aaa\n EOF\n@@ -301,10 +301,10 @@ echo '(:' > pre\n echo '(' > post\n \n cat > expect <<\\EOF\n-diff --git a/pre b/post\n-index 289cb9d..2d06f37 100644\n---- a/pre\n-+++ b/post\n+diff --git a/pre b/post\n+index 289cb9d..2d06f37 100644\n+--- a/pre\n++++ b/post\n @@ -1 +1 @@\n (:\n EOF\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 2af8f10..6dd3ce9 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -238,14 +238,47 @@ test_set_editor () {\n }\n \n test_decode_color () {\n-\tsed\t-e 's/.\\[1m//g' \\\n-\t\t-e 's/.\\[31m//g' \\\n-\t\t-e 's/.\\[32m//g' \\\n-\t\t-e 's/.\\[33m//g' \\\n-\t\t-e 's/.\\[34m//g' \\\n-\t\t-e 's/.\\[35m//g' \\\n-\t\t-e 's/.\\[36m//g' \\\n-\t\t-e 's/.\\[m//g'\n+\tawk '\n+\t\tfunction name(n) {\n+\t\t\tif (n == 0) return \"RESET\";\n+\t\t\tif (n == 1) return \"BOLD\";\n+\t\t\tif (n == 30) return \"BLACK\";\n+\t\t\tif (n == 31) return \"RED\";\n+\t\t\tif (n == 32) return \"GREEN\";\n+\t\t\tif (n == 33) return \"YELLOW\";\n+\t\t\tif (n == 34) return \"BLUE\";\n+\t\t\tif (n == 35) return \"MAGENTA\";\n+\t\t\tif (n == 36) return \"CYAN\";\n+\t\t\tif (n == 37) return \"WHITE\";\n+\t\t\tif (n == 40) return \"BLACK\";\n+\t\t\tif (n == 41) return \"BRED\";\n+\t\t\tif (n == 42) return \"BGREEN\";\n+\t\t\tif (n == 43) return \"BYELLOW\";\n+\t\t\tif (n == 44) return \"BBLUE\";\n+\t\t\tif (n == 45) return \"BMAGENTA\";\n+\t\t\tif (n == 46) return \"BCYAN\";\n+\t\t\tif (n == 47) return \"BWHITE\";\n+\t\t}\n+\t\t{\n+\t\t\twhile (match($0, /\\x1b\\[[0-9;]*m/) != 0) {\n+\t\t\t\tprintf \"%s<\", substr($0, 1, RSTART-1);\n+\t\t\t\tcodes = substr($0, RSTART+2, RLENGTH-3);\n+\t\t\t\tif (length(codes) == 0)\n+\t\t\t\t\tprintf \"%s\", name(0)\n+\t\t\t\telse {\n+\t\t\t\t\tn = split(codes, ary, \";\");\n+\t\t\t\t\tsep = \"\";\n+\t\t\t\t\tfor (i = 1; i <= n; i++) {\n+\t\t\t\t\t\tprintf \"%s%s\", sep, name(ary[i]);\n+\t\t\t\t\t\tsep = \";\"\n+\t\t\t\t\t}\n+\t\t\t\t}\n+\t\t\t\tprintf \">\";\n+\t\t\t\t$0 = substr($0, RSTART + RLENGTH, length($0) - RSTART - RLENGTH + 1);\n+\t\t\t}\n+\t\t\tprint\n+\t\t}\n+\t'\n }\n \n q_to_nul () {\n-- \n1.7.3.1.220.g19a98\n"},{"id":"153900","messageId":"1287613046-61804-2-git-send-email-kevin@sb.org","threadId":"25484","inReplyTo":"7v4ocgx2we.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2 2/2] diff: handle lines containing only whitespace and tabs better","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2010-10-20T22:17:26Z","receivedAt":"2010-10-20T22:17:26Z","isPatch":true,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"When a line contains nothing but whitespace with at least one tab\nand the core.whitespace config option contains blank-at-eol, the\nwhitespace on the line is being printed twice, once unhighlighted\n(unless otherwise matched by one of the other core.whitespace values),\nand a second time highlighted for blank-at-eol.\n\nUpdate the leading indentation check to stop checking when it reaches\nthe trailing whitespace.\n\nSigned-off-by: Kevin Ballard \n---\n t/t4015-diff-whitespace.sh | 37 +++++++++++++++++++++++++++++++++++++\n ws.c | 7 ++++---\n 2 files changed, 41 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 935d101..a8736f7 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -491,4 +491,41 @@ test_expect_success 'combined diff with autocrlf conversion' '\n \n '\n \n+# Start testing the colored format for whitespace checks\n+\n+test_expect_success 'setup diff colors' '\n+\tgit config color.diff always &&\n+\tgit config color.diff.plain normal &&\n+\tgit config color.diff.meta bold &&\n+\tgit config color.diff.frag cyan &&\n+\tgit config color.diff.func normal &&\n+\tgit config color.diff.old red &&\n+\tgit config color.diff.new green &&\n+\tgit config color.diff.commit yellow &&\n+\tgit config color.diff.whitespace \"normal red\" &&\n+\n+\tgit config core.autocrlf false\n+'\n+cat >expected <<\\EOF\n+diff --git a/x b/x\n+index 9daeafb..2874b91 100644\n+--- a/x\n++++ b/x\n+@@ -1 +1,4 @@\n+ test\n++{\n++\t\n++}\n+EOF\n+\n+test_expect_success 'diff that introduces a line with only tabs' '\n+\tgit config core.whitespace blank-at-eol &&\n+\tgit reset --hard &&\n+\techo \"test\" > x &&\n+\tgit commit -m \"initial\" x &&\n+\techo \"{NTN}\" | tr \"NT\" \"\\n\\t\" >> x &&\n+\tgit -c color.diff=always diff | test_decode_color >current &&\n+\ttest_cmp expected current\n+'\n+\n test_done\ndiff --git a/ws.c b/ws.c\nindex d7b8c33..7302f8f 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -174,8 +174,11 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,\n \t\t}\n \t}\n \n+\tif (trailing_whitespace == -1)\n+\t\ttrailing_whitespace = len;\n+\n \t/* Check indentation */\n-\tfor (i = 0; i < len; i++) {\n+\tfor (i = 0; i < trailing_whitespace; i++) {\n \t\tif (line[i] == ' ')\n \t\t\tcontinue;\n \t\tif (line[i] != '\\t')\n@@ -218,8 +221,6 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,\n \t\t * Now the rest of the line starts at \"written\".\n \t\t * The non-highlighted part ends at \"trailing_whitespace\".\n \t\t */\n-\t\tif (trailing_whitespace == -1)\n-\t\t\ttrailing_whitespace = len;\n \n \t\t/* Emit non-highlighted (middle) segment. */\n \t\tif (trailing_whitespace - written > 0) {\n-- \n1.7.3.1.220.g19a98\n"},{"id":"153908","messageId":"7v39s0v3f5.fsf@alter.siamese.dyndns.org","threadId":"25484","inReplyTo":"1287613046-61804-1-git-send-email-kevin@sb.org","subject":"Re: [PATCH v2 1/2] test-lib: extend test_decode_color to handle more color codes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-20T23:40:30Z","receivedAt":"2010-10-20T23:40:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Ballard writes:\n\n> Enhance the test_decode_color function to handle all common color codes,\n> including background colors and escapes that contain multiple codes.\n> This change necessitates changing to , so update t4034\n> as well.\n>\n> This change is necessary for the next commit in order to test\n> background colors properly.\n>\n> Signed-off-by: Kevin Ballard \n> ---\n> I turned sed into awk. Looks like awk is already used elsehwere in git,\n> so I'm assuming this is safe, but please let me know if it's not.\n\nI think calling BOLD BOLD is the right thing to do (who came up with the\nbogus WHITE in the first place anyway---my terminal is black letters on\nwhite background, thank you).\n\nEven though some scripts seem to already use awk, they are all used for\nvery small and trivial processing without exercising anything remotely\nfancy e.g. hexadecimal \\xXX quoting or match() function, so I wouldn't be\nsurprised if we see breakage reports from minority platforms.\n\nBut I do not think of a trivial way to express combination of attributes\nby extending the existing sed script (we can write loops and do the same\ncomputation as your awk script does, but it does not reduce the complexity\nnor risk of portability issues), so let's see what happens. We already\nuse Perl everywhere, which we might end up using for this if there are\nplatforms that have issues with your awk script.\n\nThanks.\n"},{"id":"153910","messageId":"389529B7-ED7E-4637-BC1C-CE9B884FAA9D@sb.org","threadId":"25484","inReplyTo":"7v39s0v3f5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] test-lib: extend test_decode_color to handle more color codes","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2010-10-21T00:11:44Z","receivedAt":"2010-10-21T00:11:44Z","isPatch":true,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"On Oct 20, 2010, at 4:40 PM, Junio C Hamano wrote:\n\n> Kevin Ballard writes:\n> \n>> Enhance the test_decode_color function to handle all common color codes,\n>> including background colors and escapes that contain multiple codes.\n>> This change necessitates changing to , so update t4034\n>> as well.\n>> \n>> This change is necessary for the next commit in order to test\n>> background colors properly.\n>> \n>> Signed-off-by: Kevin Ballard \n>> ---\n>> I turned sed into awk. Looks like awk is already used elsehwere in git,\n>> so I'm assuming this is safe, but please let me know if it's not.\n> \n> I think calling BOLD BOLD is the right thing to do (who came up with the\n> bogus WHITE in the first place anyway---my terminal is black letters on\n> white background, thank you).\n> \n> Even though some scripts seem to already use awk, they are all used for\n> very small and trivial processing without exercising anything remotely\n> fancy e.g. hexadecimal \\xXX quoting or match() function, so I wouldn't be\n> surprised if we see breakage reports from minority platforms.\n\nEntirely possible, but I'm hoping that's not the case. I used this against\nBSD awk version 20070501, so there's nothing remotely fancy there, and the\nregular expression conforms to the subset of BRE's mentioned in\nCodingGuidelines. But as you said, it's possible that minority platforms\ndon't handle this correctly.\n\n> But I do not think of a trivial way to express combination of attributes\n> by extending the existing sed script (we can write loops and do the same\n> computation as your awk script does, but it does not reduce the complexity\n> nor risk of portability issues), so let's see what happens. We already\n> use Perl everywhere, which we might end up using for this if there are\n> platforms that have issues with your awk script.\n\nI considered writing this in Perl, but my complete lack of knowledge of Perl\nmade that a non-starter. However if there are any problems with the awk\nscript then I would welcome a Perl rewrite by someone who actually knows the\nlanguage.\n\n-Kevin Ballard\n"}]}