Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> 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.
> 
> This changes semantics for lock_ref_sha1_basic slightly. With this change
> it is no longer possible to open a ref that has a badly name which breaks

s/badly name/bad name,/

> any codepaths that tries to open and repair badly named refs. The normal refs

s/tries/try/

> API should not allow neither creating nor accessing refs with invalid names.

s/not allow neither/allow neither/

> 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.

I like the sentiment, but in the real world I'm not sure we can take
such a step based only on good intentions.  Which callers would be
affected?  Where is this "git fsck" code that would be needed to help
people rescue their repos?

I can also imagine that we will tighten up the check_refname_format
checks in the future; for example, I think it would be a good idea to
prohibit reference names that start with '-' because it is almost
impossible to work with them (their names look like command-line
options).  If we ever make a change like that, we will need some amount
of tolerance in git versions around the transition.

So...I like the idea of enforcing refname checks at the lowest level
possible, but I think that the change you propose is too abrupt.  I
think it needs either more careful analysis showing that it won't hurt
anybody, or some kind of tooling or non-strict mode that people can use
to fix their repositories.

Michael

> 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 389a55f..bccf8c3 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);
>  }
>  
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu