Re: [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn

From: Junio C Hamano <gitster@pobox.com>
To: "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Han-Wen Nienhuys <hanwenn@gmail.com>,
	Han-Wen Nienhuys <hanwen@google.com>
Subject: Re: [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn
Date: Fri, 30 Apr 2021 15:02:24 +0900	[thread overview]
Message-ID: <xmqqeeescntb.fsf@gitster.g> (raw)
In-Reply-To: <xmqq35v8e982.fsf@gitster.g> (Junio C. Hamano's message of "Fri, 30 Apr 2021 12:34:37 +0900")

Junio C Hamano <gitster@pobox.com> writes:

> I like the general direction to move away from errno, which may make
> sense only in the context of files backend (e.g. ENOENT would be
> left in errno, only because the original "loose ref" implementation
> used one file per ref, when a ref we wanted to see did not exist)
> and having other backends use the same errno would not make much
> sense, as it is not the goal to make other backends like reftable
> emulate files backend.
>
> I wonder if in the ideal world, we'd rather want to define our own
> enum, not <errno.h>, that is specific to failure modes of ref API
> functions and signal failures by returning these values (and the
> enum includes 0 as a value to signal success, all other errors are
> negative values).
>
> What I am really getting at is if we need an extra "failure"
> out-parameter-pointer in the internal API.  I do not mind if it
> helps the transition to the ideal world, but I offhand do not see if
> we need result and failure as separate variables.

In any case, the change in the function signature helped me catch
that this series invalidates what has been queued on hn/reftable
without updating that topic to align with the new way to handle the
errno (it would have become a silent semantic failure if we instead
encoded the "failure" in the return value).

The following is what I am tentatively using for tonight's
integration when merging this and hn/reftable together to the 'seen'
branch.

Thanks.


 refs/reftable-backend.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git c/refs/reftable-backend.c w/refs/reftable-backend.c
index 1aff21adb4..4385d2d6f5 100644
--- c/refs/reftable-backend.c
+++ w/refs/reftable-backend.c
@@ -52,7 +52,8 @@ static struct reftable_stack *stack_for(struct git_reftable_ref_store *store,
 static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 				     const char *refname, struct object_id *oid,
 				     struct strbuf *referent,
-				     unsigned int *type);
+				     unsigned int *type,
+				     int *failure_errno);
 
 static void clear_reftable_log_record(struct reftable_log_record *log)
 {
@@ -376,7 +377,8 @@ static int fixup_symrefs(struct ref_store *ref_store,
 						&old_oid, &referent,
 						/* mutate input, like
 						   files-backend.c */
-						&update->type);
+						&update->type,
+						&errno);
 		if (err < 0 && errno == ENOENT &&
 		    is_null_oid(&update->old_oid)) {
 			err = 0;
@@ -1538,7 +1540,7 @@ static int reftable_error_to_errno(int err)
 static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 				     const char *refname, struct object_id *oid,
 				     struct strbuf *referent,
-				     unsigned int *type)
+				     unsigned int *type, int *failure_errno)
 {
 	struct git_reftable_ref_store *refs =
 		(struct git_reftable_ref_store *)ref_store;
@@ -1560,12 +1562,12 @@ static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 
 	err = reftable_stack_read_ref(stack, refname, &ref);
 	if (err > 0) {
-		errno = ENOENT;
+		*failure_errno = ENOENT;
 		err = -1;
 		goto done;
 	}
 	if (err < 0) {
-		errno = reftable_error_to_errno(err);
+		*failure_errno = reftable_error_to_errno(err);
 		err = -1;
 		goto done;
 	}
@@ -1578,7 +1580,7 @@ static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 		oidread(oid, reftable_ref_record_val1(&ref));
 	} else {
 		*type |= REF_ISBROKEN;
-		errno = EINVAL;
+		*failure_errno = EINVAL;
 		err = -1;
 	}
 done:
