git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC 1/1] Add new indent-with-tab whitespace check

From
Junio C Hamano <gitster@pobox.com>
Date
Mar 31, 2010, 16:23 UTC
Message-ID
<7vwrwstq8r.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1269698881-26443-2-git-send-email-chris@arachsys.com>
Chris Webb <chris@arachsys.com> writes:
Show 30 quoted lines
> The indent-with-tab rule warns about any tab characters used in initial
> indent, and highlights them in git diff --check.
>
> Signed-off-by: Chris Webb <chris@arachsys.com>
> ---
>  cache.h |    1 +
>  ws.c    |   26 ++++++++++++++++++++------
>  2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 2928107..d87bd85 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1040,6 +1040,7 @@ void shift_tree_by(const unsigned char *, const unsigned char *, unsigned char *
>  #define WS_INDENT_WITH_NON_TAB	04
>  #define WS_CR_AT_EOL           010
>  #define WS_BLANK_AT_EOF        020
> +#define WS_INDENT_WITH_TAB     040
>  #define WS_TRAILING_SPACE      (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF)
>  #define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB)
>  extern unsigned whitespace_rule_cfg;
> diff --git a/ws.c b/ws.c
> index c089338..e44a711 100644
> --- a/ws.c
> +++ b/ws.c
> @@ -18,6 +18,7 @@ static struct whitespace_rule {
>  	{ "cr-at-eol", WS_CR_AT_EOL, 1 },
>  	{ "blank-at-eol", WS_BLANK_AT_EOL, 0 },
>  	{ "blank-at-eof", WS_BLANK_AT_EOF, 0 },
> +	{ "indent-with-tab", WS_INDENT_WITH_TAB, 0 },

User's existing attribute setting that uses "indent" as an abbreviation for "indent-with-non-tab" will probably be broken by this naming.

This sounds more like "tab-in-indent". Naming the option that way would give it an added benefit of allowing it to be abbreviated to "tab" by the parser.

I think WS_TAB_IN_INDENT are incompatible with either WS_SPACE_BEFORE_TAB or WS_INDENT_WITH_TAB. Don't we want to notice such a combination as an error?

Show 11 quoted lines
> @@ -163,23 +169,31 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
>  		}
>  	}
>  
> -	/* Check for space before tab in initial indent. */
> +	/* Check for indent using tab or space before tab in initial indent. */
>  	for (i = 0; i < len; i++) {
>  		if (line[i] == ' ')
>  			continue;
>  		if (line[i] != '\t')
>  			break;
Show 10 quoted lines
> -		if ((ws_rule & WS_SPACE_BEFORE_TAB) && written < i) {
> +		if (ws_rule & WS_INDENT_WITH_TAB) {
> +			result |= WS_INDENT_WITH_TAB;
> +			if (stream) {
> +				fwrite(line + written, i - written, 1, stream);
> +				fputs(ws, stream);
> +				fwrite(line + i, 1, 1, stream);
> +				fputs(reset, stream);
> +			}
> +		} else if ((ws_rule & WS_SPACE_BEFORE_TAB) && written < i) {

Adding this new code as a new second branch for if-elif-else chain would match the order of options better.

Show 14 quoted lines
>  			result |= WS_SPACE_BEFORE_TAB;
>  			if (stream) {
>  				fputs(ws, stream);
>  				fwrite(line + written, i - written, 1, stream);
>  				fputs(reset, stream);
> +				fwrite(line + i, 1, 1, stream);
>  			}
> -		} else if (stream)
> -			fwrite(line + written, i - written, 1, stream);
> -		if (stream)
> -			fwrite(line + i, 1, 1, stream);
> +		} else if (stream) {
> +			fwrite(line + written, i - written + 1, 1, stream);
> +		}

I think it is a good change, and a necessary one for adding this new feature.

The original code assumed that this loop always detected an after seeing a "good" letter (hence the writing of the current letter was unconditional), but the patch makes it more clear what is being checked and what is being highlighted by the logic between SPACE_BEFORE_TAB and TAB_IN_INDENT codepaths.

I'll queue this with s/indent-with-tab/tab-in-indent/ tweak to 'pu' but the topic would need:

 - detecting incompatible settings;
 - tests;
 - docs;
 - matching change to apply --whitespace=fix;

before moving to 'next', I think. Here is what I tentatively will queue, with an updated log message.

Thanks.
-- >8 --
From: Chris Webb <chris@arachsys.com>
Date: Sat, 27 Mar 2010 14:08:01 +0000
Subject: [PATCH] Add tab-in-indent whitespace error class

Some projects and languages use coding style where no tab character is used to indent the lines.

This only adds support for "apply --whitespace=warn" and "diff --check"; follow-up patches need to add "apply --whitespace=fix", tests and docs.

Signed-off-by: Chris Webb <chris@arachsys.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h |    1 +
 ws.c    |   24 +++++++++++++++++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index 6dcb100..030af32 100644
--- a/cache.h
+++ b/cache.h
@@ -1040,6 +1040,7 @@ void shift_tree_by(const unsigned char *, const unsigned char *, unsigned char *
 #define WS_INDENT_WITH_NON_TAB	04
 #define WS_CR_AT_EOL           010
 #define WS_BLANK_AT_EOF        020
+#define WS_TAB_IN_INDENT       040
 #define WS_TRAILING_SPACE      (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF)
 #define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB)
 extern unsigned whitespace_rule_cfg;
diff --git a/ws.c b/ws.c
index c089338..d0b6c54 100644
--- a/ws.c
+++ b/ws.c
@@ -18,6 +18,7 @@ static struct whitespace_rule {
 	{ "cr-at-eol", WS_CR_AT_EOL, 1 },
 	{ "blank-at-eol", WS_BLANK_AT_EOL, 0 },
 	{ "blank-at-eof", WS_BLANK_AT_EOF, 0 },
+	{ "tab-in-indent", WS_TAB_IN_INDENT, 0 },
 };
 
 unsigned parse_whitespace_rule(const char *string)
@@ -125,6 +126,11 @@ char *whitespace_error_string(unsigned ws)
 			strbuf_addstr(&err, ", ");
 		strbuf_addstr(&err, "indent with spaces");
 	}
+	if (ws & WS_TAB_IN_INDENT) {
+		if (err.len)
+			strbuf_addstr(&err, ", ");
+		strbuf_addstr(&err, "tab in indent");
+	}
 	return strbuf_detach(&err, NULL);
 }
 
