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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 6, 2021, 00:38 UTC
Message-ID: <87v95o5ku8.fsf@evledraar.gmail.com>
In-Reply-To: <pull.1012.v3.git.git.1625518566.gitgitgadget@gmail.com>

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

> v5

v3?

Show 6 quoted lines

>  * 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 :)

Show 63 quoted lines

>        ## 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