[PATCH 0/5] "diff --check" and whitespace enhancements

[PATCH 0/5] "diff --check" and whitespace enhancements

[Wincent Colaiuta]

This is a repost of the topic I posted yesterday, rebased on top
of master and omitting the changes already applied to it.

Of the series the most interesting is [3/5], where I implement
Junio's suggestion of further refactoring to get rid of
emit_line_with_ws().

[1/5] "diff --check" should affect exit status
[2/5] New version of pre-commit hook
[3/5] Unify whitespace checking
[4/5] Make "diff --check" output match "git apply"
[5/5] Add tests for "git diff --check" with core.whitespace options

I think there are still some opportunities for further refactoring
and clean-up, but this is a good start.

Overall the refactoring saves duplication and reduces the line
count (apart from the tests, of course, where line count goes
unequivocally up).

 Documentation/diff-options.txt |    4 +-
 builtin-apply.c                |   56 +++----------
 builtin-diff-files.c           |    2 +
 builtin-diff-index.c           |    2 +
 builtin-diff-tree.c            |   28 +++---
 builtin-diff.c                 |    3 +-
 cache.h                        |    4 +
 diff.c                         |  172 +++++++++---------------------------
 diff.h                         |    1 +
 t/t4015-diff-whitespace.sh     |  189 +++++++++++++++++++++++++++++++++++++++-
 templates/hooks--pre-commit    |   67 ++-------------
 ws.c                           |  105 ++++++++++++++++++++++
 12 files changed, 379 insertions(+), 254 deletions(-)

Cheers,
Wincent

[PATCH 1/5] "diff --check" should affect exit status

[Wincent Colaiuta]

"git diff" has a --check option that can be used to check for whitespace
problems but it only reported by printing warnings to the
console.

Now when the --check option is used we give a non-zero exit status,
making "git diff --check" nicer to use in scripts and hooks.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 Documentation/diff-options.txt |    4 +-
 builtin-diff-files.c           |    2 +
 builtin-diff-index.c           |    2 +
 builtin-diff-tree.c            |   28 ++++----
 builtin-diff.c                 |    3 +-
 diff.c                         |   37 +++++++----
 diff.h                         |    1 +
 t/t4015-diff-whitespace.sh     |  137 +++++++++++++++++++++++++++++++++++++++-
 8 files changed, 184 insertions(+), 30 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 5d22b7b..9ecc1d7 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -93,7 +93,9 @@ endif::git-format-patch[]
 
 --check::
 	Warn if changes introduce trailing whitespace
-	or an indent that uses a space before a tab.
+	or an indent that uses a space before a tab. Exits with
+	non-zero status if problems are found. Not compatible with
+	--exit-code.
 
 --full-index::
 	Instead of the first handful characters, show full
diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index 046b7e3..4afc872 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -33,5 +33,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	result = run_diff_files_cmd(&rev, argc, argv);
 	if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))
 		return DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0;
+	if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
+		return DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0;
 	return result;
 }
diff --git a/builtin-diff-index.c b/builtin-diff-index.c
index 556c506..532b284 100644
--- a/builtin-diff-index.c
+++ b/builtin-diff-index.c
@@ -46,5 +46,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	result = run_diff_index(&rev, cached);
 	if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))
 		return DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0;
+	if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
+		return DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0;
 	return result;
 }
diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index 2e13716..9ab90cb 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c
@@ -117,23 +117,23 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
-	if (!read_stdin)
-		return DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS)
-			&& DIFF_OPT_TST(&opt->diffopt, HAS_CHANGES);
+	if (read_stdin) {
+		if (opt->diffopt.detect_rename)
+			opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |
+					       DIFF_SETUP_USE_CACHE);
+		while (fgets(line, sizeof(line), stdin)) {
+			unsigned char sha1[20];
 
-	if (opt->diffopt.detect_rename)
-		opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |
-				       DIFF_SETUP_USE_CACHE);
-	while (fgets(line, sizeof(line), stdin)) {
-		unsigned char sha1[20];
-
-		if (get_sha1_hex(line, sha1)) {
-			fputs(line, stdout);
-			fflush(stdout);
+			if (get_sha1_hex(line, sha1)) {
+				fputs(line, stdout);
+				fflush(stdout);
+			}
+			else
+				diff_tree_stdin(line);
 		}
-		else
-			diff_tree_stdin(line);
 	}
+	if (opt->diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
+		return DIFF_OPT_TST(&opt->diffopt, CHECK_FAILED) != 0;
 	return DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS)
 		&& DIFF_OPT_TST(&opt->diffopt, HAS_CHANGES);
 }
diff --git a/builtin-diff.c b/builtin-diff.c
index 55fb84c..86d01a3 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -353,7 +353,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 					     ent, ents);
 	if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))
 		result = DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0;
-
+	if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
+		return DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0;
 	if (1 < rev.diffopt.skip_stat_unmatch)
 		refresh_index_quietly();
 	return result;
diff --git a/diff.c b/diff.c
index 9c79ee2..39109a6 100644
--- a/diff.c
+++ b/diff.c
@@ -1031,6 +1031,7 @@ struct checkdiff_t {
 	const char *filename;
 	int lineno, color_diff;
 	unsigned ws_rule;
+	unsigned status;
 };
 
 static void checkdiff_consume(void *priv, char *line, unsigned long len)
@@ -1064,6 +1065,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
 			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");
@@ -1454,7 +1456,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 	diff_free_filespec_data(two);
 }
 
-static void builtin_checkdiff(const char *name_a, const char *name_b,
+static int builtin_checkdiff(const char *name_a, const char *name_b,
 			     struct diff_filespec *one,
 			     struct diff_filespec *two, struct diff_options *o)
 {
@@ -1462,7 +1464,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
 	struct checkdiff_t data;
 
 	if (!two)
-		return;
+		return 0;
 
 	memset(&data, 0, sizeof(data));
 	data.xm.consume = checkdiff_consume;
@@ -1491,6 +1493,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
  free_and_return:
 	diff_free_filespec_data(one);
 	diff_free_filespec_data(two);
+	return data.status;
 }
 
 struct diff_filespec *alloc_filespec(const char *path)
@@ -2075,14 +2078,14 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,
 	builtin_diffstat(name, other, p->one, p->two, diffstat, o, complete_rewrite);
 }
 
