Re: [PATCH v2 7/7] diff: drop useless "status" parameter from diff_result_code()

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 22, 2023, 23:38 UTC
Message-ID: <xmqq350air6e.fsf@gitster.g>
In-Reply-To: <20230821202046.GG1798590@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 17 quoted lines

> Many programs use diff_result_code() to get a user-visible program exit
> code from a diff result (e.g., checking opts.found_changes if
> --exit-code was requested).
>
> This function also takes a "status" parameter, which seems at first
> glance that it could be used to propagate an error encountered when
> computing the diff. But it doesn't work that way:
>
>   - negative values are passed through as-is, but are not appropriate as
>     program exit codes
>
>   - when --exit-code or --check is in effect, we _ignore_ the passed-in
>     status completely. So a failed diff which did not have a chance to
>     set opts.found_changes would erroneously report "success, no
>     changes" instead of propagating the error.
>
> After recent cleanups, neither of these bugs is possible to trigger, as

Here "after recent cleanups" refers to the changes to make them die() upon seeing an error, instead of using it to call this function with non-zero in the status parameter? At least, they were signaling errors correctly when --exit-code is not in use, but now all the callers are responsible for exiting with non-zero status to signal an error even when --exit-code is *not* used.

> every caller just passes in "0". So rather than fixing them, we can
> simply drop the useless parameter instead.

OK.