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

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

On Wed, Jan 12 2022, Junio C Hamano wrote:

> Æ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?

It's consistent with various other existing refs* code as we made this
transition, see:

    git grep -W 'errno = 0' -- 'refs*'

I.e. we'd like to not only transition existing users away from errno,
but to ensure that the file backend errno semantics don't inadvertently
leak outside the API.

Doing so is a bit pedantic for sure, but ensures that we won't need to
deal with any subtle bugs in that are with the reftable backend.