-static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)
+static int run_checkdiff(struct diff_filepair *p, struct diff_options *o)
 {
 	const char *name;
 	const char *other;
 
 	if (DIFF_PAIR_UNMERGED(p)) {
 		/* unmerged */
-		return;
+		return 0;
 	}
 
 	name = p->one->path;
@@ -2091,7 +2094,7 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)
 	diff_fill_sha1_info(p->one);
 	diff_fill_sha1_info(p->two);
 
-	builtin_checkdiff(name, other, p->one, p->two, o);
+	return builtin_checkdiff(name, other, p->one, p->two, o);
 }
 
 void diff_setup(struct diff_options *options)
@@ -2121,7 +2124,12 @@ int diff_setup_done(struct diff_options *options)
 	if (options->output_format & DIFF_FORMAT_NAME_STATUS)
 		count++;
 	if (options->output_format & DIFF_FORMAT_CHECKDIFF)
+	{
 		count++;
+		if (DIFF_OPT_TST(options, QUIET) ||
+		    DIFF_OPT_TST(options, EXIT_WITH_STATUS))
+			die("--check may not be used with --quiet or --exit-code");
+	}
 	if (options->output_format & DIFF_FORMAT_NO_OUTPUT)
 		count++;
 	if (count > 1)
@@ -2588,17 +2596,17 @@ static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,
 	run_diffstat(p, o, diffstat);
 }
 
-static void diff_flush_checkdiff(struct diff_filepair *p,
+static int diff_flush_checkdiff(struct diff_filepair *p,
 		struct diff_options *o)
 {
 	if (diff_unmodified_pair(p))
-		return;
+		return 0;
 
 	if ((DIFF_FILE_VALID(p->one) && S_ISDIR(p->one->mode)) ||
 	    (DIFF_FILE_VALID(p->two) && S_ISDIR(p->two->mode)))
-		return; /* no tree diffs in patch format */
+		return 0; /* no tree diffs in patch format */
 
-	run_checkdiff(p, o);
+	return run_checkdiff(p, o);
 }
 
 int diff_queue_is_empty(void)
@@ -2717,16 +2725,19 @@ static int check_pair_status(struct diff_filepair *p)
 	}
 }
 
-static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
+static int flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
 {
 	int fmt = opt->output_format;
 
 	if (fmt & DIFF_FORMAT_CHECKDIFF)
-		diff_flush_checkdiff(p, opt);
+		return diff_flush_checkdiff(p, opt);
 	else if (fmt & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))
 		diff_flush_raw(p, opt);
 	else if (fmt & DIFF_FORMAT_NAME)
 		write_name_quoted(p->two->path, stdout, opt->line_termination);
+
+	/* return value only matters with DIFF_FORMAT_CHECKDIFF */
+	return 0;
 }
 
 static void show_file_mode_name(const char *newdelete, struct diff_filespec *fs)
@@ -2965,8 +2976,8 @@ void diff_flush(struct diff_options *options)
 			     DIFF_FORMAT_CHECKDIFF)) {
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
-			if (check_pair_status(p))
-				flush_one_pair(p, options);
+			if (check_pair_status(p) && flush_one_pair(p, options))
+				DIFF_OPT_SET(options, CHECK_FAILED);
 		}
 		separator++;
 	}
diff --git a/diff.h b/diff.h
index a52496a..5d50d93 100644
--- a/diff.h
+++ b/diff.h
@@ -59,6 +59,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 #define DIFF_OPT_ALLOW_EXTERNAL      (1 << 13)
 #define DIFF_OPT_EXIT_WITH_STATUS    (1 << 14)
 #define DIFF_OPT_REVERSE_DIFF        (1 << 15)
+#define DIFF_OPT_CHECK_FAILED        (1 << 16)
 #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
 #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
 #define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 6adf9d1..dc538b3 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -117,7 +117,6 @@ EOF
 git diff -b > out
 test_expect_success 'another test, with -b' 'git diff expect out'
 
-
 test_expect_success 'check mixed spaces and tabs in indent' '
 
 	# This is indented with SP HT SP.
@@ -126,4 +125,140 @@ test_expect_success 'check mixed spaces and tabs in indent' '
 
 '
 
+test_expect_success 'check with no whitespace errors' '
+
+	git commit -m "snapshot" &&
+	echo "foo();" > x &&
+	git diff --check
+
+'
+
+test_expect_failure 'check with trailing whitespace' '
+
+	echo "foo(); " > x &&
+	git diff --check
+
+'
+
+test_expect_failure 'check with space before tab in indent' '
+
+	# indent has space followed by hard tab
+	echo " 	foo();" > x &&
+	git diff --check
+
+'
+
+test_expect_failure '--check and --exit-code are exclusive' '
+
+	git checkout x &&
+	git diff --check --exit-code
+
+'
+
+test_expect_failure '--check and --quiet are exclusive' '
+
+	git diff --check --quiet
+
+'
+
+test_expect_success 'check staged with no whitespace errors' '
+
+	echo "foo();" > x &&
+	git add x &&
+	git diff --cached --check
+
+'
+
+test_expect_failure 'check staged with trailing whitespace' '
+
+	echo "foo(); " > x &&
+	git add x &&
+	git diff --cached --check
+
+'
+
+test_expect_failure 'check staged with space before tab in indent' '
+
+	# indent has space followed by hard tab
+	echo " 	foo();" > x &&
+	git add x &&
+	git diff --cached --check
+
+'
+
+test_expect_success 'check with no whitespace errors (diff-index)' '
+
+	echo "foo();" > x &&
+	git add x &&
+	git diff-index --check HEAD
+
+'
+
+test_expect_failure 'check with trailing whitespace (diff-index)' '
+
+	echo "foo(); " > x &&
+	git add x &&
+	git diff-index --check HEAD
+
+'
+
+test_expect_failure 'check with space before tab in indent (diff-index)' '
+
+	# indent has space followed by hard tab
+	echo " 	foo();" > x &&
+	git add x &&
+	git diff-index --check HEAD
+
+'
+
+test_expect_success 'check staged with no whitespace errors (diff-index)' '
+
+	echo "foo();" > x &&
+	git add x &&
+	git diff-index --cached --check HEAD
+
+'
+
+test_expect_failure 'check staged with trailing whitespace (diff-index)' '
+
+	echo "foo(); " > x &&
+	git add x &&
+	git diff-index --cached --check HEAD
+
+'
+
+test_expect_failure 'check staged with space before tab in indent (diff-index)' '
+
+	# indent has space followed by hard tab
+	echo " 	foo();" > x &&
+	git add x &&
+	git diff-index --cached --check HEAD
+
+'
+
+test_expect_success 'check with no whitespace errors (diff-tree)' '
+
+	echo "foo();" > x &&
+	git commit -m "new commit" x &&
+	git diff-tree --check HEAD^ HEAD
+
+'
+
+test_expect_failure 'check with trailing whitespace (diff-tree)' '
+
+	echo "foo(); " > x &&
+	git commit -m "another commit" x &&
+	git diff-tree --check HEAD^ HEAD
+
+'
+
+test_expect_failure 'check with space before tab in indent (diff-tree)' '
+
+	# indent has space followed by hard tab
+	echo " 	foo();" > x &&
+	git commit -m "yet another" x &&
+	git diff-tree --check HEAD^ HEAD
+
+'
+
 test_done
