Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting
- From
René Scharfe <l.s.r@web.de>
- Date
- Jun 7, 2024, 08:19 UTC
- Message-ID
- <524e4728-a63c-46fc-af73-35933c1906ee@web.de>
- In-Reply-To
- <7ef4a566-6d55-4924-b02c-38137c15791a@gmail.com>
Am 06.06.24 um 11:48 schrieb Phillip Wood:
Show 18 quoted lines
> 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?
Show 11 quoted lines
> > >> 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!
Show 10 quoted lines
>
>> - 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);Show 19 quoted lines
> >> -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é