Hi René
On 09/06/2024 08:35, René Scharfe wrote:
Changes since v2:
- rebase; config strings are no longer const
- document the handling of unexpected exit codes
- document that external diffs are skipped with --quiet and trustExitCode=off
- silence external diff output with --quiet
- check output in tests
- test diff runs without --exit-code and --quiet as well
- slightly untangle the exit code handling code to make it easier to read
- fix copy/paste error in documentation of diff.<driver>.trustExitCode
The changes in this version all look good. I've re-read the patches and
didn't spot any other issues so this is ready as far as I'm concerned.
Thanks
Phillip
t4020: test exit code with external diffs
userdiff: add and use struct external_diff
diff: let external diffs report that changes are uninteresting
Documentation/config/diff.txt | 18 +++++++++
Documentation/diff-options.txt | 5 +++
Documentation/git.txt | 10 +++++
Documentation/gitattributes.txt | 5 +++
diff.c | 68 +++++++++++++++++++++++++--------
t/t4020-diff-external.sh | 66 ++++++++++++++++++++++++++++++++
userdiff.c | 8 +++-
userdiff.h | 7 +++-
8 files changed, 168 insertions(+), 19 deletions(-)
Range-Diff gegen v2:
1: 118aba667b < -: ---------- t4020: test exit code with external diffs
-: ---------- > 1: d59f0c6fdf t4020: test exit code with external diffs
2: 0b4dabebe1 ! 2: 4ad160ab1f userdiff: add and use struct external_diff
@@ diff.c
@@ diff.c: static int diff_color_moved_ws_default;
static int diff_context_default = 3;
static int diff_interhunk_context_default;
- static const char *diff_word_regex_cfg;
--static const char *external_diff_cmd_cfg;
+ static char *diff_word_regex_cfg;
+-static char *external_diff_cmd_cfg;
+static struct external_diff external_diff_cfg;
- static const char *diff_order_file_cfg;
+ static char *diff_order_file_cfg;
int diff_auto_refresh_index = 1;
static int diff_mnemonic_prefix;
@@ diff.c: int git_diff_ui_config(const char *var, const char *value,
@@ userdiff.h: struct userdiff_funcname {
};
+struct external_diff {
-+ const char *cmd;
++ char *cmd;
+};
+
struct userdiff_driver {
const char *name;
-- const char *external;
+- char *external;
+ struct external_diff external;
- const char *algorithm;
+ char *algorithm;
int binary;
struct userdiff_funcname funcname;
3: 4d54ca8281 ! 3: 29c8d3b61a diff: let external diffs report that changes are uninteresting
@@ Commit message
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
@@ Documentation/config/diff.txt: diff.external::
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 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.
++ 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
@@ Documentation/config/diff.txt: diff.<driver>.command::
+diff.<driver>.trustExitCode::
+ If this boolean value is set to true then the
+ `diff.<driver>.command` 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.
++ 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.
+ ## Documentation/diff-options.txt ##
+@@ Documentation/diff-options.txt: 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[]
+
+
## Documentation/git.txt ##
@@ Documentation/git.txt: 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`::
-+ Setting this environment variable indicates the the program
-+ specified with `GIT_EXTERNAL_DIFF` returns exit code 1 if it
-+ finds significant changes and 0 if it doesn't, like diff(1).
-+ If it's not set, the program is expected to always return
-+ exit code 0.
++ 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.
@@ diff.c: 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;
+
+ /*
@@ diff.c: static void run_external_diff(const struct external_diff *pgm,
+ * external diff program lacks the ability to tell us whether
+ * it's empty then we consider it non-empty without even asking.
+ */
-+ if (!(o->output_format & DIFF_FORMAT_PATCH) && !pgm->trust_exit_code) {
++ if (!pgm->trust_exit_code && quiet) {
+ o->found_changes = 1;
+ return;
+ }
@@ diff.c: static void run_external_diff(const struct external_diff *pgm,
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) || (pgm->trust_exit_code && rc == 1))
++ 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 if (!pgm->trust_exit_code || rc)
++ else
die(_("external diff died, stopping at %s"), name);
remove_tempfile();
@@ diff.c: void diff_setup_done(struct diff_options *options)
if (options->flags.follow_renames)
## t/t4020-diff-external.sh ##
-@@ t/t4020-diff-external.sh: test_expect_success 'no diff with -diff' '
- check_external_exit_code () {
- expect_code=$1
- command_code=$2
-- option=$3
-+ trust_exit_code=$3
-+ option=$4
-
- command="exit $command_code;"
+@@ t/t4020-diff-external.sh: 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 $option" "
+ 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 $option
- "
+ test_expect_code $expect_code git diff $options >out 2>err &&
+ test_cmp $expect_out out &&
+@@ t/t4020-diff-external.sh: check_external_diff () {
- test_expect_success "$desc via diff.external with $option" "
+ 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 $option
- "
-@@ t/t4020-diff-external.sh: check_external_exit_code () {
+ test_expect_code $expect_code git diff $options >out 2>err &&
+ test_cmp $expect_out out &&
+@@ t/t4020-diff-external.sh: 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 $option
- "
- }
-
--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
+ git diff $options >out 2>err &&
+ test_cmp $expect_out out &&
+ test_cmp $expect_err err
+@@ t/t4020-diff-external.sh: 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_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
++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
@@ userdiff.h
@@ userdiff.h: struct userdiff_funcname {
struct external_diff {
- const char *cmd;
+ char *cmd;
+ unsigned trust_exit_code:1;
};
--
2.45.2