-- 
1.5.4.rc0.4.g50348

[PATCH 2/5] New version of pre-commit hook

[Wincent Colaiuta]

Now that "git diff --check" indicates problems with its exit code the
pre-commit hook becomes a trivial one-liner.

A side effect of this is that when "git diff --check" learns to detect
problems according to core.whitespace in the future, the hook's
behaviour will evolve to match without any changes required.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 templates/hooks--pre-commit |   67 ++++--------------------------------------
 1 files changed, 7 insertions(+), 60 deletions(-)

diff --git a/templates/hooks--pre-commit b/templates/hooks--pre-commit
index 7092bae..f8c7be7 100644
--- a/templates/hooks--pre-commit
+++ b/templates/hooks--pre-commit
@@ -7,64 +7,11 @@
 #
 # To enable this hook, make this file executable.
 
-# This is slightly modified from Andrew Morton's Perfect Patch.
-# Lines you introduce should not have trailing whitespace.
-# Also check for an indentation that has SP before a TAB.
+git-diff-index -M --cached --check HEAD -- && exit 0
 
-if git-rev-parse --verify HEAD 2>/dev/null
-then
-	git-diff-index -p -M --cached HEAD --
-else
-	# NEEDSWORK: we should produce a diff with an empty tree here
-	# if we want to do the same verification for the initial import.
-	:
-fi |
-perl -e '
-    my $found_bad = 0;
-    my $filename;
-    my $reported_filename = "";
-    my $lineno;
-    sub bad_line {
-	my ($why, $line) = @_;
-	if (!$found_bad) {
-	    print STDERR "*\n";
-	    print STDERR "* You have some suspicious patch lines:\n";
-	    print STDERR "*\n";
-	    $found_bad = 1;
-	}
-	if ($reported_filename ne $filename) {
-	    print STDERR "* In $filename\n";
-	    $reported_filename = $filename;
-	}
-	print STDERR "* $why (line $lineno)\n";
-	print STDERR "$filename:$lineno:$line\n";
-    }
-    while (<>) {
-	if (m|^diff --git a/(.*) b/\1$|) {
-	    $filename = $1;
-	    next;
-	}
-	if (/^@@ -\S+ \+(\d+)/) {
-	    $lineno = $1 - 1;
-	    next;
-	}
-	if (/^ /) {
-	    $lineno++;
-	    next;
-	}
-	if (s/^\+//) {
-	    $lineno++;
-	    chomp;
-	    if (/\s$/) {
-		bad_line("trailing whitespace", $_);
-	    }
-	    if (/^\s* \t/) {
-		bad_line("indent SP followed by a TAB", $_);
-	    }
-	    if (/^(?:[<>=]){7}/) {
-		bad_line("unresolved merge conflict", $_);
-	    }
-	}
-    }
-    exit($found_bad);
-'
+cat >&2 <<EOF
+fatal: commit aborted due to whitespace problems
+specify --no-verify to bypass these checks and commit anyway
+EOF
+
+exit 1
-- 
1.5.4.rc0.4.g50348

[PATCH 3/5] Unify whitespace checking

[Wincent Colaiuta]

This commit unifies three separate places where whitespace checking was
performed:

 - the whitespace checking previously done in builtin-apply.c is
extracted into a function in ws.c

 - the equivalent logic in "git diff" is removed

 - the emit_line_with_ws() function is also removed because that also
rechecks the shitespace, and its functionality is rolled into ws.c

