Re: [PATCH v3 1/3] refs API: use "failure_errno", not "errno"
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 12, 2022, 19:59 UTC
- Message-ID
- <xmqqh7a8u3my.fsf@gitster.g>
- In-Reply-To
- <patch-v3-1.3-a45268ac24b-20220112T123117Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 6 quoted lines
> @@ -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 ...
Show 27 quoted lines
> 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?