Possible git-diff bug when using exit-code with diff filters

Possible git-diff bug when using exit-code with diff filters

[German Lashevich]

> What did you do before the bug happened? (Steps to reproduce your issue)

I configured a diff filter via gitattributes to use a custom script that,
sometimes, can change the files being compared in a way that there are no
differences between them.
Then I used `git diff --exit-code` on the changed file.

> What did you expect to happen? (Expected behavior)

I expected `git diff --exit-code` to return 0, since there are no differences
between the files after the filter is applied.

> What happened instead? (Actual behavior)

`git diff --exit-code` correctly produces no output, but returns 1.

> What's different between what you expected and what actually happened?

The difference is that `git diff --exit-code`, instead of returning 0, returns
1 even when there is no output.

> Anything else you want to add:

I have prepared a repository with a test case that reproduces the issue.
You can find it at https://github.com/Zebradil/git-diff-exit-code-bug-repro
The Readme file in the repository contains instructions on how to reproduce
the issue.

[System Info]
git version: 2.44.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.8.7-arch1-1 #1 SMP PREEMPT_DYNAMIC Wed, 17 Apr 2024
15:20:28 +0000 x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.39
$SHELL (typically, interactive shell): /bin/zsh

Best regards,
German Lashevich

Re: Possible git-diff bug when using exit-code with diff filters

[René Scharfe]

Am 20.04.24 um 03:13 schrieb German Lashevich:
>> What did you do before the bug happened? (Steps to reproduce your issue)
>
> I configured a diff filter via gitattributes to use a custom script that,
> sometimes, can change the files being compared in a way that there are no
> differences between them.
> Then I used `git diff --exit-code` on the changed file.
>
>> What did you expect to happen? (Expected behavior)
>
> I expected `git diff --exit-code` to return 0, since there are no differences
> between the files after the filter is applied.
>
>> What happened instead? (Actual behavior)
>
> `git diff --exit-code` correctly produces no output, but returns 1.
>
>> What's different between what you expected and what actually happened?
>
> The difference is that `git diff --exit-code`, instead of returning 0, returns
> 1 even when there is no output.
>
>> Anything else you want to add:
>
> I have prepared a repository with a test case that reproduces the issue.
> You can find it at https://github.com/Zebradil/git-diff-exit-code-bug-repro
> The Readme file in the repository contains instructions on how to reproduce
> the issue.

You can more easily reproduce it by setting the environment variable
GIT_EXTERNAL_DIFF or the configuration setting diff.external -- no
attributes needed.

The output of external diffs does not influence the exit code currently.
We don't even have a way to find out if there was any output.

The patch below pipes it through a buffer and sets found_changes only if
something was written.  It fixes the error code for external diffs
producing no output, but breaks several am, pull and merge tests (t4150,
t4151, t5520, t6424, t6430, t7611).  So diff_from_contents doesn't work
quite as I expected.

And slurping in the whole output of external diffs is inefficient, of
course.  That could be fixed later by using a small buffer and writing
directly as we read.

René


diff --git a/diff.c b/diff.c
index 108c187577..76631644e5 100644
--- a/diff.c
+++ b/diff.c
@@ -40,6 +40,7 @@
 #include "setup.h"
 #include "strmap.h"
 #include "ws.h"
+#include "write-or-die.h"

 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -4404,8 +4405,18 @@ static void run_external_diff(const char *pgm,
 	diff_free_filespec_data(one);
 	diff_free_filespec_data(two);
 	cmd.use_shell = 1;
-	if (run_command(&cmd))
-		die(_("external diff died, stopping at %s"), name);
+	if (o->flags.exit_with_status) {
+		struct strbuf out = STRBUF_INIT;
+		if (capture_command(&cmd, &out, 0))
+			die(_("external diff died, stopping at %s"), name);
+		if (out.len)
+			o->found_changes = 1;
+		write_or_die(1, out.buf, out.len);
+		strbuf_release(&out);
+	} else {
+		if (run_command(&cmd))
+			die(_("external diff died, stopping at %s"), name);
+	}

 	remove_tempfile();
 }
@@ -4851,6 +4862,7 @@ void diff_setup_done(struct diff_options *options)
 	 */

 	if ((options->xdl_opts & XDF_WHITESPACE_FLAGS) ||
+	    options->flags.exit_with_status ||
 	    options->ignore_regex_nr)
 		options->flags.diff_from_contents = 1;
 	else
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index fdd865f7c3..26430ca66b 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -172,6 +172,14 @@ test_expect_success 'no diff with -diff' '
 	grep Binary out
 '

+test_expect_success 'diff.external and --exit-code with output' '
+	test_expect_code 1 git -c diff.external=echo diff --exit-code
+'
+
+test_expect_success 'diff.external and --exit-code without output' '
+	git -c diff.external=true diff --exit-code
+'
+
 echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file

 test_expect_success 'force diff with "diff"' '

Re: Possible git-diff bug when using exit-code with diff filters

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> You can more easily reproduce it by setting the environment variable
> GIT_EXTERNAL_DIFF or the configuration setting diff.external -- no
> attributes needed.

Indeed.

A much simpler fix may be to declare that these two features are
imcompatible and fail the execution upfront, instead of just
silently ignoring one of the two options.

As a person who is very much used to the external diff not
contributing to the exit status (who also invented the external diff
driver interface), I would be a wrong person to judge if such a
simplified approach is desirable, of course, but just throwing it
out as a food for thought.

Thanks.

RE: Possible git-diff bug when using exit-code with diff filters

[rsbecker]

On Sunday, April 21, 2024 2:18 PM, Junio C Hamano wrote:
>René Scharfe <l.s.r@web.de> writes:
>
>> You can more easily reproduce it by setting the environment variable
>> GIT_EXTERNAL_DIFF or the configuration setting diff.external -- no
>> attributes needed.
>
>Indeed.
>
>A much simpler fix may be to declare that these two features are imcompatible and
>fail the execution upfront, instead of just silently ignoring one of the two options.
>
>As a person who is very much used to the external diff not contributing to the exit
>status (who also invented the external diff driver interface), I would be a wrong
>person to judge if such a simplified approach is desirable, of course, but just
>throwing it out as a food for thought.

