{"thread":{"id":"64737","subject":"[PATCH v2] ws: add new tab-between-non-ws check","startedAt":"2026-01-07T01:31:29Z","lastAt":"2026-01-09T13:34:05Z","messageCount":8,"participants":["Adrian Ratiu","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"533175","messageId":"20260107013051.312291-1-adrian.ratiu@collabora.com","threadId":"64737","inReplyTo":null,"subject":"[PATCH v2] ws: add new tab-between-non-ws check","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-07T01:30:51Z","receivedAt":"2026-01-07T01:31:29Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"This adds a new check to detect HT in the middle of sentences that\nshould have been a SP, as suggested by Junio in\nhttps://public-inbox.org/git/xmqqy0mwsedz.fsf@gitster.g/\n\nThe check is a bit complex because we want to detect places where\na SP was intended (HT can expand to more than one display column),\nso we need to count both the display columns (col) and the string\ncharacter columns (i) to determine if a HT looks identical to a SP\nor can cause confusion.\n\nHighlighting support for tools like git diff/show/log is added, as\nwell as git apply --whitespace=fix capability.\n\nThe middle section of the line used to be assumed non-highlighted,\nwhich is obviously not true anymore, so we split its logic into a\nseparate function named emit_middle_section().\n\nThe new check is enabled for Documentation/**/*.adoc, where these\nkinds of mistakes were seen in practice. It can also be enabled in\nother locations where it can be useful, by adding to the relevant\nattributes file.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>\n---\nChanges in v2:\n* Highlight the new error in ws_check_emit_1() output generation so\n  tools like color git diff can show the errors to the user (Junio)\n* Expanded ws_fix_copy to detect and fix this new error (Junio)\n* Added more test cases for the above new functionality (Junio)\n* Improved commit message (Junio)\n\nBased on the latest master branch.\nPushed to GitHub: https://github.com/10ne1/git/tree/dev/aratiu/whitespace-new-test-v2\nSuccessful CI run: https://github.com/10ne1/git/actions/runs/20766920987\n---\n .gitattributes             |   2 +-\n t/t4015-diff-whitespace.sh | 143 +++++++++++++++++++++++++++++\n ws.c                       | 179 ++++++++++++++++++++++++++++++++++---\n ws.h                       |   1 +\n 4 files changed, 313 insertions(+), 12 deletions(-)\n\ndiff --git a/.gitattributes b/.gitattributes\nindex 700743c3f5..d3c40a038b 100644\n--- a/.gitattributes\n+++ b/.gitattributes\n@@ -7,7 +7,7 @@\n *.py text eol=lf diff=python\n *.bat text eol=crlf\n CODE_OF_CONDUCT.md -whitespace\n-/Documentation/**/*.adoc text eol=lf whitespace=trail,space,incomplete\n+/Documentation/**/*.adoc text eol=lf whitespace=trail,space,incomplete,tab-between-non-ws\n /command-list.txt text eol=lf\n /GIT-VERSION-GEN text eol=lf\n /mergetools/* text eol=lf\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 3c8eb02e4f..f5b6ceeed9 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -2440,4 +2440,147 @@ test_expect_success 'combine --ignore-blank-lines with --function-context 2' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check tab between non-whitespace (tab-between-non-ws: off)' '\n+\tgit config core.whitespace \"-tab-between-non-ws\" &&\n+\n+\tprintf \"1234567\\tb\" >x &&\n+\tgit add x &&\n+\tgit diff --cached --check &&\n+\n+\tgit diff --cached --color >raw &&\n+\ttest_decode_color <raw >actual &&\n+\t! test_grep \"<GREEN>1234567<RESET><BLUE>\t<RESET><GREEN>b<RESET>\" actual &&\n+\ttest_grep \"<GREEN>1234567\tb<RESET>\" actual &&\n+\n+\t# should apply without error because tab-between-non-ws is off\n+\tgit diff --cached >patch.diff &&\n+\tgit checkout HEAD -- x &&\n+\tgit apply --whitespace=error patch.diff\n+'\n+\n+test_expect_success 'check tab between non-whitespace at tab stop (tab-between-non-ws: on)' '\n+\tgit config core.whitespace \"tab-between-non-ws,tabwidth=8\" &&\n+\tprintf \"1234567\\tb\" >x &&\n+\tgit add x &&\n+\ttest_must_fail git diff --cached --check &&\n+\n+\tgit diff --cached --color >raw &&\n+\ttest_decode_color <raw >actual &&\n+\ttest_grep \"<GREEN>1234567<RESET><BLUE>\t<RESET><GREEN>b<RESET>\" actual &&\n+\t! test_grep \"<GREEN>1234567\tb<RESET>\" actual &&\n+\n+\tgit diff --cached >patch.diff &&\n+\tgit checkout HEAD -- x &&\n+\ttest_must_fail git apply --whitespace=error patch.diff &&\n+\tgit apply --whitespace=fix patch.diff &&\n+\tprintf \"1234567 b\" >expected &&\n+\ttest_cmp expected x\n+'\n+\n+test_expect_success 'check tab between non-whitespace not at tab stop (tab-between-non-ws: on)' '\n+\tgit config core.whitespace \"tab-between-non-ws,tabwidth=8\" &&\n+\tprintf \"a\\tb\" >x &&\n+\tgit add x &&\n+\tgit diff --cached --check &&\n+\n+\tgit diff --cached --color >raw &&\n+\ttest_decode_color <raw >actual &&\n+\t! test_grep \"<GREEN>a<RESET><BLUE>\t<RESET><GREEN>b<RESET>\" actual &&\n+\ttest_grep \"<GREEN>a\tb<RESET>\" actual &&\n+\n+\t# should apply without error because the input is valid\n+\tgit diff --cached >patch.diff &&\n+\tgit checkout HEAD -- x &&\n+\tgit apply --whitespace=error patch.diff\n+'\n+\n+test_expect_success 'check tab between non-whitespace with tabwidth=4 (tab-between-non-ws: on)' '\n+\tgit config core.whitespace \"tab-between-non-ws,tabwidth=4\" &&\n+\tprintf \"123\\tb\" >x &&\n+\tgit add x &&\n+\ttest_must_fail git diff --cached --check &&\n+\n+\tgit diff --cached --color >raw &&\n+\ttest_decode_color <raw >actual &&\n+\ttest_grep \"<GREEN>123<RESET><BLUE>\t<RESET><GREEN>b<RESET>\" actual &&\n+\t! test_grep \"<GREEN>123\tb<RESET>\" actual &&\n+\n+\tgit diff --cached >patch.diff &&\n+\tgit checkout HEAD -- x &&\n+\ttest_must_fail git apply --whitespace=error patch.diff &&\n+\tgit apply --whitespace=fix patch.diff &&\n+\tprintf \"123 b\" >expected &&\n+\ttest_cmp expected x\n+'\n+\n+test_expect_success 'check tab between non-whitespace with tabwidth=4 (tab-between-non-ws: on)' '\n+\tgit config core.whitespace \"tab-between-non-ws,tabwidth=4\" &&\n+\tprintf \"1234\\tb\" >x &&\n+\tgit add x &&\n+\tgit diff --cached --check &&\n+\n+\tgit diff --cached --color >raw &&\n+\ttest_decode_color <raw >actual &&\n+\t! test_grep \"<GREEN>1234<RESET><BLUE>\t<RESET><GREEN>b<RESET>\" actual &&\n+\ttest_grep \"<GREEN>1234\tb<RESET>\" actual &&\n+\n+\t# should apply without error because tab is at tab stop\n+\tgit diff --cached >patch.diff &&\n+\tgit checkout HEAD -- x &&\n+\tgit apply --whitespace=error patch.diff\n+'\n+\n+test_expect_success 'check multiple tabs with one error (tab-between-non-ws: on)' '\n+\tgit config core.whitespace \"tab-between-non-ws,tabwidth=8\" &&\n+\tprintf \"a\\t1234567\\tb\" >x &&\n+\tgit add x &&\n+\ttest_must_fail git diff --cached --check &&\n+\n+\tgit diff --cached --color >raw &&\n+\ttest_decode_color <raw >actual &&\n+\ttest_grep \"<GREEN>a\t1234567<RESET><BLUE>\t<RESET><GREEN>b<RESET>\" actual &&\n+\t! test_grep \"<GREEN>a\t1234567\tb<RESET>\" actual &&\n+\n+\tgit diff --cached >patch.diff &&\n+\tgit checkout HEAD -- x &&\n+\ttest_must_fail git apply --whitespace=error patch.diff &&\n+\tgit apply --whitespace=fix patch.diff &&\n+\tprintf \"a\\t1234567 b\" >expected &&\n+\ttest_cmp expected x\n+'\n+\n+test_expect_success 'check tab at beginning of line (tab-between-non-ws: on)' '\n+\tgit config core.whitespace \"tab-between-non-ws,tabwidth=8\" &&\n+\tprintf \"\\ta\" >x &&\n+\tgit add x &&\n+\tgit diff --cached --check &&\n+\n+\tgit diff --cached --color >raw &&\n+\ttest_decode_color <raw >actual &&\n+\t! test_grep \"<BLUE>\t\" actual &&\n+\ttest_grep \"<GREEN>+<RESET>\t<GREEN>a<RESET>\" actual &&\n+\n+\t# should apply without error because tab is a valid indentation\n+\tgit diff --cached >patch.diff &&\n+\tgit checkout HEAD -- x &&\n+\tgit apply --whitespace=error patch.diff\n+'\n+\n+test_expect_success 'check tab at end of line(tab-between-non-ws: on)' '\n+\tgit config core.whitespace \"tab-between-non-ws,-trailing-space,tabwidth=8\" &&\n+\tprintf \"a\\t\" >x &&\n+\tgit add x &&\n+\tgit diff --cached --check &&\n+\n+\tgit diff --cached --color >raw &&\n+\ttest_decode_color <raw >actual &&\n+\t! test_grep \"<GREEN>a<RESET><BLUE>\t\" actual &&\n+\ttest_grep \"<GREEN>a\t<RESET>\" actual &&\n+\n+\t# should apply without error because tab is caught by another check (trailing-space)\n+\tgit diff --cached >patch.diff &&\n+\tgit checkout HEAD -- x &&\n+\tgit apply --whitespace=error patch.diff\n+'\n+\n test_done\ndiff --git a/ws.c b/ws.c\nindex 6cc2466c0c..633bc69418 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -26,6 +26,7 @@ static struct whitespace_rule {\n \t{ \"blank-at-eol\", WS_BLANK_AT_EOL, 0 },\n \t{ \"blank-at-eof\", WS_BLANK_AT_EOF, 0 },\n \t{ \"tab-in-indent\", WS_TAB_IN_INDENT, 0, 1 },\n+\t{ \"tab-between-non-ws\", WS_TAB_BETWEEN_NON_WS, 0 },\n \t{ \"incomplete-line\", WS_INCOMPLETE_LINE, 0, 0 },\n };\n \n@@ -140,6 +141,11 @@ char *whitespace_error_string(unsigned ws)\n \t\t\tstrbuf_addstr(&err, \", \");\n \t\tstrbuf_addstr(&err, \"tab in indent\");\n \t}\n+\tif (ws & WS_TAB_BETWEEN_NON_WS) {\n+\t\tif (err.len)\n+\t\t\tstrbuf_addstr(&err, \", \");\n+\t\tstrbuf_addstr(&err, \"tab between non-whitespace characters\");\n+\t}\n \tif (ws & WS_INCOMPLETE_LINE) {\n \t\tif (err.len)\n \t\t\tstrbuf_addstr(&err, \", \");\n@@ -148,6 +154,80 @@ char *whitespace_error_string(unsigned ws)\n \treturn strbuf_detach(&err, NULL);\n }\n \n+static void emit_literal(const char *line, int len, FILE *stream)\n+{\n+\tfwrite(line, len, 1, stream);\n+}\n+\n+static void highlight_tabs(const char *line, int len,\n+\t\t\t   int written, int trailing_whitespace,\n+\t\t\t   unsigned ws_rule, FILE *stream,\n+\t\t\t   const char *set, const char *reset, const char *ws)\n+{\n+\tint tabwidth = ws_tab_width(ws_rule);\n+\tint start = written, col = 0;\n+\n+\tif (!tabwidth)\n+\t\tBUG(\"a known tabwidth is required by WS_TAB_BETWEEN_NON_WS\");\n+\n+\t/*\n+\t * Calculate the visual column position (col) up to 'written'.\n+\t * Tabs expand based on 'tabwidth', so for example the 5th character in a\n+\t * string might be at the 12th visual column, if the line contains tabs.\n+\t */\n+\tfor (int i = 0; i < written; i++) {\n+\t\tif (line[i] == '\\t')\n+\t\t\tcol += tabwidth - (col % tabwidth);\n+\t\telse\n+\t\t\tcol++;\n+\t}\n+\n+\t/* Iterate through the section of the line that needs potential highlighting. */\n+\tfor (int i = written; i < trailing_whitespace; i++) {\n+\t\tif (line[i] == '\\t') {\n+\t\t\tif (i > 0 && i < len - 1 &&\n+\t\t\t    !isspace(line[i - 1]) && !isspace(line[i + 1]) &&\n+\t\t\t    (col % tabwidth) == (tabwidth - 1)) {\n+\t\t\t\t/* Print unwritten content before the highlighted tab. */\n+\t\t\t\tif (start < i)\n+\t\t\t\t\temit_literal(line + start, i - start, stream);\n+\n+\t\t\t\t/* Apply highlight for the tab. */\n+\t\t\t\tfputs(reset, stream);\n+\t\t\t\tfputs(ws, stream);\n+\t\t\t\tfputc('\\t', stream);\n+\t\t\t\tfputs(reset, stream);\n+\t\t\t\tfputs(set, stream);\n+\t\t\t\tstart = i + 1;\n+\t\t\t}\n+\t\t\tcol += tabwidth - (col % tabwidth);\n+\t\t} else {\n+\t\t\tcol++; /* non-tab character. */\n+\t\t}\n+\t}\n+\n+\t/* Print any remaining content in the current segment after the last highlight. */\n+\tif (start < trailing_whitespace)\n+\t\temit_literal(line + start, trailing_whitespace - start, stream);\n+}\n+\n+static void emit_middle_section(const char *line, int len,\n+\t\t\t       int written, int trailing_whitespace,\n+\t\t\t       unsigned ws_rule, unsigned result,\n+\t\t\t       FILE *stream, const char *set,\n+\t\t\t       const char *reset, const char *ws)\n+{\n+\tfputs(set, stream);\n+\n+\tif (result & WS_TAB_BETWEEN_NON_WS)\n+\t\thighlight_tabs(line, len, written, trailing_whitespace,\n+\t\t\t       ws_rule, stream, set, reset, ws);\n+\telse\n+\t\temit_literal(line + written, trailing_whitespace - written, stream);\n+\n+\tfputs(reset, stream);\n+}\n+\n /* If stream is non-NULL, emits the line after checking. */\n static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,\n \t\t\t\tFILE *stream, const char *set,\n@@ -228,19 +308,41 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,\n \t\twritten = i;\n \t}\n \n-\tif (stream) {\n+\tif (ws_rule & WS_TAB_BETWEEN_NON_WS) {\n \t\t/*\n-\t\t * Now the rest of the line starts at \"written\".\n-\t\t * The non-highlighted part ends at \"trailing_whitespace\".\n+\t\t * A tab surrounded by non-whitespace characters is a typo candidate\n+\t\t * (a space might have been intended). This checks for a tab that\n+\t\t * would be expanded to a single space, which is when it appears at\n+\t\t * a column that is one less than a multiple of the tabwidth.\n \t\t */\n-\n-\t\t/* Emit non-highlighted (middle) segment. */\n-\t\tif (trailing_whitespace - written > 0) {\n-\t\t\tfputs(set, stream);\n-\t\t\tfwrite(line + written,\n-\t\t\t    trailing_whitespace - written, 1, stream);\n-\t\t\tfputs(reset, stream);\n+\t\tint col = 0;\n+\t\tint tabwidth = ws_tab_width(ws_rule);\n+\n+\t\tif (!tabwidth)\n+\t\t\tBUG(\"a known tabwidth is required by WS_TAB_BETWEEN_NON_WS\");\n+\n+\t\tfor (i = 0; i < len; i++) {\n+\t\t\tif (line[i] == '\\t') {\n+\t\t\t\tif (i > 0 && i < len - 1 &&\n+\t\t\t\t    !isspace(line[i - 1]) && !isspace(line[i + 1]) &&\n+\t\t\t\t    (col % tabwidth) == (tabwidth - 1))\n+\t\t\t\t\tresult |= WS_TAB_BETWEEN_NON_WS;\n+\t\t\t\tcol += tabwidth - (col % tabwidth);\n+\t\t\t} else {\n+\t\t\t\tcol++;\n+\t\t\t}\n \t\t}\n+\t}\n+\n+\tif (stream) {\n+\t\t/*\n+\t\t * The middle section of the line starts at \"written\" and ends at\n+\t\t * \"trailing_whitespace\".\n+\t\t */\n+\t\tif (trailing_whitespace - written > 0)\n+\t\t\temit_middle_section(line, len, written, trailing_whitespace,\n+\t\t\t\t\t    ws_rule, result,\n+\t\t\t\t\t    stream, set, reset, ws);\n \n \t\t/* Highlight errors in trailing whitespace. */\n \t\tif (trailing_whitespace != len) {\n@@ -299,6 +401,7 @@ void ws_fix_copy(struct strbuf *dst, const char *src, int len, unsigned ws_rule,\n \tint last_tab_in_indent = -1;\n \tint last_space_in_indent = -1;\n \tint need_fix_leading_space = 0;\n+\tsize_t pre_indent_len = dst->len;\n \n \t/*\n \t * Remembering that we need to add '\\n' at the end\n@@ -401,7 +504,61 @@ void ws_fix_copy(struct strbuf *dst, const char *src, int len, unsigned ws_rule,\n \t\tfixed = 1;\n \t}\n \n-\tstrbuf_add(dst, src, len);\n+\tif (!(ws_rule & WS_TAB_BETWEEN_NON_WS)) {\n+\t\t/*\n+\t\t * Middle section does not need fixing, so just append src to\n+\t\t * dst because it already contains the previous ws fixes.\n+\t\t */\n+\t\tstrbuf_add(dst, src, len);\n+\t} else {\n+\t\t/* Fix middle section HTs which expand to a single display space */\n+\t\tconst char *indent_part = dst->buf + pre_indent_len;\n+\t\tint indent_len = dst->len - pre_indent_len;\n+\t\tint tabwidth = ws_tab_width(ws_rule);\n+\t\tint col = 0;\n+\n+\t\tif (!tabwidth)\n+\t\t\tBUG(\"a known tabwidth is required by WS_TAB_BETWEEN_NON_WS\");\n+\n+\t\t/*\n+\t\t * Compute indentation display column length because it might not\n+\t\t * end at a fixed tab stop position.\n+\t\t */\n+\t\tfor (i = 0; i < indent_len; i++) {\n+\t\t\tif (indent_part[i] == '\\t')\n+\t\t\t\tcol += tabwidth - (col % tabwidth);\n+\t\t\telse\n+\t\t\t\tcol++;\n+\t\t}\n+\n+\t\t/* Go through all chars in src, fix if necessary, and append to dst */\n+\t\tfor (i = 0; i < len; i++) {\n+\t\t\tchar prev_ch = i > 0 ? src[i - 1] :\n+\t\t\t\t(indent_len > 0 ? indent_part[indent_len - 1] : '\\0');\n+\t\t\tchar next_ch = i < len - 1 ? src[i + 1] : '\\0';\n+\t\t\tbool needs_fixing = prev_ch && next_ch &&\n+\t\t\t\t!isspace(prev_ch) && !isspace(next_ch) &&\n+\t\t\t\t(col % tabwidth) == (tabwidth - 1);\n+\n+\t\t\t/* non HT chars are added as is */\n+\t\t\tif (src[i] != '\\t') {\n+\t\t\t\tstrbuf_addch(dst, src[i]);\n+\t\t\t\tcol++;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n+\t\t\t/* fix HT or add them as is */\n+\t\t\tif (needs_fixing) {\n+\t\t\t\tstrbuf_addch(dst, ' ');\n+\t\t\t\tcol++;\n+\t\t\t\tfixed = 1;\n+\t\t\t} else {\n+\t\t\t\tstrbuf_addch(dst, '\\t');\n+\t\t\t\tcol += tabwidth - (col % tabwidth);\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \tif (add_cr_to_tail)\n \t\tstrbuf_addch(dst, '\\r');\n \tif (add_nl_to_tail)\ndiff --git a/ws.h b/ws.h\nindex 06d5cb73f8..35475fd320 100644\n--- a/ws.h\n+++ b/ws.h\n@@ -16,6 +16,7 @@ struct strbuf;\n #define WS_BLANK_AT_EOF         (1<<10)\n #define WS_TAB_IN_INDENT        (1<<11)\n #define WS_INCOMPLETE_LINE      (1<<12)\n+#define WS_TAB_BETWEEN_NON_WS   (1<<13)\n \n #define WS_TRAILING_SPACE       (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF)\n #define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB|8)\n-- \n2.51.2\n\n"},{"id":"533177","messageId":"xmqqsecii327.fsf@gitster.g","threadId":"64737","inReplyTo":"20260107013051.312291-1-adrian.ratiu@collabora.com","subject":"Re: [PATCH v2] ws: add new tab-between-non-ws check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-07T02:12:16Z","receivedAt":"2026-01-07T02:12:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adrian Ratiu <adrian.ratiu@collabora.com> writes:\n\n> The check is a bit complex because we want to detect places where\n> a SP was intended (HT can expand to more than one display column),\n> so we need to count both the display columns (col) and the string\n> character columns (i) to determine if a HT looks identical to a SP\n> or can cause confusion.\n>\n> +/....adoc text eol=lf whitespace=trail,space,incomplete,tab-between-non-ws\n\nThe name of the whitespace rule does not quite match what we want to\ncatch.  Can somebody find a phrasing than \"between non-ws\" that\nconveys our intent better?  We want to catch a tab that is used by\nmistsake when the writer would have used a space, and \"between\nnon-ws\" is one of the heuristics (another is \"it is at the 7th\ncolumn to make it indistinguishable from a space\") the code uses to\ntell if a tab is such a mistaken tab.   \"tab-instead-of-space\"?\n\"tab-in-place-of-space\"?  \"tab-that-should-have-been-a-space\"?\n\nThe last one is horrible and not a serious suggestion, of course.\n\n> +test_expect_success 'check tab between non-whitespace (tab-between-non-ws: off)' '\n> +\tgit config core.whitespace \"-tab-between-non-ws\" &&\n> +\n> +\tprintf \"1234567\\tb\" >x &&\n\nI notice all these printf create incomplete lines.  It is true that\nthe detection of a tab that is used when it should have been a space\nshould work even on an incomplete line, but using an incomplete\nline, which is of course rather unusual, for these tests gives a\nfalse impression that somehow this requires an incomplete line to\ntrigger, which is not what we want to give.\n\n\tprintf \"1234567\\tb\\n\" > x &&\n\nor something, perhaps?  I dunno.\n"},{"id":"533209","messageId":"87a4ypirks.fsf@gentoo.mail-host-address-is-not-set","threadId":"64737","inReplyTo":"xmqqsecii327.fsf@gitster.g","subject":"Re: [PATCH v2] ws: add new tab-between-non-ws check","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-07T11:34:59Z","receivedAt":"2026-01-07T11:35:20Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Wed, 07 Jan 2026, Junio C Hamano <gitster@pobox.com> wrote:\n> Adrian Ratiu <adrian.ratiu@collabora.com> writes:\n>\n>> The check is a bit complex because we want to detect places where\n>> a SP was intended (HT can expand to more than one display column),\n>> so we need to count both the display columns (col) and the string\n>> character columns (i) to determine if a HT looks identical to a SP\n>> or can cause confusion.\n>>\n>> +/....adoc text eol=lf whitespace=trail,space,incomplete,tab-between-non-ws\n>\n> The name of the whitespace rule does not quite match what we want to\n> catch.  Can somebody find a phrasing than \"between non-ws\" that\n> conveys our intent better?  We want to catch a tab that is used by\n> mistsake when the writer would have used a space, and \"between\n> non-ws\" is one of the heuristics (another is \"it is at the 7th\n> column to make it indistinguishable from a space\") the code uses to\n> tell if a tab is such a mistaken tab.   \"tab-instead-of-space\"?\n> \"tab-in-place-of-space\"?  \"tab-that-should-have-been-a-space\"?\n>\n> The last one is horrible and not a serious suggestion, of course.\n\nI like \"tab-instead-of-space\". :)\n\nWill wait some time in case others have suggestions and if we can't come\nup with something better, then I will use \"tab-instead-of-space\" in v3.\n\n>> +test_expect_success 'check tab between non-whitespace (tab-between-non-ws: off)' '\n>> +\tgit config core.whitespace \"-tab-between-non-ws\" &&\n>> +\n>> +\tprintf \"1234567\\tb\" >x &&\n>\n> I notice all these printf create incomplete lines.  It is true that\n> the detection of a tab that is used when it should have been a space\n> should work even on an incomplete line, but using an incomplete\n> line, which is of course rather unusual, for these tests gives a\n> false impression that somehow this requires an incomplete line to\n> trigger, which is not what we want to give.\n>\n> \tprintf \"1234567\\tb\\n\" > x &&\n>\n> or something, perhaps?  I dunno.\n\nThat is a good idea. Will fix in v3. Thanks!\n"},{"id":"533234","messageId":"d3f26459-d828-4d01-8c38-ce754e5cc576@kdbg.org","threadId":"64737","inReplyTo":"20260107013051.312291-1-adrian.ratiu@collabora.com","subject":"Re: [PATCH v2] ws: add new tab-between-non-ws check","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2026-01-07T17:33:14Z","receivedAt":"2026-01-07T17:33:24Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 07.01.26 um 02:30 schrieb Adrian Ratiu:\n> This adds a new check to detect HT in the middle of sentences that\n> should have been a SP, as suggested by Junio in\n> https://public-inbox.org/git/xmqqy0mwsedz.fsf@gitster.g/\n\nGenerally, please review the commit message to follow the project's\nstyle: Use imperative mood in sentences the describe the changes (\"Add a\nnew check to...\", \"Supoort highlighting for tools like...\", \"Enable the\nnew chaeck for...\", etc.)\n\n> The check is a bit complex because we want to detect places where\n> a SP was intended (HT can expand to more than one display column),\n> so we need to count both the display columns (col) and the string\n> character columns (i) to determine if a HT looks identical to a SP\n> or can cause confusion.\n> \n> Highlighting support for tools like git diff/show/log is added, as\n> well as git apply --whitespace=fix capability.\n> \n> The middle section of the line used to be assumed non-highlighted,\n> which is obviously not true anymore, so we split its logic into a\n> separate function named emit_middle_section().\n> \n> The new check is enabled for Documentation/**/*.adoc, where these\n> kinds of mistakes were seen in practice. It can also be enabled in\n> other locations where it can be useful, by adding to the relevant\n> attributes file.\n> \n> Suggested-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>\n> ---\n\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> index 3c8eb02e4f..f5b6ceeed9 100755\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -2440,4 +2440,147 @@ test_expect_success 'combine --ignore-blank-lines with --function-context 2' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'check tab between non-whitespace (tab-between-non-ws: off)' '\n> +\tgit config core.whitespace \"-tab-between-non-ws\" &&\n\nIt might be worthwhile using test_config here, because this setting does\nnot need to persist for the remaining tests.\n\n> +\n> +\tprintf \"1234567\\tb\" >x &&\n\nWhat if you made the test cases into\n\n\t# only the TAB in the middle must be diagnosed\n\tprintf \"\\t1234567\\t12\\t90\\n\" >x &&\n\nto test that only the second of the three TABs is diagnosed?\n\n> +\tgit add x &&\n> +\tgit diff --cached --check &&\n> +\n> +\tgit diff --cached --color >raw &&\n> +\ttest_decode_color <raw >actual &&\n> +\t! test_grep \"<GREEN>1234567<RESET><BLUE>\t<RESET><GREEN>b<RESET>\" actual &&\n\nThis must be\n\n\ttest_grep ! \"...\n\nFurthermore, a negative test with a very tight pattern is often not\ndesired: The test could fail if any single character does not occur\n(which could easily happen if the test text is changed, but not this\npattern). In this case, it would be sufficient to test only that \"BLUE\"\ndoes not occur.\n\n> +\ttest_grep \"<GREEN>1234567\tb<RESET>\" actual &&\n> +\n> +\t# should apply without error because tab-between-non-ws is off\n> +\tgit diff --cached >patch.diff &&\n> +\tgit checkout HEAD -- x &&\n> +\tgit apply --whitespace=error patch.diff\n> +'\n\nThere is t/t4124-apply-ws-rule.sh. Wouldn't the `git apply` tests be\nbetter located there?\n\nPlease consider all comments on this test case repeated (and suitably\nadusted) for all other test cases added by this patch.\n\n> +\n> +test_expect_success 'check tab between non-whitespace at tab stop (tab-between-non-ws: on)' '\n> +\tgit config core.whitespace \"tab-between-non-ws,tabwidth=8\" &&\n\nI am curious why you set tabwidth=8 here even though 8 is the default.\n\n> +test_expect_success 'check tab between non-whitespace not at tab stop (tab-between-non-ws: on)' '\n\nWith my suggested text above, this case does not need a separate test, I\nthink.\n\n> diff --git a/ws.c b/ws.c\n> index 6cc2466c0c..633bc69418 100644\n> --- a/ws.c\n> +++ b/ws.c\n> @@ -26,6 +26,7 @@ static struct whitespace_rule {\n>  \t{ \"blank-at-eol\", WS_BLANK_AT_EOL, 0 },\n>  \t{ \"blank-at-eof\", WS_BLANK_AT_EOF, 0 },\n>  \t{ \"tab-in-indent\", WS_TAB_IN_INDENT, 0, 1 },\n> +\t{ \"tab-between-non-ws\", WS_TAB_BETWEEN_NON_WS, 0 },\n\nHow about \"tab-is-1-space\"? The documentation can clarify that not any\nTAB expanding to width 1 is diagnosed, but only those that are between\nnon-space characters.\n\n>  \t{ \"incomplete-line\", WS_INCOMPLETE_LINE, 0, 0 },\n>  };\n\nI didn't look at the remaining code changes.\n\n-- Hannes\n\n"},{"id":"533238","messageId":"87y0m9guns.fsf@collabora.com","threadId":"64737","inReplyTo":"d3f26459-d828-4d01-8c38-ce754e5cc576@kdbg.org","subject":"Re: [PATCH v2] ws: add new tab-between-non-ws check","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-07T18:11:19Z","receivedAt":"2026-01-07T18:11:37Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Wed, 07 Jan 2026, Johannes Sixt <j6t@kdbg.org> wrote:\n> Am 07.01.26 um 02:30 schrieb Adrian Ratiu:\n>> This adds a new check to detect HT in the middle of sentences that\n>> should have been a SP, as suggested by Junio in\n>> https://public-inbox.org/git/xmqqy0mwsedz.fsf@gitster.g/\n>\n> Generally, please review the commit message to follow the project's\n> style: Use imperative mood in sentences the describe the changes (\"Add a\n> new check to...\", \"Supoort highlighting for tools like...\", \"Enable the\n> new chaeck for...\", etc.)\n\nAck, will fix.\n\n>> The check is a bit complex because we want to detect places where\n>> a SP was intended (HT can expand to more than one display column),\n>> so we need to count both the display columns (col) and the string\n>> character columns (i) to determine if a HT looks identical to a SP\n>> or can cause confusion.\n>> \n>> Highlighting support for tools like git diff/show/log is added, as\n>> well as git apply --whitespace=fix capability.\n>> \n>> The middle section of the line used to be assumed non-highlighted,\n>> which is obviously not true anymore, so we split its logic into a\n>> separate function named emit_middle_section().\n>> \n>> The new check is enabled for Documentation/**/*.adoc, where these\n>> kinds of mistakes were seen in practice. It can also be enabled in\n>> other locations where it can be useful, by adding to the relevant\n>> attributes file.\n>> \n>> Suggested-by: Junio C Hamano <gitster@pobox.com>\n>> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>\n>> ---\n>> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n>> index 3c8eb02e4f..f5b6ceeed9 100755\n>> --- a/t/t4015-diff-whitespace.sh\n>> +++ b/t/t4015-diff-whitespace.sh\n>> @@ -2440,4 +2440,147 @@ test_expect_success 'combine --ignore-blank-lines with --function-context 2' '\n>>  \ttest_cmp expect actual\n>>  '\n>>  \n>> +test_expect_success 'check tab between non-whitespace (tab-between-non-ws: off)' '\n>> +\tgit config core.whitespace \"-tab-between-non-ws\" &&\n>\n> It might be worthwhile using test_config here, because this setting does\n> not need to persist for the remaining tests.\n\nAck, will do.\n\n>> +\n>> +\tprintf \"1234567\\tb\" >x &&\n>\n> What if you made the test cases into\n>\n> \t# only the TAB in the middle must be diagnosed\n> \tprintf \"\\t1234567\\t12\\t90\\n\" >x &&\n>\n> to test that only the second of the three TABs is diagnosed?\n\nYes we can do it this way.\n\n>> +\tgit add x &&\n>> +\tgit diff --cached --check &&\n>> +\n>> +\tgit diff --cached --color >raw &&\n>> +\ttest_decode_color <raw >actual &&\n>> +\t! test_grep \"<GREEN>1234567<RESET><BLUE>\t<RESET><GREEN>b<RESET>\" actual &&\n>\n> This must be\n>\n> \ttest_grep ! \"...\n>\n> Furthermore, a negative test with a very tight pattern is often not\n> desired: The test could fail if any single character does not occur\n> (which could easily happen if the test text is changed, but not this\n> pattern). In this case, it would be sufficient to test only that \"BLUE\"\n> does not occur.\n\nThanks, I'm still a bit of a noob wrt the git codebase. Will do.\n\n>> +\ttest_grep \"<GREEN>1234567\tb<RESET>\" actual &&\n>> +\n>> +\t# should apply without error because tab-between-non-ws is off\n>> +\tgit diff --cached >patch.diff &&\n>> +\tgit checkout HEAD -- x &&\n>> +\tgit apply --whitespace=error patch.diff\n>> +'\n>\n> There is t/t4124-apply-ws-rule.sh. Wouldn't the `git apply` tests be\n> better located there?\n\nI think so, yes, thanks for pointing it out.\n\n>\n> Please consider all comments on this test case repeated (and suitably\n> adusted) for all other test cases added by this patch.\n\nWill do.\n\n>> +\n>> +test_expect_success 'check tab between non-whitespace at tab stop (tab-between-non-ws: on)' '\n>> +\tgit config core.whitespace \"tab-between-non-ws,tabwidth=8\" &&\n>\n> I am curious why you set tabwidth=8 here even though 8 is the default.\n\nJust to make it explicit because I also set tabwidth=4 in the following\ntests, however I can drop it.\n\n>> +test_expect_success 'check tab between non-whitespace not at tab stop (tab-between-non-ws: on)' '\n>\n> With my suggested text above, this case does not need a separate test, I\n> think.\n\nYes, it likely is redundant. I'll double check the tabwidth=8 test as\nwell because that might also be redundant and can be dropped entirely.\n\n>\n>> diff --git a/ws.c b/ws.c\n>> index 6cc2466c0c..633bc69418 100644\n>> --- a/ws.c\n>> +++ b/ws.c\n>> @@ -26,6 +26,7 @@ static struct whitespace_rule {\n>>  \t{ \"blank-at-eol\", WS_BLANK_AT_EOL, 0 },\n>>  \t{ \"blank-at-eof\", WS_BLANK_AT_EOF, 0 },\n>>  \t{ \"tab-in-indent\", WS_TAB_IN_INDENT, 0, 1 },\n>> +\t{ \"tab-between-non-ws\", WS_TAB_BETWEEN_NON_WS, 0 },\n>\n> How about \"tab-is-1-space\"? The documentation can clarify that not any\n> TAB expanding to width 1 is diagnosed, but only those that are between\n> non-space characters.\n\nI like this suggestion as well.\n\nMany thanks for the review,\nAdrian\n"},{"id":"533274","messageId":"5860c8ec-7b34-4c47-926e-67a2c44a654e@kdbg.org","threadId":"64737","inReplyTo":"87y0m9guns.fsf@collabora.com","subject":"Re: [PATCH v2] ws: add new tab-between-non-ws check","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2026-01-08T08:36:36Z","receivedAt":"2026-01-08T08:36:50Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 07.01.26 um 19:11 schrieb Adrian Ratiu:\n> On Wed, 07 Jan 2026, Johannes Sixt <j6t@kdbg.org> wrote:\n>> Am 07.01.26 um 02:30 schrieb Adrian Ratiu:\n>>> +\tgit add x &&\n>>> +\tgit diff --cached --check &&\n>>> +\n>>> +\tgit diff --cached --color >raw &&\n>>> +\ttest_decode_color <raw >actual &&\n>>> +\t! test_grep \"<GREEN>1234567<RESET><BLUE>\t<RESET><GREEN>b<RESET>\" actual &&\n>>\n>> This must be\n>>\n>> \ttest_grep ! \"...\n>>\n>> Furthermore, a negative test with a very tight pattern is often not\n>> desired: The test could fail if any single character does not occur\n>> (which could easily happen if the test text is changed, but not this\n>> pattern). In this case, it would be sufficient to test only that \"BLUE\"\n>> does not occur.\n> \n> Thanks, I'm still a bit of a noob wrt the git codebase. Will do.\n> \n>>> +\ttest_grep \"<GREEN>1234567\tb<RESET>\" actual &&\n\nReconsidering this, we have a positive test for the desired result here.\nThen the negative test is redundant, I would think.\n\n-- Hannes\n\n"},{"id":"533275","messageId":"dcd87fc4-6514-4146-9e44-1276bd739d2f@kdbg.org","threadId":"64737","inReplyTo":"d3f26459-d828-4d01-8c38-ce754e5cc576@kdbg.org","subject":"Re: [PATCH v2] ws: add new tab-between-non-ws check","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2026-01-08T09:01:26Z","receivedAt":"2026-01-08T09:01:50Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 07.01.26 um 18:33 schrieb Johannes Sixt:\n> Am 07.01.26 um 02:30 schrieb Adrian Ratiu:\n>> The check is a bit complex because we want to detect places where\n>> a SP was intended (HT can expand to more than one display column),\n>> so we need to count both the display columns (col) and the string\n>> character columns (i) to determine if a HT looks identical to a SP\n>> or can cause confusion.\n>>\n>> Highlighting support for tools like git diff/show/log is added, as\n>> well as git apply --whitespace=fix capability.\n>>\n>> The middle section of the line used to be assumed non-highlighted,\n>> which is obviously not true anymore, so we split its logic into a\n>> separate function named emit_middle_section().\n>>\n>> The new check is enabled for Documentation/**/*.adoc, where these\n>> kinds of mistakes were seen in practice. It can also be enabled in\n>> other locations where it can be useful, by adding to the relevant\n>> attributes file.\n\nThis makes me wonder how useful this check is. Yes, I has happened that\nI didn't spot at TAB that should have been a SP, but perhaps a handful\nof times in my career. Compare this to the many times that the other\nkinds of whitespace errors happened.\n\nApplying the rule to all documentation files is questionable: I can't\nformat a table with TAB characters between columns reliably, because if\na column happens to be 7 characters wide, the TAB at the 8th position\nwould be diagnosed, but I certainly do *not* want it to be replaced by a\nSP. Yet, I might want legitimate cases outside tables to be diagnosed, so...\n\nMaybe I'm too much of a devil's advocate here...\n\n-- Hannes\n\n"},{"id":"533365","messageId":"87h5sux64k.fsf@collabora.com","threadId":"64737","inReplyTo":"dcd87fc4-6514-4146-9e44-1276bd739d2f@kdbg.org","subject":"Re: [PATCH v2] ws: add new tab-between-non-ws check","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-09T13:33:47Z","receivedAt":"2026-01-09T13:34:05Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Thu, 08 Jan 2026, Johannes Sixt <j6t@kdbg.org> wrote:\n> Am 07.01.26 um 18:33 schrieb Johannes Sixt:\n>> Am 07.01.26 um 02:30 schrieb Adrian Ratiu:\n>>> The check is a bit complex because we want to detect places where\n>>> a SP was intended (HT can expand to more than one display column),\n>>> so we need to count both the display columns (col) and the string\n>>> character columns (i) to determine if a HT looks identical to a SP\n>>> or can cause confusion.\n>>>\n>>> Highlighting support for tools like git diff/show/log is added, as\n>>> well as git apply --whitespace=fix capability.\n>>>\n>>> The middle section of the line used to be assumed non-highlighted,\n>>> which is obviously not true anymore, so we split its logic into a\n>>> separate function named emit_middle_section().\n>>>\n>>> The new check is enabled for Documentation/**/*.adoc, where these\n>>> kinds of mistakes were seen in practice. It can also be enabled in\n>>> other locations where it can be useful, by adding to the relevant\n>>> attributes file.\n>\n> This makes me wonder how useful this check is. Yes, I has happened that\n> I didn't spot at TAB that should have been a SP, but perhaps a handful\n> of times in my career. Compare this to the many times that the other\n> kinds of whitespace errors happened.\n>\n> Applying the rule to all documentation files is questionable: I can't\n> format a table with TAB characters between columns reliably, because if\n> a column happens to be 7 characters wide, the TAB at the 8th position\n> would be diagnosed, but I certainly do *not* want it to be replaced by a\n> SP. Yet, I might want legitimate cases outside tables to be diagnosed, so...\n>\n> Maybe I'm too much of a devil's advocate here...\n\nI'll let Junio decide on the usefulness of this check since he's the one\nwho asked for it. :)\n\nMaybe we could improve the heuristic to detect tables, for example\npatterns like a\\tb\\tc.\n\nI'm ok either way, just let me know if I should pursue this further.\n"}]}