[PATCH 1/2] Implement parsing for new core.whitespace.* options.

[PATCH 1/2] Implement parsing for new core.whitespace.* options.

[David Symonds]

Each of the new core.whitespace.* options (enumerated below) can be set to one
of:
	* okay (default): Whitespace of this type is okay
	* warn: Whitespace of this type should be warned about
	* error: Whitespace of this type should raise an error
	* autofix: Whitespace of this type should be automatically fixed

The initial options are:
	* trailing: Whitespace at the end of a line
	* space-before-tab: SP HT sequence in the initial whitespace of a line
	* space-indent: At least 8 spaces in a row at the start of a line

Example usage:
	[core "whitespace"]
		trailing = autofix
		space-before-tab = error
		space-indent = warn

Signed-off-by: David Symonds <dsymonds@gmail.com>
---
 cache.h       |   16 ++++++++++++++++
 config.c      |   28 ++++++++++++++++++++++++++++
 environment.c |    3 +++
 3 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/cache.h b/cache.h
index bfffa05..51e3982 100644
--- a/cache.h
+++ b/cache.h
@@ -602,4 +602,20 @@ extern int diff_auto_refresh_index;
 /* match-trees.c */
 void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, int);
 
+/*
+ * whitespace rules.
+ * used by both diff and apply
+ */
+enum whitespace_mode {
+	WS_OKAY = 0,
+	WS_WARN,
+	WS_ERROR,
+	WS_AUTOFIX
+};
+extern enum whitespace_mode ws_mode_trailing;
+extern enum whitespace_mode ws_mode_space_before_tab;
+extern enum whitespace_mode ws_mode_space_indent;
+extern enum whitespace_mode git_config_whitespace_mode(const char *, const char *);
+
+
 #endif /* CACHE_H */
diff --git a/config.c b/config.c
index dc3148d..8e6f252 100644
--- a/config.c
+++ b/config.c
@@ -297,6 +297,19 @@ int git_config_bool(const char *name, const char *value)
 	return git_config_int(name, value) != 0;
 }
 
