From: "René Scharfe" <l.s.r@web•de>
To: phillip.wood@dunelm•org.uk, git@vger•kernel.org
Cc: Junio C Hamano <gitster@pobox•com>,
German Lashevich <german.lashevich@gmail•com>,
Johannes Sixt <j6t@kdbg•org>
Subject: Re: [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting
Date: Fri, 7 Jun 2024 10:19:50 +0200 [thread overview]
Message-ID: <524e4728-a63c-46fc-af73-35933c1906ee@web.de> (raw)
In-Reply-To: <7ef4a566-6d55-4924-b02c-38137c15791a@gmail.com>
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é
next prev parent reply other threads:[~2024-06-07 8:20 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-20 1:13 Possible git-diff bug when using exit-code with diff filters German Lashevich
2024-04-21 10:42 ` René Scharfe
2024-04-21 18:17 ` Junio C Hamano
2024-04-21 18:32 ` rsbecker
2024-04-21 19:09 ` Junio C Hamano
2024-04-21 20:18 ` rsbecker
2024-05-05 10:19 ` René Scharfe
2024-05-06 17:22 ` Junio C Hamano
2024-05-05 10:19 ` [PATCH 1/2] diff: report unmerged paths as changes in run_diff_cmd() René Scharfe
2024-05-05 10:20 ` [PATCH 2/2] diff: fix --exit-code with external diff René Scharfe
2024-05-05 15:25 ` Phillip Wood
2024-05-06 17:31 ` Junio C Hamano
2024-05-06 18:23 ` René Scharfe
2024-05-08 15:25 ` phillip.wood123
2024-05-11 20:32 ` René Scharfe
2024-05-12 9:38 ` René Scharfe
2024-06-05 8:31 ` [PATCH v2 0/3] " René Scharfe
2024-06-05 8:35 ` [PATCH v2 1/3] t4020: test exit code with external diffs René Scharfe
2024-06-05 8:36 ` [PATCH v2 2/3] userdiff: add and use struct external_diff René Scharfe
2024-06-05 8:38 ` [PATCH v2 3/3] diff: let external diffs report that changes are uninteresting René Scharfe
2024-06-06 6:39 ` Johannes Sixt
2024-06-06 8:28 ` René Scharfe
2024-06-06 15:49 ` Junio C Hamano
2024-06-06 9:48 ` Phillip Wood
2024-06-07 8:19 ` René Scharfe [this message]
2024-06-05 16:47 ` [PATCH v2 0/3] diff: fix --exit-code with external diff Junio C Hamano
2024-06-09 7:35 ` [PATCH v3 " René Scharfe
2024-06-09 7:38 ` [PATCH v3 1/3] t4020: test exit code with external diffs René Scharfe
2024-06-10 16:33 ` Junio C Hamano
2024-06-09 7:39 ` [PATCH v3 2/3] userdiff: add and use struct external_diff René Scharfe
2024-06-09 7:41 ` [PATCH v3 3/3] diff: let external diffs report that changes are uninteresting René Scharfe
2024-06-10 13:48 ` [PATCH v3 0/3] diff: fix --exit-code with external diff phillip.wood123
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=524e4728-a63c-46fc-af73-35933c1906ee@web.de \
--to=l.s.r@web$(echo .)de \
--cc=german.lashevich@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=j6t@kdbg$(echo .)org \
--cc=phillip.wood@dunelm$(echo .)org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox