Re: [PATCH v2 3/8] refs: make errno output explicit for read_raw_ref_fn

On Thu, Jul 1, 2021 at 1:55 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

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.

quoted

      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.

quoted

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