[PATCH 0/4] Refactoring for apply and diff

[PATCH 0/4] Refactoring for apply and diff

[Wincent Colaiuta]

4 more patches which apply on top of the "git diff --check" topic I
posted previously. If you'd prefer any of these to be prepared for
application on top of "master" or "next" let me know.

[1/4] Fix "diff --check" whitespace detection

Fix bug noticed while comparing the whitespace checks in "git apply"
and "git diff". It's exactly this kind of bug which motivates the
refactoring which follows...

[2/4] Extract and improve whitespace check from "git apply"

Extract (and improve) some stuff out of "apply"...

[3/4] Make "diff --check" use shared whitespace functions

... so that the same code can be used by "diff".

[4/4] Add tests for "git diff --check" with core.whitespace options

Cheers,
Wincent

[PATCH 1/4] Fix "diff --check" whitespace detection

[Wincent Colaiuta]

"diff --check" would only detect spaces before tabs if a tab was the
last character in the leading indent. Fix that and add a test case to
make sure the bug doesn't regress in the future.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---

Although this patch is made to apply on top of the topic I posted
earlier, it can be applied to master with a couple of tweaks. Let
me know if you want me to send such a patch.

In any case, [2/4] and [3/4] factor away the code that had the bug
in it anyway...

 diff.c                     |   13 ++++++++++---
 t/t4015-diff-whitespace.sh |    8 ++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index e89c7ce..c9b3884 100644
--- a/diff.c
+++ b/diff.c
@@ -1010,11 +1010,18 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
 		int i, spaces = 0, space_before_tab = 0, white_space_at_end = 0;
 
 		/* check space before tab */
-		for (i = 1; i < len && (line[i] == ' ' || line[i] == '\t'); i++)
+		for (i = 1; i < len; i++) {
 			if (line[i] == ' ')
 				spaces++;
-		if (line[i - 1] == '\t' && spaces)
-			space_before_tab = 1;
+			else if (line[i] == '\t') {
+				if (spaces) {
+					space_before_tab = 1;
+					break;
+				}
+			}
+			else
+				break;
+		}
 
 		/* check whitespace at line end */
 		if (line[len - 1] == '\n')
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 5aaf2db..ff77a16 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -248,4 +248,12 @@ test_expect_failure 'check with space before tab in indent (diff-tree)' '
 
 '
 
+# was a bug: space before tab only caught if tab was last in the indent
+test_expect_failure 'check mixed spaces and tabs in indent' '
+
+	echo " 	 foo();" > x &&
+	git diff --check
+
+'
+
 test_done
-- 
1.5.3.7.1159.g2f071-dirty

[PATCH 2/4] Extract and improve whitespace check from "git apply"

[Wincent Colaiuta]

This refactoring extracts the whitespace checking formerly done in
builtin-apply.c into a function in ws.c so that the exact same
whitespace logic can be used by "git diff" as well, thus reducing
code duplication and the likelihood of errors.

At the same time we teach "git apply" to report all classes of
whitespace errors found in a given line rather than reporting only
the first found error.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 builtin-apply.c |   60 +++++++++++-------------------------------------------
 cache.h         |    2 +
 ws.c            |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 47 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index f2e9a33..cf349db 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -898,58 +898,24 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
 	return -1;
 }
 
