Re: [PATCH 2/2] diff: fix --exit-code with external diff

From: René Scharfe <l.s.r@web.de>
Date: May 6, 2024, 18:23 UTC
Message-ID: <4bf12c41-a6ea-4939-ba76-e2c784952eaa@web.de>
In-Reply-To: <99337bc2-a691-45f7-9b6f-74ededbd9a78@gmail.com>

Am 05.05.24 um 17:25 schrieb Phillip Wood:

Show 7 quoted lines

> On 05/05/2024 11:20, René Scharfe wrote:
>> Finally, capture the output of the external diff and only register a
>> change by setting found_changes if the program wrote anything.
>
> I worry that this will lead to regression reports like
> https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/
> as the external diff will not see a terminal on stdout anymore.

So external diffs might no longer color their output if invoked using the option --exit-code. I assume this can be worked around by adding an option like --color=always to the diff command. This warrants a note in the docs at the very least, though.

But thinking about it, the external diff could be a GUI program that doesn't write to its stdout at all. Or it could write something if the files' contents match and stay silent if there are differences, weird as that may be.

> I'm not really clear why we ignore the exit code of the external diff
> command.

Historical reasons, I guess.

Show 5 quoted lines

> Merge strategies are expected to exit 0 on success, 1 when there are
> conflicts and another non-zero value for other errors - it would be
> nice to do something similar here where 1 means "there were
> differences" but it is probably too late to do that without a config
> value to indicate that we should trust the exit code.

Right, such a diff command protocol v2 would not need to pipe the output through an inspection loop. Sounds like a good idea. It's unfortunate that it would increase the configuration surface, which is not in an acute need to expand. We could advertise the new option when dying due to the unsupported combination of --exit-code and external diff, but that's in equal parts helpful and obnoxious, I feel.

René