On Tue, Aug 22, 2023 at 04:38:17PM -0700, Junio C Hamano wrote: > > 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. Sort of. Prior to this series, callers were responsible for ferrying exit codes to diff_result_code(), which mis-handled them (a little for the path without --exit-code, and badly with it). Now they are responsible for handling the errors themselves. I'd say that is not much more work than passing them along, and provides better outcomes (they can produce more useful error messages, or die() as appropriate). But what I really meant with "after recent cleanups" was just that nobody is even bothering to pass anything but 0 to diff_result_code(). Which was already true before the series for every code path except the few index/argument parsing paths in builtin/diff.c, and after the cleanups is entirely true. -Peff