The new function is called check_and_emit_line() and it does two things:
checks a line for whitespace errors and optionally emits it. The checking
is based on lines of content rather than patch lines (in other words, the
caller must strip the leading "+" or "-"); this was suggested by Junio on
the mailing list to allow for a future extension to "git show" to display
whitespace errors in blobs.

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

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 builtin-apply.c            |   54 +++--------------
 cache.h                    |    4 +
 diff.c                     |  138 ++++++--------------------------------------
 t/t4015-diff-whitespace.sh |    2 +-
 ws.c                       |  105 +++++++++++++++++++++++++++++++++
 5 files changed, 139 insertions(+), 164 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index f2e9a33..ab393f3 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -900,56 +900,22 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
 
 static void check_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_and_emit_line(line + 1, len - 1, ws_rule,
+	    NULL, NULL, NULL, NULL);
+	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);
+	}
 }
 
 /*
diff --git a/cache.h b/cache.h
index 27d90fe..39331c2 100644
--- a/cache.h
+++ b/cache.h
@@ -655,6 +655,10 @@ 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_and_emit_line(const char *line, int len, unsigned ws_rule,
+    FILE *stream, const char *set,
+    const char *reset, const char *ws);
+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/diff.c b/diff.c
index 39109a6..d96a4ac 100644
--- a/diff.c
+++ b/diff.c
@@ -486,88 +486,9 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix)
 
 static void emit_line(const char *set, const char *reset, const char *line, int len)
 {
-	if (len > 0 && line[len-1] == '\n')
-		len--;
 	fputs(set, stdout);
 	fwrite(line, len, 1, stdout);
-	puts(reset);
-}
-
-static void emit_line_with_ws(int nparents,
-			      const char *set, const char *reset, const char *ws,
-			      const char *line, int len, unsigned ws_rule)
-{
-	int col0 = nparents;
-	int last_tab_in_indent = -1;
-	int last_space_in_indent = -1;
-	int i;
-	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.  In
-	 * addition, under "indent with non tab" rule, there should not
-	 * be more than 8 consecutive spaces.
-	 */
-	for (i = col0; i < len; i++) {
-		if (line[i] == '\t') {
-			last_tab_in_indent = i;
-			if ((ws_rule & WS_SPACE_BEFORE_TAB) &&
-			    0 <= last_space_in_indent)
-				need_highlight_leading_space = 1;
-		}
-		else if (line[i] == ' ')
-			last_space_in_indent = i;
-		else
-			break;
-	}
-	if ((ws_rule & WS_INDENT_WITH_NON_TAB) &&
-	    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);
-	if (((i == len) || line[i] == '\n') && i != col0) {
-		/* The whole line was indent */
-		emit_line(ws, reset, line + col0, len - col0);
-		return;
-	}
-	i = col0;
-	if (need_highlight_leading_space) {
-		while (i < last_tab_in_indent) {
-			if (line[i] == ' ') {
-				fputs(ws, stdout);
-				putchar(' ');
-				fputs(reset, stdout);
-			}
-			else
-				putchar(line[i]);
-			i++;
-		}
-	}
-	tail = len - 1;
-	if (line[tail] == '\n' && i < tail)
-		tail--;
-	if (ws_rule & WS_TRAILING_SPACE) {
-		while (i < tail) {
-			if (!isspace(line[tail]))
-				break;
-			tail--;
-		}
-	}
-	if ((i < tail && line[tail + 1] != '\n')) {
-		/* This has whitespace between tail+1..len */
-		fputs(set, stdout);
-		fwrite(line + i, tail - i + 1, 1, stdout);
-		fputs(reset, stdout);
-		emit_line(ws, reset, line + tail + 1, len - tail - 1);
-	}
-	else
-		emit_line(set, reset, line + i, len - i);
 }
 
 static void emit_add_line(const char *reset, struct emit_callback *ecbdata, const char *line, int len)
@@ -577,9 +498,13 @@ static void emit_add_line(const char *reset, struct emit_callback *ecbdata, cons
 
 	if (!*ws)
 		emit_line(set, reset, line, len);
-	else
-		emit_line_with_ws(ecbdata->nparents, set, reset, ws,
-				  line, len, ecbdata->ws_rule);
+	else {
+		/* Emit just the prefix, then the rest. */
+		emit_line(set, reset, line, ecbdata->nparents);
+		(void)check_and_emit_line(line + ecbdata->nparents,
+		    len - ecbdata->nparents, ecbdata->ws_rule,
+		    stdout, set, reset, ws);
+	}
 }
 
 static void fn_out_consume(void *priv, char *line, unsigned long len)
@@ -1040,45 +965,20 @@ 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_and_emit_line(line + 1, len - 1,
+		    data->ws_rule, NULL, NULL, NULL, NULL);
+		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(set, reset, line, 1);
+		(void)check_and_emit_line(line + 1, len - 1, data->ws_rule,
+		    stdout, set, reset, ws);
 		data->lineno++;
 	} else if (line[0] == ' ')
 		data->lineno++;
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index dc538b3..a0a47dd 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -121,7 +121,7 @@ test_expect_success 'check mixed spaces and tabs in indent' '
 
 	# This is indented with SP HT SP.
 	echo " 	 foo();" > x &&
-	git diff --check | grep "space before tab"
+	git diff --check | grep "Space in indent is followed by a tab"
 
 '
 
diff --git a/ws.c b/ws.c
index 52c10ca..a0ce860 100644
--- a/ws.c
+++ b/ws.c
@@ -94,3 +94,108 @@ 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);
+}
+
+/* If stream is non-NULL, emits the line after checking. */
+unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
+                             FILE *stream, const char *set,
+                             const char *reset, const char *ws)
+{
+	unsigned result = 0;
+	int leading_space = -1;
+	int trailing_whitespace = -1;
+	int trailing_newline = 0;
+	int i;
+
+	/* Logic is simpler if we temporarily ignore the trailing newline. */
+	if (len > 0 && line[len - 1] == '\n') {
+		trailing_newline = 1;
+		len--;
+	}
+
+	/* Check for trailing whitespace. */
+	if (ws_rule & WS_TRAILING_SPACE) {
+		for (i = len - 1; i >= 0; i--) {
+			if (isspace(line[i])) {
+				trailing_whitespace = i;
+				result |= WS_TRAILING_SPACE;
+			}
+			else
+				break;
+		}
+	}
+
+	/* Check for space before tab in initial indent. */
+	for (i = 0; i < len; i++) {
+		if (line[i] == '\t') {
+			if ((ws_rule & WS_SPACE_BEFORE_TAB) &&
+			    (leading_space != -1))
+				result |= WS_SPACE_BEFORE_TAB;
+			break;
+		}
+		else if (line[i] == ' ')
+			leading_space = i;
+		else
+			break;
+	}
+
+	/* Check for indent using non-tab. */
+	if ((ws_rule & WS_INDENT_WITH_NON_TAB) && leading_space >= 8)
+		result |= WS_INDENT_WITH_NON_TAB;
+
+	if (stream) {
+		/* Highlight errors in leading whitespace. */
+		if ((result & WS_SPACE_BEFORE_TAB) ||
+		    (result & WS_INDENT_WITH_NON_TAB)) {
+			fputs(ws, stream);
+			fwrite(line, leading_space + 1, 1, stream);
+			fputs(reset, stream);
+			leading_space++;
+		}
+		else
+			leading_space = 0;
+
+		/* Now the rest of the line starts at leading_space.
+		 * The non-highlighted part ends at trailing_whitespace. */
+		if (trailing_whitespace == -1)
+			trailing_whitespace = len;
+
+		/* Emit non-highlighted (middle) segment. */
+		if (trailing_whitespace - leading_space > 0) {
+			fputs(set, stream);
+			fwrite(line + leading_space,
+			    trailing_whitespace - leading_space, 1, stream);
+			fputs(reset, stream);
+		}
+
+		/* Highlight errors in trailing whitespace. */
+		if (trailing_whitespace != len) {
+			fputs(ws, stream);
+			fwrite(line + trailing_whitespace,
+			    len - trailing_whitespace, 1, stream);
+			fputs(reset, stream);
+		}
+		if (trailing_newline)
+			fputc('\n', stream);
+	}
+	return result;
+}
-- 
1.5.4.rc0.4.g50348