-static void check_whitespace(const char *line, int len, unsigned ws_rule)
+static void check_patch_whitespace(const char *line, int len,
+                                   unsigned ws_rule)
 {
-	const char *err = "Adds trailing whitespace";
-	int seen_space = 0;
-	int i;
-
-	/*
-	 * We know len is at least two, since we have a '+' and we
-	 * checked that the last character was a '\n' before calling
-	 * this function.  That is, an addition of an empty line would
-	 * check the '+' here.  Sneaky...
-	 */
-	if ((ws_rule & WS_TRAILING_SPACE) && isspace(line[len-2]))
-		goto error;
-
-	/*
-	 * Make sure that there is no space followed by a tab in
-	 * indentation.
-	 */
-	if (ws_rule & WS_SPACE_BEFORE_TAB) {
-		err = "Space in indent is followed by a tab";
-		for (i = 1; i < len; i++) {
-			if (line[i] == '\t') {
-				if (seen_space)
-					goto error;
-			}
-			else if (line[i] == ' ')
-				seen_space = 1;
-			else
-				break;
-		}
-	}
-
-	/*
-	 * Make sure that the indentation does not contain more than
-	 * 8 spaces.
-	 */
-	if ((ws_rule & WS_INDENT_WITH_NON_TAB) &&
-	    (8 < len) && !strncmp("+        ", line, 9)) {
-		err = "Indent more than 8 places with spaces";
-		goto error;
-	}
-	return;
+	char *err;
+	unsigned result = check_whitespace(line, len, ws_rule);
+	if (!result)
+		return;
 
- error:
 	whitespace_error++;
 	if (squelch_whitespace_errors &&
 	    squelch_whitespace_errors < whitespace_error)
 		;
-	else
+	else {
+		err = whitespace_error_string(result);
 		fprintf(stderr, "%s.\n%s:%d:%.*s\n",
-			err, patch_input_file, linenr, len-2, line+1);
+		     err, patch_input_file, linenr, len - 2, line + 1);
+		free(err);
+	}
 }
 
 /*
@@ -1001,7 +967,7 @@ static int parse_fragment(char *line, unsigned long size,
 		case '-':
 			if (apply_in_reverse &&
 			    ws_error_action != nowarn_ws_error)
-				check_whitespace(line, len, patch->ws_rule);
+				check_patch_whitespace(line, len, patch->ws_rule);
 			deleted++;
 			oldlines--;
 			trailing = 0;
@@ -1009,7 +975,7 @@ static int parse_fragment(char *line, unsigned long size,
 		case '+':
 			if (!apply_in_reverse &&
 			    ws_error_action != nowarn_ws_error)
-				check_whitespace(line, len, patch->ws_rule);
+				check_patch_whitespace(line, len, patch->ws_rule);
 			added++;
 			newlines--;
 			trailing = 0;
diff --git a/cache.h b/cache.h
index 1bcb3df..6c17f27 100644
--- a/cache.h
+++ b/cache.h
@@ -655,6 +655,8 @@ void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, i
 extern unsigned whitespace_rule_cfg;
 extern unsigned whitespace_rule(const char *);
 extern unsigned parse_whitespace_rule(const char *);
+extern unsigned check_whitespace(const char *line, int len, unsigned ws_rule);
+extern char *whitespace_error_string(unsigned ws);
 
 /* ls-files */
 int pathspec_match(const char **spec, char *matched, const char *filename, int skiplen);
diff --git a/ws.c b/ws.c
index 52c10ca..884d373 100644
--- a/ws.c
+++ b/ws.c
@@ -94,3 +94,61 @@ unsigned whitespace_rule(const char *pathname)
 		return whitespace_rule_cfg;
 	}
 }
+
+/* The returned string should be freed by the caller. */
+char *whitespace_error_string(unsigned ws)
+{
+	struct strbuf err;
+	strbuf_init(&err, 0);
+	if (ws & WS_TRAILING_SPACE)
+		strbuf_addstr(&err, "Adds trailing whitespace");
+	if (ws & WS_SPACE_BEFORE_TAB) {
+		if (err.len)
+			strbuf_addstr(&err, ", ");
+		strbuf_addstr(&err, "Space in indent is followed by a tab");
+	}
+	if (ws & WS_INDENT_WITH_NON_TAB) {
+		if (err.len)
+			strbuf_addstr(&err, ", ");
+		strbuf_addstr(&err, "Indent more than 8 places with spaces");
+	}
+	return strbuf_detach(&err, NULL);
+}
+
+unsigned check_whitespace(const char *line, int len, unsigned ws_rule)
+{
+	unsigned result = 0;
+	int seen_space = 0;
+	int i;
+
+	if (ws_rule & WS_TRAILING_SPACE) {
+		/* Lines start with "+" or "-" so length is at least 1 */
+		if (line[len - 1] == '\n') {
+			if (isspace(line[len - 2]))
+				result |= WS_TRAILING_SPACE;
+		}
+		else if (isspace(line[len - 1]))
+			result |= WS_TRAILING_SPACE;
+	}
+
+	if (ws_rule & WS_SPACE_BEFORE_TAB) {
+		for (i = 1; i < len; i++) {
+			if (line[i] == '\t') {
+				if (seen_space) {
+					result |= WS_SPACE_BEFORE_TAB;
+					break;
+				}
+			}
+			else if (line[i] == ' ')
+				seen_space = 1;
+			else
+				break;
+		}
+	}
+
+	if ((ws_rule & WS_INDENT_WITH_NON_TAB) &&
+	    (8 < len) && !strncmp("+        ", line, 9)) {
+		result |= WS_INDENT_WITH_NON_TAB;
+	}
+	return result;
+}
-- 
1.5.3.7.1159.g2f071-dirty

