From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 22 Jul 2014 20:41:05 GMT
Subject: Re: [PATCH 12/12] refs.c: fix handling of badly named refs
Message-ID: <xmqqlhrlf7oe.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1405549392-27306-13-git-send-email-sahlberg@google.com>

Ronnie Sahlberg <sahlberg@google.com> writes:

> We currently do not handle badly named refs well :
> $ cp .git/refs/heads/master .git/refs/heads/master.....@\*@\\.
> $ git branch
>    fatal: Reference has invalid format: 'refs/heads/master.....@*@\.'
> $ git branch -D master.....@\*@\\.
>   error: branch 'master.....@*@\.' not found.
>
> But we can not really recover from a badly named ref with less than
> manually deleting the .git/refs/heads/<refname> file.
>
> Change resolve_ref_unsafe to take a flags field instead of a 'reading'
> boolean and update all callers that used a non-zero value for reading
> to pass the flag RESOLVE_REF_READING instead.
> Add another flag RESOLVE_REF_ALLOW_BAD_NAME that will make
> resolve_ref_unsafe skip checking the refname for sanity and use this
> from branch.c so that we will be able to call resolve_ref_unsafe on such
> refs when trying to delete it.

Makes sense.

> Add checks for refname sanity when updating (not deleting) a ref in
> ref_transaction_update and in ref_transaction_create to make the transaction
> fail if an attempt is made to create/update a badly named ref.
> Since all ref changes will later go through the transaction layer this means
> we no longer need to check for and fail for bad refnames in
> lock_ref_sha1_basic.
>
> Change lock_ref_sha1_basic to not fail for bad refnames. Just check the
> refname, and print an error, and remember that the refname is bad so that
> we can skip calling verify_lock().

This is somewhat puzzling, though.

> @@ -2180,6 +2196,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  		else
>  			unable_to_lock_index_die(ref_file, errno);
>  	}
> +	if (bad_refname)
> +		return lock;

Hmph.  If the only offence was that the ref was named badly due to
historically loose code, does the caller not still benefit from the
verify-lock check to make sure that other people did not muck with
the ref while we were planning to update it?

>  	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
>  
>   error_return: