Re: [PATCH v3 0/3] diff: fix --exit-code with external diff

> 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

>    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