Re: [PATCH 12/12] refs.c: fix handling of badly named refs

> On Tue, Jul 22, 2014 at 1:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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?
>>
>
> I don't think we need to do that.
> That would imply that we would need to be able to also allow reading
> the content of a badly named ref.
> Currently a badly named ref can not be accessed by any function except
>  git branch -D <badlynamedref> which contains the special flag that
> allows locking it eventhough the ref has an illegal name.
>
> So no one else should be able to read or modify the ref at all.
>
> I think it is sufficient for this case to just have the semantics
> "just delete it, I don't care what it used to point to." for this
> special case  git branch -D <badrefname>
> so therefore since it is not the content of the ref but the name of
> the ref itself we have a problem with I don't think we need to read
> the old value or verify it.