From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 10 Nov 2025 14:55:11 GMT
Subject: Re: [PATCH 11/12] diff: highlight and error out on incomplete lines
Message-ID: <7aa91693-bece-4fa6-ab14-f914d6fd49bd@gmail.com>
In-Reply-To: <20251104020928.582199-12-gitster@pobox.com>

On 04/11/2025 02:09, Junio C Hamano wrote:
> Teach "git diff" to highlight "\ No newline at end of file" message
> as a whitespace error when incomplete-line whitespace error class is
> in effect.  Thanks to the previous refactoring of complete rewrite
> code path, we can do this at a single place.
> 
> Unlike whitespace errors in the payload where we need to annotate in
> line, possibly using colors, the line that has whitespace problems,
> we have a dedicated line already that can serve as the error
> message, so paint it as a whitespace error message.

This explains why we don't need to call emit_line_ws_markup() in this case

> Also teach "git diff --check" to notice incomplete lines as
> whitespace errors and report when incomplete-line whitespace error
> class is in effect.

Nice. The implementation looks good, I've left a few comments on the tests
> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> index 9de7f73f42..138730cbce 100755
> --- a/t/t4015-diff-whitespace.sh
> +++ b/t/t4015-diff-whitespace.sh
> @@ -43,6 +43,49 @@ do
>   	'
>   done
>   
> +test_expect_success "incomplete line in both pre- and post-image context" '
> +	(echo foo && echo baz | tr -d "\012") >x &&

'printf "foo\nbaz"' might be clearer and save us forking "tr"

> +	git add x &&
> +	(echo bar && echo baz | tr -d "\012") >x &&
> +	git diff x &&
> +	git -c core.whitespace=incomplete diff --check x &&
> +	git diff -R x &&
> +	git -c core.whitespace=incomplete diff -R --check x
> +'
> +
> +test_expect_success "incomplete lines on both pre- and post-image" '
> +	# The interpretation taken here is "since you are toucing

s/toucing/touching/

> +	# the line anyway, you would better fix the incomplete line
> +	# while you are at it."  but this is debatable.

I think it is a reasonable default.
> +	echo foo | tr -d "\012" >x &&
> +	git add x &&
> +	echo bar | tr -d "\012" >x &&
> +	git diff x &&
> +	test_must_fail git -c core.whitespace=incomplete diff --check x &&

Do we want to check the error message here?

Looking at the tests below the coverage looks good for "diff --check" 
and for diff.wsErrorHighlight

Thanks

Phillip