+enum whitespace_mode git_config_whitespace_mode(const char *name, const char *value)
+{
+	if (!strcasecmp(value, "okay"))
+		return WS_OKAY;
+	if (!strcasecmp(value, "warn"))
+		return WS_WARN;
+	if (!strcasecmp(value, "error"))
+		return WS_ERROR;
+	if (!strcasecmp(value, "autofix"))
+		return WS_AUTOFIX;
+	die("bad config value for '%s' in %s", name, config_file_name);
+}
+
 int git_default_config(const char *var, const char *value)
 {
 	/* This needs a better name */
@@ -431,6 +444,21 @@ int git_default_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.whitespace.trailing")) {
+		ws_mode_trailing = git_config_whitespace_mode(var, value);
+		return 0;
+	}
+
+	if (!strcmp(var, "core.whitespace.space-before-tab")) {
+		ws_mode_space_before_tab = git_config_whitespace_mode(var, value);
+		return 0;
+	}
+
+	if (!strcmp(var, "core.whitespace.space-indent")) {
+		ws_mode_space_indent = git_config_whitespace_mode(var, value);
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/environment.c b/environment.c
index b5a6c69..71502fc 100644
--- a/environment.c
+++ b/environment.c
@@ -35,6 +35,9 @@ int pager_in_use;
 int pager_use_color = 1;
 char *editor_program;
 int auto_crlf = 0;	/* 1: both ways, -1: only when adding git objects */
+enum whitespace_mode ws_mode_trailing = WS_OKAY;
+enum whitespace_mode ws_mode_space_before_tab = WS_OKAY;
+enum whitespace_mode ws_mode_space_indent = WS_OKAY;
 
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
-- 
1.5.3.1

[PATCH 2/2] git-diff: Respect core.whitespace.{space-indent,space-before-tab,trailing}.

[David Symonds]

Signed-off-by: David Symonds <dsymonds@gmail.com>
---
 diff.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index a6aaaf7..aa86fa1 100644
--- a/diff.c
+++ b/diff.c
@@ -503,12 +503,15 @@ static void emit_line_with_ws(int nparents,
 	int tail = len;
 	int need_highlight_leading_space = 0;
 	/* The line is a newly added line.  Does it have funny leading
-	 * whitespaces?  In indent, SP should never precede a TAB.
+	 * whitespaces?  In indent, SP should never precede a TAB. In
+	 * addition, under "indent with non tab" rule, there should not
+	 * be 8 or more consecutive spaces.
 	 */
 	for (i = col0; i < len; i++) {
 		if (line[i] == '\t') {
 			last_tab_in_indent = i;
-			if (0 <= last_space_in_indent)
+			if ((ws_mode_space_before_tab != WS_OKAY) &&
+			    (0 <= last_space_in_indent))
 				need_highlight_leading_space = 1;
 		}
 		else if (line[i] == ' ')
@@ -516,6 +519,13 @@ static void emit_line_with_ws(int nparents,
 		else
 			break;
 	}
+	if ((ws_mode_space_indent != WS_OKAY) &&
+	    (0 <= last_space_in_indent) &&
+	    (last_tab_in_indent < 0) &&
+	    (8 <= (i - col0))) {
+		last_tab_in_indent = i;
+		need_highlight_leading_space = 1;
+	}
 	fputs(set, stdout);
 	fwrite(line, col0, 1, stdout);
 	fputs(reset, stdout);
@@ -540,10 +550,12 @@ static void emit_line_with_ws(int nparents,
 	tail = len - 1;
 	if (line[tail] == '\n' && i < tail)
 		tail--;
-	while (i < tail) {
-		if (!isspace(line[tail]))
-			break;
-		tail--;
+	if (ws_mode_trailing != WS_OKAY) {
+		while (i < tail) {
+			if (!isspace(line[tail]))
+				break;
+			tail--;
+		}
 	}
 	if ((i < tail && line[tail + 1] != '\n')) {
 		/* This has whitespace between tail+1..len */
-- 
1.5.3.1

Re: [PATCH 1/2] Implement parsing for new core.whitespace.* options.

[Junio C Hamano]

David Symonds <dsymonds@gmail.com> writes:

> Each of the new core.whitespace.* options (enumerated below) can be set to one
> of:
> 	* okay (default): Whitespace of this type is okay
> 	* warn: Whitespace of this type should be warned about
> 	* error: Whitespace of this type should raise an error
> 	* autofix: Whitespace of this type should be automatically fixed

Many problems at the conceptual level (I haven't look at the
patch yet).

We call these options (nowarn,warn,error,strip) in
apply.whitespace.  "strip" is a bit of misnomer, as we only
handled the trailing whitespace initially.  We should add "fix"
as a synonym to "strip".

The intention is to define what is an anomaly with
core.whitespace and then define what to do with it with
apply.whitespace.

And that is a good distinction.  You may usually use "fix", but
occasionally you would want to override it one-shot (when you do
want to have byte-to-byte identical application of the patch),
and the command line option "--whitespace=" lets you do so.
At least you need to extend --whitespace command line option
handling to allow these overridden.

Adding the "error" and "fix" to "diff" is a mistake --- there is
no error condition nor fixing there.  That shows how the
approach of your patch is inappropriate by trying to mix what
core.whitespace (give the definition of what is an error) and
apply.whitespace (specify what to do with an error) are designed
to do.

Defaulting to "nowarn" is wrong.  Trailing whitespace errors and
space before tab errors should be turned on by default as
before.

Re: [PATCH 1/2] Implement parsing for new core.whitespace.* options.

[David Symonds]

On 11/3/07, Junio C Hamano <gitster@pobox.com> wrote:
> David Symonds <dsymonds@gmail.com> writes:
>
> > Each of the new core.whitespace.* options (enumerated below) can be set to one
> > of:
> >       * okay (default): Whitespace of this type is okay
> >       * warn: Whitespace of this type should be warned about
> >       * error: Whitespace of this type should raise an error
> >       * autofix: Whitespace of this type should be automatically fixed
>
> Many problems at the conceptual level (I haven't look at the
> patch yet).

Sure, I thought there might be. I'm still finding my way around the git code.

> We call these options (nowarn,warn,error,strip) in
> apply.whitespace.  "strip" is a bit of misnomer, as we only
> handled the trailing whitespace initially.  We should add "fix"
> as a synonym to "strip".

I can whip that up in a separate patch if you'd like. However, it
still doesn't allow for different handling of the different whitespace
errors, does it?

> The intention is to define what is an anomaly with
> core.whitespace and then define what to do with it with
> apply.whitespace.

That seems a little counterintuitive to be splitting like that. For
overriding, a simple environment variable like GIT_EXTRA_CONFIG (or
whatever) could pass in arbitrary one-shot configuration parameters,
which seems like a better (more general) solution.

> Adding the "error" and "fix" to "diff" is a mistake --- there is
> no error condition nor fixing there.  That shows how the
> approach of your patch is inappropriate by trying to mix what
> core.whitespace (give the definition of what is an error) and
> apply.whitespace (specify what to do with an error) are designed
> to do.

Yes, I agree that there's no place for "error" or "fix" in git-diff;
that was the reason for me resending the series, because I adjusted
the diff warnings to happen whenever the relevant setting was anything
but "okay", with the idea being that any incorrect whitespace should
be flagged in git-diff, and there's a natural split between "okay" and
"warn"/"error"/"fix".

> Defaulting to "nowarn" is wrong.  Trailing whitespace errors and
> space before tab errors should be turned on by default as
> before.

Yes, you're correct. That's easy to fix.


Dave.