On Wed, Jan 12 2022, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
