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

From: Junio C Hamano <gitster@pobox.com>
Date: May 6, 2024, 17:31 UTC
Message-ID: <xmqqcypyzw1i.fsf@gitster.g>
In-Reply-To: <99337bc2-a691-45f7-9b6f-74ededbd9a78@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 6 quoted lines

>> 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.

Hmph. If it were Git that the external diff script eventually invokes that changes the behaviour (e.g. color and use of pager) based on the tty-ness of the output, we could use tricks like GIT_PAGER_IN_USE to propagate tty-ness down to the subprocess, but Git is not the only thing involved here, so it is not a very good general solution.

Show 7 quoted lines

> I'm
> not really clear why we ignore the exit code of the external diff
> command. 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.

Yeah, that thought crossed my mind. If we were designing the system from scratch today, that would certainly be what we would do. I suspect that it is because external diff drivers were invented way before "diff --exit-code" was introduced.

Thanks.