Re: [PATCH v2] ws: add new tab-between-non-ws check

[Adrian Ratiu <adrian.ratiu@collabora.com>]

On Wed, 07 Jan 2026, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 07.01.26 um 02:30 schrieb Adrian Ratiu:
>> This adds a new check to detect HT in the middle of sentences that
>> should have been a SP, as suggested by Junio in
>> https://public-inbox.org/git/xmqqy0mwsedz.fsf@gitster.g/
>
> Generally, please review the commit message to follow the project's
> style: Use imperative mood in sentences the describe the changes ("Add a
> new check to...", "Supoort highlighting for tools like...", "Enable the
> new chaeck for...", etc.)

Ack, will fix.

>> The check is a bit complex because we want to detect places where
>> a SP was intended (HT can expand to more than one display column),
>> so we need to count both the display columns (col) and the string
>> character columns (i) to determine if a HT looks identical to a SP
>> or can cause confusion.
>> 
>> Highlighting support for tools like git diff/show/log is added, as
>> well as git apply --whitespace=fix capability.
>> 
>> The middle section of the line used to be assumed non-highlighted,
>> which is obviously not true anymore, so we split its logic into a
>> separate function named emit_middle_section().
>> 
>> The new check is enabled for Documentation/**/*.adoc, where these
>> kinds of mistakes were seen in practice. It can also be enabled in
>> other locations where it can be useful, by adding to the relevant
>> attributes file.
>> 
>> Suggested-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
>> ---
>> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
>> index 3c8eb02e4f..f5b6ceeed9 100755
>> --- a/t/t4015-diff-whitespace.sh
>> +++ b/t/t4015-diff-whitespace.sh
>> @@ -2440,4 +2440,147 @@ test_expect_success 'combine --ignore-blank-lines with --function-context 2' '
>>  	test_cmp expect actual
>>  '
>>  
>> +test_expect_success 'check tab between non-whitespace (tab-between-non-ws: off)' '
>> +	git config core.whitespace "-tab-between-non-ws" &&
>
> It might be worthwhile using test_config here, because this setting does
> not need to persist for the remaining tests.

Ack, will do.

>> +
>> +	printf "1234567\tb" >x &&
>
> What if you made the test cases into
>
> 	# only the TAB in the middle must be diagnosed
> 	printf "\t1234567\t12\t90\n" >x &&
>
> to test that only the second of the three TABs is diagnosed?

Yes we can do it this way.

>> +	git add x &&
>> +	git diff --cached --check &&
>> +
>> +	git diff --cached --color >raw &&
>> +	test_decode_color <raw >actual &&
>> +	! test_grep "<GREEN>1234567<RESET><BLUE>	<RESET><GREEN>b<RESET>" actual &&
>
> This must be
>
> 	test_grep ! "...
>
> Furthermore, a negative test with a very tight pattern is often not
> desired: The test could fail if any single character does not occur
> (which could easily happen if the test text is changed, but not this
> pattern). In this case, it would be sufficient to test only that "BLUE"
> does not occur.

Thanks, I'm still a bit of a noob wrt the git codebase. Will do.

>> +	test_grep "<GREEN>1234567	b<RESET>" actual &&
>> +
>> +	# should apply without error because tab-between-non-ws is off
>> +	git diff --cached >patch.diff &&
>> +	git checkout HEAD -- x &&
>> +	git apply --whitespace=error patch.diff
>> +'
>
> There is t/t4124-apply-ws-rule.sh. Wouldn't the `git apply` tests be
> better located there?

I think so, yes, thanks for pointing it out.

>
> Please consider all comments on this test case repeated (and suitably
> adusted) for all other test cases added by this patch.

Will do.

>> +
>> +test_expect_success 'check tab between non-whitespace at tab stop (tab-between-non-ws: on)' '
>> +	git config core.whitespace "tab-between-non-ws,tabwidth=8" &&
>
> I am curious why you set tabwidth=8 here even though 8 is the default.

Just to make it explicit because I also set tabwidth=4 in the following
tests, however I can drop it.

>> +test_expect_success 'check tab between non-whitespace not at tab stop (tab-between-non-ws: on)' '
>
> With my suggested text above, this case does not need a separate test, I
> think.

Yes, it likely is redundant. I'll double check the tabwidth=8 test as
well because that might also be redundant and can be dropped entirely.

>
>> diff --git a/ws.c b/ws.c
>> index 6cc2466c0c..633bc69418 100644
>> --- a/ws.c
>> +++ b/ws.c
>> @@ -26,6 +26,7 @@ static struct whitespace_rule {
>>  	{ "blank-at-eol", WS_BLANK_AT_EOL, 0 },
>>  	{ "blank-at-eof", WS_BLANK_AT_EOF, 0 },
>>  	{ "tab-in-indent", WS_TAB_IN_INDENT, 0, 1 },
>> +	{ "tab-between-non-ws", WS_TAB_BETWEEN_NON_WS, 0 },
>
> How about "tab-is-1-space"? The documentation can clarify that not any
> TAB expanding to width 1 is diagnosed, but only those that are between
> non-space characters.

I like this suggestion as well.

Many thanks for the review,
Adrian