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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 1, 2021, 11:58 UTC
Message-ID: <87bl7mdysf.fsf@evledraar.gmail.com>
In-Reply-To: <005ee8e6fb2a0f1a38801bed1dfe082999bfec14.1623329869.git.gitgitgadget@gmail.com>

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

Show 56 quoted lines

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> lock_ref_oid_basic() in files-backend.c is the only caller of refs_resolve_ref()
> that needs error information to make logic decisions.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  refs/files-backend.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 8f969c8f711f..5a430aabf623 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 = 0;
>  
>  	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;
>  	}

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