threads / patch / 11272

patch, 5 parts"diff --check" and whitespace enhancements

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

## tl;dr

24 messages between Dec 13, 2007 and Dec 14, 2007. Diffs are folded; open one to read it.

replies: 23people: 5as markdown or json

Wincent Colaiuta· Dec 13, 2007, 13:32 UTC · lore

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

Wincent Colaiuta· Dec 13, 2007, 13:32 UTC · re: Wincent Colaiuta · lore

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

"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(-)
Show changes to 8 files +184 −30

Documentation/diff-options.txt, builtin-diff-files.c, builtin-diff-index.c, builtin-diff-tree.c, builtin-diff.c, diff.c, diff.h, t/t4015-diff-whitespace.sh

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
Wincent Colaiuta· Dec 13, 2007, 13:32 UTC · re: Wincent Colaiuta · lore

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

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(-)
Show changes to templates/hooks--pre-commit +7 −60
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
Wincent Colaiuta· Dec 13, 2007, 13:32 UTC · re: Wincent Colaiuta · lore

[PATCH 3/5] Unify whitespace checking

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(-)
Show changes to 5 files +139 −164

builtin-apply.c, cache.h, diff.c, t/t4015-diff-whitespace.sh, ws.c

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
Wincent Colaiuta· Dec 13, 2007, 13:32 UTC · re: Wincent Colaiuta · lore

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

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(-)
Show changes to 2 files +3 −4

builtin-apply.c, diff.c

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
Wincent Colaiuta· Dec 13, 2007, 13:32 UTC · re: Wincent Colaiuta · lore

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

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(-)
Show changes to t/t4015-diff-whitespace.sh +50 −0
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
Junio C Hamano· Dec 14, 2007, 00:55 UTC · re: Wincent Colaiuta · lore

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

Wincent Colaiuta <win@wincent.com> writes:
Show 17 quoted lines
> +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.

Andreas Ericsson· Dec 13, 2007, 13:40 UTC · re: Wincent Colaiuta · lore

Re: [PATCH 3/5] Unify whitespace checking

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
Junio C Hamano· Dec 14, 2007, 00:03 UTC · re: Wincent Colaiuta · lore

Re: [PATCH 3/5] Unify whitespace checking

Wincent Colaiuta <win@wincent.com> writes:
Show 6 quoted lines
> 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.

Show 12 quoted lines
> 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?

Show 5 quoted lines
> +/* 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.

Wincent Colaiuta· Dec 14, 2007, 07:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/5] Unify whitespace checking

El 14/12/2007, a las 1:03, Junio C Hamano escribió:
Show 6 quoted lines
>> -	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.

Show 11 quoted lines
>> +/* 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

Jakub Narebski· Dec 14, 2007, 00:17 UTC · re: Wincent Colaiuta · lore

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

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
Wincent Colaiuta· Dec 14, 2007, 07:24 UTC · re: Jakub Narebski · lore

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

El 14/12/2007, a las 1:17, Jakub Narebski escribió:
Show 10 quoted lines
> 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

Jakub Narebski· Dec 14, 2007, 10:23 UTC · re: Wincent Colaiuta · lore

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

Dnia piątek 14. grudnia 2007 08:24, Wincent Colaiuta napisał:
Show 14 quoted lines
> 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
Wincent Colaiuta· Dec 13, 2007, 19:45 UTC · re: Wincent Colaiuta · lore

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

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(-)
Show changes to builtin-diff.c +2 −1
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
Jeff King· Dec 14, 2007, 04:51 UTC · re: Wincent Colaiuta · lore

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

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
Junio C Hamano· Dec 14, 2007, 05:11 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> 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
Junio C Hamano· Dec 14, 2007, 07:33 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 7 files +26 −28

builtin-diff-files.c, builtin-diff-index.c, builtin-diff-tree.c, builtin-diff.c, diff.c, diff.h, t/t4015-diff-whitespace.sh

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
 
Wincent Colaiuta· Dec 14, 2007, 07:51 UTC · re: Junio C Hamano · lore

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

El 14/12/2007, a las 8:33, Junio C Hamano escribió:
Show 8 quoted lines
> 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

Junio C Hamano· Dec 14, 2007, 07:57 UTC · re: Wincent Colaiuta · lore

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

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.

Wincent Colaiuta· Dec 14, 2007, 07:47 UTC · re: Junio C Hamano · lore

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

El 14/12/2007, a las 6:11, Junio C Hamano escribió:
Show 23 quoted lines
> 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.

Show 7 quoted lines
> 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

Junio C Hamano· Dec 14, 2007, 20:54 UTC · re: Wincent Colaiuta · lore

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

Wincent Colaiuta <win@wincent.com> writes:
Show 7 quoted lines
> 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.

Junio C Hamano· Dec 13, 2007, 23:51 UTC · re: Wincent Colaiuta · lore

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

Wincent Colaiuta <win@wincent.com> writes:
Show 11 quoted lines
> @@ -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.

Junio C Hamano· Dec 14, 2007, 02:10 UTC · re: Junio C Hamano · lore

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

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.

--
Show changes to diff.c +15 −17
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

← back to recent threads