[PATCH] diff: handle lines containing only whitespace better

[PATCH] diff: handle lines containing only whitespace better

[Kevin Ballard]

When a line contains nothing but whitespace and the core.whitespace config
option contains blank-at-eol, the whitespace on the line is being printed
twice, once unhighlighted (unless otherwise matched by one of the other
core.whitespace values), and a second time highlighted for blank-at-eol.

Update the leading indentation check to stop checking when it reaches
the trailing whitespace.

Signed-off-by: Kevin Ballard <kevin@sb.org>
---
 ws.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/ws.c b/ws.c
index d7b8c33..7302f8f 100644
--- a/ws.c
+++ b/ws.c
@@ -174,8 +174,11 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
 		}
 	}
 
+	if (trailing_whitespace == -1)
+		trailing_whitespace = len;
+
 	/* Check indentation */
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < trailing_whitespace; i++) {
 		if (line[i] == ' ')
 			continue;
 		if (line[i] != '\t')
@@ -218,8 +221,6 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
 		 * Now the rest of the line starts at "written".
 		 * The non-highlighted part ends at "trailing_whitespace".
 		 */
-		if (trailing_whitespace == -1)
-			trailing_whitespace = len;
 
 		/* Emit non-highlighted (middle) segment. */
 		if (trailing_whitespace - written > 0) {
-- 
1.7.3.1.211.g81fee.dirty

Re: [PATCH] diff: handle lines containing only whitespace better

[Junio C Hamano]

Hmm, tests?

Re: [PATCH] diff: handle lines containing only whitespace better

[Kevin Ballard]

On Oct 19, 2010, at 11:16 PM, Junio C Hamano wrote:

> Hmm, tests?

I 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.

Incidentally, 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.

-Kevin Ballard

Re: [PATCH] diff: handle lines containing only whitespace better

[Nazri Ramliy]

On Wed, Oct 20, 2010 at 2:38 PM, Kevin Ballard <kevin@sb.org> wrote:
> ... 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.

Have a look at t/t4207-log-decoration-colors.sh for inspiration.

nazri

Re: [PATCH] diff: handle lines containing only whitespace better

[Junio C Hamano]

Kevin Ballard <kevin@sb.org> writes:

> Incidentally, I just realized the description of the patch is slightly
> wrong. The problem only occurs when the line contains at least one
> tab...

That exactly is why I asked for tests, as I couldn't reproduce it from the
description at all.

But I see it now.

	$ HT='   '
	$ mv Makefile Makefile+
        $ sed -e "3s/^.*/$HT/" Makefile+ >Makefile
        $ git diff --color

> ...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.

This may show us a good starting point.

    $ git grep RED t/

[PATCH v2 1/2] test-lib: extend test_decode_color to handle more color codes

[Kevin Ballard]

Enhance the test_decode_color function to handle all common color codes,
including background colors and escapes that contain multiple codes.
This change necessitates changing <WHITE> to <BOLD>, so update t4034
as well.

This change is necessary for the next commit in order to test
background colors properly.

Signed-off-by: Kevin Ballard <kevin@sb.org>
---
I turned sed into awk. Looks like awk is already used elsehwere in git,
so I'm assuming this is safe, but please let me know if it's not.

 t/t4034-diff-words.sh |   72 ++++++++++++++++++++++++------------------------
 t/test-lib.sh         |   49 ++++++++++++++++++++++++++++-----
 2 files changed, 77 insertions(+), 44 deletions(-)

diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 6f7548c..3f3c757 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -35,10 +35,10 @@ aeff = aeff * ( aaa )
 EOF
 
 cat > expect <<\EOF
-<WHITE>diff --git a/pre b/post<RESET>
-<WHITE>index 330b04f..5ed8eff 100644<RESET>
-<WHITE>--- a/pre<RESET>
-<WHITE>+++ b/post<RESET>
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 330b04f..5ed8eff 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
 <CYAN>@@ -1,3 +1,7 @@<RESET>
 <RED>h(4)<RESET><GREEN>h(4),hh[44]<RESET>
 
@@ -122,10 +122,10 @@ test_expect_success '--word-diff=plain --no-color' '
 '
 
 cat > expect <<EOF
-<WHITE>diff --git a/pre b/post<RESET>
-<WHITE>index 330b04f..5ed8eff 100644<RESET>
-<WHITE>--- a/pre<RESET>
-<WHITE>+++ b/post<RESET>
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 330b04f..5ed8eff 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
 <CYAN>@@ -1,3 +1,7 @@<RESET>
 <RED>[-h(4)-]<RESET><GREEN>{+h(4),hh[44]+}<RESET>
 
@@ -143,10 +143,10 @@ test_expect_success '--word-diff=plain --color' '
 '
 
 cat > expect <<\EOF
-<WHITE>diff --git a/pre b/post<RESET>
-<WHITE>index 330b04f..5ed8eff 100644<RESET>
-<WHITE>--- a/pre<RESET>
-<WHITE>+++ b/post<RESET>
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 330b04f..5ed8eff 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
 <CYAN>@@ -1 +1 @@<RESET>
 <RED>h(4)<RESET><GREEN>h(4),hh[44]<RESET>
 <CYAN>@@ -3,0 +4,4 @@<RESET> <RESET><MAGENTA>a = b + c<RESET>
@@ -163,10 +163,10 @@ test_expect_success 'word diff without context' '
 '
 
 cat > expect <<\EOF
-<WHITE>diff --git a/pre b/post<RESET>
-<WHITE>index 330b04f..5ed8eff 100644<RESET>
-<WHITE>--- a/pre<RESET>
-<WHITE>+++ b/post<RESET>
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 330b04f..5ed8eff 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
 <CYAN>@@ -1,3 +1,7 @@<RESET>
 h(4),<GREEN>hh<RESET>[44]
 
@@ -199,10 +199,10 @@ test_expect_success 'option overrides .gitattributes' '
 '
 
 cat > expect <<\EOF
-<WHITE>diff --git a/pre b/post<RESET>
-<WHITE>index 330b04f..5ed8eff 100644<RESET>
-<WHITE>--- a/pre<RESET>
-<WHITE>+++ b/post<RESET>
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 330b04f..5ed8eff 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
 <CYAN>@@ -1,3 +1,7 @@<RESET>
 h(4)<GREEN>,hh[44]<RESET>
 
@@ -231,10 +231,10 @@ test_expect_success 'command-line overrides config' '
 '
 
 cat > expect <<\EOF
-<WHITE>diff --git a/pre b/post<RESET>
-<WHITE>index 330b04f..5ed8eff 100644<RESET>
-<WHITE>--- a/pre<RESET>
-<WHITE>+++ b/post<RESET>
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 330b04f..5ed8eff 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
 <CYAN>@@ -1,3 +1,7 @@<RESET>
 h(4),<GREEN>{+hh+}<RESET>[44]
 
@@ -260,10 +260,10 @@ test_expect_success 'remove diff driver regex' '
 '
 
 cat > expect <<\EOF
-<WHITE>diff --git a/pre b/post<RESET>
-<WHITE>index 330b04f..5ed8eff 100644<RESET>
-<WHITE>--- a/pre<RESET>
-<WHITE>+++ b/post<RESET>
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 330b04f..5ed8eff 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
 <CYAN>@@ -1,3 +1,7 @@<RESET>
 h(4),<GREEN>hh[44<RESET>]
 
@@ -282,10 +282,10 @@ echo 'aaa (aaa)' > pre
 echo 'aaa (aaa) aaa' > post
 
 cat > expect <<\EOF
-<WHITE>diff --git a/pre b/post<RESET>
-<WHITE>index c29453b..be22f37 100644<RESET>
-<WHITE>--- a/pre<RESET>
-<WHITE>+++ b/post<RESET>
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index c29453b..be22f37 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
 <CYAN>@@ -1 +1 @@<RESET>
 aaa (aaa) <GREEN>aaa<RESET>
 EOF
@@ -301,10 +301,10 @@ echo '(:' > pre
 echo '(' > post
 
 cat > expect <<\EOF
-<WHITE>diff --git a/pre b/post<RESET>
-<WHITE>index 289cb9d..2d06f37 100644<RESET>
-<WHITE>--- a/pre<RESET>
-<WHITE>+++ b/post<RESET>
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 289cb9d..2d06f37 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
 <CYAN>@@ -1 +1 @@<RESET>
 (<RED>:<RESET>
 EOF
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 2af8f10..6dd3ce9 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -238,14 +238,47 @@ test_set_editor () {
 }
 
 test_decode_color () {
-	sed	-e 's/.\[1m/<WHITE>/g' \
-		-e 's/.\[31m/<RED>/g' \
-		-e 's/.\[32m/<GREEN>/g' \
-		-e 's/.\[33m/<YELLOW>/g' \
-		-e 's/.\[34m/<BLUE>/g' \
-		-e 's/.\[35m/<MAGENTA>/g' \
-		-e 's/.\[36m/<CYAN>/g' \
-		-e 's/.\[m/<RESET>/g'
+	awk '
+		function name(n) {
+			if (n == 0) return "RESET";
+			if (n == 1) return "BOLD";
+			if (n == 30) return "BLACK";
+			if (n == 31) return "RED";
+			if (n == 32) return "GREEN";
+			if (n == 33) return "YELLOW";
+			if (n == 34) return "BLUE";
+			if (n == 35) return "MAGENTA";
+			if (n == 36) return "CYAN";
+			if (n == 37) return "WHITE";
+			if (n == 40) return "BLACK";
+			if (n == 41) return "BRED";
+			if (n == 42) return "BGREEN";
+			if (n == 43) return "BYELLOW";
+			if (n == 44) return "BBLUE";
+			if (n == 45) return "BMAGENTA";
+			if (n == 46) return "BCYAN";
+			if (n == 47) return "BWHITE";
+		}
+		{
+			while (match($0, /\x1b\[[0-9;]*m/) != 0) {
+				printf "%s<", substr($0, 1, RSTART-1);
+				codes = substr($0, RSTART+2, RLENGTH-3);
+				if (length(codes) == 0)
+					printf "%s", name(0)
+				else {
+					n = split(codes, ary, ";");
+					sep = "";
+					for (i = 1; i <= n; i++) {
+						printf "%s%s", sep, name(ary[i]);
+						sep = ";"
+					}
+				}
+				printf ">";
+				$0 = substr($0, RSTART + RLENGTH, length($0) - RSTART - RLENGTH + 1);
+			}
+			print
+		}
+	'
 }
 
 q_to_nul () {
-- 
1.7.3.1.220.g19a98

Re: [PATCH v2 1/2] test-lib: extend test_decode_color to handle more color codes

[Junio C Hamano]

Kevin Ballard <kevin@sb.org> writes:

> Enhance the test_decode_color function to handle all common color codes,
> including background colors and escapes that contain multiple codes.
> This change necessitates changing <WHITE> to <BOLD>, so update t4034
> as well.
>
> This change is necessary for the next commit in order to test
> background colors properly.
>
> Signed-off-by: Kevin Ballard <kevin@sb.org>
> ---
> I turned sed into awk. Looks like awk is already used elsehwere in git,
> so I'm assuming this is safe, but please let me know if it's not.

I think calling BOLD BOLD is the right thing to do (who came up with the
bogus WHITE in the first place anyway---my terminal is black letters on
white background, thank you).

Even though some scripts seem to already use awk, they are all used for
very small and trivial processing without exercising anything remotely
fancy e.g. hexadecimal \xXX quoting or match() function, so I wouldn't be
surprised if we see breakage reports from minority platforms.

But I do not think of a trivial way to express combination of attributes
by extending the existing sed script (we can write loops and do the same
computation as your awk script does, but it does not reduce the complexity
nor risk of portability issues), so let's see what happens.  We already
use Perl everywhere, which we might end up using for this if there are
platforms that have issues with your awk script.

Thanks.

Re: [PATCH v2 1/2] test-lib: extend test_decode_color to handle more color codes

[Kevin Ballard]

On Oct 20, 2010, at 4:40 PM, Junio C Hamano wrote:

> Kevin Ballard <kevin@sb.org> writes:
> 
>> Enhance the test_decode_color function to handle all common color codes,
>> including background colors and escapes that contain multiple codes.
>> This change necessitates changing <WHITE> to <BOLD>, so update t4034
>> as well.
>> 
>> This change is necessary for the next commit in order to test
>> background colors properly.
>> 
>> Signed-off-by: Kevin Ballard <kevin@sb.org>
>> ---
>> I turned sed into awk. Looks like awk is already used elsehwere in git,
>> so I'm assuming this is safe, but please let me know if it's not.
> 
> I think calling BOLD BOLD is the right thing to do (who came up with the
> bogus WHITE in the first place anyway---my terminal is black letters on
> white background, thank you).
> 
> Even though some scripts seem to already use awk, they are all used for
> very small and trivial processing without exercising anything remotely
> fancy e.g. hexadecimal \xXX quoting or match() function, so I wouldn't be
> surprised if we see breakage reports from minority platforms.

Entirely possible, but I'm hoping that's not the case. I used this against
BSD awk version 20070501, so there's nothing remotely fancy there, and the
regular expression conforms to the subset of BRE's mentioned in
CodingGuidelines. But as you said, it's possible that minority platforms
don't handle this correctly.

> But I do not think of a trivial way to express combination of attributes
> by extending the existing sed script (we can write loops and do the same
> computation as your awk script does, but it does not reduce the complexity
> nor risk of portability issues), so let's see what happens.  We already
> use Perl everywhere, which we might end up using for this if there are
> platforms that have issues with your awk script.

I considered writing this in Perl, but my complete lack of knowledge of Perl
made that a non-starter. However if there are any problems with the awk
script then I would welcome a Perl rewrite by someone who actually knows the
language.

-Kevin Ballard

[PATCH v2 2/2] diff: handle lines containing only whitespace and tabs better

[Kevin Ballard]

When a line contains nothing but whitespace with at least one tab
and the core.whitespace config option contains blank-at-eol, the
whitespace on the line is being printed twice, once unhighlighted
(unless otherwise matched by one of the other core.whitespace values),
and a second time highlighted for blank-at-eol.

Update the leading indentation check to stop checking when it reaches
the trailing whitespace.

Signed-off-by: Kevin Ballard <kevin@sb.org>
---
 t/t4015-diff-whitespace.sh |   37 +++++++++++++++++++++++++++++++++++++
 ws.c                       |    7 ++++---
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 935d101..a8736f7 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -491,4 +491,41 @@ test_expect_success 'combined diff with autocrlf conversion' '
 
 '
 
+# Start testing the colored format for whitespace checks
+
+test_expect_success 'setup diff colors' '
+	git config color.diff always &&
+	git config color.diff.plain normal &&
+	git config color.diff.meta bold &&
+	git config color.diff.frag cyan &&
+	git config color.diff.func normal &&
+	git config color.diff.old red &&
+	git config color.diff.new green &&
+	git config color.diff.commit yellow &&
+	git config color.diff.whitespace "normal red" &&
+
+	git config core.autocrlf false
+'
+cat >expected <<\EOF
+<BOLD>diff --git a/x b/x<RESET>
+<BOLD>index 9daeafb..2874b91 100644<RESET>
+<BOLD>--- a/x<RESET>
+<BOLD>+++ b/x<RESET>
+<CYAN>@@ -1 +1,4 @@<RESET>
+ test<RESET>
+<GREEN>+<RESET><GREEN>{<RESET>
+<GREEN>+<RESET><BRED>	<RESET>
+<GREEN>+<RESET><GREEN>}<RESET>
+EOF
+
+test_expect_success 'diff that introduces a line with only tabs' '
+	git config core.whitespace blank-at-eol &&
+	git reset --hard &&
+	echo "test" > x &&
+	git commit -m "initial" x &&
+	echo "{NTN}" | tr "NT" "\n\t" >> x &&
+	git -c color.diff=always diff | test_decode_color >current &&
+	test_cmp expected current
+'
+
 test_done
diff --git a/ws.c b/ws.c
index d7b8c33..7302f8f 100644
--- a/ws.c
+++ b/ws.c
@@ -174,8 +174,11 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
 		}
 	}
 
+	if (trailing_whitespace == -1)
+		trailing_whitespace = len;
+
 	/* Check indentation */
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < trailing_whitespace; i++) {
 		if (line[i] == ' ')
 			continue;
 		if (line[i] != '\t')
@@ -218,8 +221,6 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
 		 * Now the rest of the line starts at "written".
 		 * The non-highlighted part ends at "trailing_whitespace".
 		 */
-		if (trailing_whitespace == -1)
-			trailing_whitespace = len;
 
 		/* Emit non-highlighted (middle) segment. */
 		if (trailing_whitespace - written > 0) {
-- 
1.7.3.1.220.g19a98