[PATCH 4/5] Make "diff --check" output match "git apply"

[Wincent Colaiuta]

For consistency, make the two tools report whitespace errors in the
same way (the output of "diff --check" has been tweaked to match
that of "git apply").

Note that although the textual content is basically the same only
"git diff --check" provides a colorized version of the problematic
lines; making "git apply" do colorization will require more extensive
changes (figuring out the diff colorization preferences of the user)
and so that will be a subject for another commit.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 builtin-apply.c |    4 ++--
 diff.c          |    3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index ab393f3..2edd83b 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -912,8 +912,8 @@ static void check_whitespace(const char *line, int len, unsigned ws_rule)
 		;
 	else {
 		err = whitespace_error_string(result);
-		fprintf(stderr, "%s.\n%s:%d:%.*s\n",
-		     err, patch_input_file, linenr, len - 2, line + 1);
+		fprintf(stderr, "%s:%d: %s.\n%.*s\n",
+		     patch_input_file, linenr, err, len - 2, line + 1);
 		free(err);
 	}
 }
diff --git a/diff.c b/diff.c
index d96a4ac..73a723d 100644
--- a/diff.c
+++ b/diff.c
@@ -973,8 +973,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
 		if (!data->status)
 			return;
 		err = whitespace_error_string(data->status);
-		printf("%s:%d: %s%s:%s ", data->filename, data->lineno,
-		    ws, err, reset);
+		printf("%s:%d: %s.\n", data->filename, data->lineno, err);
 		free(err);
 		emit_line(set, reset, line, 1);
 		(void)check_and_emit_line(line + 1, len - 1, data->ws_rule,
-- 
1.5.4.rc0.4.g50348

[PATCH 5/5] 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 |   50 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index a0a47dd..cb90ee7 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -261,4 +261,54 @@ test_expect_failure 'check with space before tab in indent (diff-tree)' '
 
 '
 
+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)' '
+
+	# indent contains space followed by HT
+	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)' '
+
+	# indent contains space followed by HT
+	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.4.rc0.4.g50348

Re: [PATCH 3/5] Unify whitespace checking

[Andreas Ericsson]

Wincent Colaiuta wrote:
> 
>  - the emit_line_with_ws() function is also removed because that also
> rechecks the shitespace, and its functionality is rolled into ws.c
> 

shitespace? It's fitting though. I'd stick with it :-)

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

[PATCH] Don't use the pager when running "git diff --check"

[Wincent Colaiuta]

In 89d07f75 "git diff" learnt to not run the pager if the user passes
the --exit-code switch. This commit does the same for the --check
switch for the same reason: we want the user to get the exit status
from "git diff", not the pager.

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

This really should have been part of my original patch,
([PATCH 1/5] "diff --check" should affect exit status"), but I only
just discovered this now.

 builtin-diff.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin-diff.c b/builtin-diff.c
index 86d01a3..ed2d218 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -247,7 +247,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	/* If the user asked for our exit code then don't start a
 	 * pager or we would end up reporting its exit code instead.
 	 */
-	if (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))
+	if (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS) &&
+	    (!&rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF))
 		setup_pager();
 
 	/* Do we have --cached and not have a pending object, then
-- 
1.5.4.rc0.5.gd201-dirty

Re: [PATCH 1/5] "diff --check" should affect exit status

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> @@ -2965,8 +2976,8 @@ void diff_flush(struct diff_options *options)
>  			     DIFF_FORMAT_CHECKDIFF)) {
>  		for (i = 0; i < q->nr; i++) {
>  			struct diff_filepair *p = q->queue[i];
> -			if (check_pair_status(p))
> -				flush_one_pair(p, options);
> +			if (check_pair_status(p) && flush_one_pair(p, options))
> +				DIFF_OPT_SET(options, CHECK_FAILED);
>  		}
>  		separator++;
>  	}

Isn't this wrong when check is not in effect?

I think highjacking the "did we encounter problems" return value of the
entire callchain for the purpose of checkdiff is very ugly and wrong to
begin with, so please do not argue "but if checkdiff is not in effect,
the caller does not check CHECK_FAILED".

Wouldn't it be much cleaner to make diff_flush_checkdiff(), or its
underlying function run_checkdiff(), set that CHECK_FAILED flag to
options structure, and return success?  The toplevel caller can decide
to exit with non-zero when --check is in effect and CHECK_FAILED flag is
set.

Re: [PATCH 3/5] Unify whitespace checking

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> The new function is called check_and_emit_line() and it does two things:
> checks a line for whitespace errors and optionally emits it. The checking
> is based on lines of content rather than patch lines (in other words, the
> caller must strip the leading "+" or "-"); this was suggested by Junio on
> the mailing list to allow for a future extension to "git show" to display
> whitespace errors in blobs.

I do not think "git show" is a realistic "future extension", by the
way.  But at least from the interface-cleanliness point of view, I think
not passing the leading "+" to the function is a right thing to do.

> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> index dc538b3..a0a47dd 100755
> --- a/t/t4015-diff-whitespace.sh
> +++ b/t/t4015-diff-whitespace.sh
> @@ -121,7 +121,7 @@ test_expect_success 'check mixed spaces and tabs in indent' '
>  
>  	# This is indented with SP HT SP.
>  	echo " 	 foo();" > x &&
> -	git diff --check | grep "space before tab"
> +	git diff --check | grep "Space in indent is followed by a tab"
>  
>  '

Hmph.  I think with the multiple detection this rewording would make the
error message very long to read.  Was the rewording really necessary?

> +/* If stream is non-NULL, emits the line after checking. */
> +unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
> +                             FILE *stream, const char *set,
> +                             const char *reset, const char *ws)
> +{

Honestly, I regretted suggesting this, fearing that it might make the
checking too costly, but the code is clean, readable, and does not look
costly at all.  Nice job.

Re: [PATCH 4/5] Make "diff --check" output match "git apply"

[Junio C Hamano]

Nice attention to the details.

Re: [PATCH 2/5] New version of pre-commit hook

[Jakub Narebski]

Wincent Colaiuta wrote:

> Now that "git diff --check" indicates problems with its exit code the
> pre-commit hook becomes a trivial one-liner.

> -         if (/^(?:[<>=]){7}/) {
> -             bad_line("unresolved merge conflict", $_);
> -         }

Aren't you losing this check with rewrite?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

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

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> +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

It is good to check both positive and negative cases, but I would prefer
avoiding test_expect_failure.  You cannot tell if the "git config"
before the real test failed or "git diff --check" correctly reported
whitespace breakage --- both would cause the test to pass.

Re: [PATCH 1/5] "diff --check" should affect exit status

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think highjacking the "did we encounter problems" return value of the
> entire callchain for the purpose of checkdiff is very ugly and wrong to
> begin with,...

How about this on top of your 1/5?  It mostly is about reverting the
damage to the higher level callchain.  Instead, builtin_checkdiff()
inspects the checkdiff data and sets the CHECK_FAILED flag to the
diffopt structure.  The callers are already checking that flag so there
is nothing to change for them.

--

diff --git a/diff.c b/diff.c
index 39109a6..fc496bf 100644
--- a/diff.c
+++ b/diff.c
@@ -1456,7 +1456,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 	diff_free_filespec_data(two);
 }
 
