[PATCH 0/2] Update "diff -w --exit-code"

[PATCH 0/2] Update "diff -w --exit-code"

[Junio C Hamano]

Here is a re-roll of the earlier patch to change the exit status of "diff"
family of commands on whitespace only changes.

Earlier, "-w" and friends only affected the displayed of textual diff
output and never caused the commands to ignore the presense of
differences.  With this, the commands exit with zero status when the
changes are only about whitespaces that you are ignoring.

E.g. starting from a clean slate:

	$ echo ' a' >a-file ;# one space
	$ git add a-file
	$ echo '  a' >a-file ;# two spaces
	$ git diff --exit-code -w >/dev/null; echo $?

will give 0, instead of 1.

The fact that you have changes (i.e. the contents of a-file as a whole has
changed) is still reported in the textual part of the output by showing
the "diff --git a/a-file b/b-file" header and the "index objname..objname mode"
line.  This is not likely to change.

Junio C Hamano (2):
  diff: change semantics of "ignore whitespace" options
  diff: Rename QUIET internal option to QUICK

 builtin-log.c                |    2 +-
 builtin-rev-list.c           |    2 +-
 diff-lib.c                   |    4 +-
 diff.c                       |   39 ++++++++++++++++++++++---
 diff.h                       |    3 +-
 revision.c                   |    2 +-
 t/t4037-whitespace-status.sh |   63 ++++++++++++++++++++++++++++++++++++++++++
 tree-diff.c                  |    3 +-
 8 files changed, 106 insertions(+), 12 deletions(-)
 create mode 100755 t/t4037-whitespace-status.sh

[PATCH 1/2] diff: change semantics of "ignore whitespace" options

[Junio C Hamano]

Traditionally, the --ignore-whitespace* options have merely meant to tell
the diff output routine that some class of differences are not worth
showing in the textual diff output, so that the end user has easier time
to review the remaining (presumably more meaningful) changes.  These
options never affected the outcome of the command, given as the exit
status when the --exit-code option was in effect (either directly or
indirectly).

When you have only whitespace changes, however, you might expect

	git diff -b --exit-code

to report that there is _no_ change with zero exit status.

Change the semantics of --ignore-whitespace* options to mean more than
"omit showing the difference in text".

The exit status, when --exit-code is in effect, is computed by checking if
we found any differences at the path level, while diff frontends feed
filepairs to the diffcore engine.  When "ignore whitespace" options are in
effect, we defer this determination until the very end of diffcore
transformation.  We simply do not know until the textual diff is
generated, which comes very late in the pipeline.

When --quiet is in effect, various diff frontends optimize by breaking out
early from the loop that enumerates the filepairs, when we find the first
path level difference; when --ignore-whitespace* is used the above change
automatically disables this optimization.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c                       |   35 +++++++++++++++++++++--
 diff.h                       |    1 +
 t/t4037-whitespace-status.sh |   63 ++++++++++++++++++++++++++++++++++++++++++
 tree-diff.c                  |    3 +-
 4 files changed, 98 insertions(+), 4 deletions(-)
 create mode 100755 t/t4037-whitespace-status.sh

diff --git a/diff.c b/diff.c
index f06876b..e4ab696 100644
--- a/diff.c
+++ b/diff.c
@@ -2370,6 +2370,21 @@ int diff_setup_done(struct diff_options *options)
 	if (count > 1)
 		die("--name-only, --name-status, --check and -s are mutually exclusive");
 
+	/*
+	 * Most of the time we can say "there are changes"
+	 * only by checking if there are changed paths, but
+	 * --ignore-whitespace* options force us to look
+	 * inside contets.
+	 */
+
+	if ((XDF_IGNORE_WHITESPACE|
+	     XDF_IGNORE_WHITESPACE_CHANGE|
+	     XDF_IGNORE_WHITESPACE_AT_EOL) & options->xdl_opts) {
+		DIFF_OPT_SET(options, DIFF_FROM_CONTENTS);
+	} else {
+		DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);
+	}
+
 	if (DIFF_OPT_TST(options, FIND_COPIES_HARDER))
 		options->detect_rename = DIFF_DETECT_COPY;
 
