Re: [PATCH v2 6/8] refs: add failure_errno to refs_read_raw_ref() signature

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 1, 2021, 12:06 UTC
Message-ID: <878s2qdy8y.fsf@evledraar.gmail.com>
In-Reply-To: <2b346caf1aed265ca4787de4208375bd5cfb7bc7.1623329869.git.gitgitgadget@gmail.com>

On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote:

Show 44 quoted lines

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> This makes the errno output of refs_read_raw_ref explicit.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  refs.c                | 29 ++++++++++++++---------------
>  refs/files-backend.c  |  8 ++++----
>  refs/packed-backend.c | 10 ++++++----
>  refs/refs-internal.h  |  6 +++---
>  4 files changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 64e2d55adcfb..ed2dde1c0c6d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1671,21 +1671,19 @@ static int refs_read_special_head(struct ref_store *ref_store,
>  	return result;
>  }
>  
> -int refs_read_raw_ref(struct ref_store *ref_store,
> -		      const char *refname, struct object_id *oid,
> -		      struct strbuf *referent, unsigned int *type)
> +int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
> +		      struct object_id *oid, struct strbuf *referent,
> +		      unsigned int *type, int *failure_errno)
>  {
> -	int result, failure;
> +	if (failure_errno)
> +		*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;
> -	result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> -					     type, &failure);
> -	errno = failure;
> -	return result;
> +	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> +					   type, failure_errno);
>  }

Ah, so here we drop the whole intermediate step of having this function not take a failure_errno itself. I think this would be better squashed into that earlier change.

Show 6 quoted lines

>  /* This function needs to return a meaningful errno on failure */
> @@ -1726,9 +1724,10 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
>  
>  	for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
>  		unsigned int read_flags = 0;
> +		int read_failure = 0;

Let's call it failure_errno consistently if we end up keeping it.

Show 17 quoted lines

> -		if (refs_read_raw_ref(refs, refname,
> -				      oid, &sb_refname, &read_flags)) {
> +		if (refs_read_raw_ref(refs, refname, oid, &sb_refname,
> +				      &read_flags, &read_failure)) {
>  			*flags |= read_flags;
>  
>  			/* In reading mode, refs must eventually resolve */
> @@ -1740,9 +1739,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
>  			 * may show errors besides ENOENT if there are
>  			 * similarly-named refs.
>  			 */
> -			if (errno != ENOENT &&
> -			    errno != EISDIR &&
> -			    errno != ENOTDIR)
> +			if (read_failure != ENOENT && read_failure != EISDIR &&
> +			    read_failure != ENOTDIR)
>  				return NULL;

But ditto my previous comments, this seems like a whole dance to avoid reading errno directly in cases where doing so is actually OK. I.e. the "last_errno" pattern is for things that encounter an errno, do some other stuff (such as a printf) where they might get /another/ errno (or reset it), but in this case we can just document "these will set errno on failure".

Show 11 quoted lines

>  			oidclr(oid);
> @@ -2254,7 +2252,8 @@ int refs_verify_refname_available(struct ref_store *refs,
>  		if (skip && string_list_has_string(skip, dirname.buf))
>  			continue;
>  
> -		if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent, &type)) {
> +		if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent,
> +				       &type, NULL)) {
>  			strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
>  				    dirname.buf, refname);
>  			goto cleanup;

And if we do care about errno at all, why would we not add it with a strerror() here instead of explicitly ignoring it?

Show 15 quoted lines

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 5a430aabf623..01c9bd0dbf04 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -383,8 +383,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
>  	if (lstat(path, &st) < 0) {
>  		if (errno != ENOENT)
>  			goto out;
> -		if (refs_read_raw_ref(refs->packed_ref_store, refname,
> -				      oid, referent, type)) {
> +		if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
> +				      referent, type, NULL)) {
>  			errno = ENOENT;
>  			goto out;
>  		}

In this case that seems to make sense, since we substitute our own errno. Maybe I haven't read this all carefully enough, but why are we not caring about the errno refs_read_raw_ref() might return here? Is it because we might take the refs_read_special_head() codepath?

In any case, I for one would appreciate a comment/commit message note about why we ignore the errno these "I'll give you an errno on failure" functions return in some cases, but not others.

I think it's probably best to split those into another commit, i.e. do the "we just ferry errno around differently now (if we'll do that at all)" as one step, and "here we might get errno, but we like our own better" as another.

Show 10 quoted lines

> @@ -423,8 +423,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
>  		 * ref is supposed to be, there could still be a
>  		 * packed ref:
>  		 */
> -		if (refs_read_raw_ref(refs->packed_ref_store, refname,
> -				      oid, referent, type)) {
> +		if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
> +				      referent, type, NULL)) {
>  			errno = EISDIR;
>  			goto out;

Ditto.

Show 13 quoted lines

> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index a457f18e93c8..03353ce48869 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -739,7 +739,8 @@ static int packed_read_raw_ref(struct ref_store *ref_store, const char *refname,
>  
>  	if (!rec) {
>  		/* refname is not a packed reference. */
> -		*failure_errno = ENOENT;
> +		if (failure_errno)
> +			*failure_errno = ENOENT;
>  		return -1;
>  	}

FWIW I'd think it's better in terms of code readability to not do this, and make callers who don't care about our errno explicitly provide a throwaway variable to show that they're not caring, but I don't have the full picture yet. I.e. make them do something like:

	int ret;
	int got_errno;
	ret = func(..., &got_errno);
	if (!ret) {
		if (!got_errno)
			BUG("error but no errno?");
		/* We don't care, use our own */
		got_errno = ENOSOMETHING
		...