Re: [PATCH] diff: handle negative status in diff_result_code()

From: Jeff King <peff@peff.net>
Date: Aug 21, 2023, 18:09 UTC
Message-ID: <20230821180943.GA2617193@coredump.intra.peff.net>
In-Reply-To: <xmqqlee4s82d.fsf@gitster.g>

On Mon, Aug 21, 2023 at 08:56:26AM -0700, Junio C Hamano wrote:

Show 16 quoted lines

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