diff: ensure consistent diff behavior with ignore options

In git-diff, options like `-w` and `-I<regex>`, two files are considered
equivalent under the specified "ignore" rules, even when they are not
bit-for-bit identical. For options like `--raw`, `--name-status`,
and `--name-only`, git-diff deliberately compares only the SHA values
to determine whether two files are equivalent, for performance reasons.
As a result, a file shown in `git diff --name-status` may not appear
in `git diff --patch`.

To quickly determine whether two files are equivalent, add a helper
function diff_flush_patch_quietly() in diff.c. Add `.dry_run` field in
`struct diff_options`. When `.dry_run` is true, builtin_diff() returns
immediately upon finding any change. Call diff_flush_patch_quietly()
to determine if we should flush `--raw`, `--name-only` or `--name-status`
output.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Lidong Yan <yldhome2d2@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/diff.c b/diff.c
index dca87e164fb61576b9a0c6a832849df8abd041c6..cb04a9a6f26f5cbf163e5b459af6a27572464c1e 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -2444,6 +2444,15 @@ static int fn_out_consume(void *priv, char *line, unsigned long len)
        return 0;
 }
 
+static int quick_consume(void *priv, char *line UNUSED, unsigned long len UNUSED)
+{
+       struct emit_callback *ecbdata = priv;
+       struct diff_options *o = ecbdata->opt;
+
+       o->found_changes = 1;
+       return 1;
+}
+
 static void pprint_rename(struct strbuf *name, const char *a, const char *b)
 {
        const char *old_name = a;
@@ -3759,8 +3768,21 @@ static void builtin_diff(const char *name_a,
 
                if (o->word_diff)
                        init_diff_words_data(&ecbdata, o, one, two);
-               if (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume,
-                                 &ecbdata, &xpp, &xecfg))
+               if (o->dry_run) {
+                       /*
+                        * Unlike the !dry_run case, we need to ignore the
+                        * return value from xdi_diff_outf() here, because
+                        * xdi_diff_outf() takes non-zero return from its
+                        * callback function as a sign of error and returns
+                        * early (which is why we return non-zero from our
+                        * callback, quick_consume()).  Unfortunately,
+                        * xdi_diff_outf() signals an error by returning
+                        * non-zero.
+                        */
+                       xdi_diff_outf(&mf1, &mf2, NULL, quick_consume,
+                                     &ecbdata, &xpp, &xecfg);
+               } else if (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume,
+                                        &ecbdata, &xpp, &xecfg))
                        die("unable to generate diff for %s", one->path);
                if (o->word_diff)
                        free_diff_words_data(&ecbdata);
@@ -6150,6 +6172,22 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)
        run_diff(p, o);
 }
 
+/* return 1 if any change is found; otherwise, return 0 */
+static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o)
+{
+       int saved_dry_run = o->dry_run;
+       int saved_found_changes = o->found_changes;
+       int ret;
+
+       o->dry_run = 1;
+       o->found_changes = 0;
+       diff_flush_patch(p, o);
+       ret = o->found_changes;
+       o->dry_run = saved_dry_run;
+       o->found_changes |= saved_found_changes;
+       return ret;
+}
+
 static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,
                            struct diffstat_t *diffstat)
 {
@@ -6778,8 +6816,15 @@ 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))
+                               continue;
+
+                       if (options->flags.diff_from_contents &&
+                           !diff_flush_patch_quietly(p, options))
+                               continue;
+
+                       flush_one_pair(p, options);
                }
                separator++;
        }