-static int builtin_checkdiff(const char *name_a, const char *name_b,
+static void builtin_checkdiff(const char *name_a, const char *name_b,
 			     struct diff_filespec *one,
 			     struct diff_filespec *two, struct diff_options *o)
 {
@@ -1464,7 +1464,7 @@ static int builtin_checkdiff(const char *name_a, const char *name_b,
 	struct checkdiff_t data;
 
 	if (!two)
-		return 0;
+		return;
 
 	memset(&data, 0, sizeof(data));
 	data.xm.consume = checkdiff_consume;
@@ -1493,7 +1493,8 @@ static int builtin_checkdiff(const char *name_a, const char *name_b,
  free_and_return:
 	diff_free_filespec_data(one);
 	diff_free_filespec_data(two);
-	return data.status;
+	if (data.status)
+		DIFF_OPT_SET(o, CHECK_FAILED);
 }
 
 struct diff_filespec *alloc_filespec(const char *path)
@@ -2078,14 +2079,14 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,
 	builtin_diffstat(name, other, p->one, p->two, diffstat, o, complete_rewrite);
 }
 
-static int run_checkdiff(struct diff_filepair *p, struct diff_options *o)
+static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)
 {
 	const char *name;
 	const char *other;
 
 	if (DIFF_PAIR_UNMERGED(p)) {
 		/* unmerged */
-		return 0;
+		return;
 	}
 
 	name = p->one->path;
@@ -2094,7 +2095,7 @@ static int run_checkdiff(struct diff_filepair *p, struct diff_options *o)
 	diff_fill_sha1_info(p->one);
 	diff_fill_sha1_info(p->two);
 
-	return builtin_checkdiff(name, other, p->one, p->two, o);
+	builtin_checkdiff(name, other, p->one, p->two, o);
 }
 
 void diff_setup(struct diff_options *options)
@@ -2596,17 +2597,17 @@ static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,
 	run_diffstat(p, o, diffstat);
 }
 
-static int diff_flush_checkdiff(struct diff_filepair *p,
+static void diff_flush_checkdiff(struct diff_filepair *p,
 		struct diff_options *o)
 {
 	if (diff_unmodified_pair(p))
-		return 0;
+		return;
 
 	if ((DIFF_FILE_VALID(p->one) && S_ISDIR(p->one->mode)) ||
 	    (DIFF_FILE_VALID(p->two) && S_ISDIR(p->two->mode)))
-		return 0; /* no tree diffs in patch format */
+		return; /* no tree diffs in patch format */
 
-	return run_checkdiff(p, o);
+	run_checkdiff(p, o);
 }
 
 int diff_queue_is_empty(void)
@@ -2725,19 +2726,16 @@ static int check_pair_status(struct diff_filepair *p)
 	}
 }
 
-static int flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
+static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
 {
 	int fmt = opt->output_format;
 
 	if (fmt & DIFF_FORMAT_CHECKDIFF)
-		return diff_flush_checkdiff(p, opt);
+		diff_flush_checkdiff(p, opt);
 	else if (fmt & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))
 		diff_flush_raw(p, opt);
 	else if (fmt & DIFF_FORMAT_NAME)
 		write_name_quoted(p->two->path, stdout, opt->line_termination);
-
-	/* return value only matters with DIFF_FORMAT_CHECKDIFF */
-	return 0;
 }
 
 static void show_file_mode_name(const char *newdelete, struct diff_filespec *fs)
@@ -2976,8 +2974,8 @@ void diff_flush(struct diff_options *options)
 			     DIFF_FORMAT_CHECKDIFF)) {
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
-			if (check_pair_status(p) && flush_one_pair(p, options))
-				DIFF_OPT_SET(options, CHECK_FAILED);
+			if (check_pair_status(p))
+				flush_one_pair(p, options);
 		}
 		separator++;
 	}
-- 
1.5.4.rc0.1.g37d0

Re: [PATCH] Don't use the pager when running "git diff --check"

[Jeff King]

On Thu, Dec 13, 2007 at 08:45:38PM +0100, Wincent Colaiuta wrote:

> In 89d07f75 "git diff" learnt to not run the pager if the user passes
> the --exit-code switch. This commit does the same for the --check
> switch for the same reason: we want the user to get the exit status
> from "git diff", not the pager.

But --check is also producing useful output, which might need paged. So
you are sacrificing existing interactive use of --check for scriptable
exit-code uses. If you really want the exit code, why not "git diff
--check --exit-code"?

OTOH, I am not too sad to lose the paging behavior; it would take quite
a few whitespace errors to scroll off the screen.

-Peff

Re: [PATCH] Don't use the pager when running "git diff --check"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Dec 13, 2007 at 08:45:38PM +0100, Wincent Colaiuta wrote:
>
>> In 89d07f75 "git diff" learnt to not run the pager if the user passes
>> the --exit-code switch. This commit does the same for the --check
>> switch for the same reason: we want the user to get the exit status
>> from "git diff", not the pager.
>
> But --check is also producing useful output, which might need paged. So
> you are sacrificing existing interactive use of --check for scriptable
> exit-code uses. If you really want the exit code, why not "git diff
> --check --exit-code"?
>
> OTOH, I am not too sad to lose the paging behavior; it would take quite
> a few whitespace errors to scroll off the screen.