[PATCH 3/3] Make "diff --check" use shared whitespace functions

[Wincent Colaiuta]

Use the logic refactored into ws.c rather than duplicating it.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 diff.c |   49 +++++++++++--------------------------------------
 1 files changed, 11 insertions(+), 38 deletions(-)

diff --git a/diff.c b/diff.c
index c9b3884..7ad3c63 100644
--- a/diff.c
+++ b/diff.c
@@ -996,7 +996,7 @@ struct checkdiff_t {
 	const char *filename;
 	int lineno, color_diff;
 	unsigned ws_rule;
-	int status;
+	unsigned status;
 };
 
 static void checkdiff_consume(void *priv, char *line, unsigned long len)
@@ -1005,45 +1005,18 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
 	const char *ws = diff_get_color(data->color_diff, DIFF_WHITESPACE);
 	const char *reset = diff_get_color(data->color_diff, DIFF_RESET);
 	const char *set = diff_get_color(data->color_diff, DIFF_FILE_NEW);
+	char *err;
 
 	if (line[0] == '+') {
-		int i, spaces = 0, space_before_tab = 0, white_space_at_end = 0;
-
-		/* check space before tab */
-		for (i = 1; i < len; i++) {
-			if (line[i] == ' ')
-				spaces++;
-			else if (line[i] == '\t') {
-				if (spaces) {
-					space_before_tab = 1;
-					break;
-				}
-			}
-			else
-				break;
-		}
-
-		/* check whitespace at line end */
-		if (line[len - 1] == '\n')
-			len--;
-		if (isspace(line[len - 1]))
-			white_space_at_end = 1;
-
-		if (space_before_tab || white_space_at_end) {
-			data->status = 1;
-			printf("%s:%d: %s", data->filename, data->lineno, ws);
-			if (space_before_tab) {
-				printf("space before tab");
-				if (white_space_at_end)
-					putchar(',');
-			}
-			if (white_space_at_end)
-				printf("whitespace at end");
-			printf(":%s ", reset);
-			emit_line_with_ws(1, set, reset, ws, line, len,
-					  data->ws_rule);
-		}
-
+		data->status = check_whitespace(line, len, data->ws_rule);
+		if (!data->status)
+			return;
+		err = whitespace_error_string(data->status);
+		printf("%s:%d: %s%s:%s ", data->filename, data->lineno,
+		    ws, err, reset);
+		free(err);
+		emit_line_with_ws(1, set, reset, ws, line, len,
+		    data->ws_rule);
 		data->lineno++;
 	} else if (line[0] == ' ')
 		data->lineno++;
-- 
1.5.3.7.1159.g2f071-dirty

[PATCH 4/4] Add tests for "git diff --check" with core.whitespace options

[Wincent Colaiuta]

Make sure that "git diff --check" does the right thing when the
core.whitespace options are set.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 t/t4015-diff-whitespace.sh |   48 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index ff77a16..8d980af 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -256,4 +256,52 @@ test_expect_failure 'check mixed spaces and tabs in indent' '
 
 '
 
