]> git.ipfire.org Git - thirdparty/git.git/commitdiff
diff: let external diffs report that changes are uninteresting
authorRené Scharfe <l.s.r@web.de>
Sun, 9 Jun 2024 07:41:44 +0000 (09:41 +0200)
committerJunio C Hamano <gitster@pobox.com>
Mon, 10 Jun 2024 16:20:46 +0000 (09:20 -0700)
The options --exit-code and --quiet instruct git diff to indicate
whether it found any significant changes by exiting with code 1 if it
did and 0 if there were none.  Currently this doesn't work if external
diff programs are involved, as we have no way to learn what they found.

Add that ability in the form of the new configuration options
diff.trustExitCode and diff.<driver>.trustExitCode and the environment
variable GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE.  They pair with the config
options diff.external and diff.<driver>.command and the environment
variable GIT_EXTERNAL_DIFF, respectively.

The new options are off by default, keeping the old behavior.  Enabling
them indicates that the external diff returns exit code 1 if it finds
significant changes and 0 if it doesn't, like diff(1).

The name of the new options is taken from the git difftool and mergetool
options of similar purpose.  (There they enable passing on the exit code
of a diff tool and to infer whether a merge done by a merge tool is
successful.)

The new feature sets the diff flag diff_from_contents in
diff_setup_done() if we need the exit code and are allowed to call
external diffs.  This disables the optimization that avoids calling the
program with --quiet.  Add it back by skipping the call if the external
diff is not able to report empty diffs.  We can only do that check after
evaluating the file-specific attributes in run_external_diff().

If we do run the external diff with --quiet, send its output to
/dev/null.

I considered checking the output of the external diff to check whether
its empty.  It was added as 11be65cfa4 (diff: fix --exit-code with
external diff, 2024-05-05) and quickly reverted, as it does not work
with external diffs that do not write to stdout.  There's no reason why
a graphical diff tool would even need to write anything there at all.

I also considered using a non-zero exit code for empty diffs, which
could be done without adding new configuration options.  We'd need to
disable the optimization that allows git diff --quiet to skip calling
external diffs, though -- that might be quite surprising if graphical
diff programs are involved.  And assigning the opposite meaning of the
exit codes compared to diff(1) and git diff --exit-code to the external
diff can cause unnecessary confusion.

Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Documentation/config/diff.txt
Documentation/diff-options.txt
Documentation/git.txt
Documentation/gitattributes.txt
diff.c
t/t4020-diff-external.sh
userdiff.c
userdiff.h

index 5ce7b91f1d0d2356928ccc5e59195a6db205d81b..190bda17e51c57a00b25bf98afa038b8a89e4ea5 100644 (file)
@@ -79,6 +79,15 @@ diff.external::
        you want to use an external diff program only on a subset of
        your files, you might want to use linkgit:gitattributes[5] instead.
 
+diff.trustExitCode::
+       If this boolean value is set to true then the
+       `diff.external` command is expected to return exit code
+       0 if it considers the input files to be equal or 1 if it
+       considers them to be different, like `diff(1)`.
+       If it is set to false, which is the default, then the command
+       is expected to return exit code 0 regardless of equality.
+       Any other exit code causes Git to report a fatal error.
+
 diff.ignoreSubmodules::
        Sets the default value of --ignore-submodules. Note that this
        affects only 'git diff' Porcelain, and not lower level 'diff'
@@ -164,6 +173,15 @@ diff.<driver>.command::
        The custom diff driver command.  See linkgit:gitattributes[5]
        for details.
 
+diff.<driver>.trustExitCode::
+       If this boolean value is set to true then the
+       `diff.<driver>.command` command is expected to return exit code
+       0 if it considers the input files to be equal or 1 if it
+       considers them to be different, like `diff(1)`.
+       If it is set to false, which is the default, then the command
+       is expected to return exit code 0 regardless of equality.
+       Any other exit code causes Git to report a fatal error.
+
 diff.<driver>.xfuncname::
        The regular expression that the diff driver should use to
        recognize the hunk header.  A built-in pattern may also be used.
index 6b73daf54050231bec6889ecfa5fd5816b761fbc..cd0b81adbb69d1586b0c45a0168aa3ac2ef15303 100644 (file)
@@ -820,7 +820,11 @@ ifndef::git-log[]
 
 --quiet::
        Disable all output of the program. Implies `--exit-code`.
