refs API: make parse_loose_ref_contents() not set errno

Change the parse_loose_ref_contents() function to stop setting "errno"
and failure, and to instead pass up a "failure_errno" via a
parameter. This requires changing its callers to do the same.

The EINVAL error from parse_loose_ref_contents is used in files-backend
to create a custom error message.

In untangling this we discovered a tricky edge case. The
refs_read_special_head() function was relying on
parse_loose_ref_contents() setting EINVAL.

By converting it to use "saved_errno" we can migrate away from "errno"
in this part of the code entirely, and do away with an existing
"save_errno" pattern, its only purpose was to not clobber the "errno"
we previously needed at the end of files_read_raw_ref().

Let's assert that we can do that by not having files_read_raw_ref()
itself operate on *failure_errno in addition to passing it on. Instead
we'll assert that if we return non-zero we actually do set errno, thus
assuring ourselves and callers that they can trust the resulting
"failure_errno".

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs.c b/refs.c
index 200c44e696375a7d337b839ede6518196007e1b9..bb09993c2f96512602b033c08e4fc540650fe881 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -1648,7 +1648,8 @@ int for_each_fullref_in_prefixes(const char *namespace,
 
 static int refs_read_special_head(struct ref_store *ref_store,
                                  const char *refname, struct object_id *oid,
-                                 struct strbuf *referent, unsigned int *type)
+                                 struct strbuf *referent, unsigned int *type,
+                                 int *failure_errno)
 {
        struct strbuf full_path = STRBUF_INIT;
        struct strbuf content = STRBUF_INIT;
@@ -1658,7 +1659,8 @@ static int refs_read_special_head(struct ref_store *ref_store,
        if (strbuf_read_file(&content, full_path.buf, 0) < 0)
                goto done;
 
-       result = parse_loose_ref_contents(content.buf, oid, referent, type);
+       result = parse_loose_ref_contents(content.buf, oid, referent, type,
+                                         failure_errno);
 
 done:
        strbuf_release(&full_path);
@@ -1673,7 +1675,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
        assert(failure_errno);
        if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
                return refs_read_special_head(ref_store, refname, oid, referent,
-                                             type);
+                                             type, failure_errno);
        }
 
        return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 94c194665ed98fa11de77586dc5d8bdf0819461f..c73ffd1ca339cd40182c7e37eefc84075f0ba59f 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -355,6 +355,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
        int fd;
        int ret = -1;
        int remaining_retries = 3;
+       int myerr = 0;
 
        *type = 0;
        strbuf_reset(&sb_path);
@@ -382,11 +383,13 @@ stat_ref:
 
        if (lstat(path, &st) < 0) {
                int ignore_errno;
-               if (errno != ENOENT)
+               myerr = errno;
+               errno = 0;
+               if (myerr != ENOENT)
                        goto out;
                if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
                                      referent, type, &ignore_errno)) {
-                       errno = ENOENT;
+                       myerr = ENOENT;
                        goto out;
                }
                ret = 0;
@@ -397,7 +400,9 @@ stat_ref:
        if (S_ISLNK(st.st_mode)) {
                strbuf_reset(&sb_contents);
                if (strbuf_readlink(&sb_contents, path, st.st_size) < 0) {
-                       if (errno == ENOENT || errno == EINVAL)
+                       myerr = errno;
+                       errno = 0;
+                       if (myerr == ENOENT || myerr == EINVAL)
                                /* inconsistent with lstat; retry */
                                goto stat_ref;
                        else
@@ -427,7 +432,7 @@ stat_ref:
                 */
                if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
                                      referent, type, &ignore_errno)) {
-                       errno = EISDIR;
+                       myerr = EISDIR;
                        goto out;
                }
                ret = 0;
@@ -440,7 +445,8 @@ stat_ref:
         */
        fd = open(path, O_RDONLY);
        if (fd < 0) {
-               if (errno == ENOENT && !S_ISLNK(st.st_mode))
+               myerr = errno;
+               if (myerr == ENOENT && !S_ISLNK(st.st_mode))
                        /* inconsistent with lstat; retry */
                        goto stat_ref;
                else
@@ -448,26 +454,29 @@ stat_ref:
        }
        strbuf_reset(&sb_contents);
        if (strbuf_read(&sb_contents, fd, 256) < 0) {
-               int save_errno = errno;
+               myerr = errno;
                close(fd);
-               errno = save_errno;
                goto out;
        }
        close(fd);
        strbuf_rtrim(&sb_contents);
        buf = sb_contents.buf;
 
-       ret = parse_loose_ref_contents(buf, oid, referent, type);
+       ret = parse_loose_ref_contents(buf, oid, referent, type, &myerr);
 
 out:
-       *failure_errno = errno;
+       if (ret && !myerr)
+               BUG("returning non-zero %d, should have set myerr!", ret);
+       *failure_errno = myerr;
+
        strbuf_release(&sb_path);
        strbuf_release(&sb_contents);
        return ret;
 }
 
 int parse_loose_ref_contents(const char *buf, struct object_id *oid,
-                            struct strbuf *referent, unsigned int *type)
+                            struct strbuf *referent, unsigned int *type,
+                            int *failure_errno)
 {
        const char *p;
        if (skip_prefix(buf, "ref:", &buf)) {
@@ -486,7 +495,7 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,
        if (parse_oid_hex(buf, oid, &p) ||
            (*p != '\0' && !isspace(*p))) {
                *type |= REF_ISBROKEN;
-               errno = EINVAL;
+               *failure_errno = EINVAL;
                return -1;
        }
        return 0;

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index c87f1135e5b6630c5e14b531e811fb221b0f9288..121b8fdad08b7ae84e85c3ac9026533f2e3bddb6 100644 (file)
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -706,10 +706,12 @@ struct ref_store {
 };
 
 /*
- * Parse contents of a loose ref file.
+ * Parse contents of a loose ref file. *failure_errno maybe be set to EINVAL for
+ * invalid contents.
  */
 int parse_loose_ref_contents(const char *buf, struct object_id *oid,
-                            struct strbuf *referent, unsigned int *type);
+                            struct strbuf *referent, unsigned int *type,
+                            int *failure_errno);
 
 /*
  * Fill in the generic part of refs and add it to our collection of