* [PATCH 0/2] Update "diff -w --exit-code" @ 2009-05-23 19:24 Junio C Hamano 2009-05-23 19:24 ` [PATCH 1/2] diff: change semantics of "ignore whitespace" options Junio C Hamano 0 siblings, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2009-05-23 19:24 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] diff: change semantics of "ignore whitespace" options 2009-05-23 19:24 [PATCH 0/2] Update "diff -w --exit-code" Junio C Hamano @ 2009-05-23 19:24 ` Junio C Hamano 2009-05-23 19:24 ` [PATCH 2/2] diff: Rename QUIET internal option to QUICK Junio C Hamano 2009-05-25 11:04 ` [PATCH 1/2] diff: change semantics of "ignore whitespace" options Jeff King 0 siblings, 2 replies; 5+ messages in thread From: Junio C Hamano @ 2009-05-23 19:24 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] diff: Rename QUIET internal option to QUICK 2009-05-23 19:24 ` [PATCH 1/2] diff: change semantics of "ignore whitespace" options Junio C Hamano @ 2009-05-23 19:24 ` Junio C Hamano 2009-05-25 11:04 ` [PATCH 1/2] diff: change semantics of "ignore whitespace" options Jeff King 1 sibling, 0 replies; 5+ messages in thread From: Junio C Hamano @ 2009-05-23 19:24 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] diff: change semantics of "ignore whitespace" options 2009-05-23 19:24 ` [PATCH 1/2] diff: change semantics of "ignore whitespace" options Junio C Hamano 2009-05-23 19:24 ` [PATCH 2/2] diff: Rename QUIET internal option to QUICK Junio C Hamano @ 2009-05-25 11:04 ` Jeff King 2009-05-25 18:48 ` Junio C Hamano 1 sibling, 1 reply; 5+ messages in thread From: Jeff King @ 2009-05-25 11:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] diff: change semantics of "ignore whitespace" options 2009-05-25 11:04 ` [PATCH 1/2] diff: change semantics of "ignore whitespace" options Jeff King @ 2009-05-25 18:48 ` Junio C Hamano 0 siblings, 0 replies; 5+ messages in thread From: Junio C Hamano @ 2009-05-25 18:48 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-05-25 18:48 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-05-23 19:24 [PATCH 0/2] Update "diff -w --exit-code" Junio C Hamano 2009-05-23 19:24 ` [PATCH 1/2] diff: change semantics of "ignore whitespace" options Junio C Hamano 2009-05-23 19:24 ` [PATCH 2/2] diff: Rename QUIET internal option to QUICK Junio C Hamano 2009-05-25 11:04 ` [PATCH 1/2] diff: change semantics of "ignore whitespace" options Jeff King 2009-05-25 18:48 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).