{"thread":{"id":"61342","subject":"Possible git-diff bug when using exit-code with diff filters","startedAt":"2024-04-20T01:13:15Z","lastAt":"2024-06-10T16:33:48Z","messageCount":32,"participants":["German Lashevich","René Scharfe","Junio C Hamano","rsbecker@nexbridge.com","Phillip Wood","phillip.wood123@gmail.com","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"493244","messageId":"CACDhgro3KXD0O9ZdE1q46jmXE0O=vf-Z+ZX50WMqmRHAeowGAA@mail.gmail.com","threadId":"61342","inReplyTo":null,"subject":"Possible git-diff bug when using exit-code with diff filters","fromName":"German Lashevich","fromEmail":"german.lashevich@gmail.com","sentAt":"2024-04-20T01:13:14Z","receivedAt":"2024-04-20T01:13:15Z","isPatch":false,"sender":{"key":"german.lashevich@gmail.com","avatar":null},"body":"> What did you do before the bug happened? (Steps to reproduce your issue)\n\nI configured a diff filter via gitattributes to use a custom script that,\nsometimes, can change the files being compared in a way that there are no\ndifferences between them.\nThen I used `git diff --exit-code` on the changed file.\n\n> What did you expect to happen? (Expected behavior)\n\nI expected `git diff --exit-code` to return 0, since there are no differences\nbetween the files after the filter is applied.\n\n> What happened instead? (Actual behavior)\n\n`git diff --exit-code` correctly produces no output, but returns 1.\n\n> What's different between what you expected and what actually happened?\n\nThe difference is that `git diff --exit-code`, instead of returning 0, returns\n1 even when there is no output.\n\n> Anything else you want to add:\n\nI have prepared a repository with a test case that reproduces the issue.\nYou can find it at https://github.com/Zebradil/git-diff-exit-code-bug-repro\nThe Readme file in the repository contains instructions on how to reproduce\nthe issue.\n\n[System Info]\ngit version: 2.44.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 6.8.7-arch1-1 #1 SMP PREEMPT_DYNAMIC Wed, 17 Apr 2024\n15:20:28 +0000 x86_64\ncompiler info: gnuc: 13.2\nlibc info: glibc: 2.39\n$SHELL (typically, interactive shell): /bin/zsh\n\nBest regards,\nGerman Lashevich\n"},{"id":"493261","messageId":"82561c70-ec33-41bf-b036-52310ffc1926@web.de","threadId":"61342","inReplyTo":"CACDhgro3KXD0O9ZdE1q46jmXE0O=vf-Z+ZX50WMqmRHAeowGAA@mail.gmail.com","subject":"Re: Possible git-diff bug when using exit-code with diff filters","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-21T10:42:18Z","receivedAt":"2024-04-21T10:42:27Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.04.24 um 03:13 schrieb German Lashevich:\n>> What did you do before the bug happened? (Steps to reproduce your issue)\n>\n> I configured a diff filter via gitattributes to use a custom script that,\n> sometimes, can change the files being compared in a way that there are no\n> differences between them.\n> Then I used `git diff --exit-code` on the changed file.\n>\n>> What did you expect to happen? (Expected behavior)\n>\n> I expected `git diff --exit-code` to return 0, since there are no differences\n> between the files after the filter is applied.\n>\n>> What happened instead? (Actual behavior)\n>\n> `git diff --exit-code` correctly produces no output, but returns 1.\n>\n>> What's different between what you expected and what actually happened?\n>\n> The difference is that `git diff --exit-code`, instead of returning 0, returns\n> 1 even when there is no output.\n>\n>> Anything else you want to add:\n>\n> I have prepared a repository with a test case that reproduces the issue.\n> You can find it at https://github.com/Zebradil/git-diff-exit-code-bug-repro\n> The Readme file in the repository contains instructions on how to reproduce\n> the issue.\n\nYou can more easily reproduce it by setting the environment variable\nGIT_EXTERNAL_DIFF or the configuration setting diff.external -- no\nattributes needed.\n\nThe output of external diffs does not influence the exit code currently.\nWe don't even have a way to find out if there was any output.\n\nThe patch below pipes it through a buffer and sets found_changes only if\nsomething was written. It fixes the error code for external diffs\nproducing no output, but breaks several am, pull and merge tests (t4150,\nt4151, t5520, t6424, t6430, t7611). So diff_from_contents doesn't work\nquite as I expected.\n\nAnd slurping in the whole output of external diffs is inefficient, of\ncourse. That could be fixed later by using a small buffer and writing\ndirectly as we read.\n\nRené\n\n\ndiff --git a/diff.c b/diff.c\nindex 108c187577..76631644e5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -40,6 +40,7 @@\n #include \"setup.h\"\n #include \"strmap.h\"\n #include \"ws.h\"\n+#include \"write-or-die.h\"\n\n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -4404,8 +4405,18 @@ static void run_external_diff(const char *pgm,\n \tdiff_free_filespec_data(one);\n \tdiff_free_filespec_data(two);\n \tcmd.use_shell = 1;\n-\tif (run_command(&cmd))\n-\t\tdie(_(\"external diff died, stopping at %s\"), name);\n+\tif (o->flags.exit_with_status) {\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\tif (capture_command(&cmd, &out, 0))\n+\t\t\tdie(_(\"external diff died, stopping at %s\"), name);\n+\t\tif (out.len)\n+\t\t\to->found_changes = 1;\n+\t\twrite_or_die(1, out.buf, out.len);\n+\t\tstrbuf_release(&out);\n+\t} else {\n+\t\tif (run_command(&cmd))\n+\t\t\tdie(_(\"external diff died, stopping at %s\"), name);\n+\t}\n\n \tremove_tempfile();\n }\n@@ -4851,6 +4862,7 @@ void diff_setup_done(struct diff_options *options)\n \t */\n\n \tif ((options->xdl_opts & XDF_WHITESPACE_FLAGS) ||\n+\t options->flags.exit_with_status ||\n \t options->ignore_regex_nr)\n \t\toptions->flags.diff_from_contents = 1;\n \telse\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex fdd865f7c3..26430ca66b 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -172,6 +172,14 @@ test_expect_success 'no diff with -diff' '\n \tgrep Binary out\n '\n\n+test_expect_success 'diff.external and --exit-code with output' '\n+\ttest_expect_code 1 git -c diff.external=echo diff --exit-code\n+'\n+\n+test_expect_success 'diff.external and --exit-code without output' '\n+\tgit -c diff.external=true diff --exit-code\n+'\n+\n echo NULZbetweenZwords | perl -pe 'y/Z/\\000/' > file\n\n test_expect_success 'force diff with \"diff\"' '\n\n"},{"id":"493277","messageId":"xmqqy1961sxf.fsf@gitster.g","threadId":"61342","inReplyTo":"82561c70-ec33-41bf-b036-52310ffc1926@web.de","subject":"Re: Possible git-diff bug when using exit-code with diff filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-21T18:17:48Z","receivedAt":"2024-04-21T18:18:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> You can more easily reproduce it by setting the environment variable\n> GIT_EXTERNAL_DIFF or the configuration setting diff.external -- no\n> attributes needed.\n\nIndeed.\n\nA much simpler fix may be to declare that these two features are\nimcompatible and fail the execution upfront, instead of just\nsilently ignoring one of the two options.\n\nAs a person who is very much used to the external diff not\ncontributing to the exit status (who also invented the external diff\ndriver interface), I would be a wrong person to judge if such a\nsimplified approach is desirable, of course, but just throwing it\nout as a food for thought.\n\nThanks.\n"},{"id":"493279","messageId":"03b501da941a$4896bdb0$d9c43910$@nexbridge.com","threadId":"61342","inReplyTo":"xmqqy1961sxf.fsf@gitster.g","subject":"RE: Possible git-diff bug when using exit-code with diff filters","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-04-21T18:32:30Z","receivedAt":"2024-04-21T18:32:46Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Sunday, April 21, 2024 2:18 PM, Junio C Hamano wrote:\n>René Scharfe writes:\n>\n>> You can more easily reproduce it by setting the environment variable\n>> GIT_EXTERNAL_DIFF or the configuration setting diff.external -- no\n>> attributes needed.\n>\n>Indeed.\n>\n>A much simpler fix may be to declare that these two features are imcompatible and\n>fail the execution upfront, instead of just silently ignoring one of the two options.\n>\n>As a person who is very much used to the external diff not contributing to the exit\n>status (who also invented the external diff driver interface), I would be a wrong\n>person to judge if such a simplified approach is desirable, of course, but just\n>throwing it out as a food for thought.\n\nMy 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.\n\nI 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.\n--Randall\n\n"},{"id":"493286","messageId":"xmqqh6fu1qk3.fsf@gitster.g","threadId":"61342","inReplyTo":"03b501da941a$4896bdb0$d9c43910$@nexbridge.com","subject":"Re: Possible git-diff bug when using exit-code with diff filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-21T19:09:00Z","receivedAt":"2024-04-21T19:09:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" writes:\n\n> On Sunday, April 21, 2024 2:18 PM, Junio C Hamano wrote:\n>>René Scharfe writes:\n>>\n>>> You can more easily reproduce it by setting the environment variable\n>>> GIT_EXTERNAL_DIFF or the configuration setting diff.external -- no\n>>> attributes needed.\n>>\n>>Indeed.\n>>\n>>A much simpler fix may be to declare that these two features are imcompatible and\n>>fail the execution upfront, instead of just silently ignoring one of the two options.\n>>\n>>As a person who is very much used to the external diff not contributing to the exit\n>>status (who also invented the external diff driver interface), I would be a wrong\n>>person to judge if such a simplified approach is desirable, of course, but just\n>>throwing it out as a food for thought.\n>\n> My suggestion would be to keep with a priority approach, where\n> GIT_EXTERNAL_DIFF overrides diff.external, assuming they set hold\n> to the same specification (the git-config page implies they do)\n> and GIT_EXTERNAL_DIFF overrides diff.external as I would expect.\n\nNobody in this discussion thread is hinting to change that, so I am\na bit confused where the above suggestion comes from...\n\n"},{"id":"493294","messageId":"03bf01da9429$10b55460$321ffd20$@nexbridge.com","threadId":"61342","inReplyTo":"xmqqh6fu1qk3.fsf@gitster.g","subject":"RE: Possible git-diff bug when using exit-code with diff filters","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-04-21T20:18:19Z","receivedAt":"2024-04-21T20:18:29Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Sunday, April 21, 2024 3:09 PM, Junio C Hamano wrote:\n> writes:\n>\n>> On Sunday, April 21, 2024 2:18 PM, Junio C Hamano wrote:\n>>>René Scharfe writes:\n>>>\n>>>> You can more easily reproduce it by setting the environment variable\n>>>> GIT_EXTERNAL_DIFF or the configuration setting diff.external -- no\n>>>> attributes needed.\n>>>\n>>>Indeed.\n>>>\n>>>A much simpler fix may be to declare that these two features are\n>>>imcompatible and fail the execution upfront, instead of just silently ignoring one\n>of the two options.\n>>>\n>>>As a person who is very much used to the external diff not\n>>>contributing to the exit status (who also invented the external diff\n>>>driver interface), I would be a wrong person to judge if such a\n>>>simplified approach is desirable, of course, but just throwing it out as a food for\n>thought.\n>>\n>> My suggestion would be to keep with a priority approach, where\n>> GIT_EXTERNAL_DIFF overrides diff.external, assuming they set hold to\n>> the same specification (the git-config page implies they do) and\n>> GIT_EXTERNAL_DIFF overrides diff.external as I would expect.\n>\n>Nobody in this discussion thread is hinting to change that, so I am a bit confused\n>where the above suggestion comes from...\n\nI must have misinterpreted. Please ignore my suggestion.\n\n"},{"id":"494085","messageId":"caa6cca4-182f-4788-b08e-35806879fdca@web.de","threadId":"61342","inReplyTo":"xmqqy1961sxf.fsf@gitster.g","subject":"Re: Possible git-diff bug when using exit-code with diff filters","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-05-05T10:19:50Z","receivedAt":"2024-05-05T10:19:58Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.04.24 um 20:17 schrieb Junio C Hamano:\n> A much simpler fix may be to declare that these two features are\n> imcompatible and fail the execution upfront, instead of just\n> silently ignoring one of the two options.\n\nIt would not be *that* simple -- if we want to error out upfront we'd\nhave to evaluate the attributes of all files (or all changed files)\nfirst to see whether they require an external diff.\n\nReporting the incompatibility in the middle of a diff would be easier,\nbut I don't see why we shouldn't support that combination. It takes\nsome effort, sure, but not prohibitively much.\n\nRené\n\n"},{"id":"494086","messageId":"ac611156-71e5-40f9-bce7-220a874ea91a@web.de","threadId":"61342","inReplyTo":"82561c70-ec33-41bf-b036-52310ffc1926@web.de","subject":"[PATCH 1/2] diff: report unmerged paths as changes in run_diff_cmd()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-05-05T10:19:56Z","receivedAt":"2024-05-05T10:20:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"You can ask the diff machinery to let the exit code indicate whether\nthere are changes, e.g. with --quiet. It as two ways to calculate that\nbit: The quick one assumes blobs with different hashes have different\ncontent, and the more elaborate way actually compares the contents,\npossibly applying transformations like ignoring whitespace.\n\nThe quick way considers an unmerged file to be a change and reports\nexit code 1, which makes sense.\n\nThe slower path uses the struct diff_options member found_changes to\nindicate whether the blobs differ even with the transformations applied.\nIt's not set for unmerged files, though, resulting in exit code 0.\n\nSet found_changes in run_diff_cmd() for unmerged files, for a consistent\nexit code of 1 if there's an unmerged file, regardless of whether\nwhitespace is ignored.\n\nSigned-off-by: René Scharfe \n---\n diff.c | 1 +\n t/t4046-diff-unmerged.sh | 8 ++++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 108c187577..ded9ac70df 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4555,6 +4555,7 @@ static void run_diff_cmd(const char *pgm,\n \t\t\t o, complete_rewrite);\n \t} else {\n \t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n+\t\to->found_changes = 1;\n \t}\n }\n\ndiff --git a/t/t4046-diff-unmerged.sh b/t/t4046-diff-unmerged.sh\nindex ffaf69335f..c606ee4c40 100755\n--- a/t/t4046-diff-unmerged.sh\n+++ b/t/t4046-diff-unmerged.sh\n@@ -96,4 +96,12 @@ test_expect_success 'diff --stat' '\n \ttest_cmp diff-stat.expect diff-stat.actual\n '\n\n+test_expect_success 'diff --quiet' '\n+\ttest_expect_code 1 git diff --cached --quiet\n+'\n+\n+test_expect_success 'diff --quiet --ignore-all-space' '\n+\ttest_expect_code 1 git diff --cached --quiet --ignore-all-space\n+'\n+\n test_done\n--\n2.45.0\n"},{"id":"494087","messageId":"e2e4a4e9-55db-403c-902d-fd8af3aea05c@web.de","threadId":"61342","inReplyTo":"82561c70-ec33-41bf-b036-52310ffc1926@web.de","subject":"[PATCH 2/2] diff: fix --exit-code with external diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-05-05T10:20:03Z","receivedAt":"2024-05-05T10:20:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"You can ask the diff machinery to let the exit code indicate whether\nthere are changes, e.g. with --exit-code. It as two ways to calculate\nthat bit: The quick one assumes blobs with different hashes have\ndifferent content, and the more elaborate way actually compares the\ncontents, possibly applying transformations like ignoring whitespace.\n\nAlways use the slower path by setting the flag diff_from_contents,\nbecause any of the files could have an external diff driver set via an\nattribute, which might consider binary differences irrelevant, like e.g.\ndiff -b.\n\nAnd don't stop walking that path in diff_flush() just because\noutput_format is unset. Instead run diff_flush_patch() with output\nredirected to /dev/null to get the exit status, like we already do for\nDIFF_FORMAT_NO_OUTPUT. That's consistent with the comments in diff.h:\n\n /* Same as output_format = 0 but we know that -s flag was given\n * and we should not give default value to output_format.\n */\n #define DIFF_FORMAT_NO_OUTPUT\t0x0800\n\nAn unset output_format is given e.g. by repo_index_has_changes(). We\ncould set it right there, but there may be other places, so simply let\ndiff_flush() handle it for all of them consistently.\n\nFinally, capture the output of the external diff and only register a\nchange by setting found_changes if the program wrote anything.\n\nReported-by: German Lashevich \nSigned-off-by: René Scharfe \n---\n diff.c | 33 ++++++++++++++++++++++++++++++---\n t/t4020-diff-external.sh | 8 ++++++++\n 2 files changed, 38 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ded9ac70df..cd3c8a3978 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -40,6 +40,7 @@\n #include \"setup.h\"\n #include \"strmap.h\"\n #include \"ws.h\"\n+#include \"write-or-die.h\"\n\n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -4404,8 +4405,33 @@ static void run_external_diff(const char *pgm,\n \tdiff_free_filespec_data(one);\n \tdiff_free_filespec_data(two);\n \tcmd.use_shell = 1;\n-\tif (run_command(&cmd))\n-\t\tdie(_(\"external diff died, stopping at %s\"), name);\n+\tif (o->flags.diff_from_contents) {\n+\t\tint got_output = 0;\n+\t\tcmd.out = -1;\n+\t\tif (start_command(&cmd))\n+\t\t\tdie(_(\"external diff died, stopping at %s\"), name);\n+\t\tfor (;;) {\n+\t\t\tchar buffer[8192];\n+\t\t\tssize_t len = xread(cmd.out, buffer, sizeof(buffer));\n+\t\t\tif (!len)\n+\t\t\t\tbreak;\n+\t\t\tif (len < 0)\n+\t\t\t\tdie(_(\"unable to read from external diff,\"\n+\t\t\t\t \" stopping at %s\"), name);\n+\t\t\tgot_output = 1;\n+\t\t\tif (write_in_full(1, buffer, len) < 0)\n+\t\t\t\tdie(_(\"unable to write output of external diff,\"\n+\t\t\t\t \" stopping at %s\"), name);\n+\t\t}\n+\t\tclose(cmd.out);\n+\t\tif (finish_command(&cmd))\n+\t\t\tdie(_(\"external diff died, stopping at %s\"), name);\n+\t\tif (got_output)\n+\t\t\to->found_changes = 1;\n+\t} else {\n+\t\tif (run_command(&cmd))\n+\t\t\tdie(_(\"external diff died, stopping at %s\"), name);\n+\t}\n\n \tremove_tempfile();\n }\n@@ -4852,6 +4878,7 @@ void diff_setup_done(struct diff_options *options)\n \t */\n\n \tif ((options->xdl_opts & XDF_WHITESPACE_FLAGS) ||\n+\t options->flags.exit_with_status ||\n \t options->ignore_regex_nr)\n \t\toptions->flags.diff_from_contents = 1;\n \telse\n@@ -6742,7 +6769,7 @@ void diff_flush(struct diff_options *options)\n \tif (output_format & DIFF_FORMAT_CALLBACK)\n \t\toptions->format_callback(q, options, options->format_callback_data);\n\n-\tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n+\tif ((!output_format || output_format & DIFF_FORMAT_NO_OUTPUT) &&\n \t options->flags.exit_with_status &&\n \t options->flags.diff_from_contents) {\n \t\t/*\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex fdd865f7c3..26430ca66b 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -172,6 +172,14 @@ test_expect_success 'no diff with -diff' '\n \tgrep Binary out\n '\n\n+test_expect_success 'diff.external and --exit-code with output' '\n+\ttest_expect_code 1 git -c diff.external=echo diff --exit-code\n+'\n+\n+test_expect_success 'diff.external and --exit-code without output' '\n+\tgit -c diff.external=true diff --exit-code\n+'\n+\n echo NULZbetweenZwords | perl -pe 'y/Z/\\000/' > file\n\n test_expect_success 'force diff with \"diff\"' '\n--\n2.45.0\n"},{"id":"494093","messageId":"99337bc2-a691-45f7-9b6f-74ededbd9a78@gmail.com","threadId":"61342","inReplyTo":"e2e4a4e9-55db-403c-902d-fd8af3aea05c@web.de","subject":"Re: [PATCH 2/2] diff: fix --exit-code with external diff","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-05-05T15:25:52Z","receivedAt":"2024-05-05T15:25:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi René\n\nThanks for working on this\n\nOn 05/05/2024 11:20, René Scharfe wrote:\n> Finally, capture the output of the external diff and only register a\n> change by setting found_changes if the program wrote anything.\n\nI worry that this will lead to regression reports like \nhttps://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/ \nas the external diff will not see a terminal on stdout anymore. I'm not \nreally clear why we ignore the exit code of the external diff command. \nMerge strategies are expected to exit 0 on success, 1 when there are \nconflicts and another non-zero value for other errors - it would be nice \nto do something similar here where 1 means \"there were differences\" but \nit is probably too late to do that without a config value to indicate \nthat we should trust the exit code.\n\nBest Wishes\n\nPhillip\n"},{"id":"494169","messageId":"xmqqh6fazwgh.fsf@gitster.g","threadId":"61342","inReplyTo":"caa6cca4-182f-4788-b08e-35806879fdca@web.de","subject":"Re: Possible git-diff bug when using exit-code with diff filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-06T17:22:54Z","receivedAt":"2024-05-06T17:23:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 21.04.24 um 20:17 schrieb Junio C Hamano:\n>> A much simpler fix may be to declare that these two features are\n>> imcompatible and fail the execution upfront, instead of just\n>> silently ignoring one of the two options.\n>\n> It would not be *that* simple -- if we want to error out upfront we'd\n> have to evaluate the attributes of all files (or all changed files)\n> first to see whether they require an external diff.\n\nYou're absolutely right.\n\n> Reporting the incompatibility in the middle of a diff would be easier,\n> but I don't see why we shouldn't support that combination. It takes\n> some effort, sure, but not prohibitively much.\n\nOK. Thanks.\n"},{"id":"494170","messageId":"xmqqcypyzw1i.fsf@gitster.g","threadId":"61342","inReplyTo":"99337bc2-a691-45f7-9b6f-74ededbd9a78@gmail.com","subject":"Re: [PATCH 2/2] diff: fix --exit-code with external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-06T17:31:53Z","receivedAt":"2024-05-06T17:31:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> Finally, capture the output of the external diff and only register a\n>> change by setting found_changes if the program wrote anything.\n>\n> I worry that this will lead to regression reports like\n> https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/\n> as the external diff will not see a terminal on stdout anymore.\n\nHmph. If it were Git that the external diff script eventually\ninvokes that changes the behaviour (e.g. color and use of pager)\nbased on the tty-ness of the output, we could use tricks like\nGIT_PAGER_IN_USE to propagate tty-ness down to the subprocess,\nbut Git is not the only thing involved here, so it is not a very\ngood general solution.\n\n> I'm\n> not really clear why we ignore the exit code of the external diff\n> command. Merge strategies are expected to exit 0 on success, 1 when\n> there are conflicts and another non-zero value for other errors - it\n> would be nice to do something similar here where 1 means \"there were\n> differences\" but it is probably too late to do that without a config\n> value to indicate that we should trust the exit code.\n\nYeah, that thought crossed my mind. If we were designing the system\nfrom scratch today, that would certainly be what we would do. I\nsuspect that it is because external diff drivers were invented way\nbefore \"diff --exit-code\" was introduced.\n\nThanks.\n"},{"id":"494173","messageId":"4bf12c41-a6ea-4939-ba76-e2c784952eaa@web.de","threadId":"61342","inReplyTo":"99337bc2-a691-45f7-9b6f-74ededbd9a78@gmail.com","subject":"Re: [PATCH 2/2] diff: fix --exit-code with external diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-05-06T18:23:06Z","receivedAt":"2024-05-06T18:23:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.05.24 um 17:25 schrieb Phillip Wood:\n> On 05/05/2024 11:20, René Scharfe wrote:\n>> Finally, capture the output of the external diff and only register a\n>> change by setting found_changes if the program wrote anything.\n>\n> I worry that this will lead to regression reports like\n> https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/\n> as the external diff will not see a terminal on stdout anymore.\n\nSo external diffs might no longer color their output if invoked using\nthe option --exit-code. I assume this can be worked around by adding an\noption like --color=always to the diff command. This warrants a note in\nthe docs at the very least, though.\n\nBut thinking about it, the external diff could be a GUI program that\ndoesn't write to its stdout at all. Or it could write something if the\nfiles' contents match and stay silent if there are differences, weird as\nthat may be.\n\n> I'm not really clear why we ignore the exit code of the external diff\n> command.\n\nHistorical reasons, I guess.\n\n> Merge strategies are expected to exit 0 on success, 1 when there are\n> conflicts and another non-zero value for other errors - it would be\n> nice to do something similar here where 1 means \"there were\n> differences\" but it is probably too late to do that without a config\n> value to indicate that we should trust the exit code.\nRight, such a diff command protocol v2 would not need to pipe the\noutput through an inspection loop. Sounds like a good idea. It's\nunfortunate that it would increase the configuration surface, which is\nnot in an acute need to expand. We could advertise the new option when\ndying due to the unsupported combination of --exit-code and external\ndiff, but that's in equal parts helpful and obnoxious, I feel.\n\nRené\n"},{"id":"494324","messageId":"539926ac-4274-4b4b-a2ee-aa48af791124@gmail.com","threadId":"61342","inReplyTo":"4bf12c41-a6ea-4939-ba76-e2c784952eaa@web.de","subject":"Re: [PATCH 2/2] diff: fix --exit-code with external diff","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-05-08T15:25:47Z","receivedAt":"2024-05-08T15:25:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi René\n\nOn 06/05/2024 19:23, René Scharfe wrote:\n> Am 05.05.24 um 17:25 schrieb Phillip Wood:\n>> Merge strategies are expected to exit 0 on success, 1 when there are\n>> conflicts and another non-zero value for other errors - it would be\n>> nice to do something similar here where 1 means \"there were\n>> differences\" but it is probably too late to do that without a config\n>> value to indicate that we should trust the exit code.\n> Right, such a diff command protocol v2 would not need to pipe the\n> output through an inspection loop. Sounds like a good idea. It's\n> unfortunate that it would increase the configuration surface, which is\n> not in an acute need to expand. We could advertise the new option when\n> dying due to the unsupported combination of --exit-code and external\n> diff, but that's in equal parts helpful and obnoxious, I feel.\n\nYes, diff dying would be annoying but the message would be useful.\n\nThinking about the external diff and some of the other diff options I \nwonder what we should do when options like \"--quiet\" and \"--name-only\" \nare combined with an external diff (I haven't checked the current \nbehavior). If we introduced a diff command protocol v2 we could include \na way to pass through \"--quiet\" though maybe just redirecting the stdout \nof the external command to /dev/null and using the exit code would be \nsufficient.\n\nBest Wishes\n\nPhillip\n\nP.S. I haven't forgotten about our unit-test discussion but I'm afraid \nit will probably be the middle of next month before I have time to reply.\n"},{"id":"494546","messageId":"f3454199-2eed-4ca1-82db-7f1419b3084e@web.de","threadId":"61342","inReplyTo":"539926ac-4274-4b4b-a2ee-aa48af791124@gmail.com","subject":"Re: [PATCH 2/2] diff: fix --exit-code with external diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-05-11T20:32:12Z","receivedAt":"2024-05-11T20:32:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.05.24 um 17:25 schrieb phillip.wood123@gmail.com:\n> Hi René\n>\n> On 06/05/2024 19:23, René Scharfe wrote:\n>> Am 05.05.24 um 17:25 schrieb Phillip Wood:\n>>> Merge strategies are expected to exit 0 on success, 1 when there are\n>>> conflicts and another non-zero value for other errors - it would be\n>>> nice to do something similar here where 1 means \"there were\n>>> differences\" but it is probably too late to do that without a config\n>>> value to indicate that we should trust the exit code.\n>> Right, such a diff command protocol v2 would not need to pipe the\n>> output through an inspection loop.  Sounds like a good idea.  It's\n>> unfortunate that it would increase the configuration surface, which is\n>> not in an acute need to expand.  We could advertise the new option when\n>> dying due to the unsupported combination of --exit-code and external\n>> diff, but that's in equal parts helpful and obnoxious, I feel.\n>\n> Yes, diff dying would be annoying but the message would be useful.\n\nHaving poked at it a bit more, I wonder if we need to add some further\nnuance/trick to avoid having to reject certain combinations of options.\n\nCurrently external diffs can't signal content equality. That doesn't\nmatter for trivial equality (where content and mode of the two files\nmatch), as this case is always handled by the diff machinery already.\nOnly lossy comparisons (e.g. ignoring whitespace) even have the need to\nsignal equality.\n\nIf we (continue to) assume that external diffs are lossless then we\ndon't need to change the code, just document that assumption. And add a\nway to specify lossy external diffs that can signal whether they found\ninteresting differences, to address the originally reported shortcoming.\n\nIs there an official term for comparisons that ignore some details?\n\"Lossy\" is rather used for compression. Filtered, partial, selective?\n\n> Thinking about the external diff and some of the other diff options I\n> wonder what we should do when options like \"--quiet\" and\n> \"--name-only\" are combined with an external diff (I haven't checked\n> the current behavior). If we introduced a diff command protocol v2 we\n> could include a way to pass through \"--quiet\" though maybe just\n> redirecting the stdout of the external command to /dev/null and using\n> the exit code would be sufficient.\n\nThe current code uses shortcuts like that. For lossy external diffs we\nneed to turn (some of) them off. Lots of possible combinations with\nspecial handling -- needs lots of tests for reasonable coverage. :-/\n\n> P.S. I haven't forgotten about our unit-test discussion but I'm\n> afraid it will probably be the middle of next month before I have\n> time to reply.\nNo worries; reminds me to polish some unit-test patches, though. I\nget distracted a lot these days..\n\nRené\n"},{"id":"494558","messageId":"e97d91db-cbb5-43f5-84d0-289681620558@web.de","threadId":"61342","inReplyTo":"f3454199-2eed-4ca1-82db-7f1419b3084e@web.de","subject":"Re: [PATCH 2/2] diff: fix --exit-code with external diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-05-12T09:38:44Z","receivedAt":"2024-05-12T09:38:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.05.24 um 22:32 schrieb René Scharfe:\n> Am 08.05.24 um 17:25 schrieb phillip.wood123@gmail.com:\n>> Hi René\n>>\n>> On 06/05/2024 19:23, René Scharfe wrote:\n>>> Am 05.05.24 um 17:25 schrieb Phillip Wood:\n>>>> Merge strategies are expected to exit 0 on success, 1 when there are\n>>>> conflicts and another non-zero value for other errors - it would be\n>>>> nice to do something similar here where 1 means \"there were\n>>>> differences\" but it is probably too late to do that without a config\n>>>> value to indicate that we should trust the exit code.\n>>> Right, such a diff command protocol v2 would not need to pipe the\n>>> output through an inspection loop.  Sounds like a good idea.  It's\n>>> unfortunate that it would increase the configuration surface, which is\n>>> not in an acute need to expand.  We could advertise the new option when\n>>> dying due to the unsupported combination of --exit-code and external\n>>> diff, but that's in equal parts helpful and obnoxious, I feel.\n>>\n>> Yes, diff dying would be annoying but the message would be useful.\n>\n> Having poked at it a bit more, I wonder if we need to add some further\n> nuance/trick to avoid having to reject certain combinations of options.\n>\n> Currently external diffs can't signal content equality. That doesn't\n> matter for trivial equality (where content and mode of the two files\n> match), as this case is always handled by the diff machinery already.\n> Only lossy comparisons (e.g. ignoring whitespace) even have the need to\n> signal equality.\n>\n> If we (continue to) assume that external diffs are lossless then we\n> don't need to change the code, just document that assumption. And add a\n> way to specify lossy external diffs that can signal whether they found\n> interesting differences, to address the originally reported shortcoming.\n\nOne more step in that direction: If we continue to use exit code 0 to\nmean \"notable changes present\" and redefine 1 to mean \"non-trivial\nequality\" instead of \"fatal error\" then we don't need to add any config\noptions.\n\nA downside is that the exit codes of diff(1) have the opposite meaning.\nWhich is weird in itself: You say \"give me a diff\" and it answers \"true,\nI got nothing for you\" or \"false, I actually had to print that dang\nthing\". Inherited from cmp(1), I guess. Which I further guess got it\nfrom bcmp(3)?\n\nBut we can't directly use diff(1) anyway because we pass either one or\nsix parameters instead of the two that it would expect. Our external\ndiff calling protocol is already special enough that we can freely\nchose the meaning of exit codes.\n\nAny other downsides? Am I missing something?\n\nRené\n"},{"id":"496342","messageId":"6fa51a62-5dc1-4865-9f79-eaf7d65189ab@web.de","threadId":"61342","inReplyTo":"e2e4a4e9-55db-403c-902d-fd8af3aea05c@web.de","subject":"[PATCH v2 0/3] diff: fix --exit-code with external diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-05T08:31:32Z","receivedAt":"2024-06-05T08:31:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Changes since v1:\n- old patch 1 was merged\n- old patch 2 was merged and reverted\n- new patch 1 adds tests that exercises diff --exit-code and --quiet\n together with all methods for specifying external diffs\n- new patch 2 adds a struct to store the flag that patch 3 adds\n- patch 3 adds configuration options and an environment variable to\n accept diff(1)-style exit codes from external diffs\n\n t4020: test exit code with external diffs\n userdiff: add and use struct external_diff\n diff: let external diffs report that changes are uninteresting\n\n Documentation/config/diff.txt | 14 ++++++++\n Documentation/git.txt | 7 ++++\n Documentation/gitattributes.txt | 5 +++\n diff.c | 62 ++++++++++++++++++++++++---------\n t/t4020-diff-external.sh | 44 +++++++++++++++++++++++\n userdiff.c | 8 +++--\n userdiff.h | 7 +++-\n 7 files changed, 128 insertions(+), 19 deletions(-)\n\n--\n2.45.2\n"},{"id":"496344","messageId":"2a5167fc-871b-4968-bacb-eaff4d7a2630@web.de","threadId":"61342","inReplyTo":"6fa51a62-5dc1-4865-9f79-eaf7d65189ab@web.de","subject":"[PATCH v2 1/3] t4020: test exit code with external diffs","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-05T08:35:23Z","receivedAt":"2024-06-05T08:35:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add tests to check the exit code of git diff with its options --quiet\nand --exit-code when using an external diff program. Currently we\ncannot tell whether it found significant changes or not.\n\nSigned-off-by: René Scharfe \n---\n t/t4020-diff-external.sh | 33 +++++++++++++++++++++++++++++++++\n 1 file changed, 33 insertions(+)\n\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex fdd865f7c3..bed640b2b1 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -172,6 +172,39 @@ test_expect_success 'no diff with -diff' '\n \tgrep Binary out\n '\n\n+check_external_exit_code () {\n+\texpect_code=$1\n+\tcommand_code=$2\n+\toption=$3\n+\n+\tcommand=\"exit $command_code;\"\n+\tdesc=\"external diff '$command'\"\n+\n+\ttest_expect_success \"$desc via attribute with $option\" \"\n+\t\ttest_config diff.foo.command \\\"$command\\\" &&\n+\t\techo \\\"file diff=foo\\\" >.gitattributes &&\n+\t\ttest_expect_code $expect_code git diff $option\n+\t\"\n+\n+\ttest_expect_success \"$desc via diff.external with $option\" \"\n+\t\ttest_config diff.external \\\"$command\\\" &&\n+\t\t>.gitattributes &&\n+\t\ttest_expect_code $expect_code git diff $option\n+\t\"\n+\n+\ttest_expect_success \"$desc via GIT_EXTERNAL_DIFF with $option\" \"\n+\t\t>.gitattributes &&\n+\t\ttest_expect_code $expect_code env \\\n+\t\t\tGIT_EXTERNAL_DIFF=\\\"$command\\\" \\\n+\t\t\tgit diff $option\n+\t\"\n+}\n+\n+check_external_exit_code 1 0 --exit-code\n+check_external_exit_code 1 0 --quiet\n+check_external_exit_code 128 1 --exit-code\n+check_external_exit_code 1 1 --quiet # we don't even call the program\n+\n echo NULZbetweenZwords | perl -pe 'y/Z/\\000/' > file\n\n test_expect_success 'force diff with \"diff\"' '\n--\n2.45.2\n"},{"id":"496345","messageId":"87251602-1dc8-4115-ad4a-b9160ec082a9@web.de","threadId":"61342","inReplyTo":"6fa51a62-5dc1-4865-9f79-eaf7d65189ab@web.de","subject":"[PATCH v2 2/3] userdiff: add and use struct external_diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-05T08:36:31Z","receivedAt":"2024-06-05T08:36:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Wrap the string specifying the external diff command in a new struct to\nsimplify adding attributes, which the next patch will do.\n\nMake sure external_diff() still returns NULL if neither the environment\nvariable GIT_EXTERNAL_DIFF nor the configuration option diff.external is\nset, to continue allowing its use in a boolean context.\n\nUse a designated initializer for the default builtin userdiff driver to\nadjust to the type change of the second struct member. Spelling out\nonly the non-zero members improves readability as a nice side-effect.\n\nNo functional change intended.\n\nSigned-off-by: René Scharfe \n---\n diff.c | 32 +++++++++++++++++---------------\n userdiff.c | 4 ++--\n userdiff.h | 6 +++++-\n 3 files changed, 24 insertions(+), 18 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ded9ac70df..286d093bfa 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -57,7 +57,7 @@ static int diff_color_moved_ws_default;\n static int diff_context_default = 3;\n static int diff_interhunk_context_default;\n static const char *diff_word_regex_cfg;\n-static const char *external_diff_cmd_cfg;\n+static struct external_diff external_diff_cfg;\n static const char *diff_order_file_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n@@ -429,7 +429,7 @@ int git_diff_ui_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"diff.external\"))\n-\t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n+\t\treturn git_config_string(&external_diff_cfg.cmd, var, value);\n \tif (!strcmp(var, \"diff.wordregex\"))\n \t\treturn git_config_string(&diff_word_regex_cfg, var, value);\n \tif (!strcmp(var, \"diff.orderfile\"))\n@@ -546,18 +546,20 @@ static char *quote_two(const char *one, const char *two)\n \treturn strbuf_detach(&res, NULL);\n }\n\n-static const char *external_diff(void)\n+static const struct external_diff *external_diff(void)\n {\n-\tstatic const char *external_diff_cmd = NULL;\n+\tstatic struct external_diff external_diff_env, *external_diff_ptr;\n \tstatic int done_preparing = 0;\n\n \tif (done_preparing)\n-\t\treturn external_diff_cmd;\n-\texternal_diff_cmd = xstrdup_or_null(getenv(\"GIT_EXTERNAL_DIFF\"));\n-\tif (!external_diff_cmd)\n-\t\texternal_diff_cmd = external_diff_cmd_cfg;\n+\t\treturn external_diff_ptr;\n+\texternal_diff_env.cmd = xstrdup_or_null(getenv(\"GIT_EXTERNAL_DIFF\"));\n+\tif (external_diff_env.cmd)\n+\t\texternal_diff_ptr = &external_diff_env;\n+\telse if (external_diff_cfg.cmd)\n+\t\texternal_diff_ptr = &external_diff_cfg;\n \tdone_preparing = 1;\n-\treturn external_diff_cmd;\n+\treturn external_diff_ptr;\n }\n\n /*\n@@ -4373,7 +4375,7 @@ static void add_external_diff_name(struct repository *r,\n * infile2 infile2-sha1 infile2-mode [ rename-to ]\n *\n */\n-static void run_external_diff(const char *pgm,\n+static void run_external_diff(const struct external_diff *pgm,\n \t\t\t const char *name,\n \t\t\t const char *other,\n \t\t\t struct diff_filespec *one,\n@@ -4384,7 +4386,7 @@ static void run_external_diff(const char *pgm,\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n\n-\tstrvec_push(&cmd.args, pgm);\n+\tstrvec_push(&cmd.args, pgm->cmd);\n \tstrvec_push(&cmd.args, name);\n\n \tif (one && two) {\n@@ -4510,7 +4512,7 @@ static void fill_metainfo(struct strbuf *msg,\n \t}\n }\n\n-static void run_diff_cmd(const char *pgm,\n+static void run_diff_cmd(const struct external_diff *pgm,\n \t\t\t const char *name,\n \t\t\t const char *other,\n \t\t\t const char *attr_path,\n@@ -4528,8 +4530,8 @@ static void run_diff_cmd(const char *pgm,\n \tif (o->flags.allow_external || !o->ignore_driver_algorithm)\n \t\tdrv = userdiff_find_by_path(o->repo->index, attr_path);\n\n-\tif (o->flags.allow_external && drv && drv->external)\n-\t\tpgm = drv->external;\n+\tif (o->flags.allow_external && drv && drv->external.cmd)\n+\t\tpgm = &drv->external;\n\n \tif (msg) {\n \t\t/*\n@@ -4595,7 +4597,7 @@ static void strip_prefix(int prefix_length, const char **namep, const char **oth\n\n static void run_diff(struct diff_filepair *p, struct diff_options *o)\n {\n-\tconst char *pgm = external_diff();\n+\tconst struct external_diff *pgm = external_diff();\n \tstruct strbuf msg;\n \tstruct diff_filespec *one = p->one;\n \tstruct diff_filespec *two = p->two;\ndiff --git a/userdiff.c b/userdiff.c\nindex 82bc76b910..f47e2d9f36 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -333,7 +333,7 @@ PATTERNS(\"scheme\",\n \t \"|([^][)(}{[ \\t])+\"),\n PATTERNS(\"tex\", \"^(\\\\\\\\((sub)*section|chapter|part)\\\\*{0,1}\\\\{.*)$\",\n \t \"\\\\\\\\[a-zA-Z@]+|\\\\\\\\.|([a-zA-Z0-9]|[^\\x01-\\x7f])+\"),\n-{ \"default\", NULL, NULL, -1, { NULL, 0 } },\n+{ .name = \"default\", .binary = -1 },\n };\n #undef PATTERNS\n #undef IPATTERN\n@@ -445,7 +445,7 @@ int userdiff_config(const char *k, const char *v)\n \tif (!strcmp(type, \"binary\"))\n \t\treturn parse_tristate(&drv->binary, k, v);\n \tif (!strcmp(type, \"command\"))\n-\t\treturn git_config_string(&drv->external, k, v);\n+\t\treturn git_config_string(&drv->external.cmd, k, v);\n \tif (!strcmp(type, \"textconv\"))\n \t\treturn git_config_string(&drv->textconv, k, v);\n \tif (!strcmp(type, \"cachetextconv\"))\ndiff --git a/userdiff.h b/userdiff.h\nindex d726804c3e..7e2b28e88d 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -11,9 +11,13 @@ struct userdiff_funcname {\n \tint cflags;\n };\n\n+struct external_diff {\n+\tconst char *cmd;\n+};\n+\n struct userdiff_driver {\n \tconst char *name;\n-\tconst char *external;\n+\tstruct external_diff external;\n \tconst char *algorithm;\n \tint binary;\n \tstruct userdiff_funcname funcname;\n--\n2.45.2\n"},{"id":"496346","messageId":"99a99e5c-4fe4-413a-9281-363e280716b8@web.de","threadId":"61342","inReplyTo":"6fa51a62-5dc1-4865-9f79-eaf7d65189ab@web.de","subject":"[PATCH v2 3/3] diff: let external diffs report that changes are uninteresting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-05T08:38:36Z","receivedAt":"2024-06-05T08:38:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The options --exit-code and --quiet instruct git diff to indicate\nwhether it found any significant changes by exiting with code 1 if it\ndid and 0 if there were none. Currently this doesn't work if external\ndiff programs are involved, as we have no way to learn what they found.\n\nAdd that ability in the form of the new configuration options\ndiff.trustExitCode and diff..trustExitCode and the environment\nvariable GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE. They pair with the config\noptions diff.external and diff..command and the environment\nvariable GIT_EXTERNAL_DIFF, respectively.\n\nThe new options are off by default, keeping the old behavior. Enabling\nthem indicates that the external diff returns exit code 1 if it finds\nsignificant changes and 0 if it doesn't, like diff(1).\n\nThe name of the new options is taken from the git difftool and mergetool\noptions of similar purpose. (There they enable passing on the exit code\nof a diff tool and to infer whether a merge done by a merge tool is\nsuccessful.)\n\nThe new feature sets the diff flag diff_from_contents in\ndiff_setup_done() if we need the exit code and are allowed to call\nexternal diffs. This disables the optimization that avoids calling the\nprogram with --quiet. Add it back by skipping the call if the external\ndiff is not able to report empty diffs. We can only do that check after\nevaluating the file-specific attributes in run_external_diff().\n\nI considered checking the output of the external diff to check whether\nits empty. It was added as 11be65cfa4 (diff: fix --exit-code with\nexternal diff, 2024-05-05) and quickly reverted, as it does not work\nwith external diffs that do not write to stdout. There's no reason why\na graphical diff tool would even need to write anything there at all.\n\nI also considered using a non-zero exit code for empty diffs, which\ncould be done without adding new configuration options. We'd need to\ndisable the optimization that allows git diff --quiet to skip calling\nexternal diffs, though -- that might be quite surprising if graphical\ndiff programs are involved. And assigning the opposite meaning of the\nexit codes compared to diff(1) and git diff --exit-code to the external\ndiff can cause unnecessary confusion.\n\nSuggested-by: Phillip Wood \nSigned-off-by: René Scharfe \n---\n Documentation/config/diff.txt | 14 ++++++++++++++\n Documentation/git.txt | 7 +++++++\n Documentation/gitattributes.txt | 5 +++++\n diff.c | 30 +++++++++++++++++++++++++++++-\n t/t4020-diff-external.sh | 23 +++++++++++++++++------\n userdiff.c | 4 ++++\n userdiff.h | 1 +\n 7 files changed, 77 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt\nindex 5ce7b91f1d..485a13236d 100644\n--- a/Documentation/config/diff.txt\n+++ b/Documentation/config/diff.txt\n@@ -79,6 +79,13 @@ diff.external::\n \tyou want to use an external diff program only on a subset of\n \tyour files, you might want to use linkgit:gitattributes[5] instead.\n\n+diff.trustExitCode::\n+\tIf this boolean value is set to true then the `diff.external`\n+\tcommand is expected to return exit code 1 if it finds\n+\tsignificant changes and 0 if it doesn't, like diff(1). If it's\n+\tfalse then the `diff.external` command is expected to always\n+\treturn exit code 0. Defaults to false.\n+\n diff.ignoreSubmodules::\n \tSets the default value of --ignore-submodules. Note that this\n \taffects only 'git diff' Porcelain, and not lower level 'diff'\n@@ -164,6 +171,13 @@ diff..command::\n \tThe custom diff driver command. See linkgit:gitattributes[5]\n \tfor details.\n\n+diff..trustExitCode::\n+\tIf this boolean value is set to true then the\n+\t`diff..command` command is expected to return exit code\n+\t1 if it finds significant changes and 0 if it doesn't, like\n+\tdiff(1). If it's false then the `diff.external` command is\n+\texpected to always return exit code 0. Defaults to false.\n+\n diff..xfuncname::\n \tThe regular expression that the diff driver should use to\n \trecognize the hunk header. A built-in pattern may also be used.\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex a31a70acca..81b806416c 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -644,6 +644,13 @@ parameter, .\n For each path `GIT_EXTERNAL_DIFF` is called, two environment variables,\n `GIT_DIFF_PATH_COUNTER` and `GIT_DIFF_PATH_TOTAL` are set.\n\n+`GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE`::\n+\tSetting this environment variable indicates the the program\n+\tspecified with `GIT_EXTERNAL_DIFF` returns exit code 1 if it\n+\tfinds significant changes and 0 if it doesn't, like diff(1).\n+\tIf it's not set, the program is expected to always return\n+\texit code 0.\n+\n `GIT_DIFF_PATH_COUNTER`::\n \tA 1-based counter incremented by one for every path.\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 4338d023d9..80cae17f37 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -776,6 +776,11 @@ with the above configuration, i.e. `j-c-diff`, with 7\n parameters, just like `GIT_EXTERNAL_DIFF` program is called.\n See linkgit:git[1] for details.\n\n+If the program is able to ignore certain changes (similar to\n+`git diff --ignore-space-change`), then also set the option\n+`trustExitCode` to true. It is then expected to return exit code 1 if\n+it finds significant changes and 0 if it doesn't.\n+\n Setting the internal diff algorithm\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\ndiff --git a/diff.c b/diff.c\nindex 286d093bfa..3dff81ec7d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -430,6 +430,10 @@ int git_diff_ui_config(const char *var, const char *value,\n \t}\n \tif (!strcmp(var, \"diff.external\"))\n \t\treturn git_config_string(&external_diff_cfg.cmd, var, value);\n+\tif (!strcmp(var, \"diff.trustexitcode\")) {\n+\t\texternal_diff_cfg.trust_exit_code = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"diff.wordregex\"))\n \t\treturn git_config_string(&diff_word_regex_cfg, var, value);\n \tif (!strcmp(var, \"diff.orderfile\"))\n@@ -554,6 +558,8 @@ static const struct external_diff *external_diff(void)\n \tif (done_preparing)\n \t\treturn external_diff_ptr;\n \texternal_diff_env.cmd = xstrdup_or_null(getenv(\"GIT_EXTERNAL_DIFF\"));\n+\tif (git_env_bool(\"GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE\", 0))\n+\t\texternal_diff_env.trust_exit_code = 1;\n \tif (external_diff_env.cmd)\n \t\texternal_diff_ptr = &external_diff_env;\n \telse if (external_diff_cfg.cmd)\n@@ -4385,6 +4391,18 @@ static void run_external_diff(const struct external_diff *pgm,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n+\tint rc;\n+\n+\t/*\n+\t * Trivial equality is handled by diff_unmodified_pair() before\n+\t * we get here. If we don't need to show the diff and the\n+\t * external diff program lacks the ability to tell us whether\n+\t * it's empty then we consider it non-empty without even asking.\n+\t */\n+\tif (!(o->output_format & DIFF_FORMAT_PATCH) && !pgm->trust_exit_code) {\n+\t\to->found_changes = 1;\n+\t\treturn;\n+\t}\n\n \tstrvec_push(&cmd.args, pgm->cmd);\n \tstrvec_push(&cmd.args, name);\n@@ -4406,7 +4424,10 @@ static void run_external_diff(const struct external_diff *pgm,\n \tdiff_free_filespec_data(one);\n \tdiff_free_filespec_data(two);\n \tcmd.use_shell = 1;\n-\tif (run_command(&cmd))\n+\trc = run_command(&cmd);\n+\tif ((!pgm->trust_exit_code && !rc) || (pgm->trust_exit_code && rc == 1))\n+\t\to->found_changes = 1;\n+\telse if (!pgm->trust_exit_code || rc)\n \t\tdie(_(\"external diff died, stopping at %s\"), name);\n\n \tremove_tempfile();\n@@ -4924,6 +4945,13 @@ void diff_setup_done(struct diff_options *options)\n \t\toptions->flags.exit_with_status = 1;\n \t}\n\n+\t/*\n+\t * External diffs could declare non-identical contents equal\n+\t * (think diff --ignore-space-change).\n+\t */\n+\tif (options->flags.allow_external && options->flags.exit_with_status)\n+\t\toptions->flags.diff_from_contents = 1;\n+\n \toptions->diff_path_counter = 0;\n\n \tif (options->flags.follow_renames)\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex bed640b2b1..bbb49b8a0a 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -175,19 +175,22 @@ test_expect_success 'no diff with -diff' '\n check_external_exit_code () {\n \texpect_code=$1\n \tcommand_code=$2\n-\toption=$3\n+\ttrust_exit_code=$3\n+\toption=$4\n\n \tcommand=\"exit $command_code;\"\n-\tdesc=\"external diff '$command'\"\n+\tdesc=\"external diff '$command' with trustExitCode=$trust_exit_code\"\n\n \ttest_expect_success \"$desc via attribute with $option\" \"\n \t\ttest_config diff.foo.command \\\"$command\\\" &&\n+\t\ttest_config diff.foo.trustExitCode $trust_exit_code &&\n \t\techo \\\"file diff=foo\\\" >.gitattributes &&\n \t\ttest_expect_code $expect_code git diff $option\n \t\"\n\n \ttest_expect_success \"$desc via diff.external with $option\" \"\n \t\ttest_config diff.external \\\"$command\\\" &&\n+\t\ttest_config diff.trustExitCode $trust_exit_code &&\n \t\t>.gitattributes &&\n \t\ttest_expect_code $expect_code git diff $option\n \t\"\n@@ -196,14 +199,22 @@ check_external_exit_code () {\n \t\t>.gitattributes &&\n \t\ttest_expect_code $expect_code env \\\n \t\t\tGIT_EXTERNAL_DIFF=\\\"$command\\\" \\\n+\t\t\tGIT_EXTERNAL_DIFF_TRUST_EXIT_CODE=$trust_exit_code \\\n \t\t\tgit diff $option\n \t\"\n }\n\n-check_external_exit_code 1 0 --exit-code\n-check_external_exit_code 1 0 --quiet\n-check_external_exit_code 128 1 --exit-code\n-check_external_exit_code 1 1 --quiet # we don't even call the program\n+check_external_exit_code 1 0 off --exit-code\n+check_external_exit_code 1 0 off --quiet\n+check_external_exit_code 128 1 off --exit-code\n+check_external_exit_code 1 1 off --quiet # we don't even call the program\n+\n+check_external_exit_code 0 0 on --exit-code\n+check_external_exit_code 0 0 on --quiet\n+check_external_exit_code 1 1 on --exit-code\n+check_external_exit_code 1 1 on --quiet\n+check_external_exit_code 128 2 on --exit-code\n+check_external_exit_code 128 2 on --quiet\n\n echo NULZbetweenZwords | perl -pe 'y/Z/\\000/' > file\n\ndiff --git a/userdiff.c b/userdiff.c\nindex f47e2d9f36..6cdfb147c3 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -446,6 +446,10 @@ int userdiff_config(const char *k, const char *v)\n \t\treturn parse_tristate(&drv->binary, k, v);\n \tif (!strcmp(type, \"command\"))\n \t\treturn git_config_string(&drv->external.cmd, k, v);\n+\tif (!strcmp(type, \"trustexitcode\")) {\n+\t\tdrv->external.trust_exit_code = git_config_bool(k, v);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(type, \"textconv\"))\n \t\treturn git_config_string(&drv->textconv, k, v);\n \tif (!strcmp(type, \"cachetextconv\"))\ndiff --git a/userdiff.h b/userdiff.h\nindex 7e2b28e88d..5f1c8cc49c 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -13,6 +13,7 @@ struct userdiff_funcname {\n\n struct external_diff {\n \tconst char *cmd;\n+\tunsigned trust_exit_code:1;\n };\n\n struct userdiff_driver {\n--\n2.45.2\n"},{"id":"496379","messageId":"xmqqed9buyj5.fsf@gitster.g","threadId":"61342","inReplyTo":"6fa51a62-5dc1-4865-9f79-eaf7d65189ab@web.de","subject":"Re: [PATCH v2 0/3] diff: fix --exit-code with external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-05T16:47:42Z","receivedAt":"2024-06-05T16:47:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> t4020: test exit code with external diffs\n> userdiff: add and use struct external_diff\n> diff: let external diffs report that changes are uninteresting\n\nOK, we now need to mark each external diff driver if we want to\nhonor their exit status, but that awkwardness is to be blamed on the\noriginal design. Thanks for addressing the misdesign I made years\nago in the mildest way possible not to introduce any unnecessary\nregressions.\n"},{"id":"496441","messageId":"1c055e96-76b0-4174-a8e2-cb6df041fabf@kdbg.org","threadId":"61342","inReplyTo":"99a99e5c-4fe4-413a-9281-363e280716b8@web.de","subject":"Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-06-06T06:39:17Z","receivedAt":"2024-06-06T07:23:45Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 05.06.24 um 10:38 schrieb René Scharfe:\n> +diff.trustExitCode::\n> +\tIf this boolean value is set to true then the `diff.external`\n> +\tcommand is expected to return exit code 1 if it finds\n> +\tsignificant changes and 0 if it doesn't, like diff(1). If it's\n> +\tfalse then the `diff.external` command is expected to always\n> +\treturn exit code 0. Defaults to false.\n\nI find this somewhat unclear. What are the consequences when this value\nis set to false, but the command exits with code other than 0? Is it\n\n If it's false then any exit code other than 0 of the `diff.external`\n command is treated as an error.\n\n-- Hannes\n\n"},{"id":"496466","messageId":"55389f35-5762-4b86-81bf-b9fe956815d2@web.de","threadId":"61342","inReplyTo":"1c055e96-76b0-4174-a8e2-cb6df041fabf@kdbg.org","subject":"Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-06T08:28:24Z","receivedAt":"2024-06-06T08:28:32Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.06.24 um 08:39 schrieb Johannes Sixt:\n> Am 05.06.24 um 10:38 schrieb René Scharfe:\n>> +diff.trustExitCode::\n>> +\tIf this boolean value is set to true then the `diff.external`\n>> +\tcommand is expected to return exit code 1 if it finds\n>> +\tsignificant changes and 0 if it doesn't, like diff(1). If it's\n>> +\tfalse then the `diff.external` command is expected to always\n>> +\treturn exit code 0. Defaults to false.\n>\n> I find this somewhat unclear. What are the consequences when this value\n> is set to false, but the command exits with code other than 0? Is it\n>\n> If it's false then any exit code other than 0 of the `diff.external`\n> command is treated as an error.\n\nYes, unexpected exit codes are reported as errors.\n\nIf trustExitCode is false and --quiet is given then the execution of\nexternal diffs is skipped, so in that situation there is no exit code to\nexpect, though. Not sure how to express it concisely, though. This\nattempt looks a bit bloated:\n\n--quiet::\n Disable all output of the program. Implies `--exit-code`.\n Disables execution of external diff helpers whose exit code\n is not trusted, i.e. their respective configuration option\n\t`diff.trustExitCode` or `diff..trustExitCode` or\n\tenvironment variable `GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE` is\n\tfalse.\n\nMight be worth documenting this original behavior somehow, anyway. It\nmakes sense in hindsight, but surprised me a bit when I wrote the tests.\n\nRené\n"},{"id":"496476","messageId":"7ef4a566-6d55-4924-b02c-38137c15791a@gmail.com","threadId":"61342","inReplyTo":"99a99e5c-4fe4-413a-9281-363e280716b8@web.de","subject":"Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-06T09:48:35Z","receivedAt":"2024-06-06T09:48:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi René\n\nOn 05/06/2024 09:38, René Scharfe wrote:\n> The options --exit-code and --quiet instruct git diff to indicate\n> whether it found any significant changes by exiting with code 1 if it\n> did and 0 if there were none. Currently this doesn't work if external\n> diff programs are involved, as we have no way to learn what they found.\n> \n> Add that ability in the form of the new configuration options\n> diff.trustExitCode and diff..trustExitCode and the environment\n> variable GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE. They pair with the config\n> options diff.external and diff..command and the environment\n> variable GIT_EXTERNAL_DIFF, respectively.\n> \n> The new options are off by default, keeping the old behavior. Enabling\n> them indicates that the external diff returns exit code 1 if it finds\n> significant changes and 0 if it doesn't, like diff(1).\n> \n> The name of the new options is taken from the git difftool and mergetool\n> options of similar purpose. (There they enable passing on the exit code\n> of a diff tool and to infer whether a merge done by a merge tool is\n> successful.)\n> \n> The new feature sets the diff flag diff_from_contents in\n> diff_setup_done() if we need the exit code and are allowed to call\n> external diffs. This disables the optimization that avoids calling the\n> program with --quiet. Add it back by skipping the call if the external\n> diff is not able to report empty diffs. We can only do that check after\n> evaluating the file-specific attributes in run_external_diff().\n> \n> I considered checking the output of the external diff to check whether\n> its empty. It was added as 11be65cfa4 (diff: fix --exit-code with\n> external diff, 2024-05-05) and quickly reverted, as it does not work\n> with external diffs that do not write to stdout. There's no reason why\n> a graphical diff tool would even need to write anything there at all.\n> \n> I also considered using a non-zero exit code for empty diffs, which\n> could be done without adding new configuration options. We'd need to\n> disable the optimization that allows git diff --quiet to skip calling\n> external diffs, though -- that might be quite surprising if graphical\n> diff programs are involved. And assigning the opposite meaning of the\n> exit codes compared to diff(1) and git diff --exit-code to the external\n> diff can cause unnecessary confusion.\n\nThanks for the comprehensive commit message, I agree that it is much \nless confusing to follow the exit code convention of diff(1). This is \nlooking good, I've left a couple of comments below.\n\n\n> +diff.trustExitCode::\n> +\tIf this boolean value is set to true then the `diff.external`\n> +\tcommand is expected to return exit code 1 if it finds\n> +\tsignificant changes and 0 if it doesn't, like diff(1). If it's\n> +\tfalse then the `diff.external` command is expected to always\n> +\treturn exit code 0. Defaults to false.\n\nI wonder if \"significant changes\" is a bit ambiguous and as Johannes \nsaid it would be good to mention that other exit codes are errors. Perhaps\n\n\tIf this boolean value is set to true then the `diff.external`\n\tcommand is expected to return exit code 0 if it considers the\n\tinput files to be equal and 1 if they are not, like diff(1).\n\tIf it is false then the `diff.external` command is expected to\n\talways return exit code 0. In both cases any other exit code\n\tis considered to be an error. Defaults to false.\n\n\n> \tstrvec_push(&cmd.args, pgm->cmd);\n> \tstrvec_push(&cmd.args, name);\n> @@ -4406,7 +4424,10 @@ static void run_external_diff(const struct external_diff *pgm,\n> \tdiff_free_filespec_data(one);\n> \tdiff_free_filespec_data(two);\n> \tcmd.use_shell = 1;\n\nShould we be redirecting stdout to /dev/null here when the user passes \n--quiet?\n\n> -\tif (run_command(&cmd))\n> +\trc = run_command(&cmd);\n> +\tif ((!pgm->trust_exit_code && !rc) || (pgm->trust_exit_code && rc == 1))\n> +\t\to->found_changes = 1;\n> +\telse if (!pgm->trust_exit_code || rc)\n> \t\tdie(_(\"external diff died, stopping at %s\"), name);\n\nThis is a bit fiddly because we may, or may not trust the exit code but \nthe logic here looks good.\n\n> -check_external_exit_code 1 0 --exit-code\n> -check_external_exit_code 1 0 --quiet\n> -check_external_exit_code 128 1 --exit-code\n> -check_external_exit_code 1 1 --quiet # we don't even call the program\n> +check_external_exit_code 1 0 off --exit-code\n> +check_external_exit_code 1 0 off --quiet\n> +check_external_exit_code 128 1 off --exit-code\n> +check_external_exit_code 1 1 off --quiet # we don't even call the program\n> +\n> +check_external_exit_code 0 0 on --exit-code\n> +check_external_exit_code 0 0 on --quiet\n> +check_external_exit_code 1 1 on --exit-code\n> +check_external_exit_code 1 1 on --quiet\n> +check_external_exit_code 128 2 on --exit-code\n> +check_external_exit_code 128 2 on --quiet\n\nIt would be nice if the tests checked that --quiet does not produce any \noutput on stdout.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"496535","messageId":"xmqqsexqnkb7.fsf@gitster.g","threadId":"61342","inReplyTo":"55389f35-5762-4b86-81bf-b9fe956815d2@web.de","subject":"Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-06T15:49:00Z","receivedAt":"2024-06-06T15:49:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>>> +diff.trustExitCode::\n>>> +\tIf this boolean value is set to true then the `diff.external`\n>>> +\tcommand is expected to return exit code 1 if it finds\n>>> +\tsignificant changes and 0 if it doesn't, like diff(1). If it's\n>>> +\tfalse then the `diff.external` command is expected to always\n>>> +\treturn exit code 0. Defaults to false.\n>>\n>> I find this somewhat unclear. What are the consequences when this value\n>> is set to false, but the command exits with code other than 0? Is it\n>>\n>> If it's false then any exit code other than 0 of the `diff.external`\n>> command is treated as an error.\n>\n> Yes, unexpected exit codes are reported as errors.\n>\n> If trustExitCode is false and --quiet is given then the execution of\n> external diffs is skipped, so in that situation there is no exit code to\n> expect, though. Not sure how to express it concisely, though. This\n> attempt looks a bit bloated:\n>\n> --quiet::\n> Disable all output of the program. Implies `--exit-code`.\n> Disables execution of external diff helpers whose exit code\n> is not trusted, i.e. their respective configuration option\n> \t`diff.trustExitCode` or `diff..trustExitCode` or\n> \tenvironment variable `GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE` is\n> \tfalse.\n>\n> Might be worth documenting this original behavior somehow, anyway. It\n> makes sense in hindsight, but surprised me a bit when I wrote the tests.\n\nYes. The explanation of trustExitCode makes sense as an explanation\nof what the variable means (i.e. if set, we pay attention to the\nexit code of the external diff driver, otherwise a non-zero exit is\nan error), but I suspect that readers are _more_ interested in how\nthe external diff driver contributes to the answer to the \"has this\npath been changed?\" question when the variable is on and off. And\nthe above description of \"--quiet\" does help answer that question\nsomewhat.\n\n"},{"id":"496633","messageId":"524e4728-a63c-46fc-af73-35933c1906ee@web.de","threadId":"61342","inReplyTo":"7ef4a566-6d55-4924-b02c-38137c15791a@gmail.com","subject":"Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-07T08:19:50Z","receivedAt":"2024-06-07T08:20:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.06.24 um 11:48 schrieb Phillip Wood:\n> On 05/06/2024 09:38, René Scharfe wrote:\n>> +diff.trustExitCode::\n>> +    If this boolean value is set to true then the `diff.external`\n>> +    command is expected to return exit code 1 if it finds\n>> +    significant changes and 0 if it doesn't, like diff(1).  If it's\n>> +    false then the `diff.external` command is expected to always\n>> +    return exit code 0.  Defaults to false.\n>\n> I wonder if \"significant changes\" is a bit ambiguous and as Johannes\n> said it would be good to mention that other exit codes are errors.\n> Perhaps\n>\n>     If this boolean value is set to true then the `diff.external`\n>     command is expected to return exit code 0 if it considers the\n>     input files to be equal and 1 if they are not, like diff(1).\n>     If it is false then the `diff.external` command is expected to\n>     always return exit code 0. In both cases any other exit code\n>     is considered to be an error. Defaults to false.\n\nThe first part looks like an obvious improvement. Stating that\nunexpected exit codes are errors looks tautological to me, though.\nPerhaps mentioning that git diff stops at that point adds would be\nuseful? Or perhaps a tad of redundancy is just what's needed here?\n\n>\n>\n>>       strvec_push(&cmd.args, pgm->cmd);\n>>       strvec_push(&cmd.args, name);\n>> @@ -4406,7 +4424,10 @@ static void run_external_diff(const struct external_diff *pgm,\n>>       diff_free_filespec_data(one);\n>>       diff_free_filespec_data(two);\n>>       cmd.use_shell = 1;\n>\n> Should we be redirecting stdout to /dev/null here when the user\n> passes --quiet?\n\nOh, yes. We didn't even start the program before, but with the\npatch it becomes necessary. Good find!\n\n>\n>> -    if (run_command(&cmd))\n>> +    rc = run_command(&cmd);\n>> +    if ((!pgm->trust_exit_code && !rc) || (pgm->trust_exit_code && rc == 1))\n>> +        o->found_changes = 1;\n>> +    else if (!pgm->trust_exit_code || rc)\n>>           die(_(\"external diff died, stopping at %s\"), name);\n>\n> This is a bit fiddly because we may, or may not trust the exit code\n> but the logic here looks good.\n\nYeah, it's not as clear as it could be. One reason is that it avoids\nredundancy at the action side and the other is adherence to the rule of\nnot explicitly comparing to zero. We could enumerate all cases:\n\n\tif (!pgm->trust_exit_code && rc == 0)\n\t\to->found_changes = 1;\n\telse if (pgm->trust_exit_code && rc == 0)\n\t\t; /* nothing */\n\telse if (pgm->trust_exit_code && rc == 1)\n\t\to->found_changes = 1;\n\telse\n\t\tdie(_(\"external diff died, stopping at %s\"), name);\n\nOr avoid redundancy in the conditions:\n\n\tif (pgm->trust_exit_code) {\n\t\tif (rc == 1)\n\t\t\to->found_changes = 1;\n\t\telse if (rc != 0)\n\t\t\tdie(_(\"external diff died, stopping at %s\"), name);\n\t} else {\n\t\tif (rc == 0)\n\t\t\to->found_changes = 1;\n\t\telse\n\t\t\tdie(_(\"external diff died, stopping at %s\"), name);\n\t}\n\nWe should not get into bit twiddling, though:\n\n\to->found_changes |= rc == pgm->trust_exit_code;\n\tif ((unsigned)rc > pgm->trust_exit_code)\n\t\tdie(_(\"external diff died, stopping at %s\"), name);\n\n>\n>> -check_external_exit_code   1 0 --exit-code\n>> -check_external_exit_code   1 0 --quiet\n>> -check_external_exit_code 128 1 --exit-code\n>> -check_external_exit_code   1 1 --quiet # we don't even call the program\n>> +check_external_exit_code   1 0 off --exit-code\n>> +check_external_exit_code   1 0 off --quiet\n>> +check_external_exit_code 128 1 off --exit-code\n>> +check_external_exit_code   1 1 off --quiet # we don't even call the program\n>> +\n>> +check_external_exit_code   0 0 on --exit-code\n>> +check_external_exit_code   0 0 on --quiet\n>> +check_external_exit_code   1 1 on --exit-code\n>> +check_external_exit_code   1 1 on --quiet\n>> +check_external_exit_code 128 2 on --exit-code\n>> +check_external_exit_code 128 2 on --quiet\n>\n> It would be nice if the tests checked that --quiet does not produce\n> any output on stdout.\n\nRight, we need this after unlocking external diff execution with\n--quiet.\n\nRené\n"},{"id":"496730","messageId":"168fecaa-2ebd-4897-b0ba-3bd2a37c01e7@web.de","threadId":"61342","inReplyTo":"e2e4a4e9-55db-403c-902d-fd8af3aea05c@web.de","subject":"[PATCH v3 0/3] diff: fix --exit-code with external diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-09T07:35:40Z","receivedAt":"2024-06-09T07:35:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Changes since v2:\n- rebase; config strings are no longer const\n- document the handling of unexpected exit codes\n- document that external diffs are skipped with --quiet and trustExitCode=off\n- silence external diff output with --quiet\n- check output in tests\n- test diff runs without --exit-code and --quiet as well\n- slightly untangle the exit code handling code to make it easier to read\n- fix copy/paste error in documentation of diff..trustExitCode\n\n t4020: test exit code with external diffs\n userdiff: add and use struct external_diff\n diff: let external diffs report that changes are uninteresting\n\n Documentation/config/diff.txt | 18 +++++++++\n Documentation/diff-options.txt | 5 +++\n Documentation/git.txt | 10 +++++\n Documentation/gitattributes.txt | 5 +++\n diff.c | 68 +++++++++++++++++++++++++--------\n t/t4020-diff-external.sh | 66 ++++++++++++++++++++++++++++++++\n userdiff.c | 8 +++-\n userdiff.h | 7 +++-\n 8 files changed, 168 insertions(+), 19 deletions(-)\n\nRange-Diff gegen v2:\n1: 118aba667b < -: ---------- t4020: test exit code with external diffs\n-: ---------- > 1: d59f0c6fdf t4020: test exit code with external diffs\n2: 0b4dabebe1 ! 2: 4ad160ab1f userdiff: add and use struct external_diff\n @@ diff.c\n @@ diff.c: static int diff_color_moved_ws_default;\n static int diff_context_default = 3;\n static int diff_interhunk_context_default;\n - static const char *diff_word_regex_cfg;\n --static const char *external_diff_cmd_cfg;\n + static char *diff_word_regex_cfg;\n +-static char *external_diff_cmd_cfg;\n +static struct external_diff external_diff_cfg;\n - static const char *diff_order_file_cfg;\n + static char *diff_order_file_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n @@ diff.c: int git_diff_ui_config(const char *var, const char *value,\n @@ userdiff.h: struct userdiff_funcname {\n };\n\n +struct external_diff {\n -+\tconst char *cmd;\n ++\tchar *cmd;\n +};\n +\n struct userdiff_driver {\n \tconst char *name;\n --\tconst char *external;\n +-\tchar *external;\n +\tstruct external_diff external;\n - \tconst char *algorithm;\n + \tchar *algorithm;\n \tint binary;\n \tstruct userdiff_funcname funcname;\n3: 4d54ca8281 ! 3: 29c8d3b61a diff: let external diffs report that changes are uninteresting\n @@ Commit message\n diff is not able to report empty diffs. We can only do that check after\n evaluating the file-specific attributes in run_external_diff().\n\n + If we do run the external diff with --quiet, send its output to\n + /dev/null.\n +\n I considered checking the output of the external diff to check whether\n its empty. It was added as 11be65cfa4 (diff: fix --exit-code with\n external diff, 2024-05-05) and quickly reverted, as it does not work\n @@ Documentation/config/diff.txt: diff.external::\n \tyour files, you might want to use linkgit:gitattributes[5] instead.\n\n +diff.trustExitCode::\n -+\tIf this boolean value is set to true then the `diff.external`\n -+\tcommand is expected to return exit code 1 if it finds\n -+\tsignificant changes and 0 if it doesn't, like diff(1). If it's\n -+\tfalse then the `diff.external` command is expected to always\n -+\treturn exit code 0. Defaults to false.\n ++\tIf this boolean value is set to true then the\n ++\t`diff.external` command is expected to return exit code\n ++\t0 if it considers the input files to be equal or 1 if it\n ++\tconsiders them to be different, like `diff(1)`.\n ++\tIf it is set to false, which is the default, then the command\n ++\tis expected to return exit code 0 regardless of equality.\n ++\tAny other exit code causes Git to report a fatal error.\n +\n diff.ignoreSubmodules::\n \tSets the default value of --ignore-submodules. Note that this\n @@ Documentation/config/diff.txt: diff..command::\n +diff..trustExitCode::\n +\tIf this boolean value is set to true then the\n +\t`diff..command` command is expected to return exit code\n -+\t1 if it finds significant changes and 0 if it doesn't, like\n -+\tdiff(1). If it's false then the `diff.external` command is\n -+\texpected to always return exit code 0. Defaults to false.\n ++\t0 if it considers the input files to be equal or 1 if it\n ++\tconsiders them to be different, like `diff(1)`.\n ++\tIf it is set to false, which is the default, then the command\n ++\tis expected to return exit code 0 regardless of equality.\n ++\tAny other exit code causes Git to report a fatal error.\n +\n diff..xfuncname::\n \tThe regular expression that the diff driver should use to\n \trecognize the hunk header. A built-in pattern may also be used.\n\n + ## Documentation/diff-options.txt ##\n +@@ Documentation/diff-options.txt: ifndef::git-log[]\n +\n + --quiet::\n + \tDisable all output of the program. Implies `--exit-code`.\n +-\tDisables execution of external diff helpers.\n ++\tDisables execution of external diff helpers whose exit code\n ++\tis not trusted, i.e. their respective configuration option\n ++\t`diff.trustExitCode` or `diff..trustExitCode` or\n ++\tenvironment variable `GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE` is\n ++\tfalse.\n + endif::git-log[]\n + endif::git-format-patch[]\n +\n +\n ## Documentation/git.txt ##\n @@ Documentation/git.txt: parameter, .\n For each path `GIT_EXTERNAL_DIFF` is called, two environment variables,\n `GIT_DIFF_PATH_COUNTER` and `GIT_DIFF_PATH_TOTAL` are set.\n\n +`GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE`::\n -+\tSetting this environment variable indicates the the program\n -+\tspecified with `GIT_EXTERNAL_DIFF` returns exit code 1 if it\n -+\tfinds significant changes and 0 if it doesn't, like diff(1).\n -+\tIf it's not set, the program is expected to always return\n -+\texit code 0.\n ++\tIf this Boolean environment variable is set to true then the\n ++\t`GIT_EXTERNAL_DIFF` command is expected to return exit code\n ++\t0 if it considers the input files to be equal or 1 if it\n ++\tconsiders them to be different, like `diff(1)`.\n ++\tIf it is set to false, which is the default, then the command\n ++\tis expected to return exit code 0 regardless of equality.\n ++\tAny other exit code causes Git to report a fatal error.\n ++\n +\n `GIT_DIFF_PATH_COUNTER`::\n \tA 1-based counter incremented by one for every path.\n @@ diff.c: static void run_external_diff(const struct external_diff *pgm,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n ++\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH);\n +\tint rc;\n +\n +\t/*\n @@ diff.c: static void run_external_diff(const struct external_diff *pgm,\n +\t * external diff program lacks the ability to tell us whether\n +\t * it's empty then we consider it non-empty without even asking.\n +\t */\n -+\tif (!(o->output_format & DIFF_FORMAT_PATCH) && !pgm->trust_exit_code) {\n ++\tif (!pgm->trust_exit_code && quiet) {\n +\t\to->found_changes = 1;\n +\t\treturn;\n +\t}\n @@ diff.c: static void run_external_diff(const struct external_diff *pgm,\n \tdiff_free_filespec_data(two);\n \tcmd.use_shell = 1;\n -\tif (run_command(&cmd))\n ++\tcmd.no_stdout = quiet;\n +\trc = run_command(&cmd);\n -+\tif ((!pgm->trust_exit_code && !rc) || (pgm->trust_exit_code && rc == 1))\n ++\tif (!pgm->trust_exit_code && rc == 0)\n ++\t\to->found_changes = 1;\n ++\telse if (pgm->trust_exit_code && rc == 0)\n ++\t\t; /* nothing */\n ++\telse if (pgm->trust_exit_code && rc == 1)\n +\t\to->found_changes = 1;\n -+\telse if (!pgm->trust_exit_code || rc)\n ++\telse\n \t\tdie(_(\"external diff died, stopping at %s\"), name);\n\n \tremove_tempfile();\n @@ diff.c: void diff_setup_done(struct diff_options *options)\n \tif (options->flags.follow_renames)\n\n ## t/t4020-diff-external.sh ##\n -@@ t/t4020-diff-external.sh: test_expect_success 'no diff with -diff' '\n - check_external_exit_code () {\n - \texpect_code=$1\n - \tcommand_code=$2\n --\toption=$3\n -+\ttrust_exit_code=$3\n -+\toption=$4\n -\n - \tcommand=\"exit $command_code;\"\n +@@ t/t4020-diff-external.sh: check_external_diff () {\n + \texpect_out=$2\n + \texpect_err=$3\n + \tcommand_code=$4\n +-\tshift 4\n ++\ttrust_exit_code=$5\n ++\tshift 5\n + \toptions=\"$@\"\n +\n + \tcommand=\"echo output; exit $command_code;\"\n -\tdesc=\"external diff '$command'\"\n +\tdesc=\"external diff '$command' with trustExitCode=$trust_exit_code\"\n + \twith_options=\"${options:+ with }$options\"\n\n - \ttest_expect_success \"$desc via attribute with $option\" \"\n + \ttest_expect_success \"$desc via attribute$with_options\" \"\n \t\ttest_config diff.foo.command \\\"$command\\\" &&\n +\t\ttest_config diff.foo.trustExitCode $trust_exit_code &&\n \t\techo \\\"file diff=foo\\\" >.gitattributes &&\n - \t\ttest_expect_code $expect_code git diff $option\n - \t\"\n + \t\ttest_expect_code $expect_code git diff $options >out 2>err &&\n + \t\ttest_cmp $expect_out out &&\n +@@ t/t4020-diff-external.sh: check_external_diff () {\n\n - \ttest_expect_success \"$desc via diff.external with $option\" \"\n + \ttest_expect_success \"$desc via diff.external$with_options\" \"\n \t\ttest_config diff.external \\\"$command\\\" &&\n +\t\ttest_config diff.trustExitCode $trust_exit_code &&\n \t\t>.gitattributes &&\n - \t\ttest_expect_code $expect_code git diff $option\n - \t\"\n -@@ t/t4020-diff-external.sh: check_external_exit_code () {\n + \t\ttest_expect_code $expect_code git diff $options >out 2>err &&\n + \t\ttest_cmp $expect_out out &&\n +@@ t/t4020-diff-external.sh: check_external_diff () {\n \t\t>.gitattributes &&\n \t\ttest_expect_code $expect_code env \\\n \t\t\tGIT_EXTERNAL_DIFF=\\\"$command\\\" \\\n +\t\t\tGIT_EXTERNAL_DIFF_TRUST_EXIT_CODE=$trust_exit_code \\\n - \t\t\tgit diff $option\n - \t\"\n - }\n -\n --check_external_exit_code 1 0 --exit-code\n --check_external_exit_code 1 0 --quiet\n --check_external_exit_code 128 1 --exit-code\n --check_external_exit_code 1 1 --quiet # we don't even call the program\n -+check_external_exit_code 1 0 off --exit-code\n -+check_external_exit_code 1 0 off --quiet\n -+check_external_exit_code 128 1 off --exit-code\n -+check_external_exit_code 1 1 off --quiet # we don't even call the program\n + \t\t\tgit diff $options >out 2>err &&\n + \t\ttest_cmp $expect_out out &&\n + \t\ttest_cmp $expect_err err\n +@@ t/t4020-diff-external.sh: test_expect_success 'setup output files' '\n + \techo \"fatal: external diff died, stopping at file\" >error\n + '\n +\n +-check_external_diff 0 output empty 0\n +-check_external_diff 128 output error 1\n +-\n +-check_external_diff 1 output empty 0 --exit-code\n +-check_external_diff 128 output error 1 --exit-code\n +-\n +-check_external_diff 1 empty empty 0 --quiet\n +-check_external_diff 1 empty empty 1 --quiet # we don't even call the program\n ++check_external_diff 0 output empty 0 off\n ++check_external_diff 128 output error 1 off\n ++check_external_diff 0 output empty 0 on\n ++check_external_diff 0 output empty 1 on\n ++check_external_diff 128 output error 2 on\n ++\n ++check_external_diff 1 output empty 0 off --exit-code\n ++check_external_diff 128 output error 1 off --exit-code\n ++check_external_diff 0 output empty 0 on --exit-code\n ++check_external_diff 1 output empty 1 on --exit-code\n ++check_external_diff 128 output error 2 on --exit-code\n +\n -+check_external_exit_code 0 0 on --exit-code\n -+check_external_exit_code 0 0 on --quiet\n -+check_external_exit_code 1 1 on --exit-code\n -+check_external_exit_code 1 1 on --quiet\n -+check_external_exit_code 128 2 on --exit-code\n -+check_external_exit_code 128 2 on --quiet\n ++check_external_diff 1 empty empty 0 off --quiet\n ++check_external_diff 1 empty empty 1 off --quiet # we don't even call the program\n ++check_external_diff 0 empty empty 0 on --quiet\n ++check_external_diff 1 empty empty 1 on --quiet\n ++check_external_diff 128 empty error 2 on --quiet\n\n echo NULZbetweenZwords | perl -pe 'y/Z/\\000/' > file\n\n @@ userdiff.h\n @@ userdiff.h: struct userdiff_funcname {\n\n struct external_diff {\n - \tconst char *cmd;\n + \tchar *cmd;\n +\tunsigned trust_exit_code:1;\n };\n\n--\n2.45.2\n"},{"id":"496731","messageId":"79a951f8-cf35-4159-a90e-f95d69773413@web.de","threadId":"61342","inReplyTo":"168fecaa-2ebd-4897-b0ba-3bd2a37c01e7@web.de","subject":"[PATCH v3 1/3] t4020: test exit code with external diffs","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-09T07:38:24Z","receivedAt":"2024-06-09T07:38:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add tests to check the exit code of git diff with its options --quiet\nand --exit-code when using an external diff program. Currently we\ncannot tell whether it found significant changes or not.\n\nWhile at it, document briefly that --quiet turns off execution of\nexternal diff programs because that behavior surprised me for a moment\nwhile writing the tests.\n\nSigned-off-by: René Scharfe \n---\n Documentation/diff-options.txt | 1 +\n t/t4020-diff-external.sh | 53 ++++++++++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex c7df20e571..6b73daf540 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -820,6 +820,7 @@ ifndef::git-log[]\n\n --quiet::\n \tDisable all output of the program. Implies `--exit-code`.\n+\tDisables execution of external diff helpers.\n endif::git-log[]\n endif::git-format-patch[]\n\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex fdd865f7c3..4070523cdb 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -172,6 +172,59 @@ test_expect_success 'no diff with -diff' '\n \tgrep Binary out\n '\n\n+check_external_diff () {\n+\texpect_code=$1\n+\texpect_out=$2\n+\texpect_err=$3\n+\tcommand_code=$4\n+\tshift 4\n+\toptions=\"$@\"\n+\n+\tcommand=\"echo output; exit $command_code;\"\n+\tdesc=\"external diff '$command'\"\n+\twith_options=\"${options:+ with }$options\"\n+\n+\ttest_expect_success \"$desc via attribute$with_options\" \"\n+\t\ttest_config diff.foo.command \\\"$command\\\" &&\n+\t\techo \\\"file diff=foo\\\" >.gitattributes &&\n+\t\ttest_expect_code $expect_code git diff $options >out 2>err &&\n+\t\ttest_cmp $expect_out out &&\n+\t\ttest_cmp $expect_err err\n+\t\"\n+\n+\ttest_expect_success \"$desc via diff.external$with_options\" \"\n+\t\ttest_config diff.external \\\"$command\\\" &&\n+\t\t>.gitattributes &&\n+\t\ttest_expect_code $expect_code git diff $options >out 2>err &&\n+\t\ttest_cmp $expect_out out &&\n+\t\ttest_cmp $expect_err err\n+\t\"\n+\n+\ttest_expect_success \"$desc via GIT_EXTERNAL_DIFF$with_options\" \"\n+\t\t>.gitattributes &&\n+\t\ttest_expect_code $expect_code env \\\n+\t\t\tGIT_EXTERNAL_DIFF=\\\"$command\\\" \\\n+\t\t\tgit diff $options >out 2>err &&\n+\t\ttest_cmp $expect_out out &&\n+\t\ttest_cmp $expect_err err\n+\t\"\n+}\n+\n+test_expect_success 'setup output files' '\n+\t: >empty &&\n+\techo output >output &&\n+\techo \"fatal: external diff died, stopping at file\" >error\n+'\n+\n+check_external_diff 0 output empty 0\n+check_external_diff 128 output error 1\n+\n+check_external_diff 1 output empty 0 --exit-code\n+check_external_diff 128 output error 1 --exit-code\n+\n+check_external_diff 1 empty empty 0 --quiet\n+check_external_diff 1 empty empty 1 --quiet # we don't even call the program\n+\n echo NULZbetweenZwords | perl -pe 'y/Z/\\000/' > file\n\n test_expect_success 'force diff with \"diff\"' '\n--\n2.45.2\n"},{"id":"496732","messageId":"0dd72a49-1c6d-4cd9-9257-d942443e1bc3@web.de","threadId":"61342","inReplyTo":"168fecaa-2ebd-4897-b0ba-3bd2a37c01e7@web.de","subject":"[PATCH v3 2/3] userdiff: add and use struct external_diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-09T07:39:08Z","receivedAt":"2024-06-09T07:39:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Wrap the string specifying the external diff command in a new struct to\nsimplify adding attributes, which the next patch will do.\n\nMake sure external_diff() still returns NULL if neither the environment\nvariable GIT_EXTERNAL_DIFF nor the configuration option diff.external is\nset, to continue allowing its use in a boolean context.\n\nUse a designated initializer for the default builtin userdiff driver to\nadjust to the type change of the second struct member. Spelling out\nonly the non-zero members improves readability as a nice side-effect.\n\nNo functional change intended.\n\nSigned-off-by: René Scharfe \n---\n diff.c | 32 +++++++++++++++++---------------\n userdiff.c | 4 ++--\n userdiff.h | 6 +++++-\n 3 files changed, 24 insertions(+), 18 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e70301df76..4b86c61631 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -57,7 +57,7 @@ static int diff_color_moved_ws_default;\n static int diff_context_default = 3;\n static int diff_interhunk_context_default;\n static char *diff_word_regex_cfg;\n-static char *external_diff_cmd_cfg;\n+static struct external_diff external_diff_cfg;\n static char *diff_order_file_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n@@ -431,7 +431,7 @@ int git_diff_ui_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"diff.external\"))\n-\t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n+\t\treturn git_config_string(&external_diff_cfg.cmd, var, value);\n \tif (!strcmp(var, \"diff.wordregex\"))\n \t\treturn git_config_string(&diff_word_regex_cfg, var, value);\n \tif (!strcmp(var, \"diff.orderfile\"))\n@@ -548,18 +548,20 @@ static char *quote_two(const char *one, const char *two)\n \treturn strbuf_detach(&res, NULL);\n }\n\n-static const char *external_diff(void)\n+static const struct external_diff *external_diff(void)\n {\n-\tstatic const char *external_diff_cmd = NULL;\n+\tstatic struct external_diff external_diff_env, *external_diff_ptr;\n \tstatic int done_preparing = 0;\n\n \tif (done_preparing)\n-\t\treturn external_diff_cmd;\n-\texternal_diff_cmd = xstrdup_or_null(getenv(\"GIT_EXTERNAL_DIFF\"));\n-\tif (!external_diff_cmd)\n-\t\texternal_diff_cmd = external_diff_cmd_cfg;\n+\t\treturn external_diff_ptr;\n+\texternal_diff_env.cmd = xstrdup_or_null(getenv(\"GIT_EXTERNAL_DIFF\"));\n+\tif (external_diff_env.cmd)\n+\t\texternal_diff_ptr = &external_diff_env;\n+\telse if (external_diff_cfg.cmd)\n+\t\texternal_diff_ptr = &external_diff_cfg;\n \tdone_preparing = 1;\n-\treturn external_diff_cmd;\n+\treturn external_diff_ptr;\n }\n\n /*\n@@ -4375,7 +4377,7 @@ static void add_external_diff_name(struct repository *r,\n * infile2 infile2-sha1 infile2-mode [ rename-to ]\n *\n */\n-static void run_external_diff(const char *pgm,\n+static void run_external_diff(const struct external_diff *pgm,\n \t\t\t const char *name,\n \t\t\t const char *other,\n \t\t\t struct diff_filespec *one,\n@@ -4386,7 +4388,7 @@ static void run_external_diff(const char *pgm,\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n\n-\tstrvec_push(&cmd.args, pgm);\n+\tstrvec_push(&cmd.args, pgm->cmd);\n \tstrvec_push(&cmd.args, name);\n\n \tif (one && two) {\n@@ -4512,7 +4514,7 @@ static void fill_metainfo(struct strbuf *msg,\n \t}\n }\n\n-static void run_diff_cmd(const char *pgm,\n+static void run_diff_cmd(const struct external_diff *pgm,\n \t\t\t const char *name,\n \t\t\t const char *other,\n \t\t\t const char *attr_path,\n@@ -4530,8 +4532,8 @@ static void run_diff_cmd(const char *pgm,\n \tif (o->flags.allow_external || !o->ignore_driver_algorithm)\n \t\tdrv = userdiff_find_by_path(o->repo->index, attr_path);\n\n-\tif (o->flags.allow_external && drv && drv->external)\n-\t\tpgm = drv->external;\n+\tif (o->flags.allow_external && drv && drv->external.cmd)\n+\t\tpgm = &drv->external;\n\n \tif (msg) {\n \t\t/*\n@@ -4597,7 +4599,7 @@ static void strip_prefix(int prefix_length, const char **namep, const char **oth\n\n static void run_diff(struct diff_filepair *p, struct diff_options *o)\n {\n-\tconst char *pgm = external_diff();\n+\tconst struct external_diff *pgm = external_diff();\n \tstruct strbuf msg;\n \tstruct diff_filespec *one = p->one;\n \tstruct diff_filespec *two = p->two;\ndiff --git a/userdiff.c b/userdiff.c\nindex 82bc76b910..f47e2d9f36 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -333,7 +333,7 @@ PATTERNS(\"scheme\",\n \t \"|([^][)(}{[ \\t])+\"),\n PATTERNS(\"tex\", \"^(\\\\\\\\((sub)*section|chapter|part)\\\\*{0,1}\\\\{.*)$\",\n \t \"\\\\\\\\[a-zA-Z@]+|\\\\\\\\.|([a-zA-Z0-9]|[^\\x01-\\x7f])+\"),\n-{ \"default\", NULL, NULL, -1, { NULL, 0 } },\n+{ .name = \"default\", .binary = -1 },\n };\n #undef PATTERNS\n #undef IPATTERN\n@@ -445,7 +445,7 @@ int userdiff_config(const char *k, const char *v)\n \tif (!strcmp(type, \"binary\"))\n \t\treturn parse_tristate(&drv->binary, k, v);\n \tif (!strcmp(type, \"command\"))\n-\t\treturn git_config_string(&drv->external, k, v);\n+\t\treturn git_config_string(&drv->external.cmd, k, v);\n \tif (!strcmp(type, \"textconv\"))\n \t\treturn git_config_string(&drv->textconv, k, v);\n \tif (!strcmp(type, \"cachetextconv\"))\ndiff --git a/userdiff.h b/userdiff.h\nindex cc8e5abfef..2d59a8fc56 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -11,9 +11,13 @@ struct userdiff_funcname {\n \tint cflags;\n };\n\n+struct external_diff {\n+\tchar *cmd;\n+};\n+\n struct userdiff_driver {\n \tconst char *name;\n-\tchar *external;\n+\tstruct external_diff external;\n \tchar *algorithm;\n \tint binary;\n \tstruct userdiff_funcname funcname;\n--\n2.45.2\n"},{"id":"496733","messageId":"6ed75dd5-f4e7-4dac-a8e8-a0c44376e5af@web.de","threadId":"61342","inReplyTo":"168fecaa-2ebd-4897-b0ba-3bd2a37c01e7@web.de","subject":"[PATCH v3 3/3] diff: let external diffs report that changes are uninteresting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-09T07:41:44Z","receivedAt":"2024-06-09T07:41:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The options --exit-code and --quiet instruct git diff to indicate\nwhether it found any significant changes by exiting with code 1 if it\ndid and 0 if there were none. Currently this doesn't work if external\ndiff programs are involved, as we have no way to learn what they found.\n\nAdd that ability in the form of the new configuration options\ndiff.trustExitCode and diff..trustExitCode and the environment\nvariable GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE. They pair with the config\noptions diff.external and diff..command and the environment\nvariable GIT_EXTERNAL_DIFF, respectively.\n\nThe new options are off by default, keeping the old behavior. Enabling\nthem indicates that the external diff returns exit code 1 if it finds\nsignificant changes and 0 if it doesn't, like diff(1).\n\nThe name of the new options is taken from the git difftool and mergetool\noptions of similar purpose. (There they enable passing on the exit code\nof a diff tool and to infer whether a merge done by a merge tool is\nsuccessful.)\n\nThe new feature sets the diff flag diff_from_contents in\ndiff_setup_done() if we need the exit code and are allowed to call\nexternal diffs. This disables the optimization that avoids calling the\nprogram with --quiet. Add it back by skipping the call if the external\ndiff is not able to report empty diffs. We can only do that check after\nevaluating the file-specific attributes in run_external_diff().\n\nIf we do run the external diff with --quiet, send its output to\n/dev/null.\n\nI considered checking the output of the external diff to check whether\nits empty. It was added as 11be65cfa4 (diff: fix --exit-code with\nexternal diff, 2024-05-05) and quickly reverted, as it does not work\nwith external diffs that do not write to stdout. There's no reason why\na graphical diff tool would even need to write anything there at all.\n\nI also considered using a non-zero exit code for empty diffs, which\ncould be done without adding new configuration options. We'd need to\ndisable the optimization that allows git diff --quiet to skip calling\nexternal diffs, though -- that might be quite surprising if graphical\ndiff programs are involved. And assigning the opposite meaning of the\nexit codes compared to diff(1) and git diff --exit-code to the external\ndiff can cause unnecessary confusion.\n\nSuggested-by: Phillip Wood \nSigned-off-by: René Scharfe \n---\n Documentation/config/diff.txt | 18 +++++++++++++++++\n Documentation/diff-options.txt | 6 +++++-\n Documentation/git.txt | 10 +++++++++\n Documentation/gitattributes.txt | 5 +++++\n diff.c | 36 ++++++++++++++++++++++++++++++++-\n t/t4020-diff-external.sh | 33 +++++++++++++++++++++---------\n userdiff.c | 4 ++++\n userdiff.h | 1 +\n 8 files changed, 101 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt\nindex 5ce7b91f1d..190bda17e5 100644\n--- a/Documentation/config/diff.txt\n+++ b/Documentation/config/diff.txt\n@@ -79,6 +79,15 @@ diff.external::\n \tyou want to use an external diff program only on a subset of\n \tyour files, you might want to use linkgit:gitattributes[5] instead.\n\n+diff.trustExitCode::\n+\tIf this boolean value is set to true then the\n+\t`diff.external` command is expected to return exit code\n+\t0 if it considers the input files to be equal or 1 if it\n+\tconsiders them to be different, like `diff(1)`.\n+\tIf it is set to false, which is the default, then the command\n+\tis expected to return exit code 0 regardless of equality.\n+\tAny other exit code causes Git to report a fatal error.\n+\n diff.ignoreSubmodules::\n \tSets the default value of --ignore-submodules. Note that this\n \taffects only 'git diff' Porcelain, and not lower level 'diff'\n@@ -164,6 +173,15 @@ diff..command::\n \tThe custom diff driver command. See linkgit:gitattributes[5]\n \tfor details.\n\n+diff..trustExitCode::\n+\tIf this boolean value is set to true then the\n+\t`diff..command` command is expected to return exit code\n+\t0 if it considers the input files to be equal or 1 if it\n+\tconsiders them to be different, like `diff(1)`.\n+\tIf it is set to false, which is the default, then the command\n+\tis expected to return exit code 0 regardless of equality.\n+\tAny other exit code causes Git to report a fatal error.\n+\n diff..xfuncname::\n \tThe regular expression that the diff driver should use to\n \trecognize the hunk header. A built-in pattern may also be used.\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 6b73daf540..cd0b81adbb 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -820,7 +820,11 @@ ifndef::git-log[]\n\n --quiet::\n \tDisable all output of the program. Implies `--exit-code`.\n-\tDisables execution of external diff helpers.\n+\tDisables execution of external diff helpers whose exit code\n+\tis not trusted, i.e. their respective configuration option\n+\t`diff.trustExitCode` or `diff..trustExitCode` or\n+\tenvironment variable `GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE` is\n+\tfalse.\n endif::git-log[]\n endif::git-format-patch[]\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex a31a70acca..4489e2297a 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -644,6 +644,16 @@ parameter, .\n For each path `GIT_EXTERNAL_DIFF` is called, two environment variables,\n `GIT_DIFF_PATH_COUNTER` and `GIT_DIFF_PATH_TOTAL` are set.\n\n+`GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE`::\n+\tIf this Boolean environment variable is set to true then the\n+\t`GIT_EXTERNAL_DIFF` command is expected to return exit code\n+\t0 if it considers the input files to be equal or 1 if it\n+\tconsiders them to be different, like `diff(1)`.\n+\tIf it is set to false, which is the default, then the command\n+\tis expected to return exit code 0 regardless of equality.\n+\tAny other exit code causes Git to report a fatal error.\n+\n+\n `GIT_DIFF_PATH_COUNTER`::\n \tA 1-based counter incremented by one for every path.\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 4338d023d9..80cae17f37 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -776,6 +776,11 @@ with the above configuration, i.e. `j-c-diff`, with 7\n parameters, just like `GIT_EXTERNAL_DIFF` program is called.\n See linkgit:git[1] for details.\n\n+If the program is able to ignore certain changes (similar to\n+`git diff --ignore-space-change`), then also set the option\n+`trustExitCode` to true. It is then expected to return exit code 1 if\n+it finds significant changes and 0 if it doesn't.\n+\n Setting the internal diff algorithm\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\ndiff --git a/diff.c b/diff.c\nindex 4b86c61631..39ba842049 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -432,6 +432,10 @@ int git_diff_ui_config(const char *var, const char *value,\n \t}\n \tif (!strcmp(var, \"diff.external\"))\n \t\treturn git_config_string(&external_diff_cfg.cmd, var, value);\n+\tif (!strcmp(var, \"diff.trustexitcode\")) {\n+\t\texternal_diff_cfg.trust_exit_code = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"diff.wordregex\"))\n \t\treturn git_config_string(&diff_word_regex_cfg, var, value);\n \tif (!strcmp(var, \"diff.orderfile\"))\n@@ -556,6 +560,8 @@ static const struct external_diff *external_diff(void)\n \tif (done_preparing)\n \t\treturn external_diff_ptr;\n \texternal_diff_env.cmd = xstrdup_or_null(getenv(\"GIT_EXTERNAL_DIFF\"));\n+\tif (git_env_bool(\"GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE\", 0))\n+\t\texternal_diff_env.trust_exit_code = 1;\n \tif (external_diff_env.cmd)\n \t\texternal_diff_ptr = &external_diff_env;\n \telse if (external_diff_cfg.cmd)\n@@ -4387,6 +4393,19 @@ static void run_external_diff(const struct external_diff *pgm,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n+\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH);\n+\tint rc;\n+\n+\t/*\n+\t * Trivial equality is handled by diff_unmodified_pair() before\n+\t * we get here. If we don't need to show the diff and the\n+\t * external diff program lacks the ability to tell us whether\n+\t * it's empty then we consider it non-empty without even asking.\n+\t */\n+\tif (!pgm->trust_exit_code && quiet) {\n+\t\to->found_changes = 1;\n+\t\treturn;\n+\t}\n\n \tstrvec_push(&cmd.args, pgm->cmd);\n \tstrvec_push(&cmd.args, name);\n@@ -4408,7 +4427,15 @@ static void run_external_diff(const struct external_diff *pgm,\n \tdiff_free_filespec_data(one);\n \tdiff_free_filespec_data(two);\n \tcmd.use_shell = 1;\n-\tif (run_command(&cmd))\n+\tcmd.no_stdout = quiet;\n+\trc = run_command(&cmd);\n+\tif (!pgm->trust_exit_code && rc == 0)\n+\t\to->found_changes = 1;\n+\telse if (pgm->trust_exit_code && rc == 0)\n+\t\t; /* nothing */\n+\telse if (pgm->trust_exit_code && rc == 1)\n+\t\to->found_changes = 1;\n+\telse\n \t\tdie(_(\"external diff died, stopping at %s\"), name);\n\n \tremove_tempfile();\n@@ -4926,6 +4953,13 @@ void diff_setup_done(struct diff_options *options)\n \t\toptions->flags.exit_with_status = 1;\n \t}\n\n+\t/*\n+\t * External diffs could declare non-identical contents equal\n+\t * (think diff --ignore-space-change).\n+\t */\n+\tif (options->flags.allow_external && options->flags.exit_with_status)\n+\t\toptions->flags.diff_from_contents = 1;\n+\n \toptions->diff_path_counter = 0;\n\n \tif (options->flags.follow_renames)\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex 4070523cdb..3baa52a9bf 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -177,15 +177,17 @@ check_external_diff () {\n \texpect_out=$2\n \texpect_err=$3\n \tcommand_code=$4\n-\tshift 4\n+\ttrust_exit_code=$5\n+\tshift 5\n \toptions=\"$@\"\n\n \tcommand=\"echo output; exit $command_code;\"\n-\tdesc=\"external diff '$command'\"\n+\tdesc=\"external diff '$command' with trustExitCode=$trust_exit_code\"\n \twith_options=\"${options:+ with }$options\"\n\n \ttest_expect_success \"$desc via attribute$with_options\" \"\n \t\ttest_config diff.foo.command \\\"$command\\\" &&\n+\t\ttest_config diff.foo.trustExitCode $trust_exit_code &&\n \t\techo \\\"file diff=foo\\\" >.gitattributes &&\n \t\ttest_expect_code $expect_code git diff $options >out 2>err &&\n \t\ttest_cmp $expect_out out &&\n@@ -194,6 +196,7 @@ check_external_diff () {\n\n \ttest_expect_success \"$desc via diff.external$with_options\" \"\n \t\ttest_config diff.external \\\"$command\\\" &&\n+\t\ttest_config diff.trustExitCode $trust_exit_code &&\n \t\t>.gitattributes &&\n \t\ttest_expect_code $expect_code git diff $options >out 2>err &&\n \t\ttest_cmp $expect_out out &&\n@@ -204,6 +207,7 @@ check_external_diff () {\n \t\t>.gitattributes &&\n \t\ttest_expect_code $expect_code env \\\n \t\t\tGIT_EXTERNAL_DIFF=\\\"$command\\\" \\\n+\t\t\tGIT_EXTERNAL_DIFF_TRUST_EXIT_CODE=$trust_exit_code \\\n \t\t\tgit diff $options >out 2>err &&\n \t\ttest_cmp $expect_out out &&\n \t\ttest_cmp $expect_err err\n@@ -216,14 +220,23 @@ test_expect_success 'setup output files' '\n \techo \"fatal: external diff died, stopping at file\" >error\n '\n\n-check_external_diff 0 output empty 0\n-check_external_diff 128 output error 1\n-\n-check_external_diff 1 output empty 0 --exit-code\n-check_external_diff 128 output error 1 --exit-code\n-\n-check_external_diff 1 empty empty 0 --quiet\n-check_external_diff 1 empty empty 1 --quiet # we don't even call the program\n+check_external_diff 0 output empty 0 off\n+check_external_diff 128 output error 1 off\n+check_external_diff 0 output empty 0 on\n+check_external_diff 0 output empty 1 on\n+check_external_diff 128 output error 2 on\n+\n+check_external_diff 1 output empty 0 off --exit-code\n+check_external_diff 128 output error 1 off --exit-code\n+check_external_diff 0 output empty 0 on --exit-code\n+check_external_diff 1 output empty 1 on --exit-code\n+check_external_diff 128 output error 2 on --exit-code\n+\n+check_external_diff 1 empty empty 0 off --quiet\n+check_external_diff 1 empty empty 1 off --quiet # we don't even call the program\n+check_external_diff 0 empty empty 0 on --quiet\n+check_external_diff 1 empty empty 1 on --quiet\n+check_external_diff 128 empty error 2 on --quiet\n\n echo NULZbetweenZwords | perl -pe 'y/Z/\\000/' > file\n\ndiff --git a/userdiff.c b/userdiff.c\nindex f47e2d9f36..6cdfb147c3 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -446,6 +446,10 @@ int userdiff_config(const char *k, const char *v)\n \t\treturn parse_tristate(&drv->binary, k, v);\n \tif (!strcmp(type, \"command\"))\n \t\treturn git_config_string(&drv->external.cmd, k, v);\n+\tif (!strcmp(type, \"trustexitcode\")) {\n+\t\tdrv->external.trust_exit_code = git_config_bool(k, v);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(type, \"textconv\"))\n \t\treturn git_config_string(&drv->textconv, k, v);\n \tif (!strcmp(type, \"cachetextconv\"))\ndiff --git a/userdiff.h b/userdiff.h\nindex 2d59a8fc56..f5cb53d0d4 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -13,6 +13,7 @@ struct userdiff_funcname {\n\n struct external_diff {\n \tchar *cmd;\n+\tunsigned trust_exit_code:1;\n };\n\n struct userdiff_driver {\n--\n2.45.2\n"},{"id":"496771","messageId":"7aabf47d-7e0e-44e3-b85e-ba46951ad0e0@gmail.com","threadId":"61342","inReplyTo":"168fecaa-2ebd-4897-b0ba-3bd2a37c01e7@web.de","subject":"Re: [PATCH v3 0/3] diff: fix --exit-code with external diff","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-10T13:48:42Z","receivedAt":"2024-06-10T13:48:51Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi René\n\nOn 09/06/2024 08:35, René Scharfe wrote:\n> Changes since v2:\n> - rebase; config strings are no longer const\n> - document the handling of unexpected exit codes\n> - document that external diffs are skipped with --quiet and trustExitCode=off\n> - silence external diff output with --quiet\n> - check output in tests\n> - test diff runs without --exit-code and --quiet as well\n> - slightly untangle the exit code handling code to make it easier to read\n> - fix copy/paste error in documentation of diff..trustExitCode\n\nThe changes in this version all look good. I've re-read the patches and \ndidn't spot any other issues so this is ready as far as I'm concerned.\n\nThanks\n\nPhillip\n\n> t4020: test exit code with external diffs\n> userdiff: add and use struct external_diff\n> diff: let external diffs report that changes are uninteresting\n> \n> Documentation/config/diff.txt | 18 +++++++++\n> Documentation/diff-options.txt | 5 +++\n> Documentation/git.txt | 10 +++++\n> Documentation/gitattributes.txt | 5 +++\n> diff.c | 68 +++++++++++++++++++++++++--------\n> t/t4020-diff-external.sh | 66 ++++++++++++++++++++++++++++++++\n> userdiff.c | 8 +++-\n> userdiff.h | 7 +++-\n> 8 files changed, 168 insertions(+), 19 deletions(-)\n> \n> Range-Diff gegen v2:\n> 1: 118aba667b < -: ---------- t4020: test exit code with external diffs\n> -: ---------- > 1: d59f0c6fdf t4020: test exit code with external diffs\n> 2: 0b4dabebe1 ! 2: 4ad160ab1f userdiff: add and use struct external_diff\n> @@ diff.c\n> @@ diff.c: static int diff_color_moved_ws_default;\n> static int diff_context_default = 3;\n> static int diff_interhunk_context_default;\n> - static const char *diff_word_regex_cfg;\n> --static const char *external_diff_cmd_cfg;\n> + static char *diff_word_regex_cfg;\n> +-static char *external_diff_cmd_cfg;\n> +static struct external_diff external_diff_cfg;\n> - static const char *diff_order_file_cfg;\n> + static char *diff_order_file_cfg;\n> int diff_auto_refresh_index = 1;\n> static int diff_mnemonic_prefix;\n> @@ diff.c: int git_diff_ui_config(const char *var, const char *value,\n> @@ userdiff.h: struct userdiff_funcname {\n> };\n> \n> +struct external_diff {\n> -+\tconst char *cmd;\n> ++\tchar *cmd;\n> +};\n> +\n> struct userdiff_driver {\n> \tconst char *name;\n> --\tconst char *external;\n> +-\tchar *external;\n> +\tstruct external_diff external;\n> - \tconst char *algorithm;\n> + \tchar *algorithm;\n> \tint binary;\n> \tstruct userdiff_funcname funcname;\n> 3: 4d54ca8281 ! 3: 29c8d3b61a diff: let external diffs report that changes are uninteresting\n> @@ Commit message\n> diff is not able to report empty diffs. We can only do that check after\n> evaluating the file-specific attributes in run_external_diff().\n> \n> + If we do run the external diff with --quiet, send its output to\n> + /dev/null.\n> +\n> I considered checking the output of the external diff to check whether\n> its empty. It was added as 11be65cfa4 (diff: fix --exit-code with\n> external diff, 2024-05-05) and quickly reverted, as it does not work\n> @@ Documentation/config/diff.txt: diff.external::\n> \tyour files, you might want to use linkgit:gitattributes[5] instead.\n> \n> +diff.trustExitCode::\n> -+\tIf this boolean value is set to true then the `diff.external`\n> -+\tcommand is expected to return exit code 1 if it finds\n> -+\tsignificant changes and 0 if it doesn't, like diff(1). If it's\n> -+\tfalse then the `diff.external` command is expected to always\n> -+\treturn exit code 0. Defaults to false.\n> ++\tIf this boolean value is set to true then the\n> ++\t`diff.external` command is expected to return exit code\n> ++\t0 if it considers the input files to be equal or 1 if it\n> ++\tconsiders them to be different, like `diff(1)`.\n> ++\tIf it is set to false, which is the default, then the command\n> ++\tis expected to return exit code 0 regardless of equality.\n> ++\tAny other exit code causes Git to report a fatal error.\n> +\n> diff.ignoreSubmodules::\n> \tSets the default value of --ignore-submodules. Note that this\n> @@ Documentation/config/diff.txt: diff..command::\n> +diff..trustExitCode::\n> +\tIf this boolean value is set to true then the\n> +\t`diff..command` command is expected to return exit code\n> -+\t1 if it finds significant changes and 0 if it doesn't, like\n> -+\tdiff(1). If it's false then the `diff.external` command is\n> -+\texpected to always return exit code 0. Defaults to false.\n> ++\t0 if it considers the input files to be equal or 1 if it\n> ++\tconsiders them to be different, like `diff(1)`.\n> ++\tIf it is set to false, which is the default, then the command\n> ++\tis expected to return exit code 0 regardless of equality.\n> ++\tAny other exit code causes Git to report a fatal error.\n> +\n> diff..xfuncname::\n> \tThe regular expression that the diff driver should use to\n> \trecognize the hunk header. A built-in pattern may also be used.\n> \n> + ## Documentation/diff-options.txt ##\n> +@@ Documentation/diff-options.txt: ifndef::git-log[]\n> +\n> + --quiet::\n> + \tDisable all output of the program. Implies `--exit-code`.\n> +-\tDisables execution of external diff helpers.\n> ++\tDisables execution of external diff helpers whose exit code\n> ++\tis not trusted, i.e. their respective configuration option\n> ++\t`diff.trustExitCode` or `diff..trustExitCode` or\n> ++\tenvironment variable `GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE` is\n> ++\tfalse.\n> + endif::git-log[]\n> + endif::git-format-patch[]\n> +\n> +\n> ## Documentation/git.txt ##\n> @@ Documentation/git.txt: parameter, .\n> For each path `GIT_EXTERNAL_DIFF` is called, two environment variables,\n> `GIT_DIFF_PATH_COUNTER` and `GIT_DIFF_PATH_TOTAL` are set.\n> \n> +`GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE`::\n> -+\tSetting this environment variable indicates the the program\n> -+\tspecified with `GIT_EXTERNAL_DIFF` returns exit code 1 if it\n> -+\tfinds significant changes and 0 if it doesn't, like diff(1).\n> -+\tIf it's not set, the program is expected to always return\n> -+\texit code 0.\n> ++\tIf this Boolean environment variable is set to true then the\n> ++\t`GIT_EXTERNAL_DIFF` command is expected to return exit code\n> ++\t0 if it considers the input files to be equal or 1 if it\n> ++\tconsiders them to be different, like `diff(1)`.\n> ++\tIf it is set to false, which is the default, then the command\n> ++\tis expected to return exit code 0 regardless of equality.\n> ++\tAny other exit code causes Git to report a fatal error.\n> ++\n> +\n> `GIT_DIFF_PATH_COUNTER`::\n> \tA 1-based counter incremented by one for every path.\n> @@ diff.c: static void run_external_diff(const struct external_diff *pgm,\n> {\n> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n> \tstruct diff_queue_struct *q = &diff_queued_diff;\n> ++\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH);\n> +\tint rc;\n> +\n> +\t/*\n> @@ diff.c: static void run_external_diff(const struct external_diff *pgm,\n> +\t * external diff program lacks the ability to tell us whether\n> +\t * it's empty then we consider it non-empty without even asking.\n> +\t */\n> -+\tif (!(o->output_format & DIFF_FORMAT_PATCH) && !pgm->trust_exit_code) {\n> ++\tif (!pgm->trust_exit_code && quiet) {\n> +\t\to->found_changes = 1;\n> +\t\treturn;\n> +\t}\n> @@ diff.c: static void run_external_diff(const struct external_diff *pgm,\n> \tdiff_free_filespec_data(two);\n> \tcmd.use_shell = 1;\n> -\tif (run_command(&cmd))\n> ++\tcmd.no_stdout = quiet;\n> +\trc = run_command(&cmd);\n> -+\tif ((!pgm->trust_exit_code && !rc) || (pgm->trust_exit_code && rc == 1))\n> ++\tif (!pgm->trust_exit_code && rc == 0)\n> ++\t\to->found_changes = 1;\n> ++\telse if (pgm->trust_exit_code && rc == 0)\n> ++\t\t; /* nothing */\n> ++\telse if (pgm->trust_exit_code && rc == 1)\n> +\t\to->found_changes = 1;\n> -+\telse if (!pgm->trust_exit_code || rc)\n> ++\telse\n> \t\tdie(_(\"external diff died, stopping at %s\"), name);\n> \n> \tremove_tempfile();\n> @@ diff.c: void diff_setup_done(struct diff_options *options)\n> \tif (options->flags.follow_renames)\n> \n> ## t/t4020-diff-external.sh ##\n> -@@ t/t4020-diff-external.sh: test_expect_success 'no diff with -diff' '\n> - check_external_exit_code () {\n> - \texpect_code=$1\n> - \tcommand_code=$2\n> --\toption=$3\n> -+\ttrust_exit_code=$3\n> -+\toption=$4\n> -\n> - \tcommand=\"exit $command_code;\"\n> +@@ t/t4020-diff-external.sh: check_external_diff () {\n> + \texpect_out=$2\n> + \texpect_err=$3\n> + \tcommand_code=$4\n> +-\tshift 4\n> ++\ttrust_exit_code=$5\n> ++\tshift 5\n> + \toptions=\"$@\"\n> +\n> + \tcommand=\"echo output; exit $command_code;\"\n> -\tdesc=\"external diff '$command'\"\n> +\tdesc=\"external diff '$command' with trustExitCode=$trust_exit_code\"\n> + \twith_options=\"${options:+ with }$options\"\n> \n> - \ttest_expect_success \"$desc via attribute with $option\" \"\n> + \ttest_expect_success \"$desc via attribute$with_options\" \"\n> \t\ttest_config diff.foo.command \\\"$command\\\" &&\n> +\t\ttest_config diff.foo.trustExitCode $trust_exit_code &&\n> \t\techo \\\"file diff=foo\\\" >.gitattributes &&\n> - \t\ttest_expect_code $expect_code git diff $option\n> - \t\"\n> + \t\ttest_expect_code $expect_code git diff $options >out 2>err &&\n> + \t\ttest_cmp $expect_out out &&\n> +@@ t/t4020-diff-external.sh: check_external_diff () {\n> \n> - \ttest_expect_success \"$desc via diff.external with $option\" \"\n> + \ttest_expect_success \"$desc via diff.external$with_options\" \"\n> \t\ttest_config diff.external \\\"$command\\\" &&\n> +\t\ttest_config diff.trustExitCode $trust_exit_code &&\n> \t\t>.gitattributes &&\n> - \t\ttest_expect_code $expect_code git diff $option\n> - \t\"\n> -@@ t/t4020-diff-external.sh: check_external_exit_code () {\n> + \t\ttest_expect_code $expect_code git diff $options >out 2>err &&\n> + \t\ttest_cmp $expect_out out &&\n> +@@ t/t4020-diff-external.sh: check_external_diff () {\n> \t\t>.gitattributes &&\n> \t\ttest_expect_code $expect_code env \\\n> \t\t\tGIT_EXTERNAL_DIFF=\\\"$command\\\" \\\n> +\t\t\tGIT_EXTERNAL_DIFF_TRUST_EXIT_CODE=$trust_exit_code \\\n> - \t\t\tgit diff $option\n> - \t\"\n> - }\n> -\n> --check_external_exit_code 1 0 --exit-code\n> --check_external_exit_code 1 0 --quiet\n> --check_external_exit_code 128 1 --exit-code\n> --check_external_exit_code 1 1 --quiet # we don't even call the program\n> -+check_external_exit_code 1 0 off --exit-code\n> -+check_external_exit_code 1 0 off --quiet\n> -+check_external_exit_code 128 1 off --exit-code\n> -+check_external_exit_code 1 1 off --quiet # we don't even call the program\n> + \t\t\tgit diff $options >out 2>err &&\n> + \t\ttest_cmp $expect_out out &&\n> + \t\ttest_cmp $expect_err err\n> +@@ t/t4020-diff-external.sh: test_expect_success 'setup output files' '\n> + \techo \"fatal: external diff died, stopping at file\" >error\n> + '\n> +\n> +-check_external_diff 0 output empty 0\n> +-check_external_diff 128 output error 1\n> +-\n> +-check_external_diff 1 output empty 0 --exit-code\n> +-check_external_diff 128 output error 1 --exit-code\n> +-\n> +-check_external_diff 1 empty empty 0 --quiet\n> +-check_external_diff 1 empty empty 1 --quiet # we don't even call the program\n> ++check_external_diff 0 output empty 0 off\n> ++check_external_diff 128 output error 1 off\n> ++check_external_diff 0 output empty 0 on\n> ++check_external_diff 0 output empty 1 on\n> ++check_external_diff 128 output error 2 on\n> ++\n> ++check_external_diff 1 output empty 0 off --exit-code\n> ++check_external_diff 128 output error 1 off --exit-code\n> ++check_external_diff 0 output empty 0 on --exit-code\n> ++check_external_diff 1 output empty 1 on --exit-code\n> ++check_external_diff 128 output error 2 on --exit-code\n> +\n> -+check_external_exit_code 0 0 on --exit-code\n> -+check_external_exit_code 0 0 on --quiet\n> -+check_external_exit_code 1 1 on --exit-code\n> -+check_external_exit_code 1 1 on --quiet\n> -+check_external_exit_code 128 2 on --exit-code\n> -+check_external_exit_code 128 2 on --quiet\n> ++check_external_diff 1 empty empty 0 off --quiet\n> ++check_external_diff 1 empty empty 1 off --quiet # we don't even call the program\n> ++check_external_diff 0 empty empty 0 on --quiet\n> ++check_external_diff 1 empty empty 1 on --quiet\n> ++check_external_diff 128 empty error 2 on --quiet\n> \n> echo NULZbetweenZwords | perl -pe 'y/Z/\\000/' > file\n> \n> @@ userdiff.h\n> @@ userdiff.h: struct userdiff_funcname {\n> \n> struct external_diff {\n> - \tconst char *cmd;\n> + \tchar *cmd;\n> +\tunsigned trust_exit_code:1;\n> };\n> \n> --\n> 2.45.2\n"},{"id":"496780","messageId":"xmqqmsns4v14.fsf@gitster.g","threadId":"61342","inReplyTo":"79a951f8-cf35-4159-a90e-f95d69773413@web.de","subject":"Re: [PATCH v3 1/3] t4020: test exit code with external diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-10T16:33:43Z","receivedAt":"2024-06-10T16:33:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> +check_external_diff () {\n> +\texpect_code=$1\n> +\texpect_out=$2\n> +\texpect_err=$3\n> +\tcommand_code=$4\n> +\tshift 4\n> +\toptions=\"$@\"\n\nTiny nit, but I'd prefer to see \"$@\" reserved for its magic and all\nother times where it is equivalent to \"$*\", see the latter used.\n\nOther than that, all three patches looked good to me. Thanks.\n"}]}