@@ -6831,19 +6876,10 @@ void diff_flush(struct diff_options *options)
        if (output_format & DIFF_FORMAT_NO_OUTPUT &&
            options->flags.exit_with_status &&
            options->flags.diff_from_contents) {
-               /*
-                * run diff_flush_patch for the exit status. setting
-                * options->file to /dev/null should be safe, because we
-                * aren't supposed to produce any output anyway.
-                */
-               diff_free_file(options);
-               options->file = xfopen("/dev/null", "w");
-               options->close_file = 1;
-               options->color_moved = 0;
                for (i = 0; i < q->nr; i++) {
                        struct diff_filepair *p = q->queue[i];
                        if (check_pair_status(p))
-                               diff_flush_patch(p, options);
+                               diff_flush_patch_quietly(p, options);
                        if (options->found_changes)
                                break;
                }

diff --git a/diff.h b/diff.h
index 62e5768a9a379e4f319937e2fc8608a9c2cd3675..91b3e1c5cf791495a3618e4128438dc12337385b 100644 (file)
--- a/diff.h
+++ b/diff.h
@@ -400,6 +400,8 @@ struct diff_options {
        #define COLOR_MOVED_WS_ERROR (1<<0)
        unsigned color_moved_ws_handling;
 
+       bool dry_run;
+
        struct repository *repo;
        struct strmap *additional_path_headers;
 

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 8ebd170451d02516ecb005eefc7df9882e150483..cfeec239e0946c3bd4da3d60e35f5a9c2301df47 100755 (executable)
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -648,6 +648,19 @@ test_expect_success 'diff -I<regex>: detect malformed regex' '
        test_grep "invalid regex given to -I: " error
 '
 
+test_expect_success 'diff -I<regex>: ignore matching file' '
+       test_when_finished "git rm -f file1" &&
+       test_seq 50 >file1 &&
+       git add file1 &&
+       test_seq 50 | sed -e "s/13/ten and three/" -e "s/^[124-9].*/& /" >file1 &&
+
+       : >actual &&
+       git diff --raw --ignore-blank-lines -I"ten.*e" -I"^[124-9]" >>actual &&
+       git diff --name-only --ignore-blank-lines -I"ten.*e" -I"^[124-9]" >>actual &&
+       git diff --name-status --ignore-blank-lines -I"ten.*e" -I"^[124-9]" >>actual &&
+       test_grep ! "file1" actual
+'
+
 # check_prefix <patch> <src> <dst>
 # check only lines with paths to avoid dependency on exact oid/contents
 check_prefix () {

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 52e3e476ffa5a956e01eb2eb606130d54cd098ab..9de7f73f42b53461bfacbba61cceb00cbcba593f 100755 (executable)
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -11,12 +11,8 @@ test_description='Test special whitespace in diff engine.
 . "$TEST_DIRECTORY"/lib-diff.sh
 
 for opt_res in --patch --quiet -s --stat --shortstat --dirstat=lines \
-              --raw! --name-only! --name-status!
+              --raw --name-only --name-status
 do
-       opts=${opt_res%!} expect_failure=
-       test "$opts" = "$opt_res" ||
-               expect_failure="test_expect_code 1"
-
        test_expect_success "status with $opts (different)" '
                echo foo >x &&
                git add x &&
@@ -43,7 +39,7 @@ do
                echo foo >x &&
                git add x &&
                echo " foo" >x &&
-               $expect_failure git diff -w $opts --exit-code x
+               git diff -w $opts --exit-code x
        '
 done
 

diff --git a/xdiff-interface.h b/xdiff-interface.h
index 1ed430b622adf7bb25df454cc4388c5c7016817d..dfc55daddf46ed8815ea10aa245eece0918bcb8d 100644 (file)
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -28,9 +28,9 @@
  * from an error internal to xdiff, xdiff itself will see that
  * non-zero return and translate it to -1.
  *
- * See "diff_grep" in diffcore-pickaxe.c for a trick to work around
- * this, i.e. using the "consume_callback_data" to note the desired
- * early return.
+ * See "diff_grep" in diffcore-pickaxe.c and "quick_consume" in diff.c
+ * for a trick to work around this, i.e. using the "consume_callback_data"
+ * to note the desired early return.
  */
 typedef int (*xdiff_emit_line_fn)(void *, char *, unsigned long);
 typedef void (*xdiff_emit_hunk_fn)(void *data,