My suggestion would be to keep with a priority approach, where GIT_EXTERNAL_DIFF overrides diff.external, assuming they set hold to the same specification (the git-config page implies they do) and GIT_EXTERNAL_DIFF overrides diff.external as I would expect. The behaviour of the two should be identical and definitely not in conflict. From my experience with this feature, and what I would prefer to preserve, is a formal separation between the diff engine and the underlying textconv. If the textconv processor fails in some way, the git diff should also fail, but the exit code should reflect that git diff failed, not the status of the textconv processor. If the external diff wrapper fails, the completion should be passed up since git would/should/could delegate the diff processing to the external engine. The only real completion code requirement I see is that 0 = no difference, 1 = difference exists, other is an indeterminate failure.

I might be restating the obvious, but this is an important capability, at least to me and my teams. We use this extensively for binary content diffs. It is also pretty important for structured document (e.g., MS-Word, RTF, PDF) and executable diffs that git does not handle out of the box.
--Randall

Re: Possible git-diff bug when using exit-code with diff filters

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> On Sunday, April 21, 2024 2:18 PM, Junio C Hamano wrote:
>>René Scharfe <l.s.r@web.de> writes:
>>
>>> You can more easily reproduce it by setting the environment variable
>>> GIT_EXTERNAL_DIFF or the configuration setting diff.external -- no
>>> attributes needed.
>>
>>Indeed.
>>
>>A much simpler fix may be to declare that these two features are imcompatible and
>>fail the execution upfront, instead of just silently ignoring one of the two options.
>>
>>As a person who is very much used to the external diff not contributing to the exit
>>status (who also invented the external diff driver interface), I would be a wrong
>>person to judge if such a simplified approach is desirable, of course, but just
>>throwing it out as a food for thought.
>
> My suggestion would be to keep with a priority approach, where
> GIT_EXTERNAL_DIFF overrides diff.external, assuming they set hold
> to the same specification (the git-config page implies they do)
> and GIT_EXTERNAL_DIFF overrides diff.external as I would expect.

Nobody in this discussion thread is hinting to change that, so I am
a bit confused where the above suggestion comes from...

RE: Possible git-diff bug when using exit-code with diff filters

[rsbecker]

On Sunday, April 21, 2024 3:09 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>> On Sunday, April 21, 2024 2:18 PM, Junio C Hamano wrote:
>>>René Scharfe <l.s.r@web.de> writes:
>>>
>>>> You can more easily reproduce it by setting the environment variable
>>>> GIT_EXTERNAL_DIFF or the configuration setting diff.external -- no
>>>> attributes needed.
>>>
>>>Indeed.
>>>
>>>A much simpler fix may be to declare that these two features are
>>>imcompatible and fail the execution upfront, instead of just silently ignoring one
>of the two options.
>>>
>>>As a person who is very much used to the external diff not
>>>contributing to the exit status (who also invented the external diff
>>>driver interface), I would be a wrong person to judge if such a
>>>simplified approach is desirable, of course, but just throwing it out as a food for
>thought.
>>
>> My suggestion would be to keep with a priority approach, where
>> GIT_EXTERNAL_DIFF overrides diff.external, assuming they set hold to
>> the same specification (the git-config page implies they do) and
>> GIT_EXTERNAL_DIFF overrides diff.external as I would expect.
>
>Nobody in this discussion thread is hinting to change that, so I am a bit confused
>where the above suggestion comes from...

I must have misinterpreted. Please ignore my suggestion.

Re: Possible git-diff bug when using exit-code with diff filters

[René Scharfe]

Am 21.04.24 um 20:17 schrieb Junio C Hamano:
> A much simpler fix may be to declare that these two features are
> imcompatible and fail the execution upfront, instead of just
> silently ignoring one of the two options.

It would not be *that* simple -- if we want to error out upfront we'd
have to evaluate the attributes of all files (or all changed files)
first to see whether they require an external diff.

Reporting the incompatibility in the middle of a diff would be easier,
but I don't see why we shouldn't support that combination.  It takes
some effort, sure, but not prohibitively much.

René

Re: Possible git-diff bug when using exit-code with diff filters

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 21.04.24 um 20:17 schrieb Junio C Hamano:
>> A much simpler fix may be to declare that these two features are
>> imcompatible and fail the execution upfront, instead of just
>> silently ignoring one of the two options.
>
> It would not be *that* simple -- if we want to error out upfront we'd
> have to evaluate the attributes of all files (or all changed files)
> first to see whether they require an external diff.

You're absolutely right.

> Reporting the incompatibility in the middle of a diff would be easier,
> but I don't see why we shouldn't support that combination.  It takes
> some effort, sure, but not prohibitively much.

OK.  Thanks.

[PATCH 1/2] diff: report unmerged paths as changes in run_diff_cmd()

[René Scharfe]

You can ask the diff machinery to let the exit code indicate whether
there are changes, e.g. with --quiet.  It as two ways to calculate that
bit: The quick one assumes blobs with different hashes have different
content, and the more elaborate way actually compares the contents,
possibly applying transformations like ignoring whitespace.

The quick way considers an unmerged file to be a change and reports
exit code 1, which makes sense.

The slower path uses the struct diff_options member found_changes to
indicate whether the blobs differ even with the transformations applied.
It's not set for unmerged files, though, resulting in exit code 0.

Set found_changes in run_diff_cmd() for unmerged files, for a consistent
exit code of 1 if there's an unmerged file, regardless of whether
whitespace is ignored.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 diff.c                   | 1 +
 t/t4046-diff-unmerged.sh | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/diff.c b/diff.c
index 108c187577..ded9ac70df 100644
--- a/diff.c
+++ b/diff.c
@@ -4555,6 +4555,7 @@ static void run_diff_cmd(const char *pgm,
 			     o, complete_rewrite);
 	} else {
 		fprintf(o->file, "* Unmerged path %s\n", name);
+		o->found_changes = 1;
 	}
 }

diff --git a/t/t4046-diff-unmerged.sh b/t/t4046-diff-unmerged.sh
index ffaf69335f..c606ee4c40 100755
--- a/t/t4046-diff-unmerged.sh
+++ b/t/t4046-diff-unmerged.sh
@@ -96,4 +96,12 @@ test_expect_success 'diff --stat' '
 	test_cmp diff-stat.expect diff-stat.actual
 '

+test_expect_success 'diff --quiet' '
+	test_expect_code 1 git diff --cached --quiet
+'
+
+test_expect_success 'diff --quiet --ignore-all-space' '
+	test_expect_code 1 git diff --cached --quiet --ignore-all-space
+'
+
 test_done
--
2.45.0

[PATCH 2/2] diff: fix --exit-code with external diff

[René Scharfe]