You are right.  While I do not personally miss paging output, it is a
regression not to page --check output by default.

By the way, there is no reason to make --check and --exit-code mutually
exclusive either.  You could say with --exit-code the command will exit
with status 01 or'ed in if trees are not identical, and with --check the
command will exit with status 02 or'ed in.  Loosely written scripted
callers can continue doing:

	if git --no-pager diff --exit-code
        then
        	they are different
        fi

	if git --no-pager diff --check
	then
        	there are funky blanks
	fi

while the ones that are aware of the new behaviour of --check can:

	git --no-pager diff --check --exit-code
        case $? in
        0)	all is well ;;
        1)	clean difference there ;;
        3)	dirty difference there ;;
        2)	cannot happen ;;
	*)	bombed out, as die exits with 128 ;;
	esac

Re: [PATCH 2/5] New version of pre-commit hook

[Wincent Colaiuta]

El 14/12/2007, a las 1:17, Jakub Narebski escribió:

> Wincent Colaiuta wrote:
>
>> Now that "git diff --check" indicates problems with its exit code the
>> pre-commit hook becomes a trivial one-liner.
>
>> -         if (/^(?:[<>=]){7}/) {
>> -             bad_line("unresolved merge conflict", $_);
>> -         }
>
> Aren't you losing this check with rewrite?

Yes. If that's a problem then this is definitely a "no-goer".

Cheers,
Wincent

Re: [PATCH] Don't use the pager when running "git diff --check"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> By the way, there is no reason to make --check and --exit-code mutually
> exclusive either.

Perhaps something like this.  Regardless of the exclusivity issue, I
think the "diff_result_code()" helper function is a good clean-up.

---
 builtin-diff-files.c       |    6 +-----
 builtin-diff-index.c       |    6 +-----
 builtin-diff-tree.c        |    6 ++----
 builtin-diff.c             |   11 ++++-------
 diff.c                     |   19 ++++++++++++++-----
 diff.h                     |    2 ++
 t/t4015-diff-whitespace.sh |    4 ++--
 7 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index 4afc872..9c04111 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -31,9 +31,5 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 	result = run_diff_files_cmd(&rev, argc, argv);
-	if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))
-		return DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0;
-	if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
-		return DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0;
-	return result;
+	return diff_result_code(&rev.diffopt, result);
 }
diff --git a/builtin-diff-index.c b/builtin-diff-index.c
index 532b284..0f2390a 100644
--- a/builtin-diff-index.c
+++ b/builtin-diff-index.c
@@ -44,9 +44,5 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 		return -1;
 	}
 	result = run_diff_index(&rev, cached);
-	if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))
-		return DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0;
-	if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
-		return DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0;
-	return result;
+	return diff_result_code(&rev.diffopt, result);
 }
diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index 9ab90cb..ebc50ef 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c
@@ -132,8 +132,6 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 				diff_tree_stdin(line);
 		}
 	}
-	if (opt->diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
-		return DIFF_OPT_TST(&opt->diffopt, CHECK_FAILED) != 0;
-	return DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS)
-		&& DIFF_OPT_TST(&opt->diffopt, HAS_CHANGES);
+
+	return diff_result_code(&opt->diffopt, 0);
 }
diff --git a/builtin-diff.c b/builtin-diff.c
index 9d878f6..29365a0 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -244,11 +244,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
 	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
 
-	/* If the user asked for our exit code then don't start a
+	/*
+	 * If the user asked for our exit code then don't start a
 	 * pager or we would end up reporting its exit code instead.
 	 */
-	if (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS) &&
-	    (!(rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)))
+	if (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))
 		setup_pager();
 
 	/* Do we have --cached and not have a pending object, then
@@ -352,10 +352,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	else
 		result = builtin_diff_combined(&rev, argc, argv,
 					     ent, ents);
-	if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))
-		result = DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0;
-	if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
-		return DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0;
+	result = diff_result_code(&rev.diffopt, result);
 	if (1 < rev.diffopt.skip_stat_unmatch)
 		refresh_index_quietly();
 	return result;
diff --git a/diff.c b/diff.c
index 8237075..3e46ff8 100644
--- a/diff.c
+++ b/diff.c
@@ -2125,12 +2125,7 @@ int diff_setup_done(struct diff_options *options)
 	if (options->output_format & DIFF_FORMAT_NAME_STATUS)
 		count++;
 	if (options->output_format & DIFF_FORMAT_CHECKDIFF)
-	{
 		count++;
-		if (DIFF_OPT_TST(options, QUIET) ||
-		    DIFF_OPT_TST(options, EXIT_WITH_STATUS))
-			die("--check may not be used with --quiet or --exit-code");
-	}
 	if (options->output_format & DIFF_FORMAT_NO_OUTPUT)
 		count++;
 	if (count > 1)
@@ -3180,6 +3175,20 @@ void diffcore_std(struct diff_options *options)
 		DIFF_OPT_CLR(options, HAS_CHANGES);
 }
 
+int diff_result_code(struct diff_options *opt, int status)
+{
+	int result = 0;
+	if (!DIFF_OPT_TST(opt, EXIT_WITH_STATUS) &&
+	    !(opt->output_format & DIFF_FORMAT_CHECKDIFF))
+		return status;
+	if (DIFF_OPT_TST(opt, EXIT_WITH_STATUS) &&
+	    DIFF_OPT_TST(opt, HAS_CHANGES))
+		result |= 01;
+	if ((opt->output_format & DIFF_FORMAT_CHECKDIFF) &&
+	    DIFF_OPT_TST(opt, CHECK_FAILED))
+		result |= 02;
+	return result;
+}
 
 void diff_addremove(struct diff_options *options,
 		    int addremove, unsigned mode,
diff --git a/diff.h b/diff.h
index 5d50d93..7e8000a 100644
--- a/diff.h
+++ b/diff.h
@@ -247,4 +247,6 @@ extern int run_diff_index(struct rev_info *revs, int cached);
 extern int do_diff_cache(const unsigned char *, struct diff_options *);
 extern int diff_flush_patch_id(struct diff_options *, unsigned char *);
 
+extern int diff_result_code(struct diff_options *, int);
+
 #endif /* DIFF_H */
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index dc538b3..757a27a 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -148,14 +148,14 @@ test_expect_failure 'check with space before tab in indent' '
 
 '
 
