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

On Thu, Jun 3, 2021 at 4:19 AM Jonathan Tan [off-list ref] wrote:

quoted

From: Han-Wen Nienhuys <redacted>

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.

quoted

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.

quoted

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