Re: [PATCH v3 1/3] refs API: use "failure_errno", not "errno"

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> @@ -1722,8 +1722,6 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
>  		if (refs_read_raw_ref(refs, refname, oid, &sb_refname,
>  				      &read_flags, failure_errno)) {
>  			*flags |= read_flags;
> -			if (errno)
> -				*failure_errno = errno;

Looks good.  

The whole point of passing failure_errno down to refs_read_raw_ref()
is that we capture the reason of the failure there without having to
rely on errno at this point in the flow.  Removal of this assignment
makes perfect sense.

But ...

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index b529fdf237e..43a3b882d7c 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -382,7 +382,6 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
>  	if (lstat(path, &st) < 0) {
>  		int ignore_errno;
>  		myerr = errno;
> -		errno = 0;
>  		if (myerr != ENOENT)
>  			goto out;
>  		if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
> @@ -399,7 +398,6 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
>  		strbuf_reset(&sb_contents);
>  		if (strbuf_readlink(&sb_contents, path, st.st_size) < 0) {
>  			myerr = errno;
> -			errno = 0;
>  			if (myerr == ENOENT || myerr == EINVAL)
>  				/* inconsistent with lstat; retry */
>  				goto stat_ref;
> @@ -469,6 +467,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
>  
>  	strbuf_release(&sb_path);
>  	strbuf_release(&sb_contents);
> +	errno = 0;
>  	return ret;
>  }

... it is not clear to me if this part makes sense.  If everybody
above the callstack uses failure_errno as the source of truth,
clearing errno here in this function should not be necessary and is
misleading to readers of the code, no?