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

[Ronnie Sahlberg <sahlberg@google.com>]

On Tue, Jul 22, 2014 at 2:30 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
> 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.

It is also that prior to this change we could not access these badly
named refs at all.
This change tries to be careful to not open up too much as it tries to
only allow   git branch -D  and nothing else to start working for such
refs.
(To avoid accidentally opening things up so that it becomes possible
to start using/depending on such refs)