Re: [PATCH 06/12] refs.c: move the check for valid refname to lock_ref_sha1_basic

[Junio C Hamano <gitster@pobox.com>]

Ronnie Sahlberg <sahlberg@google.com> writes:

> Move the check for check_refname_format from lock_any_ref_for_update
> to lock_ref_sha1_basic. At some later stage we will get rid of
> lock_any_ref_for_update completely.
>
> If lock_ref_sha1_basic fails the check_refname_format test, set errno to
> EINVAL before returning NULL. This to guarantee that we will not return an
> error without updating errno.
>
> This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.
> But this wrapper is also called from an external caller and we will soon
> make changes to the signature to lock_ref_sha1_basic that we do not want to
> expose to that caller.

That might be sensible if our only goal were to remove
lock-any-ref-for-updates, but I wonder what the impact of this
change to other existing callers of lock-ref-sha1-basic.  I may be
recalling things incorrectly, but I suspect that it was deliberate
to keep the lowest-level internal helper function (i.e. _basic()) to
be lenient so that those who do not want the format checks can
choose to pass refnames that are not exactly kosher.

> If we need such recovery code we could add it as an option to git fsck and have
> git fsck be the only sanctioned way of bypassing the normal API and checks.

But fsck is about checking and never about recovering, isn't it?
Does it offer to remove misnamed refs?  Should it?



> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  refs.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 0df6894..f29f18a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2088,6 +2088,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  	int missing = 0;
>  	int attempts_remaining = 3;
>  
> +	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> +		errno = EINVAL;
> +		return NULL;
> +	}
> +
>  	lock = xcalloc(1, sizeof(struct ref_lock));
>  	lock->lock_fd = -1;
>  
> @@ -2179,8 +2184,6 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,
>  					 const unsigned char *old_sha1,
>  					 int flags, int *type_p)
>  {
> -	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
> -		return NULL;
>  	return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
>  }