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

Message ID	ebd7b8380bf7aed84d23973a4809d1441aa21692.1619710329.git.gitgitgadget@gmail.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <git-owner@kernel.org> Message-Id: <ebd7b8380bf7aed84d23973a4809d1441aa21692.1619710329.git.gitgitgadget@gmail.com> In-Reply-To: <pull.1012.git.git.1619710329.gitgitgadget@gmail.com> References: <pull.1012.git.git.1619710329.gitgitgadget@gmail.com> Date: Thu, 29 Apr 2021 15:32:03 +0000 Subject: [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn Fcc: Sent Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MIME-Version: 1.0 To: git@vger.kernel.org Cc: Han-Wen Nienhuys <hanwenn@gmail.com>, Han-Wen Nienhuys <hanwen@google.com> Precedence: bulk From: Han-Wen Nienhuys <hanwen@google.com>
Series	refs: cleanup errno sideband ref related functions \| expand [0/8] refs: cleanup errno sideband ref related functions [1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn [2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic [3/8] refs: make errno output explicit for read_raw_ref_fn [4/8] refs: make errno output explicit for refs_resolve_ref_unsafe [5/8] refs: add failure_errno to refs_read_raw_ref() signature [6/8] refs: clear errno return in refs_resolve_ref_unsafe() [7/8] refs: stop setting EINVAL and ELOOP in symref resolution [8/8] refs: explicitly propagate errno from refs_read_raw_ref

Han-Wen Nienhuys April 29, 2021, 3:32 p.m. UTC

From: Han-Wen Nienhuys <hanwen@google.com>

read_raw_ref_fn needs to supply a credible errno for a number of cases. These
are primarily:

1) The files backend calls read_raw_ref from lock_raw_ref, and uses the
resulting error codes to create/remove directories as needed.

2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set,
returning the last successfully resolved symref. This is necessary so
read_raw_ref("HEAD") on an empty repo returns refs/heads/main (or the default branch
du-jour), and we know on which branch to create the first commit.

Make this information flow explicit by adding a failure_errno to the signature
of read_raw_ref. All errnos from the files backend are still propagated
unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are
relevant.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs.c                |  7 +++++--
 refs/debug.c          |  4 ++--
 refs/files-backend.c  | 24 ++++++++++++------------
 refs/packed-backend.c |  8 ++++----
 refs/refs-internal.h  | 16 +++++++++-------
 5 files changed, 32 insertions(+), 27 deletions(-)

Junio C Hamano April 30, 2021, 3:34 a.m. UTC | #1

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> read_raw_ref_fn needs to supply a credible errno for a number of cases. These
> are primarily:
>
> 1) The files backend calls read_raw_ref from lock_raw_ref, and uses the
> resulting error codes to create/remove directories as needed.
>
> 2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set,
> returning the last successfully resolved symref. This is necessary so
> read_raw_ref("HEAD") on an empty repo returns refs/heads/main (or the default branch
> du-jour), and we know on which branch to create the first commit.
>
> Make this information flow explicit by adding a failure_errno to the signature
> of read_raw_ref. All errnos from the files backend are still propagated
> unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are
> relevant.

I like the general direction to move away from errno, which may make
sense only in the context of files backend (e.g. ENOENT would be
left in errno, only because the original "loose ref" implementation
used one file per ref, when a ref we wanted to see did not exist)
and having other backends use the same errno would not make much
sense, as it is not the goal to make other backends like reftable
emulate files backend.

I wonder if in the ideal world, we'd rather want to define our own
enum, not <errno.h>, that is specific to failure modes of ref API
functions and signal failures by returning these values (and the
enum includes 0 as a value to signal success, all other errors are
negative values).

What I am really getting at is if we need an extra "failure"
out-parameter-pointer in the internal API.  I do not mind if it
helps the transition to the ideal world, but I offhand do not see if
we need result and failure as separate variables.

> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  refs.c                |  7 +++++--
>  refs/debug.c          |  4 ++--
>  refs/files-backend.c  | 24 ++++++++++++------------
>  refs/packed-backend.c |  8 ++++----
>  refs/refs-internal.h  | 16 +++++++++-------
>  5 files changed, 32 insertions(+), 27 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 261fd82beb98..43e2ad6b612a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1675,13 +1675,16 @@ 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);
>  	}
>  
> -	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> -					   type);
> +	result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> +					     type, &failure);
> +	errno = failure;
> +	return result;
>  }
>  
>  /* This function needs to return a meaningful errno on failure */
> diff --git a/refs/debug.c b/refs/debug.c
> index 922e64fa6ad9..887dbb14be6e 100644
> --- a/refs/debug.c
> +++ b/refs/debug.c
> @@ -238,14 +238,14 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
>  
>  static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
>  			      struct object_id *oid, struct strbuf *referent,
> -			      unsigned int *type)
> +			      unsigned int *type, int *failure_errno)
>  {
>  	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
>  	int res = 0;
>  
>  	oidcpy(oid, &null_oid);
>  	res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent,
> -					    type);
> +					    type, failure_errno);
>  
>  	if (res == 0) {
>  		trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index c9511da1d387..efe493ca1425 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -341,9 +341,9 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
>  	return refs->loose;
>  }
>  
> -static int files_read_raw_ref(struct ref_store *ref_store,
> -			      const char *refname, struct object_id *oid,
> -			      struct strbuf *referent, unsigned int *type)
> +static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
> +			      struct object_id *oid, struct strbuf *referent,
> +			      unsigned int *type, int *failure_errno)
>  {
>  	struct files_ref_store *refs =
>  		files_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
> @@ -354,7 +354,6 @@ static int files_read_raw_ref(struct ref_store *ref_store,
>  	struct stat st;
>  	int fd;
>  	int ret = -1;
> -	int save_errno;
>  	int remaining_retries = 3;
>  
>  	*type = 0;
> @@ -459,10 +458,10 @@ static int files_read_raw_ref(struct ref_store *ref_store,
>  	ret = parse_loose_ref_contents(buf, oid, referent, type);
>  
>  out:
> -	save_errno = errno;
> +	if (failure_errno)
> +		*failure_errno = errno;
>  	strbuf_release(&sb_path);
>  	strbuf_release(&sb_contents);
> -	errno = save_errno;
>  	return ret;
>  }
>  
> @@ -541,6 +540,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
>  	struct strbuf ref_file = STRBUF_INIT;
>  	int attempts_remaining = 3;
>  	int ret = TRANSACTION_GENERIC_ERROR;
> +	int failure_errno = 0;
>  
>  	assert(err);
>  	files_assert_main_repository(refs, "lock_raw_ref");
> @@ -629,9 +629,9 @@ static int lock_raw_ref(struct files_ref_store *refs,
>  	 * fear that its value will change.
>  	 */
>  
> -	if (files_read_raw_ref(&refs->base, refname,
> -			       &lock->old_oid, referent, type)) {
> -		if (errno == ENOENT) {
> +	if (files_read_raw_ref(&refs->base, refname, &lock->old_oid, referent,
> +			       type, &failure_errno)) {
> +		if (failure_errno == ENOENT) {
>  			if (mustexist) {
>  				/* Garden variety missing reference. */
>  				strbuf_addf(err, "unable to resolve reference '%s'",
> @@ -655,7 +655,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
>  				 *   reference named "refs/foo/bar/baz".
>  				 */
>  			}
> -		} else if (errno == EISDIR) {
> +		} else if (failure_errno == EISDIR) {
>  			/*
>  			 * There is a directory in the way. It might have
>  			 * contained references that have been deleted. If
> @@ -693,13 +693,13 @@ static int lock_raw_ref(struct files_ref_store *refs,
>  					goto error_return;
>  				}
>  			}
> -		} else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
> +		} else if (failure_errno == EINVAL && (*type & REF_ISBROKEN)) {
>  			strbuf_addf(err, "unable to resolve reference '%s': "
>  				    "reference broken", refname);
>  			goto error_return;
>  		} else {
>  			strbuf_addf(err, "unable to resolve reference '%s': %s",
> -				    refname, strerror(errno));
> +				    refname, strerror(failure_errno));
>  			goto error_return;
>  		}
>  
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index dfecdbc1db60..a457f18e93c8 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -724,9 +724,9 @@ static struct snapshot *get_snapshot(struct packed_ref_store *refs)
>  	return refs->snapshot;
>  }
>  
> -static int packed_read_raw_ref(struct ref_store *ref_store,
> -			       const char *refname, struct object_id *oid,
> -			       struct strbuf *referent, unsigned int *type)
> +static int packed_read_raw_ref(struct ref_store *ref_store, const char *refname,
> +			       struct object_id *oid, struct strbuf *referent,
> +			       unsigned int *type, int *failure_errno)
>  {
>  	struct packed_ref_store *refs =
>  		packed_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
> @@ -739,7 +739,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,
>  
>  	if (!rec) {
>  		/* refname is not a packed reference. */
> -		errno = ENOENT;
> +		*failure_errno = ENOENT;
>  		return -1;
>  	}
>  
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 29728a339fed..ac8a14086724 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -617,10 +617,12 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,
>   * properly-formatted or even safe reference name. NEITHER INPUT NOR
>   * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION.
>   *
> - * Return 0 on success. If the ref doesn't exist, set 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. If there is another error
> - * reading the ref, set errno appropriately and return -1.
> + * 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. For the files
> + * backend, EISDIR and ENOTDIR may be set if the ref name is a directory. If
> + * there is another error reading the ref, set failure_errno appropriately and
> + * return -1.
>   *
>   * Backend-specific flags might be set in type as well, regardless of
>   * outcome.
> @@ -634,9 +636,9 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,
>   * - in all other cases, referent will be untouched, and therefore
>   *   refname will still be valid and unchanged.
>   */
> -typedef int read_raw_ref_fn(struct ref_store *ref_store,
> -			    const char *refname, struct object_id *oid,
> -			    struct strbuf *referent, unsigned int *type);
> +typedef int read_raw_ref_fn(struct ref_store *ref_store, const char *refname,
> +			    struct object_id *oid, struct strbuf *referent,
> +			    unsigned int *type, int *failure_errno);
>  
>  struct ref_storage_be {
>  	struct ref_storage_be *next;

Junio C Hamano April 30, 2021, 6:02 a.m. UTC | #2

Junio C Hamano <gitster@pobox.com> writes:

> I like the general direction to move away from errno, which may make
> sense only in the context of files backend (e.g. ENOENT would be
> left in errno, only because the original "loose ref" implementation
> used one file per ref, when a ref we wanted to see did not exist)
> and having other backends use the same errno would not make much
> sense, as it is not the goal to make other backends like reftable
> emulate files backend.
>
> I wonder if in the ideal world, we'd rather want to define our own
> enum, not <errno.h>, that is specific to failure modes of ref API
> functions and signal failures by returning these values (and the
> enum includes 0 as a value to signal success, all other errors are
> negative values).
>
> What I am really getting at is if we need an extra "failure"
> out-parameter-pointer in the internal API.  I do not mind if it
> helps the transition to the ideal world, but I offhand do not see if
> we need result and failure as separate variables.

In any case, the change in the function signature helped me catch
that this series invalidates what has been queued on hn/reftable
without updating that topic to align with the new way to handle the
errno (it would have become a silent semantic failure if we instead
encoded the "failure" in the return value).

The following is what I am tentatively using for tonight's
integration when merging this and hn/reftable together to the 'seen'
branch.

Thanks.


 refs/reftable-backend.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git c/refs/reftable-backend.c w/refs/reftable-backend.c
index 1aff21adb4..4385d2d6f5 100644
--- c/refs/reftable-backend.c
+++ w/refs/reftable-backend.c
@@ -52,7 +52,8 @@ static struct reftable_stack *stack_for(struct git_reftable_ref_store *store,
 static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 				     const char *refname, struct object_id *oid,
 				     struct strbuf *referent,
-				     unsigned int *type);
+				     unsigned int *type,
+				     int *failure_errno);
 
 static void clear_reftable_log_record(struct reftable_log_record *log)
 {
@@ -376,7 +377,8 @@ static int fixup_symrefs(struct ref_store *ref_store,
 						&old_oid, &referent,
 						/* mutate input, like
 						   files-backend.c */
-						&update->type);
+						&update->type,
+						&errno);
 		if (err < 0 && errno == ENOENT &&
 		    is_null_oid(&update->old_oid)) {
 			err = 0;
@@ -1538,7 +1540,7 @@ static int reftable_error_to_errno(int err)
 static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 				     const char *refname, struct object_id *oid,
 				     struct strbuf *referent,
-				     unsigned int *type)
+				     unsigned int *type, int *failure_errno)
 {
 	struct git_reftable_ref_store *refs =
 		(struct git_reftable_ref_store *)ref_store;
@@ -1560,12 +1562,12 @@ static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 
 	err = reftable_stack_read_ref(stack, refname, &ref);
 	if (err > 0) {
-		errno = ENOENT;
+		*failure_errno = ENOENT;
 		err = -1;
 		goto done;
 	}
 	if (err < 0) {
-		errno = reftable_error_to_errno(err);
+		*failure_errno = reftable_error_to_errno(err);
 		err = -1;
 		goto done;
 	}
@@ -1578,7 +1580,7 @@ static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 		oidread(oid, reftable_ref_record_val1(&ref));
 	} else {
 		*type |= REF_ISBROKEN;
-		errno = EINVAL;
+		*failure_errno = EINVAL;
 		err = -1;
 	}
 done:

Han-Wen Nienhuys May 19, 2021, 12:33 p.m. UTC | #3

On Fri, Apr 30, 2021 at 8:02 AM Junio C Hamano <gitster@pobox.com> wrote:
> > I like the general direction to move away from errno, which may make
> > sense only in the context of files backend (e.g. ENOENT would be
> > left in errno, only because the original "loose ref" implementation
> > used one file per ref, when a ref we wanted to see did not exist)
> > and having other backends use the same errno would not make much
> > sense, as it is not the goal to make other backends like reftable
> > emulate files backend.
> >
> > I wonder if in the ideal world, we'd rather want to define our own
> > enum, not <errno.h>, that is specific to failure modes of ref API
> > functions and signal failures by returning these values (and the
> > enum includes 0 as a value to signal success, all other errors are
> > negative values).

I think it would be healthy to have a set of canonical error codes in
general,  but I don't know if this will help here. The files and
packed backend want to communicate about very specific conditions
(ENOENT, EISDIR, ENOTDIR), which seem too low-level to put in a
generic error code.

> In any case, the change in the function signature helped me catch
> that this series invalidates what has been queued on hn/reftable
> without updating that topic to align with the new way to handle the
> errno (it would have become a silent semantic failure if we instead
> encoded the "failure" in the return value).
>
> The following is what I am tentatively using for tonight's
> integration when merging this and hn/reftable together to the 'seen'
> branch.

That sounds right. How should I post the next update to hn/reftable ?
(based on which branch?)

Jonathan Tan June 3, 2021, 2:37 a.m. UTC | #4

> From: Han-Wen Nienhuys <hanwen@google.com>
> 
> read_raw_ref_fn needs to supply a credible errno for a number of cases. These
> are primarily:
> 
> 1) The files backend calls read_raw_ref from lock_raw_ref, and uses the
> resulting error codes to create/remove directories as needed.
> 
> 2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set,
> returning the last successfully resolved symref. This is necessary so
> read_raw_ref("HEAD") on an empty repo returns refs/heads/main (or the default branch
> du-jour), and we know on which branch to create the first commit.
> 
> Make this information flow explicit by adding a failure_errno to the signature
> of read_raw_ref. All errnos from the files backend are still propagated
> unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are
> relevant.

Looks good.

> @@ -1675,13 +1675,16 @@ 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);
>  	}
>  
> -	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> -					   type);
> +	result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> +					     type, &failure);
> +	errno = failure;
> +	return result;
>  }

Should we initialize "failure" to 0 here? As a reader, I would assume
that "failure" has the semantics of errno here, which upon success, may
or may not be set.

Han-Wen Nienhuys June 10, 2021, 10:05 a.m. UTC | #5

On Thu, Jun 3, 2021 at 4:37 AM Jonathan Tan <jonathantanmy@google.com> wrote:
> Should we initialize "failure" to 0 here? As a reader, I would assume
> that "failure" has the semantics of errno here, which upon success, may
> or may not be set.

Done.

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

Commit Message

Comments

Patch