[RFC PATCH 0/2] diff whitespace tweaks

[RFC PATCH 0/2] diff whitespace tweaks

[Wincent Colaiuta]

[1/2] Use "whitespace" consistently

This one is a simple consistency fix that should go on top of "master".

[2/2] "diff --check" should affect exit status

This one is why I put "RFC" in the subject line. Basically, when I
discovered that "diff --check" didn't affect the exit status of the
command I thought it might be a bug, in which case this fix or one like
it should go in for 1.5.4. This patch also applies on top of "master".

But if you don't consider that to be a bug then I guess it's "next"
or even "pu" material and I can bring it back up after 1.5.4 is out.

Another reason why I label this as "RFC" is that the diff machinery
is fairly bulky and I was not familiar with it. So this patch is the
result of someone who has looked at the source for half an hour and
deduced enough to implement the desired behaviour; but somone who
truly groks the diff code may be able to think of a nicer
implementation.

[PATCH 1/2] Use "whitespace" consistently

[Wincent Colaiuta]

For consistency, change "white space" and "whitespaces" to
"whitespace", fixing a couple of adjacent grammar problems in the
docs.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 Documentation/diff-options.txt |   12 ++++++------
 Documentation/glossary.txt     |    2 +-
 diff.c                         |    4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index d0154bb..5d22b7b 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -175,19 +175,19 @@ endif::git-format-patch[]
 	Shorthand for "--text".
 
 --ignore-space-at-eol::
-	Ignore changes in white spaces at EOL.
+	Ignore changes in whitespace at EOL.
 
 --ignore-space-change::
-	Ignore changes in amount of white space.  This ignores white
-	space at line end, and consider all other sequences of one or
-	more white space characters to be equivalent.
+	Ignore changes in amount of whitespace.  This ignores whitespace
+	at line end, and considers all other sequences of one or
+	more whitespace characters to be equivalent.
 
 -b::
 	Shorthand for "--ignore-space-change".
 
 --ignore-all-space::
-	Ignore white space when comparing lines.  This ignores
-	difference even if one line has white space where the other
+	Ignore whitespace when comparing lines.  This ignores
+	differences even if one line has whitespace where the other
 	line has none.
 
 -w::
diff --git a/Documentation/glossary.txt b/Documentation/glossary.txt
index fc18744..f9f5a32 100644
--- a/Documentation/glossary.txt
+++ b/Documentation/glossary.txt
@@ -245,7 +245,7 @@ This commit is referred to as a "merge commit", or sometimes just a
 	of the object's contents using the Secure Hash Algorithm
 	1 and usually represented by the 40 character hexadecimal encoding of
 	the <<def_hash,hash>> of the object (possibly followed by
-	a white space).
+	whitespace).
 
 [[def_object_type]]object type::
 	One of the identifiers
diff --git a/diff.c b/diff.c
index f780e3e..e74a303 100644
--- a/diff.c
+++ b/diff.c
@@ -1015,7 +1015,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
 		if (line[i - 1] == '\t' && spaces)
 			space_before_tab = 1;
 
-		/* check white space at line end */
+		/* check whitespace at line end */
 		if (line[len - 1] == '\n')
 			len--;
 		if (isspace(line[len - 1]))
@@ -1029,7 +1029,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
 					putchar(',');
 			}
 			if (white_space_at_end)
-				printf("white space at end");
+				printf("whitespace at end");
 			printf(":%s ", reset);
 			emit_line_with_ws(1, set, reset, ws, line, len,
 					  data->ws_rule);
-- 
1.5.3.7.1116.gf11de

[PATCH 2/2] "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            |    2 ++
 builtin-diff.c                 |    3 ++-
 diff.c                         |   37 ++++++++++++++++++++++++-------------
 diff.h                         |    1 +
 t/t4015-diff-whitespace.sh     |   34 ++++++++++++++++++++++++++++++++++
 8 files changed, 70 insertions(+), 15 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..556c31d 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c
@@ -134,6 +134,8 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 		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 1b61599..6aeea93 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -365,7 +365,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 e74a303..e89c7ce 100644
--- a/diff.c
+++ b/diff.c
@@ -996,6 +996,7 @@ struct checkdiff_t {
 	const char *filename;
 	int lineno, color_diff;
 	unsigned ws_rule;
+	int status;
 };
 
 static void checkdiff_consume(void *priv, char *line, unsigned long len)
@@ -1022,6 +1023,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");
@@ -1412,7 +1414,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)
 {
@@ -1420,7 +1422,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;
@@ -1449,6 +1451,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)
@@ -2033,14 +2036,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;
@@ -2049,7 +2052,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)
@@ -2079,7 +2082,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)
@@ -2546,17 +2554,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)
@@ -2675,16 +2683,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)
@@ -2923,8 +2934,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 79fdff3..c110201 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -117,4 +117,38 @@ EOF
 git diff -b > out
 test_expect_success 'another test, with -b' 'git diff expect out'
 
+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' '
+
+	echo " 	foo();" > x &&
+	git diff --check
+
+'
+
+test_expect_failure '--check and --exit-code are exclusive' '
+
+	git diff --check --exit-code
+
+'
+
+test_expect_failure '--check and --quiet are exclusive' '
+
+	git diff --check --quiet
+
+'
+
 test_done
-- 
1.5.3.7.1116.gf11de

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

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> "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.

The primary use of this option used to be to see where the breakages are
in order to fix them up (IOW, the option was not about seeing if
breakage exists or not), but that is much easier with --color output
these days.

It is probably a good idea to do this, although I do not care much about
it either way.  It probably could replace the sample pre-commit hook
currently implemented in Perl.  As I happen to be fairly familiar with
the area, I'll take a look at the patch if/when I find time.

Thanks.

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

[Wincent Colaiuta]

El 12/12/2007, a las 10:16, Junio C Hamano escribió:

> Wincent Colaiuta <win@wincent.com> writes:
>
>> "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.
>
> The primary use of this option used to be to see where the breakages  
> are
> in order to fix them up (IOW, the option was not about seeing if
> breakage exists or not), but that is much easier with --color output
> these days.
>
> It is probably a good idea to do this, although I do not care much  
> about
> it either way.  It probably could replace the sample pre-commit hook
> currently implemented in Perl.

Actually, that's exactly why I prepared this patch; I was looking at  
the pre-commit hook and though, surely there must be a simpler way  
than this... And was surprised when I saw that "git diff --check"  
didn't alter the exit code...

> As I happen to be fairly familiar with
> the area, I'll take a look at the patch if/when I find time.

Thanks. I'm about to resend this patch with more tests, and one fix  
discovered while adding the tests ("git diff-tree" was only setting  
the exit status with --stdin set), as well as a new version of the pre- 
commit hook.

One nice thing about this change is that when "diff --check" learns  
about the new core.whitespace stuff, the hook will automatically  
inherit the new behaviour.

Cheers,
Wincent