-       Disables execution of external diff helpers.
+       Disables execution of external diff helpers whose exit code
+       is not trusted, i.e. their respective configuration option
+       `diff.trustExitCode` or `diff.<driver>.trustExitCode` or
+       environment variable `GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE` is
+       false.
 endif::git-log[]
 endif::git-format-patch[]
 
index a31a70accab580c8cebe045279946bd861e18913..4489e2297a396b13108ed75a71e665236fb019cb 100644 (file)
@@ -644,6 +644,16 @@ parameter, <path>.
 For each path `GIT_EXTERNAL_DIFF` is called, two environment variables,
 `GIT_DIFF_PATH_COUNTER` and `GIT_DIFF_PATH_TOTAL` are set.
 
+`GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE`::
+       If this Boolean environment variable is set to true then the
+       `GIT_EXTERNAL_DIFF` command is expected to return exit code
+       0 if it considers the input files to be equal or 1 if it
+       considers them to be different, like `diff(1)`.
+       If it is set to false, which is the default, then the command
+       is expected to return exit code 0 regardless of equality.
+       Any other exit code causes Git to report a fatal error.
+
+
 `GIT_DIFF_PATH_COUNTER`::
        A 1-based counter incremented by one for every path.
 
index 4338d023d9a313edae0bae8b2ac8087a3d15a003..80cae17f37d1df7a66bbd88850b9722585c83391 100644 (file)
@@ -776,6 +776,11 @@ with the above configuration, i.e. `j-c-diff`, with 7
 parameters, just like `GIT_EXTERNAL_DIFF` program is called.
 See linkgit:git[1] for details.
 
+If the program is able to ignore certain changes (similar to
+`git diff --ignore-space-change`), then also set the option
+`trustExitCode` to true.  It is then expected to return exit code 1 if
+it finds significant changes and 0 if it doesn't.
+
 Setting the internal diff algorithm
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/diff.c b/diff.c
index 4b86c616317e3cb5673f08ce540b22b197bbd13c..39ba842049dc9469f6527cb658b4c641f8ffb90e 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -432,6 +432,10 @@ int git_diff_ui_config(const char *var, const char *value,
        }
        if (!strcmp(var, "diff.external"))
                return git_config_string(&external_diff_cfg.cmd, var, value);
+       if (!strcmp(var, "diff.trustexitcode")) {
+               external_diff_cfg.trust_exit_code = git_config_bool(var, value);
+               return 0;
+       }
        if (!strcmp(var, "diff.wordregex"))
                return git_config_string(&diff_word_regex_cfg, var, value);
        if (!strcmp(var, "diff.orderfile"))
@@ -556,6 +560,8 @@ static const struct external_diff *external_diff(void)
        if (done_preparing)
                return external_diff_ptr;
        external_diff_env.cmd = xstrdup_or_null(getenv("GIT_EXTERNAL_DIFF"));
+       if (git_env_bool("GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE", 0))
+               external_diff_env.trust_exit_code = 1;
        if (external_diff_env.cmd)
                external_diff_ptr = &external_diff_env;
        else if (external_diff_cfg.cmd)
