diff mbox series

[v2,7/8] refs: clear errno return in refs_resolve_ref_unsafe()

Message ID d86516219689e9179f3ba18c2ad2391d8ca27076.1623329869.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series refs: cleanup errno sideband ref related functions | expand

Commit Message

Han-Wen Nienhuys June 10, 2021, 12:57 p.m. UTC
From: Han-Wen Nienhuys <hanwen@google.com>

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@google.com>
Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
---
 refs.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Comments

Ævar Arnfjörð Bjarmason July 1, 2021, 12:19 p.m. UTC | #1
On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> 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@google.com>
> Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  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?
diff mbox series

Patch

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 *
+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;
 }