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

[Han-Wen Nienhuys <hanwen@google.com>]

On Fri, Apr 23, 2021 at 2:57 PM Jeff King <peff@peff.net> wrote:
>
> 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.

Well spotted. I reverted this part (I did glance over existing
callers, and couldn't find anyone inspecting errno)

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

would you say this is warranted here? refs.h doesn't mention the word
errno, so this behavior isn't documented at all. I also looked over
the current callers of read_raw_ref, and outside of refs/*.c none seem
to inspect errno.

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

The function has several exit paths that don't set errno at all, so
the result is kind of random anyway, but I can't see the code I don't
have. I've updated the series, with some real progress to stamping out
errno. Hope this pleases you better.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado