On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
>
> This makes the errno output of refs_read_raw_ref explicit.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
> Reviewed-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx>
> ---
> 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.
> /* 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.
> - 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".
> 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?
> 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.
> @@ -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.
> 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
...
