From: Adrian Ratiu Date: Wed, 07 Jan 2026 18:11:19 GMT Subject: Re: [PATCH v2] ws: add new tab-between-non-ws check Message-ID: <87y0m9guns.fsf@collabora.com> In-Reply-To: On Wed, 07 Jan 2026, Johannes Sixt 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 >> Signed-off-by: Adrian Ratiu >> --- >> 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 actual && >> + ! test_grep "1234567 b" 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 "1234567 b" 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