From: Johannes Sixt <j6t@kdbg•org>
To: Adrian Ratiu <adrian.ratiu@collabora•com>
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, 7 Jan 2026 18:33:14 +0100 [thread overview]
Message-ID: <d3f26459-d828-4d01-8c38-ce754e5cc576@kdbg.org> (raw)
In-Reply-To: <20260107013051.312291-1-adrian.ratiu@collabora.com>
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.)
> 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.
> +
> + 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?
> + 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.
> + 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?
Please consider all comments on this test case repeated (and suitably
adusted) for all other test cases added by this patch.
> +
> +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.
> +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.
> 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.
> { "incomplete-line", WS_INCOMPLETE_LINE, 0, 0 },
> };
I didn't look at the remaining code changes.
-- Hannes
next prev parent reply other threads:[~2026-01-07 17:33 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 [this message]
2026-01-07 18:11 ` Adrian Ratiu
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=d3f26459-d828-4d01-8c38-ce754e5cc576@kdbg.org \
--to=j6t@kdbg$(echo .)org \
--cc=adrian.ratiu@collabora$(echo .)com \
--cc=emilyshaffer@google$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--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