Re: [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Jun 3, 2021, 02:19 UTC
- Message-ID
- <20210603021927.343351-1-jonathantanmy@google.com>
- In-Reply-To
- <7e8181e77d409af7595e357ad233b7781e026b78.1619710329.git.gitgitgadget@gmail.com>
Show 9 quoted lines
> 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.
> 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.
[snip]
Show 21 quoted lines
> --- > refs/refs-internal.h | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/refs/refs-internal.h b/refs/refs-internal.h > index 467f4b3c936d..29728a339fed 100644 > --- a/refs/refs-internal.h > +++ b/refs/refs-internal.h > @@ -617,11 +617,10 @@ typedef int reflog_expire_fn(struct ref_store *ref_store, > * properly-formatted or even safe reference name. NEITHER INPUT NOR > * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION. > * > - * 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, set errno to > - * EINVAL, and return -1. If there is another error reading the ref, > - * 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.