-test_expect_failure '--check and --exit-code are exclusive' '
+test_expect_success '--check and --exit-code are not exclusive' '
 
 	git checkout x &&
 	git diff --check --exit-code
 
 '
 
-test_expect_failure '--check and --quiet are exclusive' '
+test_expect_success '--check and --quiet are not exclusive' '
 
 	git diff --check --quiet
 

Re: [PATCH 3/5] Unify whitespace checking

[Wincent Colaiuta]

El 14/12/2007, a las 1:03, Junio C Hamano escribió:

>> -	git diff --check | grep "space before tab"
>> +	git diff --check | grep "Space in indent is followed by a tab"
>>
> Hmph.  I think with the multiple detection this rewording would make  
> the
> error message very long to read.  Was the rewording really necessary?

Well, that wording came from builtin-apply.c so it wasn't really a "re- 
wording" but an extraction. The other string comes from "diff.c". So I  
had to pick one of them in unifying them and basically the one from  
builtin-apply.c won because it was the site where I extracted from  
first. So basically it was arbitrary.

If I were to actually *re-word* the message I think something like  
this would be the compromise between clarity and conciseness:

"space before tab in indent"

And in the same spirit, the other two strings extracted from builtin- 
apply.c into the new whitespace_error_string() function:

"Adds trailing whitespace"
"Indent more than 8 places with spaces"

Could be shortened to:

"trailing whitespace" (diff.c says "white space at end")
"indent using spaces"

But I personally have no strong conviction about this, so I'm happy  
for it to be whatever you want.

>> +/* If stream is non-NULL, emits the line after checking. */
>> +unsigned check_and_emit_line(const char *line, int len, unsigned  
>> ws_rule,
>> +                             FILE *stream, const char *set,
>> +                             const char *reset, const char *ws)
>> +{
>
> Honestly, I regretted suggesting this, fearing that it might make the
> checking too costly, but the code is clean, readable, and does not  
> look
> costly at all.  Nice job.

Thanks. The check_and_emit_line function itself is fairly clean  
internally (apart from the somewhat ugly parameter list, but that was  
mostly inherited from the emit_line_with_ws function which it  
replaces). It's the sites where check_and_emit_line is called that  
look a bit ugly, but those can hopefully be cleaned up at some point  
in the future.

Cheers,
Wincent

Re: [PATCH] Don't use the pager when running "git diff --check"

[Wincent Colaiuta]

El 14/12/2007, a las 6:11, Junio C Hamano escribió:

> Jeff King <peff@peff.net> writes:
>
>> On Thu, Dec 13, 2007 at 08:45:38PM +0100, Wincent Colaiuta wrote:
>>
>>> In 89d07f75 "git diff" learnt to not run the pager if the user  
>>> passes
>>> the --exit-code switch. This commit does the same for the --check
>>> switch for the same reason: we want the user to get the exit status
>>> from "git diff", not the pager.
>>
>> But --check is also producing useful output, which might need  
>> paged. So
>> you are sacrificing existing interactive use of --check for  
>> scriptable
>> exit-code uses. If you really want the exit code, why not "git diff
>> --check --exit-code"?
>>
>> OTOH, I am not too sad to lose the paging behavior; it would take  
>> quite
>> a few whitespace errors to scroll off the screen.
>
> You are right.  While I do not personally miss paging output, it is a
> regression not to page --check output by default.

I thought this was ok because "git diff --exit-code" also produces  
useful output and also turns off the pager.

> By the way, there is no reason to make --check and --exit-code  
> mutually
> exclusive either.  You could say with --exit-code the command will  
> exit
> with status 01 or'ed in if trees are not identical, and with --check  
> the
> command will exit with status 02 or'ed in.

Yes, this sounds fine to me.

Cheers,
Wincent

Re: [PATCH] Don't use the pager when running "git diff --check"

[Wincent Colaiuta]

El 14/12/2007, a las 8:33, Junio C Hamano escribió:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> By the way, there is no reason to make --check and --exit-code  
>> mutually
>> exclusive either.
>
> Perhaps something like this.  Regardless of the exclusivity issue, I
> think the "diff_result_code()" helper function is a good clean-up.

Totally agreed. It replaces some groups of very-similar lines.

I'll integrate these (and the other suggestions) some time this  
morning and post a new version of the series that incorporates all the  
feedback and patch suggestions.

Cheers,
Wincent

Re: [PATCH] Don't use the pager when running "git diff --check"

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> I'll integrate these (and the other suggestions) some time this  
> morning and post a new version of the series that incorporates all the  
> feedback and patch suggestions.

Heh, I am about to push out fixed-up results, so it might save both of
us some time if you looked at it first and then complained on my
screwups.

Re: [PATCH 2/5] New version of pre-commit hook

[Jakub Narebski]

Dnia piątek 14. grudnia 2007 08:24, Wincent Colaiuta napisał:
> El 14/12/2007, a las 1:17, Jakub Narebski escribió:
> 
>> Wincent Colaiuta wrote:
>>
>>> Now that "git diff --check" indicates problems with its exit code the
>>> pre-commit hook becomes a trivial one-liner.
>>
>>> -         if (/^(?:[<>=]){7}/) {
>>> -             bad_line("unresolved merge conflict", $_);
>>> -         }
>>
>> Aren't you losing this check with rewrite?
> 
> Yes. If that's a problem then this is definitely a "no-goer".

Well, this only means that it wouldn't be one-liner, but replacing
all whitespace damage checks by "git diff --check" is still a good
idea.

Perhaps we could add 'merge conflict' check to git diff/git applyt too...
-- 
Jakub Narebski
Poland

Re: [PATCH] Don't use the pager when running "git diff --check"

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> El 14/12/2007, a las 6:11, Junio C Hamano escribió:
>
>> You are right.  While I do not personally miss paging output, it is a
>> regression not to page --check output by default.
>
> I thought this was ok because "git diff --exit-code" also produces
> useful output and also turns off the pager.

It is different.  --exit-code was that way from day one.  The primary
use of --check has been (and I suspect it will continue to be) for
people to _view_ the diff, spot problems so that they can fix them up,
and for that use case, exit code does not matter but pageability does.
You are introducing a new behaviour and a new use case --- that does not
give you a license to break other existing use cases.