Re: [PATCH] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn

[Jeff King <peff@peff.net>]

On Fri, Apr 23, 2021 at 10:24:06AM +0000, Han-Wen Nienhuys via GitGitGadget wrote:

> Subject: refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn

The subject says "read_raw_ref_fn", but the patch is touching
refs_resolve_ref_unsafe(). The former is an abstract type, and I didn't
dig to see the relationships, but I'll focus on the code change in the
patch.

> A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
> references.

I don't think that's sufficient, for two reasons:

  - in general we try to be careful about forks and topics in flight,
    which might end up with semantic conflicts. So we don't necessarily
    assume that we can see all code, and prefer if any subtle changes
    like this at least result in a compile failure (e.g., changing
    function name or signature). In practice, this is balanced with how
    likely such code is, how bad the breakage would be, what we're
    gaining, etc.

  - just because they are not looking for EINVAL specifically doesn't
    mean they are not looking at errno at all (e.g., after calling
    refs_resolve_ref_unsafe(), lock_ref_oid_basic() does so). So we have
    to set errno to _something_ after the error. After your patch, we
    don't set it at all for these error returns, and so we'll be left
    with whatever junk was in errno from a previous unrelated syscall,
    which could be very misleading. Since we have to set it to
    something, EINVAL seems like a reasonable value.

I certainly buy the argument that errno is a pretty lousy channel for
passing back error data, for a number of reasons.  If we were going all
the way towards getting rid of errno in this function (and replacing it
with something better, as we must, since some callers _do_ care about
more detailed information), I could see the value. But this patch
doesn't get us anywhere useful and risks regressions in the meantime.

-Peff