On Mon, Aug 21, 2023 at 08:56:26AM -0700, Junio C Hamano wrote:
> > - when --exit-code is in use, we ignore the incoming "status" variable
> > entirely, and rely on the "has_changes" field. But if we saw an
> > error, this field is almost certainly invalid, and means we'd likely
> > just say "no changes", which is completely bogus. Likewise for
> > the "--check" format.
>
> Inspecting some callers of diff_result_code() further finds
> curiosities. wt-status.c for example feeds results form
> run_diff_index() and run_diff_files() to the function, neither of
> which returns anything other than 0. They die or exit(128)
> themselves, though, so they are OK without this fix.
> builtin/describe.c is also in the same boat with its use of
> run_diff_index().
>
> But of course the presense of such codepaths does not invalidate the
> fix in your patch.
Yeah, I admit I did not dig too much into the callers, as it was obvious
that any negative values were currently being mis-handled, and I knew at
least one caller passed them in (the one in builtin/diff.c).
And in fact I think that is the only problematic caller. Every other
caller either passes the result of run_diff_*(), which always return 0,
or just directly pass zero themselves!
It is only builtin/diff.c that calls its local builtin_diff_blobs(), and
so on, and those may return errors. So one direction is something like:
- convert those calls to die() more directly; this has the potential
side effect of producing a better message for the case that started
this thread by calling usage(builtin_diff_usage) instead of a short
error
- drop the now-useless return codes from those functions, along with
the already-useless ones from run_diff_files(), etc. At this point
everybody will just be passing a blind "0" to diff_result_code()
- drop the "status" parameter to diff_result_code()
That would make the code simpler. It does feel a bit like going in the
opposite direction of recent "pass errors up the stack rather than
dying" libification efforts. I think that's OK for the builtin_* helpers
in diff.c, which are just serving the diff porcelain. But things like
run_diff_files(), while pretty big operations, are something we might
call as small part of another operation (like git-describe).
We are not making things worse there, in the sense that their return
codes are currently meaningless. But removing the code entirely feels
like a step in the wrong direction. I dunno. Maybe we should retain
their return codes and have the callers die() if they fail (which would
currently be dead-code, but future-proof us for later cleanups).
-Peff
