On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
>
> This is done in a separate commit, to pinpoint the precise cause should there be
> regressions in error reporting.
>
> This is implemented by renaming the existing logic to a static function
> refs_resolve_unsafe_implicit_errno(), minimizing the code diff.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
> Reviewed-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx>
> ---
> refs.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index ed2dde1c0c6d..191cbf5a330f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1687,10 +1687,10 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
> }
>
> /* This function needs to return a meaningful errno on failure */
> -const char *refs_resolve_ref_unsafe(struct ref_store *refs,
> - const char *refname,
> - int resolve_flags,
> - struct object_id *oid, int *flags)
> +static const char *
The "let's make this static" seems like an unrelated change we should
squash into the earlier "refs: use refs_resolve_ref_unsafe_with_errno()
where needed", we stopped using it in the files backend then. No?
> +refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs,
> + const char *refname, int resolve_flags,
> + struct object_id *oid, int *flags)
> {
> static struct strbuf sb_refname = STRBUF_INIT;
> struct object_id unused_oid;
> @@ -1779,14 +1779,24 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
> return NULL;
> }
>
> +const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,
> + int resolve_flags, struct object_id *oid,
> + int *flags)
> +{
> + const char *result = refs_resolve_ref_unsafe_implicit_errno(
> + refs, refname, resolve_flags, oid, flags);
> + errno = 0;
> + return result;
> +}
> +
> const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs,
> const char *refname,
> int resolve_flags,
> struct object_id *oid,
> int *flags, int *failure_errno)
> {
> - const char *result = refs_resolve_ref_unsafe(refs, refname,
> - resolve_flags, oid, flags);
> + const char *result = refs_resolve_ref_unsafe_implicit_errno(
> + refs, refname, resolve_flags, oid, flags);
> *failure_errno = errno;
> return result;
> }
Per my earlier comments this whole thing again seems a bit backwards. We
explicitly clear errno instead of telling the caller to care.
Has this whole thing perhaps had the unstated aim that you were trying
to distinguish the now-remaining refs_resolve_ref_unsafe() callers from
the "I care about errno" by explicitly clearing out errno for them, thus
ensuring that they need to use the wrapper function with "errno" in the
name to get the errno they'd otherwise get?