@@ -163,7 +169,7 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
 		}
 	}
 
-	/* Check for space before tab in initial indent. */
+	/* Check indentation */
 	for (i = 0; i < len; i++) {
 		if (line[i] == ' ')
 			continue;
@@ -175,11 +181,19 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
 				fputs(ws, stream);
 				fwrite(line + written, i - written, 1, stream);
 				fputs(reset, stream);
+				fwrite(line + i, 1, 1, stream);
 			}
-		} else if (stream)
-			fwrite(line + written, i - written, 1, stream);
-		if (stream)
-			fwrite(line + i, 1, 1, stream);
+		} else if (ws_rule & WS_TAB_IN_INDENT) {
+			result |= WS_TAB_IN_INDENT;
+			if (stream) {
+				fwrite(line + written, i - written, 1, stream);
+				fputs(ws, stream);
+				fwrite(line + i, 1, 1, stream);
+				fputs(reset, stream);
+			}
+		} else if (stream) {
+			fwrite(line + written, i - written + 1, 1, stream);
+		}
 		written = i + 1;
 	}
 

-- 
1.7.0.3.513.gc8ed0
Previous: Chris WebbNext: Junio C Hamano
Message 3 of 6 in “[RFC 0/1] diff --check for indent-with-spaces style”
  1. Chris WebbMar 27, 2010
  2. 1/1 Add new indent-with-tab whitespace checkChris Webb, Mar 27, 2010
  3. Junio C HamanoMar 31, 2010
  4. Re* [RFC 1/1] Add new indent-with-tab whitespace checkJunio C Hamano, Apr 1, 2010
  5. Junio C HamanoApr 1, 2010
  6. Chris WebbApr 2, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.