> +	git diff -R x &&
> +	test_must_fail git -c core.whitespace=incomplete diff -R --check x
> +'
> +
> +test_expect_success "fix incomplete line in pre-image" '
> +	echo foo | tr -d "\012" >x &&
> +	git add x &&
> +	echo bar >x &&
> +	git diff x &&
> +	git -c core.whitespace=incomplete diff --check x &&
> +	git diff -R x &&
> +	test_must_fail git -c core.whitespace=incomplete diff -R --check x
> +'
> +
> +test_expect_success "new incomplete line in post-image" '
> +	echo foo >x &&
> +	git add x &&
> +	echo bar | tr -d "\012" >x &&
> +	git diff x &&
> +	test_must_fail git -c core.whitespace=incomplete diff --check x &&
> +	git diff -R x &&
> +	git -c core.whitespace=incomplete diff -R --check x
> +'
> +
>   test_expect_success "Ray Lehtiniemi's example" '
>   	cat <<-\EOF >x &&
>   	do {
> @@ -1040,7 +1083,8 @@ test_expect_success 'ws-error-highlight test setup' '
>   	{
>   		echo "0. blank-at-eol " &&
>   		echo "1. still-blank-at-eol " &&
> -		echo "2. and a new line "
> +		echo "2. and a new line " &&
> +		printf "3. and more"
>   	} >x &&
>   	new_hash_x=$(git hash-object x) &&
>   	after=$(git rev-parse --short "$new_hash_x") &&
> @@ -1050,11 +1094,13 @@ test_expect_success 'ws-error-highlight test setup' '
>   	<BOLD>index $before..$after 100644<RESET>
>   	<BOLD>--- a/x<RESET>
>   	<BOLD>+++ b/x<RESET>
> -	<CYAN>@@ -1,2 +1,3 @@<RESET>
> +	<CYAN>@@ -1,2 +1,4 @@<RESET>
>   	 0. blank-at-eol <RESET>
>   	<RED>-<RESET><RED>1. blank-at-eol<RESET><BLUE> <RESET>
>   	<GREEN>+<RESET><GREEN>1. still-blank-at-eol<RESET><BLUE> <RESET>
>   	<GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET>
> +	<GREEN>+<RESET><GREEN>3. and more<RESET>
> +	<BLUE>\ No newline at end of file<RESET>
>   	EOF
>   
>   	cat >expect.all <<-EOF &&
> @@ -1062,11 +1108,13 @@ test_expect_success 'ws-error-highlight test setup' '
>   	<BOLD>index $before..$after 100644<RESET>
>   	<BOLD>--- a/x<RESET>
>   	<BOLD>+++ b/x<RESET>
> -	<CYAN>@@ -1,2 +1,3 @@<RESET>
> +	<CYAN>@@ -1,2 +1,4 @@<RESET>
>   	 <RESET>0. blank-at-eol<RESET><BLUE> <RESET>
>   	<RED>-<RESET><RED>1. blank-at-eol<RESET><BLUE> <RESET>
>   	<GREEN>+<RESET><GREEN>1. still-blank-at-eol<RESET><BLUE> <RESET>
>   	<GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET>
> +	<GREEN>+<RESET><GREEN>3. and more<RESET>
> +	<BLUE>\ No newline at end of file<RESET>
>   	EOF
>   
>   	cat >expect.none <<-EOF
> @@ -1074,16 +1122,19 @@ test_expect_success 'ws-error-highlight test setup' '
>   	<BOLD>index $before..$after 100644<RESET>
>   	<BOLD>--- a/x<RESET>
>   	<BOLD>+++ b/x<RESET>
> -	<CYAN>@@ -1,2 +1,3 @@<RESET>
> +	<CYAN>@@ -1,2 +1,4 @@<RESET>
>   	 0. blank-at-eol <RESET>
>   	<RED>-1. blank-at-eol <RESET>
>   	<GREEN>+1. still-blank-at-eol <RESET>
>   	<GREEN>+2. and a new line <RESET>
> +	<GREEN>+3. and more<RESET>
> +	\ No newline at end of file<RESET>
>   	EOF
>   
>   '
>   
>   test_expect_success 'test --ws-error-highlight option' '
> +	git config core.whitespace blank-at-eol,incomplete-line &&
>   
>   	git diff --color --ws-error-highlight=default,old >current.raw &&
>   	test_decode_color <current.raw >current &&
> @@ -1100,6 +1151,7 @@ test_expect_success 'test --ws-error-highlight option' '
>   '
>   
>   test_expect_success 'test diff.wsErrorHighlight config' '
> +	git config core.whitespace blank-at-eol,incomplete-line &&
>   
>   	git -c diff.wsErrorHighlight=default,old diff --color >current.raw &&
>   	test_decode_color <current.raw >current &&
> @@ -1116,6 +1168,7 @@ test_expect_success 'test diff.wsErrorHighlight config' '
>   '
>   
>   test_expect_success 'option overrides diff.wsErrorHighlight' '
> +	git config core.whitespace blank-at-eol,incomplete-line &&
>   
>   	git -c diff.wsErrorHighlight=none \
>   		diff --color --ws-error-highlight=default,old >current.raw &&
> @@ -1135,6 +1188,8 @@ test_expect_success 'option overrides diff.wsErrorHighlight' '
>   '
>   
>   test_expect_success 'detect moved code, complete file' '
> +	git config core.whitespace blank-at-eol &&
> +
>   	git reset --hard &&
>   	cat <<-\EOF >test.c &&
>   	#include<stdio.h>