+test_expect_success 'check trailing whitespace (trailing-space: off)' '
+
+	git config core.whitespace "-trailing-space" &&
+	echo "foo ();   " > x &&
+	git diff --check
+
+'
+
+test_expect_failure 'check trailing whitespace (trailing-space: on)' '
+
+	git config core.whitespace "trailing-space" &&
+	echo "foo ();   " > x &&
+	git diff --check
+
+'
+
+test_expect_success 'check space before tab in indent (space-before-tab: off)' '
+
+	git config core.whitespace "-space-before-tab" &&
+	echo " 	foo ();" > x &&
+	git diff --check
+
+'
+
+test_expect_failure 'check space before tab in indent (space-before-tab: on)' '
+
+	git config core.whitespace "space-before-tab" &&
+	echo " 	foo ();   " > x &&
+	git diff --check
+
+'
+
+test_expect_success 'check spaces as indentation (indent-with-non-tab: off)' '
+
+	git config core.whitespace "-indent-with-non-tab"
+	echo "                foo ();" > x &&
+	git diff --check
+
+'
+
+test_expect_failure 'check spaces as indentation (indent-with-non-tab: on)' '
+
+	git config core.whitespace "indent-with-non-tab" &&
+	echo "                foo ();" > x &&
+	git diff --check
+
+'
+
 test_done
-- 
1.5.3.7.1159.g2f071-dirty

Re: [PATCH 2/4] Extract and improve whitespace check from "git apply"

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> diff --git a/ws.c b/ws.c
> index 52c10ca..884d373 100644
> --- a/ws.c
> +++ b/ws.c
> +unsigned check_whitespace(const char *line, int len, unsigned ws_rule)
> +{
> + ...
> +	if (ws_rule & WS_TRAILING_SPACE) {
> +		/* Lines start with "+" or "-" so length is at least 1 */
> +		if (line[len - 1] == '\n') {
> +			if (isspace(line[len - 2]))
> +				result |= WS_TRAILING_SPACE;
> +		}

I like the direction, but I think it would make much more sense if you
make check_whitespace() not about "a line in a patch that adds a line",
but about "here is a line, check if that is acceptable".  IOW, make line
variable zero-based (and len = strlen(line)).  The change would mean
that existing callers need to be modified to do something like:

	if (line[0] == '+')
        	check_whitespace(line+1, len-1, ...);

but at the same time we could conceivably teach "git show" to show
whitespace errors in a blob, i.e. "git show --show-ws-error HEAD:ws.c"
by using such a check_whitespace().

The highlighting code may need similar changes.  I was actually hoping
you would consolidate the logic there that decides which segment of the
string to highlight, and the logic in check_whitespace() to decide if
there is an error to begin with.  Conceptually, if emit_line_with_ws()
decides there is nothing to highlight with DIFF_WHITESPACE color, that
means there is no whitespace error on the line and vice-versa, no?

Re: [PATCH 2/4] Extract and improve whitespace check from "git apply"

[Wincent Colaiuta]

El 12/12/2007, a las 20:39, Junio C Hamano escribió:

> I like the direction, but I think it would make much more sense if you
> make check_whitespace() not about "a line in a patch that adds a  
> line",
> but about "here is a line, check if that is acceptable".  IOW, make  
> line
> variable zero-based (and len = strlen(line)).  The change would mean
> that existing callers need to be modified to do something like:
>
> 	if (line[0] == '+')
>        	check_whitespace(line+1, len-1, ...);

Yes, good point. As you say, it can then potentially be used for any  
line, not just lines in patches.

> but at the same time we could conceivably teach "git show" to show
> whitespace errors in a blob, i.e. "git show --show-ws-error HEAD:ws.c"
> by using such a check_whitespace().
>
> The highlighting code may need similar changes.  I was actually hoping
> you would consolidate the logic there that decides which segment of  
> the
> string to highlight, and the logic in check_whitespace() to decide if
> there is an error to begin with.  Conceptually, if emit_line_with_ws()
> decides there is nothing to highlight with DIFF_WHITESPACE color, that
> means there is no whitespace error on the line and vice-versa, no?

You're dead right. As I said in my original message, there is a *lot*  
of diff code and I basically only looked at enough of it to figure out  
what changes would be necessary to implement what I wanted. I hadn't  
looked inside emit_line_with_ws but it is an obvious site of  
duplication and as you point out there is definitely scope for more  
refactoring here; instead of doing the whitespace checking in three  
places (the three that I know about so far) we can do it in just one  
place. I'll see what I can cook up.

Cheers,
Wincent