You can ask the diff machinery to let the exit code indicate whether
there are changes, e.g. with --exit-code.  It as two ways to calculate
that bit: The quick one assumes blobs with different hashes have
different content, and the more elaborate way actually compares the
contents, possibly applying transformations like ignoring whitespace.

Always use the slower path by setting the flag diff_from_contents,
because any of the files could have an external diff driver set via an
attribute, which might consider binary differences irrelevant, like e.g.
diff -b.

And don't stop walking that path in diff_flush() just because
output_format is unset.  Instead run diff_flush_patch() with output
redirected to /dev/null to get the exit status, like we already do for
DIFF_FORMAT_NO_OUTPUT.  That's consistent with the comments in diff.h:

   /* Same as output_format = 0 but we know that -s flag was given
    * and we should not give default value to output_format.
    */
   #define DIFF_FORMAT_NO_OUTPUT	0x0800

An unset output_format is given e.g. by repo_index_has_changes().  We
could set it right there, but there may be other places, so simply let
diff_flush() handle it for all of them consistently.

Finally, capture the output of the external diff and only register a
change by setting found_changes if the program wrote anything.

Reported-by: German Lashevich <german.lashevich@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 diff.c                   | 33 ++++++++++++++++++++++++++++++---
 t/t4020-diff-external.sh |  8 ++++++++
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index ded9ac70df..cd3c8a3978 100644
--- a/diff.c
+++ b/diff.c
@@ -40,6 +40,7 @@
 #include "setup.h"
 #include "strmap.h"
 #include "ws.h"
+#include "write-or-die.h"

 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -4404,8 +4405,33 @@ static void run_external_diff(const char *pgm,
 	diff_free_filespec_data(one);
 	diff_free_filespec_data(two);
 	cmd.use_shell = 1;
-	if (run_command(&cmd))
-		die(_("external diff died, stopping at %s"), name);
+	if (o->flags.diff_from_contents) {
+		int got_output = 0;
+		cmd.out = -1;
+		if (start_command(&cmd))
+			die(_("external diff died, stopping at %s"), name);
+		for (;;) {
+			char buffer[8192];
+			ssize_t len = xread(cmd.out, buffer, sizeof(buffer));
+			if (!len)
+				break;
+			if (len < 0)
+				die(_("unable to read from external diff,"
+				      " stopping at %s"), name);
+			got_output = 1;
+			if (write_in_full(1, buffer, len) < 0)
+				die(_("unable to write output of external diff,"
+				      " stopping at %s"), name);
+		}
+		close(cmd.out);
+		if (finish_command(&cmd))
+			die(_("external diff died, stopping at %s"), name);
+		if (got_output)
+			o->found_changes = 1;
+	} else {
+		if (run_command(&cmd))
+			die(_("external diff died, stopping at %s"), name);
+	}

 	remove_tempfile();
 }
@@ -4852,6 +4878,7 @@ void diff_setup_done(struct diff_options *options)
 	 */

 	if ((options->xdl_opts & XDF_WHITESPACE_FLAGS) ||
+	    options->flags.exit_with_status ||
 	    options->ignore_regex_nr)
 		options->flags.diff_from_contents = 1;
 	else
@@ -6742,7 +6769,7 @@ void diff_flush(struct diff_options *options)
 	if (output_format & DIFF_FORMAT_CALLBACK)
 		options->format_callback(q, options, options->format_callback_data);

