Re: [PATCH v2 5/8] refs: use refs_resolve_ref_unsafe_with_errno() where needed

On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote:

So, having read 4/8 and this I wonder why
refs_resolve_ref_unsafe_with_errno() is needed at all. It's just a
wrapper that sets errno to a variable you give it, but we could just
document that tha caller should check errno.

So far I haven't seen anything that suggests the below diff-on-top
wouldn't be OK (and all tests pass with it). It steps on the toes of
some of my earlier suggestions, but I'm doing these one-at-a-time.

In any case the comment I adjusted seems like something you should
adjust to. It looks like a TODO for having the sort of function you've
just implemented in refs_resolve_ref_unsafe_with_errno().

	diff --git a/refs.c b/refs.c
	index 64e2d55adcf..a07d852fcdc 100644
	--- a/refs.c
	+++ b/refs.c
	@@ -1688,7 +1688,10 @@ int refs_read_raw_ref(struct ref_store *ref_store,
	 	return result;
	 }
	 
	-/* This function needs to return a meaningful errno on failure */
	+/*
	+ * This function clears errno at the beginning. If it fails the errno
	+ * will be meaningful.
	+ */
	 const char *refs_resolve_ref_unsafe(struct ref_store *refs,
	 				    const char *refname,
	 				    int resolve_flags,
	@@ -1698,6 +1701,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
	 	struct object_id unused_oid;
	 	int unused_flags;
	 	int symref_count;
	+	errno = 0;
	 
	 	if (!oid)
	 		oid = &unused_oid;
	@@ -1781,18 +1785,6 @@ 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/files-backend.c b/refs/files-backend.c
	index 5a430aabf62..5a400e55cbf 100644
	--- a/refs/files-backend.c
	+++ b/refs/files-backend.c
	@@ -924,7 +924,6 @@ 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 = 0;
	 
	 	files_assert_main_repository(refs, "lock_ref_oid_basic");
	 	assert(err);
	@@ -937,11 +936,10 @@ 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_with_errno(&refs->base, refname,
	-							resolve_flags,
	-							&lock->old_oid, type,
	-							&resolve_errno);
	-	if (!resolved && resolve_errno == EISDIR) {
	+	resolved = !!refs_resolve_ref_unsafe(&refs->base, refname,
	+					     resolve_flags,
	+					     &lock->old_oid, type);
	+	if (!resolved && errno == EISDIR) {
	 		/*
	 		 * we are trying to lock foo but we used to
	 		 * have foo/bar which now does not exist;
	@@ -961,11 +959,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
	 						     &lock->old_oid, type);
	 	}
	 	if (!resolved) {
	-		if (resolve_errno != ENOTDIR ||
	+		if (errno != ENOTDIR ||
	 		    !refs_verify_refname_available(&refs->base, refname, extras,
	 						   skip, err))
	 			strbuf_addf(err, "unable to resolve reference '%s': %s",
	-				    refname, strerror(resolve_errno));
	+				    refname, strerror(errno));
	 
	 		goto error_return;
	 	}