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

>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
> > references.
>
> We often use a pattern (which is common) like this:
>
>         if (some_func_in_ref_API(...) < 0) {
>                 if (errno == ENOENT || errno == EISDIR)
>                         ... it is OK for the file to be missing ...
>                 else
>                         ... error ...
>         }
>
> If a piece of code currently sets EINVAL to errno manually when
> signalling a failure by returning a negative value to communicate
> with such a caller, we wouldn't see EINVAL mentioned, so such a grep
> alone would not help us guarantee the correctness of an update to
> stop assignment of EINVAL at all.  The callers must be vetted more
> carefully than "we are happy that nobody explicitly mentions EINVAL".

> > 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.
>
> This paragraph is confusing.  It says EINVAL is used to signal
> lock_raw_ref(), and it says EINVAL is not used by the same files
> backend.  Which is correct?  If one part of the backend uses it, and
> other parts don't, wouldn't the backend as a whole still use it?

> That is because there is a codeflow like this:
>
>         if (files_read_raw_ref(...)) {
>                 if (errno == ENOENT) {
>                         ... do various things ...
>                 } else if (errno == EISDIR) {
>                         ... do different and various things ...
>                 } else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
>                         ... deal with broken ref ...
>                 }
>                  ...
>         }

> > + * 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.
>
> So, this is not sufficient to let caller correctly and safely handle
> errors.  "set REF_ISBROKEN in type, set errno to something other
> than ENOENT or EISDIR, and then return -1" is necessary, I would
> think.