From: Phillip Wood Date: Thu, 06 Jun 2024 09:48:35 GMT Subject: Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting Message-ID: <7ef4a566-6d55-4924-b02c-38137c15791a@gmail.com> In-Reply-To: <99a99e5c-4fe4-413a-9281-363e280716b8@web.de> 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..trustExitCode and the environment > variable GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE. They pair with the config > options diff.external and diff..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