Hi René
On 05/06/2024 09:38, René Scharfe wrote:
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().
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.
Thanks for the comprehensive commit message, I agree that it is much
less confusing to follow the exit code convention of diff(1). This is
looking good, I've left a couple of comments below.
+diff.trustExitCode::
+ If this boolean value is set to true then the `diff.external`
+ command is expected to return exit code 1 if it finds
+ significant changes and 0 if it doesn't, like diff(1). If it's
+ false then the `diff.external` command is expected to always
+ return exit code 0. Defaults to false.
I wonder if "significant changes" is a bit ambiguous and as Johannes
said it would be good to mention that other exit codes are errors. Perhaps
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 and 1 if they are not, like diff(1).
If it is false then the `diff.external` command is expected to
always return exit code 0. In both cases any other exit code
is considered to be an error. Defaults to false.
strvec_push(&cmd.args, pgm->cmd);
strvec_push(&cmd.args, name);
@@ -4406,7 +4424,10 @@ static void run_external_diff(const struct external_diff *pgm,
diff_free_filespec_data(one);
diff_free_filespec_data(two);
cmd.use_shell = 1;
Should we be redirecting stdout to /dev/null here when the user passes
--quiet?
- if (run_command(&cmd))
+ rc = run_command(&cmd);
+ if ((!pgm->trust_exit_code && !rc) || (pgm->trust_exit_code && rc == 1))
+ o->found_changes = 1;
+ else if (!pgm->trust_exit_code || rc)
die(_("external diff died, stopping at %s"), name);
This is a bit fiddly because we may, or may not trust the exit code but
the logic here looks good.
-check_external_exit_code 1 0 --exit-code
-check_external_exit_code 1 0 --quiet
-check_external_exit_code 128 1 --exit-code
-check_external_exit_code 1 1 --quiet # we don't even call the program
+check_external_exit_code 1 0 off --exit-code
+check_external_exit_code 1 0 off --quiet
+check_external_exit_code 128 1 off --exit-code
+check_external_exit_code 1 1 off --quiet # we don't even call the program
+
+check_external_exit_code 0 0 on --exit-code
+check_external_exit_code 0 0 on --quiet
+check_external_exit_code 1 1 on --exit-code
+check_external_exit_code 1 1 on --quiet
+check_external_exit_code 128 2 on --exit-code
+check_external_exit_code 128 2 on --quiet
It would be nice if the tests checked that --quiet does not produce any
output on stdout.
Best Wishes
Phillip
