Re: [PATCH v3 0/5] refs: cleanup errno sideband ref related functions

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jul 05 2021, Han-Wen Nienhuys via GitGitGadget wrote:

> v5

v3?

>  * address Ævar's comment; punt on clearing errno.
> [...]
>       
>           Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
>           Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
>      +    Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

FWIW per Documentation/SubmittingPatches:
    
    . `Reviewed-by:`, unlike the other tags, can only be offered by the
      reviewer and means that she is completely satisfied that the patch
      is ready for application.  It is usually offered only after a
      detailed review.

It's not that I'm hard to please, but I can honestly say that I don't
quite understand some parts of what you're gonig for, so that trailer is
probably premature :)

>        ## refs/files-backend.c ##
>       @@ refs/files-backend.c: static int create_reflock(const char *path, void *cb)
>      @@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_re
>       -		last_errno = errno;
>       +		int last_errno = errno;
>        		if (last_errno != ENOTDIR ||
>      - 		    !refs_verify_refname_available(&refs->base, refname,
>      - 						   extras, skip, err))
>      +-		    !refs_verify_refname_available(&refs->base, refname,
>      +-						   extras, skip, err))
>      ++		    /* in case of D/F conflict, try to generate a better error
>      ++		     * message. If that fails, fall back to strerror(ENOTDIR).
>      ++		     */
>      ++		    !refs_verify_refname_available(&refs->base, refname, extras,
>      ++						   skip, err))
>      + 			strbuf_addf(err, "unable to resolve reference '%s': %s",
>      + 				    refname, strerror(last_errno));
>      + 
>       @@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
>        	if (is_null_oid(&lock->old_oid) &&
>        	    refs_verify_refname_available(refs->packed_ref_store, refname,
>  3:  3e2831e59c8 ! 3:  b017caf54ba refs: make errno output explicit for read_raw_ref_fn
>      @@ Metadata
>        ## Commit message ##
>           refs: make errno output explicit for read_raw_ref_fn
>       
>      +    This makes it explicit how alternative ref backends should report errors in
>      +    read_raw_ref_fn.
>      +
>           read_raw_ref_fn needs to supply a credible errno for a number of cases. These
>           are primarily:
>       
>      @@ Commit message
>       
>           Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
>           Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
>      +    Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>        ## refs.c ##
>       @@ refs.c: 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;
>      ++	int failure_errno;
>        	if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
>        		return refs_read_special_head(ref_store, refname, oid, referent,
>        					      type);
>      @@ refs.c: int refs_read_raw_ref(struct ref_store *ref_store,
>        
>       -	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
>       -					   type);
>      -+	failure = 0;
>      ++	failure_errno = 0;
>       +	result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
>      -+					     type, &failure);
>      -+	errno = failure;
>      ++					     type, &failure_errno);
>      ++	if (failure_errno)
>      ++		errno = failure_errno;
>       +	return result;
>        }

To rephrase my comment on the v2 to hopefully better get at the
point/question I had.

It wasn't that I don't get why you wouldn't save/restore errno in
general.

It's that the pattern of doing so seems backwards to me. I.e. surely the
goal here should be to one function at a time, and from the bottom-up,
figure out where we rely on "errno" and convert that to a
"failure_errno".

Instead not even files_read_raw_ref() resets "errno = 0" at the end, so
the errno /there/ can propagate upwards, and in this v3 we're not clearing it at all.

I'm all for clearing it as mentioned in another mail, surely that should
be the point of this whole thing, i.e. to refactor this part of the API
so that we're assured that nothing upstream of us relies on errno & prep
things for reftable (which won't set it at all, except to fake it).

Having dug a bit further, it seems what you're doing, whether it's
intentional or not, is relying on the parse_loose_ref_contents() setting
EINVAL, but you clobber your *failure_errno with it whether it returned
-1 or not.

Seemingly to make that logic work in files_read_raw_ref() you, after
getting an errno, assigned it "errno", instead of to a
saved_errno". Thus requiring an inverse of the pattern where you need a
"saved_errno" dance to save away an errno you explicitly don't care
about (because you didn't save the one you wanted earlier).

I think something like the following diff on top, this whole dance was
only needed because you didn't pass the failure_errno down to
refs_read_special_head() and parse_loose_ref_contents().

diff --git a/refs.c b/refs.c
index 25d80e544d0..eca7310e7a4 100644
--- a/refs.c
+++ b/refs.c
@@ -1653,7 +1653,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;
@@ -1663,7 +1664,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);
@@ -1683,7 +1685,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
 
 	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 8506c8b3bde..823325cc97f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -381,11 +381,12 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
 		goto out;
 
 	if (lstat(path, &st) < 0) {
-		if (errno != ENOENT)
+		*failure_errno = errno;
+		if (*failure_errno != ENOENT)
 			goto out;
 		if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
 				      referent, type, NULL)) {
-			errno = ENOENT;
+			*failure_errno = ENOENT;
 			goto out;
 		}
 		ret = 0;
@@ -396,7 +397,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
 	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)
+			*failure_errno = errno;
+			if (*failure_errno == ENOENT || *failure_errno == EINVAL)
 				/* inconsistent with lstat; retry */
 				goto stat_ref;
 			else
@@ -425,7 +427,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
 		 */
 		if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
 				      referent, type, NULL)) {
-			errno = EISDIR;
+			*failure_errno = errno = EISDIR;
 			goto out;
 		}
 		ret = 0;
@@ -438,7 +440,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
 	 */
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
-		if (errno == ENOENT && !S_ISLNK(st.st_mode))
+		if (*failure_errno == ENOENT && !S_ISLNK(st.st_mode))
 			/* inconsistent with lstat; retry */
 			goto stat_ref;
 		else
@@ -446,26 +448,25 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
 	}
 	strbuf_reset(&sb_contents);
 	if (strbuf_read(&sb_contents, fd, 256) < 0) {
-		int save_errno = 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, failure_errno);
 
 out:
-	*failure_errno = errno;
+	errno = 0; /* saved in *failure_errno */
 	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)) {
@@ -484,7 +485,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 c52a64b081b..530999f50df 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -700,7 +700,8 @@ struct ref_store {
  * Parse contents of a loose ref file.
  */
 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