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

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

On Thu, Jun 3, 2021 at 4:19 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
> > references.
> >
> > The files ref backend does use EINVAL so parse_loose_ref_contents() can
> > communicate to lock_raw_ref() about garbage following the hex SHA1, or a short
> > read in files_read_raw_ref(), but the files backend does not call into
> > refs_read_raw_ref(), so its EINVAL sideband error is unused.
>
> Does this mean that there is some code that sets EINVAL, but no code
> uses it? If yes, that seems to mean that we can't remove EINVAL from the
> documentation, since some code still sets it.

It means that an alternate ref backend doesn't have to return EINVAL
to work properly.

added to commit message.

> > As the errno sideband is unintuitive and error-prone, remove EINVAL
> > value, as a step towards getting rid of the errno sideband altogether.
>
> How is removing one possible value a step towards getting rid of the
> errno sideband? I would have thought that we would be working towards
> transmitting all existing values to the new sideband (the out param).
> Unless we are planning to get rid of all values in the sideband - in
> which case, this should be described in the commit message.

It means that we don't have to spend braincycles in further commits of
this series reasoning about EINVAL.

The existing functions potentially transmit all kinds of errors, eg.
EIO if there was a problem with the media, or ENOTCONN if the git repo
happens to be on a FUSE filesystem that crashed. These are not
relevant to the ref backend, so we don't treat them specially either.

> > - * set errno appropriately and return -1.
> > + * Return 0 on success. If the ref doesn't exist, set errno to ENOENT and return
> > + * -1. If the ref exists but is neither a symbolic ref nor an object ID, it is
> > + * broken; set REF_ISBROKEN in type, and return -1. If there is another error
> > + * reading the ref, set errno appropriately and return -1.
>
> The rewrapping was unnecessary and makes it hard to see what changed.

Fixed.  I suppose .clang-format settings should add some configuration
about the line size for wrapping comments.

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