On Thu, Jul 1, 2021 at 1:55 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
> > diff --git a/refs.c b/refs.c
> > index 8c9490235ea6..bebe3f584da7 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -1675,13 +1675,17 @@ int refs_read_raw_ref(struct ref_store *ref_store,
> > const char *refname, struct object_id *oid,
> > struct strbuf *referent, unsigned int *type)
> > {
> > + int result, failure;
>
> Style nit: Different variables should be on different lines, except
> where they're the same, so "int i, j", not "int ret, i". Perhaps "ret"
> and "errno" are similar enough, but I'd split them, just my 0.02.
>
> Also nit: s/failure/failure_errno/, just like the function signature.
done.
> > if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
> > return refs_read_special_head(ref_store, refname, oid, referent,
> > type);
> > }
> >
> > - return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> > - type);
> > + failure = 0;
> > + result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> > + type, &failure);
> > + errno = failure;
> > + return result;
> > }
>
> Is there some subtlety here where this isn't equivalent to the more
> simple/straightforward:
>
> diff --git a/refs.c b/refs.c
> index bebe3f584da..49ab7555de9 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1675,17 +1675,14 @@ int refs_read_raw_ref(struct ref_store *ref_store,
> const char *refname, struct object_id *oid,
> struct strbuf *referent, unsigned int *type)
> {
> - int result, failure;
> if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
> return refs_read_special_head(ref_store, refname, oid, referent,
> type);
> }
>
> - failure = 0;
> - result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> - type, &failure);
> - errno = failure;
> - return result;
> + errno = 0;
> + return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> + type, &errno);
There is a big difference: in this case, in read_raw_ref, the
failure_errno pointer is aliased to errno. Which means that things
like
*failure_errno = 0
some_syscall();
might mysteriously clobber *failure_errno again, which is the kind of
effect-at-a-distance we're trying to get rid of.
> I can only imagine a case where we think files_read_raw_ref() would
> encounter a new errno after it assigned to *failure_errno, which is just
> a couple of strbuf_release() calls.
>
> if that is a case we're worried about then like in my comment on 2/8
> shouldn't we be explicitly checking for such a lost/different errno?
> I.e. something like (should probably be less fatal than BUG(...):
>
> diff --git a/refs.c b/refs.c
> index bebe3f584da..9584ddae392 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1675,16 +1675,22 @@ int refs_read_raw_ref(struct ref_store *ref_store,
> const char *refname, struct object_id *oid,
> struct strbuf *referent, unsigned int *type)
> {
> - int result, failure;
> + int result, failure_errno = 0;
> +
> if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
> return refs_read_special_head(ref_store, refname, oid, referent,
> type);
> }
>
> - failure = 0;
> + errno = 0;
> result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> - type, &failure);
> - errno = failure;
> + type, &failure_errno);
> + if (errno)
> + BUG("Got another errno from read_raw_ref()?: %s, failure errno: %s",
> + strerror(errno),
> + strerror(failure_errno));
The whole idea is to make sure that important (ie. errno values that
influence downstream logic) are separate from unimportant (ie. used
for error messages). By adding a BUG() here, you're making a drastic
downstream logic change depending on errno, so your example code is
going precisely in the opposite direction of where I want to go, and
forces implementers of read_raw_ref to walk on eggshells again wrt.
errno handling to avoid triggering error messages.
> > + * Return 0 on success. If the ref doesn't exist, set failure_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 (failure_errno
> > + * should not be ENOENT). The files backend may return EISDIR (if the ref name
> > + * is a directory) and ENOTDIR (if a ref prefix is not a directory). If there is
> > + * another error reading the ref, set failure_errno appropriately and return -1.
> > *
>
> This documentation is a bit confusing in light of the above comments. I
> assume that "set failure_errno appropriately" here means that it will do
> it, but it's really worded like it's suggesting that you should do it,
> and does the "another error" suggest that a caller may need to deal with
> an arbitrary errno, not just the ENOENT|EISDIR|ENOTDIR?
Tried to clarify this.
--
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