-	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
+	if ((!output_format || output_format & DIFF_FORMAT_NO_OUTPUT) &&
 	    options->flags.exit_with_status &&
 	    options->flags.diff_from_contents) {
 		/*
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index fdd865f7c3..26430ca66b 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -172,6 +172,14 @@ test_expect_success 'no diff with -diff' '
 	grep Binary out
 '

+test_expect_success 'diff.external and --exit-code with output' '
+	test_expect_code 1 git -c diff.external=echo diff --exit-code
+'
+
+test_expect_success 'diff.external and --exit-code without output' '
+	git -c diff.external=true diff --exit-code
+'
+
 echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file

 test_expect_success 'force diff with "diff"' '
--
2.45.0

Re: [PATCH 2/2] diff: fix --exit-code with external diff

[Phillip Wood]

Hi René

Thanks for working on this

On 05/05/2024 11:20, René Scharfe wrote:
> Finally, capture the output of the external diff and only register a
> change by setting found_changes if the program wrote anything.

I worry that this will lead to regression reports like 
https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/ 
as the external diff will not see a terminal on stdout anymore. I'm not 
really clear why we ignore the exit code of the external diff command. 
Merge strategies are expected to exit 0 on success, 1 when there are 
conflicts and another non-zero value for other errors - it would be nice 
to do something similar here where 1 means "there were differences" but 
it is probably too late to do that without a config value to indicate 
that we should trust the exit code.

Best Wishes

Phillip

Re: [PATCH 2/2] diff: fix --exit-code with external diff

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> Finally, capture the output of the external diff and only register a
>> change by setting found_changes if the program wrote anything.
>
> I worry that this will lead to regression reports like
> https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/
> as the external diff will not see a terminal on stdout anymore.

Hmph.  If it were Git that the external diff script eventually
invokes that changes the behaviour (e.g. color and use of pager)
based on the tty-ness of the output, we could use tricks like
GIT_PAGER_IN_USE to propagate tty-ness down to the subprocess,
but Git is not the only thing involved here, so it is not a very
good general solution.

> I'm
> not really clear why we ignore the exit code of the external diff
> command. Merge strategies are expected to exit 0 on success, 1 when
> there are conflicts and another non-zero value for other errors - it
> would be nice to do something similar here where 1 means "there were
> differences" but it is probably too late to do that without a config
> value to indicate that we should trust the exit code.

Yeah, that thought crossed my mind.  If we were designing the system
from scratch today, that would certainly be what we would do.  I
suspect that it is because external diff drivers were invented way
before "diff --exit-code" was introduced.

Thanks.

Re: [PATCH 2/2] diff: fix --exit-code with external diff

[René Scharfe]

Am 05.05.24 um 17:25 schrieb Phillip Wood:
> On 05/05/2024 11:20, René Scharfe wrote:
>> Finally, capture the output of the external diff and only register a
>> change by setting found_changes if the program wrote anything.
>
> I worry that this will lead to regression reports like
> https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/
> as the external diff will not see a terminal on stdout anymore.

So external diffs might no longer color their output if invoked using
the option --exit-code.  I assume this can be worked around by adding an
option like --color=always to the diff command.  This warrants a note in
the docs at the very least, though.

But thinking about it, the external diff could be a GUI program that
doesn't write to its stdout at all.  Or it could write something if the
files' contents match and stay silent if there are differences, weird as
that may be.

> I'm not really clear why we ignore the exit code of the external diff
> command.

Historical reasons, I guess.

> Merge strategies are expected to exit 0 on success, 1 when there are
> conflicts and another non-zero value for other errors - it would be
> nice to do something similar here where 1 means "there were
> differences" but it is probably too late to do that without a config
> value to indicate that we should trust the exit code.
Right, such a diff command protocol v2 would not need to pipe the
output through an inspection loop.  Sounds like a good idea.  It's
unfortunate that it would increase the configuration surface, which is
not in an acute need to expand.  We could advertise the new option when
dying due to the unsupported combination of --exit-code and external
diff, but that's in equal parts helpful and obnoxious, I feel.

René

Re: [PATCH 2/2] diff: fix --exit-code with external diff

[phillip.wood123]

Hi René

On 06/05/2024 19:23, René Scharfe wrote:
> Am 05.05.24 um 17:25 schrieb Phillip Wood:
>> Merge strategies are expected to exit 0 on success, 1 when there are
>> conflicts and another non-zero value for other errors - it would be
>> nice to do something similar here where 1 means "there were
>> differences" but it is probably too late to do that without a config
>> value to indicate that we should trust the exit code.
> Right, such a diff command protocol v2 would not need to pipe the
> output through an inspection loop.  Sounds like a good idea.  It's
> unfortunate that it would increase the configuration surface, which is
> not in an acute need to expand.  We could advertise the new option when
> dying due to the unsupported combination of --exit-code and external
> diff, but that's in equal parts helpful and obnoxious, I feel.

Yes, diff dying would be annoying but the message would be useful.

Thinking about the external diff and some of the other diff options I 
wonder what we should do when options like "--quiet" and "--name-only" 
are combined with an external diff (I haven't checked the current 
behavior). If we introduced a diff command protocol v2 we could include 
a way to pass through "--quiet" though maybe just redirecting the stdout 
of the external command to /dev/null and using the exit code would be 
sufficient.

Best Wishes

Phillip

P.S. I haven't forgotten about our unit-test discussion but I'm afraid 
it will probably be the middle of next month before I have time to reply.

Re: [PATCH 2/2] diff: fix --exit-code with external diff

[René Scharfe]

Am 08.05.24 um 17:25 schrieb phillip.wood123@gmail.com:
> Hi René
>
> On 06/05/2024 19:23, René Scharfe wrote:
>> Am 05.05.24 um 17:25 schrieb Phillip Wood:
>>> Merge strategies are expected to exit 0 on success, 1 when there are
>>> conflicts and another non-zero value for other errors - it would be
>>> nice to do something similar here where 1 means "there were
>>> differences" but it is probably too late to do that without a config
>>> value to indicate that we should trust the exit code.
>> Right, such a diff command protocol v2 would not need to pipe the
>> output through an inspection loop.  Sounds like a good idea.  It's
>> unfortunate that it would increase the configuration surface, which is
>> not in an acute need to expand.  We could advertise the new option when
>> dying due to the unsupported combination of --exit-code and external
>> diff, but that's in equal parts helpful and obnoxious, I feel.
>
> Yes, diff dying would be annoying but the message would be useful.

Having poked at it a bit more, I wonder if we need to add some further
nuance/trick to avoid having to reject certain combinations of options.

Currently external diffs can't signal content equality.  That doesn't
matter for trivial equality (where content and mode of the two files
match), as this case is always handled by the diff machinery already.
Only lossy comparisons (e.g. ignoring whitespace) even have the need to
signal equality.

If we (continue to) assume that external diffs are lossless then we
don't need to change the code, just document that assumption.  And add a
way to specify lossy external diffs that can signal whether they found
interesting differences, to address the originally reported shortcoming.

Is there an official term for comparisons that ignore some details?
"Lossy" is rather used for compression.  Filtered, partial, selective?

> Thinking about the external diff and some of the other diff options I
> wonder what we should do when options like "--quiet" and
> "--name-only" are combined with an external diff (I haven't checked
> the current behavior). If we introduced a diff command protocol v2 we
> could include a way to pass through "--quiet" though maybe just
> redirecting the stdout of the external command to /dev/null and using
> the exit code would be sufficient.

The current code uses shortcuts like that.  For lossy external diffs we
need to turn (some of) them off.  Lots of possible combinations with
special handling -- needs lots of tests for reasonable coverage. :-/

> P.S. I haven't forgotten about our unit-test discussion but I'm
> afraid it will probably be the middle of next month before I have
> time to reply.
No worries; reminds me to polish some unit-test patches, though.  I
get distracted a lot these days..

René

Re: [PATCH 2/2] diff: fix --exit-code with external diff

[René Scharfe]

Am 11.05.24 um 22:32 schrieb René Scharfe:
> Am 08.05.24 um 17:25 schrieb phillip.wood123@gmail.com:
>> Hi René
>>
>> On 06/05/2024 19:23, René Scharfe wrote:
>>> Am 05.05.24 um 17:25 schrieb Phillip Wood:
>>>> Merge strategies are expected to exit 0 on success, 1 when there are
>>>> conflicts and another non-zero value for other errors - it would be
>>>> nice to do something similar here where 1 means "there were
>>>> differences" but it is probably too late to do that without a config
>>>> value to indicate that we should trust the exit code.
>>> Right, such a diff command protocol v2 would not need to pipe the
>>> output through an inspection loop.  Sounds like a good idea.  It's
>>> unfortunate that it would increase the configuration surface, which is
>>> not in an acute need to expand.  We could advertise the new option when
>>> dying due to the unsupported combination of --exit-code and external
>>> diff, but that's in equal parts helpful and obnoxious, I feel.
>>
>> Yes, diff dying would be annoying but the message would be useful.
>
> Having poked at it a bit more, I wonder if we need to add some further
> nuance/trick to avoid having to reject certain combinations of options.
>
> Currently external diffs can't signal content equality.  That doesn't
> matter for trivial equality (where content and mode of the two files
> match), as this case is always handled by the diff machinery already.
> Only lossy comparisons (e.g. ignoring whitespace) even have the need to
> signal equality.
>
> If we (continue to) assume that external diffs are lossless then we
> don't need to change the code, just document that assumption.  And add a
> way to specify lossy external diffs that can signal whether they found
> interesting differences, to address the originally reported shortcoming.

One more step in that direction: If we continue to use exit code 0 to
mean "notable changes present" and redefine 1 to mean "non-trivial
equality"  instead of "fatal error" then we don't need to add any config
options.

A downside is that the exit codes of diff(1) have the opposite meaning.
Which is weird in itself: You say "give me a diff" and it answers "true,
I got nothing for you" or "false, I actually had to print that dang
thing".  Inherited from cmp(1), I guess.  Which I further guess got it
from bcmp(3)?

But we can't directly use diff(1) anyway because we pass either one or
six parameters instead of the two that it would expect.  Our external
diff calling protocol is already special enough that we can freely
chose the meaning of exit codes.

Any other downsides?  Am I missing something?

René

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

[René Scharfe]

Changes since v1:
- old patch 1 was merged
- old patch 2 was merged and reverted
- new patch 1 adds tests that exercises diff --exit-code and --quiet
  together with all methods for specifying external diffs
- new patch 2 adds a struct to store the flag that patch 3 adds
- patch 3 adds configuration options and an environment variable to
  accept diff(1)-style exit codes from external diffs

  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   | 14 ++++++++
 Documentation/git.txt           |  7 ++++
 Documentation/gitattributes.txt |  5 +++
 diff.c                          | 62 ++++++++++++++++++++++++---------
 t/t4020-diff-external.sh        | 44 +++++++++++++++++++++++
 userdiff.c                      |  8 +++--
 userdiff.h                      |  7 +++-
 7 files changed, 128 insertions(+), 19 deletions(-)

--
2.45.2

[PATCH v2 1/3] t4020: test exit code with external diffs

[René Scharfe]

Add tests to check the exit code of git diff with its options --quiet
and --exit-code when using an external diff program.  Currently we
cannot tell whether it found significant changes or not.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/t4020-diff-external.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index fdd865f7c3..bed640b2b1 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -172,6 +172,39 @@ test_expect_success 'no diff with -diff' '
 	grep Binary out
 '

+check_external_exit_code () {
+	expect_code=$1
+	command_code=$2
+	option=$3
+
+	command="exit $command_code;"
+	desc="external diff '$command'"
+
+	test_expect_success "$desc via attribute with $option" "
+		test_config diff.foo.command \"$command\" &&
+		echo \"file diff=foo\" >.gitattributes &&
+		test_expect_code $expect_code git diff $option
+	"
+
+	test_expect_success "$desc via diff.external with $option" "
+		test_config diff.external \"$command\" &&
+		>.gitattributes &&
+		test_expect_code $expect_code git diff $option
+	"
+
+	test_expect_success "$desc via GIT_EXTERNAL_DIFF with $option" "
+		>.gitattributes &&
+		test_expect_code $expect_code env \
+			GIT_EXTERNAL_DIFF=\"$command\" \
+			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
+
 echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file

 test_expect_success 'force diff with "diff"' '
--
2.45.2

[PATCH v2 2/3] userdiff: add and use struct external_diff

[René Scharfe]

Wrap the string specifying the external diff command in a new struct to
simplify adding attributes, which the next patch will do.

Make sure external_diff() still returns NULL if neither the environment
variable GIT_EXTERNAL_DIFF nor the configuration option diff.external is
set, to continue allowing its use in a boolean context.

Use a designated initializer for the default builtin userdiff driver to
adjust to the type change of the second struct member.  Spelling out
only the non-zero members improves readability as a nice side-effect.

No functional change intended.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 diff.c     | 32 +++++++++++++++++---------------
 userdiff.c |  4 ++--
 userdiff.h |  6 +++++-
 3 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/diff.c b/diff.c
index ded9ac70df..286d093bfa 100644
--- a/diff.c
+++ b/diff.c
@@ -57,7 +57,7 @@ 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 struct external_diff external_diff_cfg;
 static const char *diff_order_file_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
@@ -429,7 +429,7 @@ int git_diff_ui_config(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp(var, "diff.external"))
-		return git_config_string(&external_diff_cmd_cfg, var, value);
+		return git_config_string(&external_diff_cfg.cmd, var, value);
 	if (!strcmp(var, "diff.wordregex"))
 		return git_config_string(&diff_word_regex_cfg, var, value);
 	if (!strcmp(var, "diff.orderfile"))
@@ -546,18 +546,20 @@ static char *quote_two(const char *one, const char *two)
 	return strbuf_detach(&res, NULL);
 }

-static const char *external_diff(void)
+static const struct external_diff *external_diff(void)
 {
-	static const char *external_diff_cmd = NULL;
+	static struct external_diff external_diff_env, *external_diff_ptr;
 	static int done_preparing = 0;

 	if (done_preparing)
-		return external_diff_cmd;
-	external_diff_cmd = xstrdup_or_null(getenv("GIT_EXTERNAL_DIFF"));
-	if (!external_diff_cmd)
-		external_diff_cmd = external_diff_cmd_cfg;
+		return external_diff_ptr;
+	external_diff_env.cmd = xstrdup_or_null(getenv("GIT_EXTERNAL_DIFF"));
+	if (external_diff_env.cmd)
+		external_diff_ptr = &external_diff_env;
+	else if (external_diff_cfg.cmd)
+		external_diff_ptr = &external_diff_cfg;
 	done_preparing = 1;
-	return external_diff_cmd;
+	return external_diff_ptr;
 }

 /*
@@ -4373,7 +4375,7 @@ static void add_external_diff_name(struct repository *r,
  *               infile2 infile2-sha1 infile2-mode [ rename-to ]
  *
  */
-static void run_external_diff(const char *pgm,
+static void run_external_diff(const struct external_diff *pgm,
 			      const char *name,
 			      const char *other,
 			      struct diff_filespec *one,
@@ -4384,7 +4386,7 @@ static void run_external_diff(const char *pgm,
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;

-	strvec_push(&cmd.args, pgm);
+	strvec_push(&cmd.args, pgm->cmd);
 	strvec_push(&cmd.args, name);

 	if (one && two) {
@@ -4510,7 +4512,7 @@ static void fill_metainfo(struct strbuf *msg,
 	}
 }

-static void run_diff_cmd(const char *pgm,
+static void run_diff_cmd(const struct external_diff *pgm,
 			 const char *name,
 			 const char *other,
 			 const char *attr_path,
@@ -4528,8 +4530,8 @@ static void run_diff_cmd(const char *pgm,
 	if (o->flags.allow_external || !o->ignore_driver_algorithm)
 		drv = userdiff_find_by_path(o->repo->index, attr_path);

-	if (o->flags.allow_external && drv && drv->external)
-		pgm = drv->external;
+	if (o->flags.allow_external && drv && drv->external.cmd)
+		pgm = &drv->external;

 	if (msg) {
 		/*
@@ -4595,7 +4597,7 @@ static void strip_prefix(int prefix_length, const char **namep, const char **oth

 static void run_diff(struct diff_filepair *p, struct diff_options *o)
 {
-	const char *pgm = external_diff();
+	const struct external_diff *pgm = external_diff();
 	struct strbuf msg;
 	struct diff_filespec *one = p->one;
 	struct diff_filespec *two = p->two;
diff --git a/userdiff.c b/userdiff.c
index 82bc76b910..f47e2d9f36 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -333,7 +333,7 @@ PATTERNS("scheme",
 	 "|([^][)(}{[ \t])+"),
 PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
 	 "\\\\[a-zA-Z@]+|\\\\.|([a-zA-Z0-9]|[^\x01-\x7f])+"),
-{ "default", NULL, NULL, -1, { NULL, 0 } },
+{ .name = "default", .binary = -1 },
 };
 #undef PATTERNS
 #undef IPATTERN
@@ -445,7 +445,7 @@ int userdiff_config(const char *k, const char *v)
 	if (!strcmp(type, "binary"))
 		return parse_tristate(&drv->binary, k, v);
 	if (!strcmp(type, "command"))
-		return git_config_string(&drv->external, k, v);
+		return git_config_string(&drv->external.cmd, k, v);
 	if (!strcmp(type, "textconv"))
 		return git_config_string(&drv->textconv, k, v);
 	if (!strcmp(type, "cachetextconv"))
diff --git a/userdiff.h b/userdiff.h
index d726804c3e..7e2b28e88d 100644
--- a/userdiff.h
+++ b/userdiff.h
@@ -11,9 +11,13 @@ struct userdiff_funcname {
 	int cflags;
 };

+struct external_diff {
+	const char *cmd;
+};
+
 struct userdiff_driver {
 	const char *name;
-	const char *external;
+	struct external_diff external;
 	const char *algorithm;
 	int binary;
 	struct userdiff_funcname funcname;
--
2.45.2

[PATCH v2 3/3] diff: let external diffs report that changes are uninteresting

[René Scharfe]

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.

Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/config/diff.txt   | 14 ++++++++++++++
 Documentation/git.txt           |  7 +++++++
 Documentation/gitattributes.txt |  5 +++++
 diff.c                          | 30 +++++++++++++++++++++++++++++-
 t/t4020-diff-external.sh        | 23 +++++++++++++++++------
 userdiff.c                      |  4 ++++
 userdiff.h                      |  1 +
 7 files changed, 77 insertions(+), 7 deletions(-)

diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
index 5ce7b91f1d..485a13236d 100644
--- a/Documentation/config/diff.txt
+++ b/Documentation/config/diff.txt
@@ -79,6 +79,13 @@ 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 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.
+
 diff.ignoreSubmodules::
 	Sets the default value of --ignore-submodules. Note that this
 	affects only 'git diff' Porcelain, and not lower level 'diff'
@@ -164,6 +171,13 @@ 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
+	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.
+
 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.
diff --git a/Documentation/git.txt b/Documentation/git.txt
index a31a70acca..81b806416c 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -644,6 +644,13 @@ 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.
+
 `GIT_DIFF_PATH_COUNTER`::
 	A 1-based counter incremented by one for every path.

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 4338d023d9..80cae17f37 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -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 286d093bfa..3dff81ec7d 100644
--- a/diff.c
+++ b/diff.c
@@ -430,6 +430,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"))
@@ -554,6 +558,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)
@@ -4385,6 +4391,18 @@ 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 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 (!(o->output_format & DIFF_FORMAT_PATCH) && !pgm->trust_exit_code) {
+		o->found_changes = 1;
+		return;
+	}

 	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;
-	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);

 	remove_tempfile();
@@ -4924,6 +4945,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)
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index bed640b2b1..bbb49b8a0a 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -175,19 +175,22 @@ 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;"
-	desc="external diff '$command'"
+	desc="external diff '$command' with trustExitCode=$trust_exit_code"

 	test_expect_success "$desc via attribute with $option" "
 		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_success "$desc via diff.external with $option" "
 		test_config diff.external \"$command\" &&
+		test_config diff.trustExitCode $trust_exit_code &&
 		>.gitattributes &&
 		test_expect_code $expect_code git diff $option
 	"
@@ -196,14 +199,22 @@ check_external_exit_code () {
 		>.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
+
+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

 echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file

diff --git a/userdiff.c b/userdiff.c
index f47e2d9f36..6cdfb147c3 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -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"))
diff --git a/userdiff.h b/userdiff.h
index 7e2b28e88d..5f1c8cc49c 100644
--- a/userdiff.h
+++ b/userdiff.h
@@ -13,6 +13,7 @@ struct userdiff_funcname {

 struct external_diff {
 	const char *cmd;
+	unsigned trust_exit_code:1;
 };

 struct userdiff_driver {
--
2.45.2

Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting

[Johannes Sixt]

Am 05.06.24 um 10:38 schrieb René Scharfe:
> +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 find this somewhat unclear. What are the consequences when this value
is set to false, but the command exits with code other than 0? Is it

    If it's false then any exit code other than 0 of the `diff.external`
    command is treated as an error.

-- Hannes

Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting

[René Scharfe]

Am 06.06.24 um 08:39 schrieb Johannes Sixt:
> Am 05.06.24 um 10:38 schrieb René Scharfe:
>> +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 find this somewhat unclear. What are the consequences when this value
> is set to false, but the command exits with code other than 0? Is it
>
>     If it's false then any exit code other than 0 of the `diff.external`
>     command is treated as an error.

Yes, unexpected exit codes are reported as errors.

If trustExitCode is false and --quiet is given then the execution of
external diffs is skipped, so in that situation there is no exit code to
expect, though.  Not sure how to express it concisely, though.  This
attempt looks a bit bloated:

--quiet::
        Disable all output of the program. Implies `--exit-code`.
        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.

Might be worth documenting this original behavior somehow, anyway.  It
makes sense in hindsight, but surprised me a bit when I wrote the tests.

René

Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>>> +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 find this somewhat unclear. What are the consequences when this value
>> is set to false, but the command exits with code other than 0? Is it
>>
>>     If it's false then any exit code other than 0 of the `diff.external`
>>     command is treated as an error.
>
> Yes, unexpected exit codes are reported as errors.
>
> If trustExitCode is false and --quiet is given then the execution of
> external diffs is skipped, so in that situation there is no exit code to
> expect, though.  Not sure how to express it concisely, though.  This
> attempt looks a bit bloated:
>
> --quiet::
>         Disable all output of the program. Implies `--exit-code`.
>         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.
>
> Might be worth documenting this original behavior somehow, anyway.  It
> makes sense in hindsight, but surprised me a bit when I wrote the tests.

Yes.  The explanation of trustExitCode makes sense as an explanation
of what the variable means (i.e. if set, we pay attention to the
exit code of the external diff driver, otherwise a non-zero exit is
an error), but I suspect that readers are _more_ interested in how
the external diff driver contributes to the answer to the "has this
path been changed?" question when the variable is on and off.  And
the above description of "--quiet" does help answer that question
somewhat.

Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting

[Phillip Wood]

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

Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting

[René Scharfe]

Am 06.06.24 um 11:48 schrieb Phillip Wood:
> On 05/06/2024 09:38, René Scharfe wrote:
>> +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.

The first part looks like an obvious improvement.  Stating that
unexpected exit codes are errors looks tautological to me, though.
Perhaps mentioning that git diff stops at that point adds would be
useful?  Or perhaps a tad of redundancy is just what's needed here?

>
>
>>       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?

Oh, yes.  We didn't even start the program before, but with the
patch it becomes necessary.  Good find!

>
>> -    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.

Yeah, it's not as clear as it could be.  One reason is that it avoids
redundancy at the action side and the other is adherence to the rule of
not explicitly comparing to zero.  We could enumerate all cases:

	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);

Or avoid redundancy in the conditions:

	if (pgm->trust_exit_code) {
		if (rc == 1)
			o->found_changes = 1;
		else if (rc != 0)
			die(_("external diff died, stopping at %s"), name);
	} else {
		if (rc == 0)
			o->found_changes = 1;
		else
			die(_("external diff died, stopping at %s"), name);
	}

We should not get into bit twiddling, though:

	o->found_changes |= rc == pgm->trust_exit_code;
	if ((unsigned)rc > pgm->trust_exit_code)
		die(_("external diff died, stopping at %s"), name);

>
>> -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.

Right, we need this after unlocking external diff execution with
--quiet.

René

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>   t4020: test exit code with external diffs
>   userdiff: add and use struct external_diff
>   diff: let external diffs report that changes are uninteresting

OK, we now need to mark each external diff driver if we want to
honor their exit status, but that awkwardness is to be blamed on the
original design.  Thanks for addressing the misdesign I made years
ago in the mildest way possible not to introduce any unnecessary
regressions.

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

[René Scharfe]

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

[PATCH v3 1/3] t4020: test exit code with external diffs

[René Scharfe]

Add tests to check the exit code of git diff with its options --quiet
and --exit-code when using an external diff program.  Currently we
cannot tell whether it found significant changes or not.

While at it, document briefly that --quiet turns off execution of
external diff programs because that behavior surprised me for a moment
while writing the tests.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/diff-options.txt |  1 +
 t/t4020-diff-external.sh       | 53 ++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index c7df20e571..6b73daf540 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -820,6 +820,7 @@ ifndef::git-log[]

 --quiet::
 	Disable all output of the program. Implies `--exit-code`.
+	Disables execution of external diff helpers.
 endif::git-log[]
 endif::git-format-patch[]

diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index fdd865f7c3..4070523cdb 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -172,6 +172,59 @@ test_expect_success 'no diff with -diff' '
 	grep Binary out
 '

+check_external_diff () {
+	expect_code=$1
+	expect_out=$2
+	expect_err=$3
+	command_code=$4
+	shift 4
+	options="$@"
+
+	command="echo output; exit $command_code;"
+	desc="external diff '$command'"
+	with_options="${options:+ with }$options"
+
+	test_expect_success "$desc via attribute$with_options" "
+		test_config diff.foo.command \"$command\" &&
+		echo \"file diff=foo\" >.gitattributes &&
+		test_expect_code $expect_code git diff $options >out 2>err &&
+		test_cmp $expect_out out &&
+		test_cmp $expect_err err
+	"
+
+	test_expect_success "$desc via diff.external$with_options" "
+		test_config diff.external \"$command\" &&
+		>.gitattributes &&
+		test_expect_code $expect_code git diff $options >out 2>err &&
+		test_cmp $expect_out out &&
+		test_cmp $expect_err err
+	"
+
+	test_expect_success "$desc via GIT_EXTERNAL_DIFF$with_options" "
+		>.gitattributes &&
+		test_expect_code $expect_code env \
+			GIT_EXTERNAL_DIFF=\"$command\" \
+			git diff $options >out 2>err &&
+		test_cmp $expect_out out &&
+		test_cmp $expect_err err
+	"
+}
+
+test_expect_success 'setup output files' '
+	: >empty &&
+	echo output >output &&
+	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
+
 echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file

 test_expect_success 'force diff with "diff"' '
--
2.45.2

Re: [PATCH v3 1/3] t4020: test exit code with external diffs

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> +check_external_diff () {
> +	expect_code=$1
> +	expect_out=$2
> +	expect_err=$3
> +	command_code=$4
> +	shift 4
> +	options="$@"

Tiny nit, but I'd prefer to see "$@" reserved for its magic and all
other times where it is equivalent to "$*", see the latter used.

Other than that, all three patches looked good to me.  Thanks.

[PATCH v3 2/3] userdiff: add and use struct external_diff

[René Scharfe]

Wrap the string specifying the external diff command in a new struct to
simplify adding attributes, which the next patch will do.

Make sure external_diff() still returns NULL if neither the environment
variable GIT_EXTERNAL_DIFF nor the configuration option diff.external is
set, to continue allowing its use in a boolean context.

Use a designated initializer for the default builtin userdiff driver to
adjust to the type change of the second struct member.  Spelling out
only the non-zero members improves readability as a nice side-effect.

No functional change intended.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 diff.c     | 32 +++++++++++++++++---------------
 userdiff.c |  4 ++--
 userdiff.h |  6 +++++-
 3 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/diff.c b/diff.c
index e70301df76..4b86c61631 100644
--- a/diff.c
+++ b/diff.c
@@ -57,7 +57,7 @@ static int diff_color_moved_ws_default;
 static int diff_context_default = 3;
 static int diff_interhunk_context_default;
 static char *diff_word_regex_cfg;
-static char *external_diff_cmd_cfg;
+static struct external_diff external_diff_cfg;
 static char *diff_order_file_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
@@ -431,7 +431,7 @@ int git_diff_ui_config(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp(var, "diff.external"))
-		return git_config_string(&external_diff_cmd_cfg, var, value);
+		return git_config_string(&external_diff_cfg.cmd, var, value);
 	if (!strcmp(var, "diff.wordregex"))
 		return git_config_string(&diff_word_regex_cfg, var, value);
 	if (!strcmp(var, "diff.orderfile"))
@@ -548,18 +548,20 @@ static char *quote_two(const char *one, const char *two)
 	return strbuf_detach(&res, NULL);
 }

-static const char *external_diff(void)
+static const struct external_diff *external_diff(void)
 {
-	static const char *external_diff_cmd = NULL;
+	static struct external_diff external_diff_env, *external_diff_ptr;
 	static int done_preparing = 0;

 	if (done_preparing)
-		return external_diff_cmd;
-	external_diff_cmd = xstrdup_or_null(getenv("GIT_EXTERNAL_DIFF"));
-	if (!external_diff_cmd)
-		external_diff_cmd = external_diff_cmd_cfg;
+		return external_diff_ptr;
+	external_diff_env.cmd = xstrdup_or_null(getenv("GIT_EXTERNAL_DIFF"));
+	if (external_diff_env.cmd)
+		external_diff_ptr = &external_diff_env;
+	else if (external_diff_cfg.cmd)
+		external_diff_ptr = &external_diff_cfg;
 	done_preparing = 1;
-	return external_diff_cmd;
+	return external_diff_ptr;
 }

 /*
@@ -4375,7 +4377,7 @@ static void add_external_diff_name(struct repository *r,
  *               infile2 infile2-sha1 infile2-mode [ rename-to ]
  *
  */
-static void run_external_diff(const char *pgm,
+static void run_external_diff(const struct external_diff *pgm,
 			      const char *name,
 			      const char *other,
 			      struct diff_filespec *one,
@@ -4386,7 +4388,7 @@ static void run_external_diff(const char *pgm,
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;

-	strvec_push(&cmd.args, pgm);
+	strvec_push(&cmd.args, pgm->cmd);
 	strvec_push(&cmd.args, name);

 	if (one && two) {
@@ -4512,7 +4514,7 @@ static void fill_metainfo(struct strbuf *msg,
 	}
 }

-static void run_diff_cmd(const char *pgm,
+static void run_diff_cmd(const struct external_diff *pgm,
 			 const char *name,
 			 const char *other,
 			 const char *attr_path,
@@ -4530,8 +4532,8 @@ static void run_diff_cmd(const char *pgm,
 	if (o->flags.allow_external || !o->ignore_driver_algorithm)
 		drv = userdiff_find_by_path(o->repo->index, attr_path);

-	if (o->flags.allow_external && drv && drv->external)
-		pgm = drv->external;
+	if (o->flags.allow_external && drv && drv->external.cmd)
+		pgm = &drv->external;

 	if (msg) {
 		/*
@@ -4597,7 +4599,7 @@ static void strip_prefix(int prefix_length, const char **namep, const char **oth

 static void run_diff(struct diff_filepair *p, struct diff_options *o)
 {
-	const char *pgm = external_diff();
+	const struct external_diff *pgm = external_diff();
 	struct strbuf msg;
 	struct diff_filespec *one = p->one;
 	struct diff_filespec *two = p->two;
diff --git a/userdiff.c b/userdiff.c
index 82bc76b910..f47e2d9f36 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -333,7 +333,7 @@ PATTERNS("scheme",
 	 "|([^][)(}{[ \t])+"),
 PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
 	 "\\\\[a-zA-Z@]+|\\\\.|([a-zA-Z0-9]|[^\x01-\x7f])+"),
-{ "default", NULL, NULL, -1, { NULL, 0 } },
+{ .name = "default", .binary = -1 },
 };
 #undef PATTERNS
 #undef IPATTERN
@@ -445,7 +445,7 @@ int userdiff_config(const char *k, const char *v)
 	if (!strcmp(type, "binary"))
 		return parse_tristate(&drv->binary, k, v);
 	if (!strcmp(type, "command"))
-		return git_config_string(&drv->external, k, v);
+		return git_config_string(&drv->external.cmd, k, v);
 	if (!strcmp(type, "textconv"))
 		return git_config_string(&drv->textconv, k, v);
 	if (!strcmp(type, "cachetextconv"))
diff --git a/userdiff.h b/userdiff.h
index cc8e5abfef..2d59a8fc56 100644
--- a/userdiff.h
+++ b/userdiff.h
@@ -11,9 +11,13 @@ struct userdiff_funcname {
 	int cflags;
 };

+struct external_diff {
+	char *cmd;
+};
+
 struct userdiff_driver {
 	const char *name;
-	char *external;
+	struct external_diff external;
 	char *algorithm;
 	int binary;
 	struct userdiff_funcname funcname;
--
2.45.2

[PATCH v3 3/3] diff: let external diffs report that changes are uninteresting

[René Scharfe]

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>
---
 Documentation/config/diff.txt   | 18 +++++++++++++++++
 Documentation/diff-options.txt  |  6 +++++-
 Documentation/git.txt           | 10 +++++++++
 Documentation/gitattributes.txt |  5 +++++
 diff.c                          | 36 ++++++++++++++++++++++++++++++++-
 t/t4020-diff-external.sh        | 33 +++++++++++++++++++++---------
 userdiff.c                      |  4 ++++
 userdiff.h                      |  1 +
 8 files changed, 101 insertions(+), 12 deletions(-)

diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
index 5ce7b91f1d..190bda17e5 100644
--- a/Documentation/config/diff.txt
+++ b/Documentation/config/diff.txt
@@ -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.
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 6b73daf540..cd0b81adbb 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -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[]

diff --git a/Documentation/git.txt b/Documentation/git.txt
index a31a70acca..4489e2297a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -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.

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 4338d023d9..80cae17f37 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -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 4b86c61631..39ba842049 100644
--- 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)
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 4070523cdb..3baa52a9bf 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -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

diff --git a/userdiff.c b/userdiff.c
index f47e2d9f36..6cdfb147c3 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -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"))
diff --git a/userdiff.h b/userdiff.h
index 2d59a8fc56..f5cb53d0d4 100644
--- a/userdiff.h
+++ b/userdiff.h
@@ -13,6 +13,7 @@ struct userdiff_funcname {

 struct external_diff {
 	char *cmd;
+	unsigned trust_exit_code:1;
 };

 struct userdiff_driver {
--
2.45.2

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

[phillip.wood123]

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