@@ -3322,6 +3337,18 @@ free_queue:
 	q->nr = q->alloc = 0;
 	if (options->close_file)
 		fclose(options->file);
+
+	/*
+	 * Report the contents level differences with HAS_CHANGES;
+	 * diff_addremove/diff_change does not set the bit when
+	 * DIFF_FROM_CONTENTS is in effect (e.g. with -w).
+	 */
+	if (DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
+		if (options->found_changes)
+			DIFF_OPT_SET(options, HAS_CHANGES);
+		else
+			DIFF_OPT_CLR(options, HAS_CHANGES);
+	}
 }
 
 static void diffcore_apply_filter(const char *filter)
@@ -3458,7 +3485,7 @@ void diffcore_std(struct diff_options *options)
 	diff_resolve_rename_copy();
 	diffcore_apply_filter(options->filter);
 
-	if (diff_queued_diff.nr)
+	if (diff_queued_diff.nr && !DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))
 		DIFF_OPT_SET(options, HAS_CHANGES);
 	else
 		DIFF_OPT_CLR(options, HAS_CHANGES);
@@ -3518,7 +3545,8 @@ void diff_addremove(struct diff_options *options,
 		fill_filespec(two, sha1, mode);
 
 	diff_queue(&diff_queued_diff, one, two);
-	DIFF_OPT_SET(options, HAS_CHANGES);
+	if (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))
+		DIFF_OPT_SET(options, HAS_CHANGES);
 }
 
 void diff_change(struct diff_options *options,
@@ -3550,7 +3578,8 @@ void diff_change(struct diff_options *options,
 	fill_filespec(two, new_sha1, new_mode);
 
 	diff_queue(&diff_queued_diff, one, two);
-	DIFF_OPT_SET(options, HAS_CHANGES);
+	if (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))
+		DIFF_OPT_SET(options, HAS_CHANGES);
 }
 
 void diff_unmerge(struct diff_options *options,
diff --git a/diff.h b/diff.h
index 6616877..538e4f0 100644
--- a/diff.h
+++ b/diff.h
@@ -66,6 +66,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 #define DIFF_OPT_DIRSTAT_CUMULATIVE  (1 << 19)
 #define DIFF_OPT_DIRSTAT_BY_FILE     (1 << 20)
 #define DIFF_OPT_ALLOW_TEXTCONV      (1 << 21)
+#define DIFF_OPT_DIFF_FROM_CONTENTS  (1 << 22)
 #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/t4037-whitespace-status.sh b/t/t4037-whitespace-status.sh
new file mode 100755
index 0000000..a30b03b
--- /dev/null
+++ b/t/t4037-whitespace-status.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+test_description='diff --exit-code with whitespace'
+. ./test-lib.sh
+
+test_expect_success setup '
+	mkdir a b &&
+	echo >c &&
+	echo >a/d &&
+	echo >b/e &&
+	git add . &&
+	test_tick &&
+	git commit -m initial &&
+	echo " " >a/d &&
+	test_tick &&
+	git commit -a -m second &&
+	echo "  " >a/d &&
+	echo " " >b/e &&
+	git add a/d
+'
+
+test_expect_success 'diff-tree --exit-code' '
+	test_must_fail git diff --exit-code HEAD^ HEAD &&
+	test_must_fail git diff-tree --exit-code HEAD^ HEAD
+'
+
+test_expect_success 'diff-tree -b --exit-code' '
+	git diff -b --exit-code HEAD^ HEAD &&
+	git diff-tree -b -p --exit-code HEAD^ HEAD &&
+	git diff-tree -b --exit-code HEAD^ HEAD
+'
+
+test_expect_success 'diff-index --cached --exit-code' '
+	test_must_fail git diff --cached --exit-code HEAD &&
+	test_must_fail git diff-index --cached --exit-code HEAD
+'
+
+test_expect_success 'diff-index -b -p --cached --exit-code' '
+	git diff -b --cached --exit-code HEAD &&
+	git diff-index -b -p --cached --exit-code HEAD
+'
+
+test_expect_success 'diff-index --exit-code' '
+	test_must_fail git diff --exit-code HEAD &&
+	test_must_fail git diff-index --exit-code HEAD
+'
+
+test_expect_success 'diff-index -b -p --exit-code' '
+	git diff -b --exit-code HEAD &&
+	git diff-index -b -p --exit-code HEAD
+'
+
+test_expect_success 'diff-files --exit-code' '
+	test_must_fail git diff --exit-code &&
+	test_must_fail git diff-files --exit-code
+'
+
+test_expect_success 'diff-files -b -p --exit-code' '
+	git diff -b --exit-code &&
+	git diff-files -b -p --exit-code
+'
+
+test_done
diff --git a/tree-diff.c b/tree-diff.c
index edd8394..4b1e3f2 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -280,7 +280,8 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2, const char *base, stru
 	int baselen = strlen(base);
 
 	for (;;) {
-		if (DIFF_OPT_TST(opt, QUIET) && DIFF_OPT_TST(opt, HAS_CHANGES))
+		if (DIFF_OPT_TST(opt, QUIET) &&
+		    DIFF_OPT_TST(opt, HAS_CHANGES))
 			break;
 		if (opt->nr_paths) {
 			skip_uninteresting(t1, base, baselen, opt);
-- 
1.6.3.1.145.gb74d77

[PATCH 2/2] diff: Rename QUIET internal option to QUICK

[Junio C Hamano]

The option "QUIET" primarily meant "find if we have _any_ difference as
quick as possible and report", which means we often do not even have to
look at blobs if we know the trees are different by looking at the higher
level (e.g. "diff-tree A B").  As a side effect, because there is no point
showing one change that we happened to have found first, it also enables
NO_OUTPUT and EXIT_WITH_STATUS options, making the end result look quiet.

Rename the internal option to QUICK to reflect this better; it also makes
grepping the source tree much easier, as there are other kinds of QUIET
option everywhere.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-log.c      |    2 +-
 builtin-rev-list.c |    2 +-
 diff-lib.c         |    4 ++--
 diff.c             |    4 ++--
 diff.h             |    2 +-
 revision.c         |    2 +-
 tree-diff.c        |    2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 5eaec5d..80624f5 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -536,7 +536,7 @@ static int reopen_stdout(struct commit *commit, struct rev_info *rev)
 
 	get_patch_filename(commit, rev->nr, fmt_patch_suffix, &filename);
 
-	if (!DIFF_OPT_TST(&rev->diffopt, QUIET))
+	if (!DIFF_OPT_TST(&rev->diffopt, QUICK))
 		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
 
 	if (freopen(filename.buf, "w", stdout) == NULL)
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 38a8f23..61d3126 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -315,7 +315,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	memset(&info, 0, sizeof(info));
 	info.revs = &revs;
 
-	quiet = DIFF_OPT_TST(&revs.diffopt, QUIET);
+	quiet = DIFF_OPT_TST(&revs.diffopt, QUICK);
 	for (i = 1 ; i < argc; i++) {
 		const char *arg = argv[i];
 
diff --git a/diff-lib.c b/diff-lib.c
index a310fb2..a549ee6 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -73,7 +73,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		struct cache_entry *ce = active_cache[i];
 		int changed;
 
-		if (DIFF_OPT_TST(&revs->diffopt, QUIET) &&
+		if (DIFF_OPT_TST(&revs->diffopt, QUICK) &&
 			DIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))
 			break;
 
@@ -520,7 +520,7 @@ int index_differs_from(const char *def, int diff_flags)
 
 	init_revisions(&rev, NULL);
 	setup_revisions(0, NULL, &rev, def);
-	DIFF_OPT_SET(&rev.diffopt, QUIET);
+	DIFF_OPT_SET(&rev.diffopt, QUICK);
 	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
 	rev.diffopt.flags |= diff_flags;
 	run_diff_index(&rev, 1);
diff --git a/diff.c b/diff.c
index e4ab696..eb3a713 100644
--- a/diff.c
+++ b/diff.c
@@ -2445,7 +2445,7 @@ int diff_setup_done(struct diff_options *options)
 	 * to have found.  It does not make sense not to return with
 	 * exit code in such a case either.
 	 */
-	if (DIFF_OPT_TST(options, QUIET)) {
+	if (DIFF_OPT_TST(options, QUICK)) {
 		options->output_format = DIFF_FORMAT_NO_OUTPUT;
 		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
 	}
@@ -2636,7 +2636,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	else if (!strcmp(arg, "--exit-code"))
 		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
 	else if (!strcmp(arg, "--quiet"))
-		DIFF_OPT_SET(options, QUIET);
+		DIFF_OPT_SET(options, QUICK);
 	else if (!strcmp(arg, "--ext-diff"))
 		DIFF_OPT_SET(options, ALLOW_EXTERNAL);
 	else if (!strcmp(arg, "--no-ext-diff"))
diff --git a/diff.h b/diff.h
index 538e4f0..a7e7ccb 100644
--- a/diff.h
+++ b/diff.h
@@ -55,7 +55,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 #define DIFF_OPT_COLOR_DIFF          (1 <<  8)
 #define DIFF_OPT_COLOR_DIFF_WORDS    (1 <<  9)
 #define DIFF_OPT_HAS_CHANGES         (1 << 10)
-#define DIFF_OPT_QUIET               (1 << 11)
+#define DIFF_OPT_QUICK               (1 << 11)
 #define DIFF_OPT_NO_INDEX            (1 << 12)
 #define DIFF_OPT_ALLOW_EXTERNAL      (1 << 13)
 #define DIFF_OPT_EXIT_WITH_STATUS    (1 << 14)
diff --git a/revision.c b/revision.c
index 18b7ebb..1c114ab 100644
--- a/revision.c
+++ b/revision.c
@@ -800,7 +800,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->ignore_merges = 1;
 	revs->simplify_history = 1;
 	DIFF_OPT_SET(&revs->pruning, RECURSIVE);
-	DIFF_OPT_SET(&revs->pruning, QUIET);
+	DIFF_OPT_SET(&revs->pruning, QUICK);
 	revs->pruning.add_remove = file_add_remove;
 	revs->pruning.change = file_change;
 	revs->lifo = 1;
diff --git a/tree-diff.c b/tree-diff.c
index 4b1e3f2..6dec88b 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -280,7 +280,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2, const char *base, stru
 	int baselen = strlen(base);
 
 	for (;;) {
-		if (DIFF_OPT_TST(opt, QUIET) &&
+		if (DIFF_OPT_TST(opt, QUICK) &&
 		    DIFF_OPT_TST(opt, HAS_CHANGES))
 			break;
 		if (opt->nr_paths) {
-- 
1.6.3.1.145.gb74d77

Re: [PATCH 1/2] diff: change semantics of "ignore whitespace" options

[Jeff King]

On Sat, May 23, 2009 at 12:24:49PM -0700, Junio C Hamano wrote:

> When --quiet is in effect, various diff frontends optimize by breaking out
> early from the loop that enumerates the filepairs, when we find the first
> path level difference; when --ignore-whitespace* is used the above change
> automatically disables this optimization.

Both patches in this series look good to me, with one slight exception:

> +	if ((XDF_IGNORE_WHITESPACE|
> +	     XDF_IGNORE_WHITESPACE_CHANGE|
> +	     XDF_IGNORE_WHITESPACE_AT_EOL) & options->xdl_opts) {
> +		DIFF_OPT_SET(options, DIFF_FROM_CONTENTS);
> +	} else {
> +		DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);
> +	}

We have DIFF_XDL_TST these days; while it is currently equivalent to
what you do here, I thought part of the purpose was to abstract it to
make it simpler to later add master/slave magic.

-Peff

Re: [PATCH 1/2] diff: change semantics of "ignore whitespace" options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +	if ((XDF_IGNORE_WHITESPACE|
>> +	     XDF_IGNORE_WHITESPACE_CHANGE|
>> +	     XDF_IGNORE_WHITESPACE_AT_EOL) & options->xdl_opts) {
>> +		DIFF_OPT_SET(options, DIFF_FROM_CONTENTS);
>> +	} else {
>> +		DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);
>> +	}
>
> We have DIFF_XDL_TST these days;...

Heh, nobody seems to use it though.

Now the part reads

	if (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||
	    DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||
	    DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))
		DIFF_OPT_SET(options, DIFF_FROM_CONTENTS);
	else
		DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);

Thanks.