* Possible git-diff bug when using exit-code with diff filters
@ 2024-04-20 1:13 German Lashevich
2024-04-21 10:42 ` René Scharfe
0 siblings, 1 reply; 32+ messages in thread
From: German Lashevich @ 2024-04-20 1:13 UTC (permalink / raw
To: git
> 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Possible git-diff bug when using exit-code with diff filters
2024-04-20 1:13 Possible git-diff bug when using exit-code with diff filters German Lashevich
@ 2024-04-21 10:42 ` René Scharfe
2024-04-21 18:17 ` Junio C Hamano
` (2 more replies)
0 siblings, 3 replies; 32+ messages in thread
From: René Scharfe @ 2024-04-21 10:42 UTC (permalink / raw
To: German Lashevich, git
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"' '
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: Possible git-diff bug when using exit-code with diff filters
2024-04-21 10:42 ` René Scharfe
@ 2024-04-21 18:17 ` Junio C Hamano
2024-04-21 18:32 ` rsbecker
2024-05-05 10:19 ` René Scharfe
2024-05-05 10:19 ` [PATCH 1/2] diff: report unmerged paths as changes in run_diff_cmd() René Scharfe
2024-05-05 10:20 ` [PATCH 2/2] diff: fix --exit-code with external diff René Scharfe
2 siblings, 2 replies; 32+ messages in thread
From: Junio C Hamano @ 2024-04-21 18:17 UTC (permalink / raw
To: René Scharfe; +Cc: German Lashevich, git
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: Possible git-diff bug when using exit-code with diff filters
2024-04-21 18:17 ` Junio C Hamano
@ 2024-04-21 18:32 ` rsbecker
2024-04-21 19:09 ` Junio C Hamano
2024-05-05 10:19 ` René Scharfe
1 sibling, 1 reply; 32+ messages in thread
From: rsbecker @ 2024-04-21 18:32 UTC (permalink / raw
To: 'Junio C Hamano', 'René Scharfe'
Cc: 'German Lashevich', git
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Possible git-diff bug when using exit-code with diff filters
2024-04-21 18:32 ` rsbecker
@ 2024-04-21 19:09 ` Junio C Hamano
2024-04-21 20:18 ` rsbecker
0 siblings, 1 reply; 32+ messages in thread
From: Junio C Hamano @ 2024-04-21 19:09 UTC (permalink / raw
To: rsbecker; +Cc: 'René Scharfe', 'German Lashevich', git
<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...
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: Possible git-diff bug when using exit-code with diff filters
2024-04-21 19:09 ` Junio C Hamano
@ 2024-04-21 20:18 ` rsbecker
0 siblings, 0 replies; 32+ messages in thread
From: rsbecker @ 2024-04-21 20:18 UTC (permalink / raw
To: 'Junio C Hamano'
Cc: 'René Scharfe', 'German Lashevich', git
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Possible git-diff bug when using exit-code with diff filters
2024-04-21 18:17 ` Junio C Hamano
2024-04-21 18:32 ` rsbecker
@ 2024-05-05 10:19 ` René Scharfe
2024-05-06 17:22 ` Junio C Hamano
1 sibling, 1 reply; 32+ messages in thread
From: René Scharfe @ 2024-05-05 10:19 UTC (permalink / raw
To: Junio C Hamano; +Cc: German Lashevich, git
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é
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 1/2] diff: report unmerged paths as changes in run_diff_cmd()
2024-04-21 10:42 ` René Scharfe
2024-04-21 18:17 ` Junio C Hamano
@ 2024-05-05 10:19 ` René Scharfe
2024-05-05 10:20 ` [PATCH 2/2] diff: fix --exit-code with external diff René Scharfe
2 siblings, 0 replies; 32+ messages in thread
From: René Scharfe @ 2024-05-05 10:19 UTC (permalink / raw
To: German Lashevich, git; +Cc: Junio C Hamano
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/2] diff: fix --exit-code with external diff
2024-04-21 10:42 ` René Scharfe
2024-04-21 18:17 ` Junio C Hamano
2024-05-05 10:19 ` [PATCH 1/2] diff: report unmerged paths as changes in run_diff_cmd() René Scharfe
@ 2024-05-05 10:20 ` René Scharfe
2024-05-05 15:25 ` Phillip Wood
` (2 more replies)
2 siblings, 3 replies; 32+ messages in thread
From: René Scharfe @ 2024-05-05 10:20 UTC (permalink / raw
To: German Lashevich, git; +Cc: Junio C Hamano
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] diff: fix --exit-code with external diff
2024-05-05 10:20 ` [PATCH 2/2] diff: fix --exit-code with external diff René Scharfe
@ 2024-05-05 15:25 ` Phillip Wood
2024-05-06 17:31 ` Junio C Hamano
2024-05-06 18:23 ` René Scharfe
2024-06-05 8:31 ` [PATCH v2 0/3] " René Scharfe
2024-06-09 7:35 ` [PATCH v3 " René Scharfe
2 siblings, 2 replies; 32+ messages in thread
From: Phillip Wood @ 2024-05-05 15:25 UTC (permalink / raw
To: René Scharfe, German Lashevich, git; +Cc: Junio C Hamano
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Possible git-diff bug when using exit-code with diff filters
2024-05-05 10:19 ` René Scharfe
@ 2024-05-06 17:22 ` Junio C Hamano
0 siblings, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2024-05-06 17:22 UTC (permalink / raw
To: René Scharfe; +Cc: German Lashevich, git
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] diff: fix --exit-code with external diff
2024-05-05 15:25 ` Phillip Wood
@ 2024-05-06 17:31 ` Junio C Hamano
2024-05-06 18:23 ` René Scharfe
1 sibling, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2024-05-06 17:31 UTC (permalink / raw
To: Phillip Wood; +Cc: René Scharfe, German Lashevich, git
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] diff: fix --exit-code with external diff
2024-05-05 15:25 ` Phillip Wood
2024-05-06 17:31 ` Junio C Hamano
@ 2024-05-06 18:23 ` René Scharfe
2024-05-08 15:25 ` phillip.wood123
1 sibling, 1 reply; 32+ messages in thread
From: René Scharfe @ 2024-05-06 18:23 UTC (permalink / raw
To: phillip.wood, German Lashevich, git; +Cc: Junio C Hamano
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é
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] diff: fix --exit-code with external diff
2024-05-06 18:23 ` René Scharfe
@ 2024-05-08 15:25 ` phillip.wood123
2024-05-11 20:32 ` René Scharfe
0 siblings, 1 reply; 32+ messages in thread
From: phillip.wood123 @ 2024-05-08 15:25 UTC (permalink / raw
To: René Scharfe, phillip.wood, German Lashevich, git; +Cc: Junio C Hamano
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] diff: fix --exit-code with external diff
2024-05-08 15:25 ` phillip.wood123
@ 2024-05-11 20:32 ` René Scharfe
2024-05-12 9:38 ` René Scharfe
0 siblings, 1 reply; 32+ messages in thread
From: René Scharfe @ 2024-05-11 20:32 UTC (permalink / raw
To: phillip.wood, German Lashevich, git; +Cc: Junio C Hamano
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é
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] diff: fix --exit-code with external diff
2024-05-11 20:32 ` René Scharfe
@ 2024-05-12 9:38 ` René Scharfe
0 siblings, 0 replies; 32+ messages in thread
From: René Scharfe @ 2024-05-12 9:38 UTC (permalink / raw
To: phillip.wood, German Lashevich, git; +Cc: Junio C Hamano
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é
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 0/3] diff: fix --exit-code with external diff
2024-05-05 10:20 ` [PATCH 2/2] diff: fix --exit-code with external diff René Scharfe
2024-05-05 15:25 ` Phillip Wood
@ 2024-06-05 8:31 ` René Scharfe
2024-06-05 8:35 ` [PATCH v2 1/3] t4020: test exit code with external diffs René Scharfe
` (3 more replies)
2024-06-09 7:35 ` [PATCH v3 " René Scharfe
2 siblings, 4 replies; 32+ messages in thread
From: René Scharfe @ 2024-06-05 8:31 UTC (permalink / raw
To: git; +Cc: Junio C Hamano, German Lashevich, Phillip Wood
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 1/3] t4020: test exit code with external diffs
2024-06-05 8:31 ` [PATCH v2 0/3] " René Scharfe
@ 2024-06-05 8:35 ` René Scharfe
2024-06-05 8:36 ` [PATCH v2 2/3] userdiff: add and use struct external_diff René Scharfe
` (2 subsequent siblings)
3 siblings, 0 replies; 32+ messages in thread
From: René Scharfe @ 2024-06-05 8:35 UTC (permalink / raw
To: git; +Cc: Junio C Hamano, German Lashevich, Phillip Wood
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2 2/3] userdiff: add and use struct external_diff
2024-06-05 8:31 ` [PATCH v2 0/3] " René Scharfe
2024-06-05 8:35 ` [PATCH v2 1/3] t4020: test exit code with external diffs René Scharfe
@ 2024-06-05 8:36 ` René Scharfe
2024-06-05 8:38 ` [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting René Scharfe
2024-06-05 16:47 ` [PATCH v2 0/3] diff: fix --exit-code with external diff Junio C Hamano
3 siblings, 0 replies; 32+ messages in thread
From: René Scharfe @ 2024-06-05 8:36 UTC (permalink / raw
To: git; +Cc: Junio C Hamano, German Lashevich, Phillip Wood
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting
2024-06-05 8:31 ` [PATCH v2 0/3] " René Scharfe
2024-06-05 8:35 ` [PATCH v2 1/3] t4020: test exit code with external diffs René Scharfe
2024-06-05 8:36 ` [PATCH v2 2/3] userdiff: add and use struct external_diff René Scharfe
@ 2024-06-05 8:38 ` René Scharfe
2024-06-06 6:39 ` Johannes Sixt
2024-06-06 9:48 ` Phillip Wood
2024-06-05 16:47 ` [PATCH v2 0/3] diff: fix --exit-code with external diff Junio C Hamano
3 siblings, 2 replies; 32+ messages in thread
From: René Scharfe @ 2024-06-05 8:38 UTC (permalink / raw
To: git; +Cc: Junio C Hamano, German Lashevich, Phillip Wood
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v2 0/3] diff: fix --exit-code with external diff
2024-06-05 8:31 ` [PATCH v2 0/3] " René Scharfe
` (2 preceding siblings ...)
2024-06-05 8:38 ` [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting René Scharfe
@ 2024-06-05 16:47 ` Junio C Hamano
3 siblings, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2024-06-05 16:47 UTC (permalink / raw
To: René Scharfe; +Cc: git, German Lashevich, Phillip Wood
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting
2024-06-05 8:38 ` [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting René Scharfe
@ 2024-06-06 6:39 ` Johannes Sixt
2024-06-06 8:28 ` René Scharfe
2024-06-06 9:48 ` Phillip Wood
1 sibling, 1 reply; 32+ messages in thread
From: Johannes Sixt @ 2024-06-06 6:39 UTC (permalink / raw
To: René Scharfe; +Cc: Junio C Hamano, German Lashevich, Phillip Wood, git
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting
2024-06-06 6:39 ` Johannes Sixt
@ 2024-06-06 8:28 ` René Scharfe
2024-06-06 15:49 ` Junio C Hamano
0 siblings, 1 reply; 32+ messages in thread
From: René Scharfe @ 2024-06-06 8:28 UTC (permalink / raw
To: Johannes Sixt; +Cc: Junio C Hamano, German Lashevich, Phillip Wood, git
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é
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting
2024-06-05 8:38 ` [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting René Scharfe
2024-06-06 6:39 ` Johannes Sixt
@ 2024-06-06 9:48 ` Phillip Wood
2024-06-07 8:19 ` René Scharfe
1 sibling, 1 reply; 32+ messages in thread
From: Phillip Wood @ 2024-06-06 9:48 UTC (permalink / raw
To: René Scharfe, git
Cc: Junio C Hamano, German Lashevich, Phillip Wood, Johannes Sixt
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting
2024-06-06 8:28 ` René Scharfe
@ 2024-06-06 15:49 ` Junio C Hamano
0 siblings, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2024-06-06 15:49 UTC (permalink / raw
To: René Scharfe; +Cc: Johannes Sixt, German Lashevich, Phillip Wood, git
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting
2024-06-06 9:48 ` Phillip Wood
@ 2024-06-07 8:19 ` René Scharfe
0 siblings, 0 replies; 32+ messages in thread
From: René Scharfe @ 2024-06-07 8:19 UTC (permalink / raw
To: phillip.wood, git; +Cc: Junio C Hamano, German Lashevich, Johannes Sixt
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é
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v3 0/3] diff: fix --exit-code with external diff
2024-05-05 10:20 ` [PATCH 2/2] diff: fix --exit-code with external diff René Scharfe
2024-05-05 15:25 ` Phillip Wood
2024-06-05 8:31 ` [PATCH v2 0/3] " René Scharfe
@ 2024-06-09 7:35 ` René Scharfe
2024-06-09 7:38 ` [PATCH v3 1/3] t4020: test exit code with external diffs René Scharfe
` (3 more replies)
2 siblings, 4 replies; 32+ messages in thread
From: René Scharfe @ 2024-06-09 7:35 UTC (permalink / raw
To: git; +Cc: Junio C Hamano, German Lashevich, Phillip Wood, Johannes Sixt
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v3 1/3] t4020: test exit code with external diffs
2024-06-09 7:35 ` [PATCH v3 " René Scharfe
@ 2024-06-09 7:38 ` René Scharfe
2024-06-10 16:33 ` Junio C Hamano
2024-06-09 7:39 ` [PATCH v3 2/3] userdiff: add and use struct external_diff René Scharfe
` (2 subsequent siblings)
3 siblings, 1 reply; 32+ messages in thread
From: René Scharfe @ 2024-06-09 7:38 UTC (permalink / raw
To: git; +Cc: Junio C Hamano, German Lashevich, Phillip Wood, Johannes Sixt
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v3 2/3] userdiff: add and use struct external_diff
2024-06-09 7:35 ` [PATCH v3 " René Scharfe
2024-06-09 7:38 ` [PATCH v3 1/3] t4020: test exit code with external diffs René Scharfe
@ 2024-06-09 7:39 ` René Scharfe
2024-06-09 7:41 ` [PATCH v3 3/3] diff: let external diffs report that changes are uninteresting René Scharfe
2024-06-10 13:48 ` [PATCH v3 0/3] diff: fix --exit-code with external diff phillip.wood123
3 siblings, 0 replies; 32+ messages in thread
From: René Scharfe @ 2024-06-09 7:39 UTC (permalink / raw
To: git; +Cc: Junio C Hamano, German Lashevich, Phillip Wood, Johannes Sixt
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v3 3/3] diff: let external diffs report that changes are uninteresting
2024-06-09 7:35 ` [PATCH v3 " René Scharfe
2024-06-09 7:38 ` [PATCH v3 1/3] t4020: test exit code with external diffs René Scharfe
2024-06-09 7:39 ` [PATCH v3 2/3] userdiff: add and use struct external_diff René Scharfe
@ 2024-06-09 7:41 ` René Scharfe
2024-06-10 13:48 ` [PATCH v3 0/3] diff: fix --exit-code with external diff phillip.wood123
3 siblings, 0 replies; 32+ messages in thread
From: René Scharfe @ 2024-06-09 7:41 UTC (permalink / raw
To: git; +Cc: Junio C Hamano, German Lashevich, Phillip Wood, Johannes Sixt
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v3 0/3] diff: fix --exit-code with external diff
2024-06-09 7:35 ` [PATCH v3 " René Scharfe
` (2 preceding siblings ...)
2024-06-09 7:41 ` [PATCH v3 3/3] diff: let external diffs report that changes are uninteresting René Scharfe
@ 2024-06-10 13:48 ` phillip.wood123
3 siblings, 0 replies; 32+ messages in thread
From: phillip.wood123 @ 2024-06-10 13:48 UTC (permalink / raw
To: René Scharfe, git
Cc: Junio C Hamano, German Lashevich, Phillip Wood, Johannes Sixt
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v3 1/3] t4020: test exit code with external diffs
2024-06-09 7:38 ` [PATCH v3 1/3] t4020: test exit code with external diffs René Scharfe
@ 2024-06-10 16:33 ` Junio C Hamano
0 siblings, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2024-06-10 16:33 UTC (permalink / raw
To: René Scharfe; +Cc: git, German Lashevich, Phillip Wood, Johannes Sixt
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2024-06-10 16:33 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-20 1:13 Possible git-diff bug when using exit-code with diff filters German Lashevich
2024-04-21 10:42 ` René Scharfe
2024-04-21 18:17 ` Junio C Hamano
2024-04-21 18:32 ` rsbecker
2024-04-21 19:09 ` Junio C Hamano
2024-04-21 20:18 ` rsbecker
2024-05-05 10:19 ` René Scharfe
2024-05-06 17:22 ` Junio C Hamano
2024-05-05 10:19 ` [PATCH 1/2] diff: report unmerged paths as changes in run_diff_cmd() René Scharfe
2024-05-05 10:20 ` [PATCH 2/2] diff: fix --exit-code with external diff René Scharfe
2024-05-05 15:25 ` Phillip Wood
2024-05-06 17:31 ` Junio C Hamano
2024-05-06 18:23 ` René Scharfe
2024-05-08 15:25 ` phillip.wood123
2024-05-11 20:32 ` René Scharfe
2024-05-12 9:38 ` René Scharfe
2024-06-05 8:31 ` [PATCH v2 0/3] " René Scharfe
2024-06-05 8:35 ` [PATCH v2 1/3] t4020: test exit code with external diffs René Scharfe
2024-06-05 8:36 ` [PATCH v2 2/3] userdiff: add and use struct external_diff René Scharfe
2024-06-05 8:38 ` [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting René Scharfe
2024-06-06 6:39 ` Johannes Sixt
2024-06-06 8:28 ` René Scharfe
2024-06-06 15:49 ` Junio C Hamano
2024-06-06 9:48 ` Phillip Wood
2024-06-07 8:19 ` René Scharfe
2024-06-05 16:47 ` [PATCH v2 0/3] diff: fix --exit-code with external diff Junio C Hamano
2024-06-09 7:35 ` [PATCH v3 " René Scharfe
2024-06-09 7:38 ` [PATCH v3 1/3] t4020: test exit code with external diffs René Scharfe
2024-06-10 16:33 ` Junio C Hamano
2024-06-09 7:39 ` [PATCH v3 2/3] userdiff: add and use struct external_diff René Scharfe
2024-06-09 7:41 ` [PATCH v3 3/3] diff: let external diffs report that changes are uninteresting René Scharfe
2024-06-10 13:48 ` [PATCH v3 0/3] diff: fix --exit-code with external diff phillip.wood123
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).