diff mbox series

[4/8] refs: make errno output explicit for refs_resolve_ref_unsafe

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

Commit Message

Han-Wen Nienhuys April 29, 2021, 3:32 p.m. UTC
From: Han-Wen Nienhuys <hanwen@google.com>

This introduces refs_resolve_ref_unsafe_with_errno(), which makes the API
contract for the errno output explicit. The implementation still relies on
the global errno variable to ensure no side effects.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs.c               | 12 ++++++++++++
 refs.h               |  1 +
 refs/files-backend.c | 19 ++++++++++---------
 refs/refs-internal.h |  8 ++++++++
 4 files changed, 31 insertions(+), 9 deletions(-)

Comments

Jonathan Tan June 3, 2021, 2:51 a.m. UTC | #1
> From: Han-Wen Nienhuys <hanwen@google.com>
> 
> This introduces refs_resolve_ref_unsafe_with_errno(), which makes the API
> contract for the errno output explicit. The implementation still relies on
> the global errno variable to ensure no side effects.

Looking at the next few patches, I think the plan is:

 - Introduce a new function refs_resolve_ref_unsafe_with_errno() which
   returns the error information in an out param instead of errno. Right
   now, it wraps refs_resolve_ref_unsafe().
 - Migrate all callers that require the error information to
   refs_resolve_ref_unsafe_with_errno(), and leave all callers that do
   not require the error information alone (that is, using
   refs_resolve_ref_unsafe()).
 - To ensure that all callers using refs_resolve_ref_unsafe() really do
   not use errno, set it to 0. (This is allowed by the errno contract -
   successes can set errno to whatever they want.)

If this is the plan, it should be written in the commit message.

As it is, this patch handles the first and maybe part of the second
step, and patch 5 handles the rest of the second step? I think the
patches should be more clearly divided.
Han-Wen Nienhuys June 10, 2021, 11:27 a.m. UTC | #2
On Thu, Jun 3, 2021 at 4:51 AM Jonathan Tan <jonathantanmy@google.com> wrote:
> > This introduces refs_resolve_ref_unsafe_with_errno(), which makes the API
> > contract for the errno output explicit. The implementation still relies on
> > the global errno variable to ensure no side effects.
>
> Looking at the next few patches, I think the plan is:
>
>  - Introduce a new function refs_resolve_ref_unsafe_with_errno() which
>    returns the error information in an out param instead of errno. Right
>    now, it wraps refs_resolve_ref_unsafe().
>  - Migrate all callers that require the error information to
>    refs_resolve_ref_unsafe_with_errno(), and leave all callers that do
>    not require the error information alone (that is, using
>    refs_resolve_ref_unsafe()).
>  - To ensure that all callers using refs_resolve_ref_unsafe() really do
>    not use errno, set it to 0. (This is allowed by the errno contract -
>    successes can set errno to whatever they want.)
>
> If this is the plan, it should be written in the commit message.

I added more description to the commit message.

> As it is, this patch handles the first and maybe part of the second
> step, and patch 5 handles the rest of the second step? I think the
> patches should be more clearly divided.

Done.
diff mbox series

Patch

diff --git a/refs.c b/refs.c
index 43e2ad6b612a..e8ce88040b3e 100644
--- a/refs.c
+++ b/refs.c
@@ -1780,6 +1780,18 @@  const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 	return NULL;
 }
 
+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);
+	*failure_errno = errno;
+	return result;
+}
+
 /* backend functions */
 int refs_init_db(struct strbuf *err)
 {
diff --git a/refs.h b/refs.h
index 48970dfc7e0f..ede405ac3874 100644
--- a/refs.h
+++ b/refs.h
@@ -68,6 +68,7 @@  const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 				    int resolve_flags,
 				    struct object_id *oid,
 				    int *flags);
+
 const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 			       struct object_id *oid, int *flags);
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index efe493ca1425..3ab09871db5e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -924,6 +924,7 @@  static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 	int mustexist = (old_oid && !is_null_oid(old_oid));
 	int resolve_flags = RESOLVE_REF_NO_RECURSE;
 	int resolved;
+	int resolve_errno;
 
 	files_assert_main_repository(refs, "lock_ref_oid_basic");
 	assert(err);
@@ -936,10 +937,11 @@  static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
 
 	files_ref_path(refs, &ref_file, refname);
-	resolved = !!refs_resolve_ref_unsafe(&refs->base,
-					     refname, resolve_flags,
-					     &lock->old_oid, type);
-	if (!resolved && errno == EISDIR) {
+	resolved = !!refs_resolve_ref_unsafe_with_errno(&refs->base, refname,
+							resolve_flags,
+							&lock->old_oid, type,
+							&resolve_errno);
+	if (!resolved && resolve_errno == EISDIR) {
 		/*
 		 * we are trying to lock foo but we used to
 		 * have foo/bar which now does not exist;
@@ -959,12 +961,11 @@  static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 						     &lock->old_oid, type);
 	}
 	if (!resolved) {
-		int last_errno = errno;
-		if (last_errno != ENOTDIR ||
-		    !refs_verify_refname_available(&refs->base, refname,
-						   extras, skip, err))
+		if (resolve_errno != ENOTDIR ||
+		    !refs_verify_refname_available(&refs->base, refname, extras,
+						   skip, err))
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
-				    refname, strerror(last_errno));
+				    refname, strerror(resolve_errno));
 
 		goto error_return;
 	}
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index ac8a14086724..b9f713b5acd6 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -153,6 +153,14 @@  int refs_read_raw_ref(struct ref_store *ref_store,
 		      const char *refname, struct object_id *oid,
 		      struct strbuf *referent, unsigned int *type);
 
+/* Like refs_resolve_ref_unsafe, but provide access to errno code that lead to a
+ * failure. */
+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);
+
 /*
  * Write an error to `err` and return a nonzero value iff the same
  * refname appears multiple times in `refnames`. `refnames` must be