@@ -4387,6 +4393,19 @@ static void run_external_diff(const struct external_diff *pgm,
 {
        struct child_process cmd = CHILD_PROCESS_INIT;
        struct diff_queue_struct *q = &diff_queued_diff;
+       int quiet = !(o->output_format & DIFF_FORMAT_PATCH);
+       int rc;
+
+       /*
+        * Trivial equality is handled by diff_unmodified_pair() before
+        * we get here.  If we don't need to show the diff and the
+        * external diff program lacks the ability to tell us whether
+        * it's empty then we consider it non-empty without even asking.
+        */
+       if (!pgm->trust_exit_code && quiet) {
+               o->found_changes = 1;
+               return;
+       }
 
        strvec_push(&cmd.args, pgm->cmd);
        strvec_push(&cmd.args, name);
@@ -4408,7 +4427,15 @@ static void run_external_diff(const struct external_diff *pgm,
        diff_free_filespec_data(one);
        diff_free_filespec_data(two);
        cmd.use_shell = 1;
-       if (run_command(&cmd))
+       cmd.no_stdout = quiet;
+       rc = run_command(&cmd);
+       if (!pgm->trust_exit_code && rc == 0)
+               o->found_changes = 1;
+       else if (pgm->trust_exit_code && rc == 0)
+               ; /* nothing */
+       else if (pgm->trust_exit_code && rc == 1)
+               o->found_changes = 1;
+       else
                die(_("external diff died, stopping at %s"), name);
 
        remove_tempfile();
@@ -4926,6 +4953,13 @@ void diff_setup_done(struct diff_options *options)
                options->flags.exit_with_status = 1;
        }
 
+       /*
+        * External diffs could declare non-identical contents equal
+        * (think diff --ignore-space-change).
+        */
+       if (options->flags.allow_external && options->flags.exit_with_status)
+               options->flags.diff_from_contents = 1;
+
        options->diff_path_counter = 0;
 
        if (options->flags.follow_renames)
index 4070523cdbedcd99cfc8a2f9844eaeb14c9a0377..3baa52a9bf653da9e45df720435cb23d0713e1db 100755 (executable)
@@ -177,15 +177,17 @@ check_external_diff () {
        expect_out=$2
        expect_err=$3
        command_code=$4
-       shift 4
+       trust_exit_code=$5
+       shift 5
        options="$@"
 
        command="echo output; exit $command_code;"
-       desc="external diff '$command'"
+       desc="external diff '$command' with trustExitCode=$trust_exit_code"
        with_options="${options:+ with }$options"
 
        test_expect_success "$desc via attribute$with_options" "
                test_config diff.foo.command \"$command\" &&
+               test_config diff.foo.trustExitCode $trust_exit_code &&
                echo \"file diff=foo\" >.gitattributes &&
                test_expect_code $expect_code git diff $options >out 2>err &&
                test_cmp $expect_out out &&
@@ -194,6 +196,7 @@ check_external_diff () {
 
        test_expect_success "$desc via diff.external$with_options" "
                test_config diff.external \"$command\" &&
+               test_config diff.trustExitCode $trust_exit_code &&
                >.gitattributes &&
                test_expect_code $expect_code git diff $options >out 2>err &&
                test_cmp $expect_out out &&
@@ -204,6 +207,7 @@ check_external_diff () {
                >.gitattributes &&
                test_expect_code $expect_code env \
                        GIT_EXTERNAL_DIFF=\"$command\" \
+                       GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE=$trust_exit_code \
                        git diff $options >out 2>err &&
                test_cmp $expect_out out &&
                test_cmp $expect_err err
@@ -216,14 +220,23 @@ test_expect_success 'setup output files' '
        echo "fatal: external diff died, stopping at file" >error
 '
 
-check_external_diff   0 output empty 0
-check_external_diff 128 output error 1
-
-check_external_diff   1 output empty 0 --exit-code
-check_external_diff 128 output error 1 --exit-code
-
-check_external_diff   1 empty  empty 0 --quiet
-check_external_diff   1 empty  empty 1 --quiet # we don't even call the program
+check_external_diff   0 output empty 0 off
+check_external_diff 128 output error 1 off
+check_external_diff   0 output empty 0 on
+check_external_diff   0 output empty 1 on
+check_external_diff 128 output error 2 on
+
+check_external_diff   1 output empty 0 off --exit-code
+check_external_diff 128 output error 1 off --exit-code
+check_external_diff   0 output empty 0 on  --exit-code
+check_external_diff   1 output empty 1 on  --exit-code
+check_external_diff 128 output error 2 on  --exit-code
+
+check_external_diff   1 empty  empty 0 off --quiet
+check_external_diff   1 empty  empty 1 off --quiet # we don't even call the program
+check_external_diff   0 empty  empty 0 on  --quiet
+check_external_diff   1 empty  empty 1 on  --quiet
+check_external_diff 128 empty  error 2 on  --quiet
 
 echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file
 
index f47e2d9f366b29fd65e08a54382f35c5229a8d54..6cdfb147c3efcd17c31cbc168d8d2387773a3e85 100644 (file)
@@ -446,6 +446,10 @@ int userdiff_config(const char *k, const char *v)
                return parse_tristate(&drv->binary, k, v);
        if (!strcmp(type, "command"))
                return git_config_string(&drv->external.cmd, k, v);
+       if (!strcmp(type, "trustexitcode")) {
+               drv->external.trust_exit_code = git_config_bool(k, v);
+               return 0;
+       }
        if (!strcmp(type, "textconv"))
                return git_config_string(&drv->textconv, k, v);
        if (!strcmp(type, "cachetextconv"))
index 2d59a8fc5643c0d4e3238b28e647539426a9e15c..f5cb53d0d4f446ff7d2439c999effbeb4c6dcefb 100644 (file)
@@ -13,6 +13,7 @@ struct userdiff_funcname {
 
 struct external_diff {
        char *cmd;
+       unsigned trust_exit_code:1;
 };
 
 struct userdiff_driver {