threads / patch / 25484

patchdiff: handle lines containing only whitespace better

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

## tl;dr

9 messages between Oct 20, 2010 and Oct 21, 2010. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Kevin Ballard· Oct 20, 2010, 04:46 UTC · lore

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(-)
Show changes to ws.c +4 −3
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
Kevin Ballard· Oct 20, 2010, 06:38 UTC · re: Junio C Hamano · lore

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

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
Nazri Ramliy· Oct 20, 2010, 07:51 UTC · re: Kevin Ballard · lore

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

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
Junio C Hamano· Oct 20, 2010, 16:08 UTC · re: Kevin Ballard · lore

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

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/
Kevin Ballard· Oct 20, 2010, 22:17 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +77 −44

t/t4034-diff-words.sh, t/test-lib.sh

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
Junio C Hamano· Oct 20, 2010, 23:40 UTC · re: Kevin Ballard · lore

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

Kevin Ballard <kevin@sb.org> writes:
Show 12 quoted lines
> 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.
Kevin Ballard· Oct 21, 2010, 00:11 UTC · re: Junio C Hamano · lore

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

On Oct 20, 2010, at 4:40 PM, Junio C Hamano wrote:
Show 23 quoted lines
> 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.

Show 6 quoted lines
> 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
Kevin Ballard· Oct 20, 2010, 22:17 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +41 −3

t/t4015-diff-whitespace.sh, ws.c

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

← back to recent threads