| Message ID | 99a99e5c-4fe4-413a-9281-363e280716b8@web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | diff: fix --exit-code with external diff | expand |
Am 05.06.24 um 10:38 schrieb René Scharfe: > +diff.trustExitCode:: > + If this boolean value is set to true then the `diff.external` > + command is expected to return exit code 1 if it finds > + significant changes and 0 if it doesn't, like diff(1). If it's > + false then the `diff.external` command is expected to always > + return exit code 0. Defaults to false. I find this somewhat unclear. What are the consequences when this value is set to false, but the command exits with code other than 0? Is it If it's false then any exit code other than 0 of the `diff.external` command is treated as an error. -- Hannes
Am 06.06.24 um 08:39 schrieb Johannes Sixt: > Am 05.06.24 um 10:38 schrieb René Scharfe: >> +diff.trustExitCode:: >> + If this boolean value is set to true then the `diff.external` >> + command is expected to return exit code 1 if it finds >> + significant changes and 0 if it doesn't, like diff(1). If it's >> + false then the `diff.external` command is expected to always >> + return exit code 0. Defaults to false. > > I find this somewhat unclear. What are the consequences when this value > is set to false, but the command exits with code other than 0? Is it > > If it's false then any exit code other than 0 of the `diff.external` > command is treated as an error. Yes, unexpected exit codes are reported as errors. If trustExitCode is false and --quiet is given then the execution of external diffs is skipped, so in that situation there is no exit code to expect, though. Not sure how to express it concisely, though. This attempt looks a bit bloated: --quiet:: Disable all output of the program. Implies `--exit-code`. Disables execution of external diff helpers whose exit code is not trusted, i.e. their respective configuration option `diff.trustExitCode` or `diff.<driver>.trustExitCode` or environment variable `GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE` is false. Might be worth documenting this original behavior somehow, anyway. It makes sense in hindsight, but surprised me a bit when I wrote the tests. René
Hi René On 05/06/2024 09:38, René Scharfe wrote: > The options --exit-code and --quiet instruct git diff to indicate > whether it found any significant changes by exiting with code 1 if it > did and 0 if there were none. Currently this doesn't work if external > diff programs are involved, as we have no way to learn what they found. > > Add that ability in the form of the new configuration options > diff.trustExitCode and diff.<driver>.trustExitCode and the environment > variable GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE. They pair with the config > options diff.external and diff.<driver>.command and the environment > variable GIT_EXTERNAL_DIFF, respectively. > > The new options are off by default, keeping the old behavior. Enabling > them indicates that the external diff returns exit code 1 if it finds > significant changes and 0 if it doesn't, like diff(1). > > The name of the new options is taken from the git difftool and mergetool > options of similar purpose. (There they enable passing on the exit code > of a diff tool and to infer whether a merge done by a merge tool is > successful.) > > The new feature sets the diff flag diff_from_contents in > diff_setup_done() if we need the exit code and are allowed to call > external diffs. This disables the optimization that avoids calling the > program with --quiet. Add it back by skipping the call if the external > diff is not able to report empty diffs. We can only do that check after > evaluating the file-specific attributes in run_external_diff(). > > I considered checking the output of the external diff to check whether > its empty. It was added as 11be65cfa4 (diff: fix --exit-code with > external diff, 2024-05-05) and quickly reverted, as it does not work > with external diffs that do not write to stdout. There's no reason why > a graphical diff tool would even need to write anything there at all. > > I also considered using a non-zero exit code for empty diffs, which > could be done without adding new configuration options. We'd need to > disable the optimization that allows git diff --quiet to skip calling > external diffs, though -- that might be quite surprising if graphical > diff programs are involved. And assigning the opposite meaning of the > exit codes compared to diff(1) and git diff --exit-code to the external > diff can cause unnecessary confusion. Thanks for the comprehensive commit message, I agree that it is much less confusing to follow the exit code convention of diff(1). This is looking good, I've left a couple of comments below. > +diff.trustExitCode:: > + If this boolean value is set to true then the `diff.external` > + command is expected to return exit code 1 if it finds > + significant changes and 0 if it doesn't, like diff(1). If it's > + false then the `diff.external` command is expected to always > + return exit code 0. Defaults to false. I wonder if "significant changes" is a bit ambiguous and as Johannes said it would be good to mention that other exit codes are errors. Perhaps If this boolean value is set to true then the `diff.external` command is expected to return exit code 0 if it considers the input files to be equal and 1 if they are not, like diff(1). If it is false then the `diff.external` command is expected to always return exit code 0. In both cases any other exit code is considered to be an error. Defaults to false. > strvec_push(&cmd.args, pgm->cmd); > strvec_push(&cmd.args, name); > @@ -4406,7 +4424,10 @@ static void run_external_diff(const struct external_diff *pgm, > diff_free_filespec_data(one); > diff_free_filespec_data(two); > cmd.use_shell = 1; Should we be redirecting stdout to /dev/null here when the user passes --quiet? > - if (run_command(&cmd)) > + rc = run_command(&cmd); > + if ((!pgm->trust_exit_code && !rc) || (pgm->trust_exit_code && rc == 1)) > + o->found_changes = 1; > + else if (!pgm->trust_exit_code || rc) > die(_("external diff died, stopping at %s"), name); This is a bit fiddly because we may, or may not trust the exit code but the logic here looks good. > -check_external_exit_code 1 0 --exit-code > -check_external_exit_code 1 0 --quiet > -check_external_exit_code 128 1 --exit-code > -check_external_exit_code 1 1 --quiet # we don't even call the program > +check_external_exit_code 1 0 off --exit-code > +check_external_exit_code 1 0 off --quiet > +check_external_exit_code 128 1 off --exit-code > +check_external_exit_code 1 1 off --quiet # we don't even call the program > + > +check_external_exit_code 0 0 on --exit-code > +check_external_exit_code 0 0 on --quiet > +check_external_exit_code 1 1 on --exit-code > +check_external_exit_code 1 1 on --quiet > +check_external_exit_code 128 2 on --exit-code > +check_external_exit_code 128 2 on --quiet It would be nice if the tests checked that --quiet does not produce any output on stdout. Best Wishes Phillip
René Scharfe <l.s.r@web.de> writes: >>> +diff.trustExitCode:: >>> + If this boolean value is set to true then the `diff.external` >>> + command is expected to return exit code 1 if it finds >>> + significant changes and 0 if it doesn't, like diff(1). If it's >>> + false then the `diff.external` command is expected to always >>> + return exit code 0. Defaults to false. >> >> I find this somewhat unclear. What are the consequences when this value >> is set to false, but the command exits with code other than 0? Is it >> >> If it's false then any exit code other than 0 of the `diff.external` >> command is treated as an error. > > Yes, unexpected exit codes are reported as errors. > > If trustExitCode is false and --quiet is given then the execution of > external diffs is skipped, so in that situation there is no exit code to > expect, though. Not sure how to express it concisely, though. This > attempt looks a bit bloated: > > --quiet:: > Disable all output of the program. Implies `--exit-code`. > Disables execution of external diff helpers whose exit code > is not trusted, i.e. their respective configuration option > `diff.trustExitCode` or `diff.<driver>.trustExitCode` or > environment variable `GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE` is > false. > > Might be worth documenting this original behavior somehow, anyway. It > makes sense in hindsight, but surprised me a bit when I wrote the tests. Yes. The explanation of trustExitCode makes sense as an explanation of what the variable means (i.e. if set, we pay attention to the exit code of the external diff driver, otherwise a non-zero exit is an error), but I suspect that readers are _more_ interested in how the external diff driver contributes to the answer to the "has this path been changed?" question when the variable is on and off. And the above description of "--quiet" does help answer that question somewhat.
Am 06.06.24 um 11:48 schrieb Phillip Wood: > On 05/06/2024 09:38, René Scharfe wrote: >> +diff.trustExitCode:: >> + If this boolean value is set to true then the `diff.external` >> + command is expected to return exit code 1 if it finds >> + significant changes and 0 if it doesn't, like diff(1). If it's >> + false then the `diff.external` command is expected to always >> + return exit code 0. Defaults to false. > > I wonder if "significant changes" is a bit ambiguous and as Johannes > said it would be good to mention that other exit codes are errors. > Perhaps > > If this boolean value is set to true then the `diff.external` > command is expected to return exit code 0 if it considers the > input files to be equal and 1 if they are not, like diff(1). > If it is false then the `diff.external` command is expected to > always return exit code 0. In both cases any other exit code > is considered to be an error. Defaults to false. The first part looks like an obvious improvement. Stating that unexpected exit codes are errors looks tautological to me, though. Perhaps mentioning that git diff stops at that point adds would be useful? Or perhaps a tad of redundancy is just what's needed here? > > >> strvec_push(&cmd.args, pgm->cmd); >> strvec_push(&cmd.args, name); >> @@ -4406,7 +4424,10 @@ static void run_external_diff(const struct external_diff *pgm, >> diff_free_filespec_data(one); >> diff_free_filespec_data(two); >> cmd.use_shell = 1; > > Should we be redirecting stdout to /dev/null here when the user > passes --quiet? Oh, yes. We didn't even start the program before, but with the patch it becomes necessary. Good find! > >> - if (run_command(&cmd)) >> + rc = run_command(&cmd); >> + if ((!pgm->trust_exit_code && !rc) || (pgm->trust_exit_code && rc == 1)) >> + o->found_changes = 1; >> + else if (!pgm->trust_exit_code || rc) >> die(_("external diff died, stopping at %s"), name); > > This is a bit fiddly because we may, or may not trust the exit code > but the logic here looks good. Yeah, it's not as clear as it could be. One reason is that it avoids redundancy at the action side and the other is adherence to the rule of not explicitly comparing to zero. We could enumerate all cases: if (!pgm->trust_exit_code && rc == 0) o->found_changes = 1; else if (pgm->trust_exit_code && rc == 0) ; /* nothing */ else if (pgm->trust_exit_code && rc == 1) o->found_changes = 1; else die(_("external diff died, stopping at %s"), name); Or avoid redundancy in the conditions: if (pgm->trust_exit_code) { if (rc == 1) o->found_changes = 1; else if (rc != 0) die(_("external diff died, stopping at %s"), name); } else { if (rc == 0) o->found_changes = 1; else die(_("external diff died, stopping at %s"), name); } We should not get into bit twiddling, though: o->found_changes |= rc == pgm->trust_exit_code; if ((unsigned)rc > pgm->trust_exit_code) die(_("external diff died, stopping at %s"), name); > >> -check_external_exit_code 1 0 --exit-code >> -check_external_exit_code 1 0 --quiet >> -check_external_exit_code 128 1 --exit-code >> -check_external_exit_code 1 1 --quiet # we don't even call the program >> +check_external_exit_code 1 0 off --exit-code >> +check_external_exit_code 1 0 off --quiet >> +check_external_exit_code 128 1 off --exit-code >> +check_external_exit_code 1 1 off --quiet # we don't even call the program >> + >> +check_external_exit_code 0 0 on --exit-code >> +check_external_exit_code 0 0 on --quiet >> +check_external_exit_code 1 1 on --exit-code >> +check_external_exit_code 1 1 on --quiet >> +check_external_exit_code 128 2 on --exit-code >> +check_external_exit_code 128 2 on --quiet > > It would be nice if the tests checked that --quiet does not produce > any output on stdout. Right, we need this after unlocking external diff execution with --quiet. René
diff programs are involved, as we have no way to learn what they found. Add that ability in the form of the new configuration options diff.trustExitCode and diff.<driver>.trustExitCode and the environment variable GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE. They pair with the config options diff.external and diff.<driver>.command and the environment variable GIT_EXTERNAL_DIFF, respectively. The new options are off by default, keeping the old behavior. Enabling them indicates that the external diff returns exit code 1 if it finds significant changes and 0 if it doesn't, like diff(1). The name of the new options is taken from the git difftool and mergetool options of similar purpose. (There they enable passing on the exit code of a diff tool and to infer whether a merge done by a merge tool is successful.) The new feature sets the diff flag diff_from_contents in diff_setup_done() if we need the exit code and are allowed to call external diffs. This disables the optimization that avoids calling the program with --quiet. Add it back by skipping the call if the external diff is not able to report empty diffs. We can only do that check after evaluating the file-specific attributes in run_external_diff(). I considered checking the output of the external diff to check whether its empty. It was added as 11be65cfa4 (diff: fix --exit-code with external diff, 2024-05-05) and quickly reverted, as it does not work with external diffs that do not write to stdout. There's no reason why a graphical diff tool would even need to write anything there at all. I also considered using a non-zero exit code for empty diffs, which could be done without adding new configuration options. We'd need to disable the optimization that allows git diff --quiet to skip calling external diffs, though -- that might be quite surprising if graphical diff programs are involved. And assigning the opposite meaning of the exit codes compared to diff(1) and git diff --exit-code to the external diff can cause unnecessary confusion. Suggested-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: René Scharfe <l.s.r@web.de> --- Documentation/config/diff.txt | 14 ++++++++++++++ Documentation/git.txt | 7 +++++++ Documentation/gitattributes.txt | 5 +++++ diff.c | 30 +++++++++++++++++++++++++++++- t/t4020-diff-external.sh | 23 +++++++++++++++++------ userdiff.c | 4 ++++ userdiff.h | 1 + 7 files changed, 77 insertions(+), 7 deletions(-) diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt index 5ce7b91f1d..485a13236d 100644 --- a/Documentation/config/diff.txt +++ b/Documentation/config/diff.txt @@ -79,6 +79,13 @@ diff.external:: you want to use an external diff program only on a subset of your files, you might want to use linkgit:gitattributes[5] instead. +diff.trustExitCode:: + If this boolean value is set to true then the `diff.external` + command is expected to return exit code 1 if it finds + significant changes and 0 if it doesn't, like diff(1). If it's + false then the `diff.external` command is expected to always + return exit code 0. Defaults to false. + diff.ignoreSubmodules:: Sets the default value of --ignore-submodules. Note that this affects only 'git diff' Porcelain, and not lower level 'diff' @@ -164,6 +171,13 @@ diff.<driver>.command:: The custom diff driver command. See linkgit:gitattributes[5] for details. +diff.<driver>.trustExitCode:: + If this boolean value is set to true then the + `diff.<driver>.command` command is expected to return exit code + 1 if it finds significant changes and 0 if it doesn't, like + diff(1). If it's false then the `diff.external` command is + expected to always return exit code 0. Defaults to false. + diff.<driver>.xfuncname:: The regular expression that the diff driver should use to recognize the hunk header. A built-in pattern may also be used. diff --git a/Documentation/git.txt b/Documentation/git.txt index a31a70acca..81b806416c 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -644,6 +644,13 @@ parameter, <path>. For each path `GIT_EXTERNAL_DIFF` is called, two environment variables, `GIT_DIFF_PATH_COUNTER` and `GIT_DIFF_PATH_TOTAL` are set. +`GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE`:: + Setting this environment variable indicates the the program + specified with `GIT_EXTERNAL_DIFF` returns exit code 1 if it + finds significant changes and 0 if it doesn't, like diff(1). + If it's not set, the program is expected to always return + exit code 0. + `GIT_DIFF_PATH_COUNTER`:: A 1-based counter incremented by one for every path. diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt index 4338d023d9..80cae17f37 100644 --- a/Documentation/gitattributes.txt +++ b/Documentation/gitattributes.txt @@ -776,6 +776,11 @@ with the above configuration, i.e. `j-c-diff`, with 7 parameters, just like `GIT_EXTERNAL_DIFF` program is called. See linkgit:git[1] for details. +If the program is able to ignore certain changes (similar to +`git diff --ignore-space-change`), then also set the option +`trustExitCode` to true. It is then expected to return exit code 1 if +it finds significant changes and 0 if it doesn't. + Setting the internal diff algorithm ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/diff.c b/diff.c index 286d093bfa..3dff81ec7d 100644 --- a/diff.c +++ b/diff.c @@ -430,6 +430,10 @@ int git_diff_ui_config(const char *var, const char *value, } if (!strcmp(var, "diff.external")) return git_config_string(&external_diff_cfg.cmd, var, value); + if (!strcmp(var, "diff.trustexitcode")) { + external_diff_cfg.trust_exit_code = git_config_bool(var, value); + return 0; + } if (!strcmp(var, "diff.wordregex")) return git_config_string(&diff_word_regex_cfg, var, value); if (!strcmp(var, "diff.orderfile")) @@ -554,6 +558,8 @@ static const struct external_diff *external_diff(void) if (done_preparing) return external_diff_ptr; external_diff_env.cmd = xstrdup_or_null(getenv("GIT_EXTERNAL_DIFF")); + if (git_env_bool("GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE", 0)) + external_diff_env.trust_exit_code = 1; if (external_diff_env.cmd) external_diff_ptr = &external_diff_env; else if (external_diff_cfg.cmd) @@ -4385,6 +4391,18 @@ static void run_external_diff(const struct external_diff *pgm, { struct child_process cmd = CHILD_PROCESS_INIT; struct diff_queue_struct *q = &diff_queued_diff; + int rc; + + /* + * Trivial equality is handled by diff_unmodified_pair() before + * we get here. If we don't need to show the diff and the + * external diff program lacks the ability to tell us whether + * it's empty then we consider it non-empty without even asking. + */ + if (!(o->output_format & DIFF_FORMAT_PATCH) && !pgm->trust_exit_code) { + o->found_changes = 1; + return; + } strvec_push(&cmd.args, pgm->cmd); strvec_push(&cmd.args, name); @@ -4406,7 +4424,10 @@ static void run_external_diff(const struct external_diff *pgm, diff_free_filespec_data(one); diff_free_filespec_data(two); cmd.use_shell = 1; - if (run_command(&cmd)) + rc = run_command(&cmd); + if ((!pgm->trust_exit_code && !rc) || (pgm->trust_exit_code && rc == 1)) + o->found_changes = 1; + else if (!pgm->trust_exit_code || rc) die(_("external diff died, stopping at %s"), name); remove_tempfile(); @@ -4924,6 +4945,13 @@ void diff_setup_done(struct diff_options *options) options->flags.exit_with_status = 1; } + /* + * External diffs could declare non-identical contents equal + * (think diff --ignore-space-change). + */ + if (options->flags.allow_external && options->flags.exit_with_status) + options->flags.diff_from_contents = 1; + options->diff_path_counter = 0; if (options->flags.follow_renames) diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh index bed640b2b1..bbb49b8a0a 100755 --- a/t/t4020-diff-external.sh +++ b/t/t4020-diff-external.sh @@ -175,19 +175,22 @@ test_expect_success 'no diff with -diff' ' check_external_exit_code () { expect_code=$1 command_code=$2 - option=$3 + trust_exit_code=$3 + option=$4 command="exit $command_code;" - desc="external diff '$command'" + desc="external diff '$command' with trustExitCode=$trust_exit_code" test_expect_success "$desc via attribute with $option" " test_config diff.foo.command \"$command\" && + test_config diff.foo.trustExitCode $trust_exit_code && echo \"file diff=foo\" >.gitattributes && test_expect_code $expect_code git diff $option " test_expect_success "$desc via diff.external with $option" " test_config diff.external \"$command\" && + test_config diff.trustExitCode $trust_exit_code && >.gitattributes && test_expect_code $expect_code git diff $option " @@ -196,14 +199,22 @@ check_external_exit_code () { >.gitattributes && test_expect_code $expect_code env \ GIT_EXTERNAL_DIFF=\"$command\" \ + GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE=$trust_exit_code \ git diff $option " } -check_external_exit_code 1 0 --exit-code -check_external_exit_code 1 0 --quiet -check_external_exit_code 128 1 --exit-code -check_external_exit_code 1 1 --quiet # we don't even call the program +check_external_exit_code 1 0 off --exit-code +check_external_exit_code 1 0 off --quiet +check_external_exit_code 128 1 off --exit-code +check_external_exit_code 1 1 off --quiet # we don't even call the program + +check_external_exit_code 0 0 on --exit-code +check_external_exit_code 0 0 on --quiet +check_external_exit_code 1 1 on --exit-code +check_external_exit_code 1 1 on --quiet +check_external_exit_code 128 2 on --exit-code +check_external_exit_code 128 2 on --quiet echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file diff --git a/userdiff.c b/userdiff.c index f47e2d9f36..6cdfb147c3 100644 --- a/userdiff.c +++ b/userdiff.c @@ -446,6 +446,10 @@ int userdiff_config(const char *k, const char *v) return parse_tristate(&drv->binary, k, v); if (!strcmp(type, "command")) return git_config_string(&drv->external.cmd, k, v); + if (!strcmp(type, "trustexitcode")) { + drv->external.trust_exit_code = git_config_bool(k, v); + return 0; + } if (!strcmp(type, "textconv")) return git_config_string(&drv->textconv, k, v); if (!strcmp(type, "cachetextconv")) diff --git a/userdiff.h b/userdiff.h index 7e2b28e88d..5f1c8cc49c 100644 --- a/userdiff.h +++ b/userdiff.h @@ -13,6 +13,7 @@ struct userdiff_funcname { struct external_diff { const char *cmd; + unsigned trust_exit_code:1; }; struct userdiff_driver {