diff mbox series

[7/8] refs: stop setting EINVAL and ELOOP in symref resolution

Message ID 95d64d73353d8689e3928b8c9444490d0cdebfc9.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>

The only caller of refs_resolve_ref_unsafe_with_errno() is in
refs/files-backend.c, and it only cares about EISDIR and ENOTDIR.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Jonathan Tan June 3, 2021, 2:55 a.m. UTC | #1
> From: Han-Wen Nienhuys <hanwen@google.com>
> 
> The only caller of refs_resolve_ref_unsafe_with_errno() is in
> refs/files-backend.c, and it only cares about EISDIR and ENOTDIR.
> 
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  refs.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 6e746cb01f24..597e4d1f18f9 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1706,7 +1706,6 @@ static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs,
>  	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
>  		if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
>  		    !refname_is_safe(refname)) {
> -			errno = EINVAL;
>  			return NULL;
>  		}
>  

I don't think this is related to avoiding errno and conveying error
information through an out param. But besides that, as it is, I'm not
sure that this is correct. Even if EINVAL is not checked by the caller,
setting errno to EINVAL here means avoiding exposing a potential
EISDIR/ENOTDIR that a preceding call set. Same comment for the other
instances.
Han-Wen Nienhuys June 10, 2021, 11:58 a.m. UTC | #2
On Thu, Jun 3, 2021 at 4:55 AM Jonathan Tan <jonathantanmy@google.com> wrote:
> I don't think this is related to avoiding errno and conveying error
> information through an out param. But besides that, as it is, I'm not
> sure that this is correct. Even if EINVAL is not checked by the caller,
> setting errno to EINVAL here means avoiding exposing a potential
> EISDIR/ENOTDIR that a preceding call set. Same comment for the other
> instances.

You are right, but it's probably moot, because the follow-up commit
stops using errno (making it impossible for EISDIR/ENOTDIR to
haphazardly appear), but I dropped this commit from the series.
diff mbox series

Patch

diff --git a/refs.c b/refs.c
index 6e746cb01f24..597e4d1f18f9 100644
--- a/refs.c
+++ b/refs.c
@@ -1706,7 +1706,6 @@  static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs,
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
 		if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
 		    !refname_is_safe(refname)) {
-			errno = EINVAL;
 			return NULL;
 		}
 
@@ -1766,7 +1765,6 @@  static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs,
 		if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
 			if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
 			    !refname_is_safe(refname)) {
-				errno = EINVAL;
 				return NULL;
 			}
 
@@ -1774,7 +1772,6 @@  static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs,
 		}
 	}
 
-	errno = ELOOP;
 	return NULL;
 }