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é
