From: Adrian Ratiu <adrian.ratiu@collabora•com>
To: Johannes Sixt <j6t@kdbg•org>
Cc: Junio C Hamano <gitster@pobox•com>,
Patrick Steinhardt <ps@pks•im>,
Emily Shaffer <emilyshaffer@google•com>,
git@vger•kernel.org
Subject: Re: [PATCH v2] ws: add new tab-between-non-ws check
Date: Wed, 07 Jan 2026 20:11:19 +0200 [thread overview]
Message-ID: <87y0m9guns.fsf@collabora.com> (raw)
In-Reply-To: <d3f26459-d828-4d01-8c38-ce754e5cc576@kdbg.org>
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
next prev parent reply other threads:[~2026-01-07 18:11 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-01-07 1:30 [PATCH v2] ws: add new tab-between-non-ws check Adrian Ratiu
2026-01-07 2:12 ` Junio C Hamano
2026-01-07 11:34 ` Adrian Ratiu
2026-01-07 17:33 ` Johannes Sixt
2026-01-07 18:11 ` Adrian Ratiu [this message]
2026-01-08 8:36 ` Johannes Sixt
2026-01-08 9:01 ` Johannes Sixt
2026-01-09 13:33 ` Adrian Ratiu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y0m9guns.fsf@collabora.com \
--to=adrian.ratiu@collabora$(echo .)com \
--cc=emilyshaffer@google$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=j6t@kdbg$(echo .)org \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox