Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

> On 02/12/2015 12:25 AM, Stefan Beller wrote:
>> On Wed, Feb 11, 2015 at 2:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>>>> When processing the reflog of a symbolic ref, hold the lock on the
>>>>> symbolic reference itself, not on the reference that it points to.
>>>>
>>>> I am not sure if that makes sense.
>>>> So when expiring HEAD, you want to have a .git/HEAD.lock file
>>>> instead of a .git/refs/heads/master.lock file?
>>>>
>>>> What would happen if there is a concurrent modification
>>>> to the master branch?
>>>
>>> The HEAD may be pointing at 'master' and the other party that is
>>> trying to modify it would fail when it tries to update the reflog
>>> for HEAD thanks to HEAD.lock being held by us.  The HEAD may be
>>> pointing at 'next' and the other part that updates 'master' would
>>> not try to modify HEAD reflog and we do not conflict.
>>>
>>> At least, I think that is the rationale behind this change.
>>
>> That makes sense! Do we have documentation on symrefs?
>>
>> Originally I was wondering if this would make things
>> complicated for  symbolic branches which are not HEAD.
>> Then you could update the branch pointed to, because it
>> has no lock as the lock is on the symref. On the other hand
>> this seems to be an improvement, as you cannot move the
>> symref itself, as it has the lock and we don't really have other
>> symrefs?
>
> The convention is that holding lock $GIT_DIR/$refname.lock (where
> $refname might be, for example, "HEAD" or "refs/heads/master") protects
> two things:
>
> * The loose-reference file $GIT_DIR/$refname
> * The reflog file $GIT_DIR/logs/$refname
>
> And this makes sense:
>
> Suppose that HEAD is refs/heads/master. These two thing have independent
> reflogs, so there is no reason that one process can't be expiring the
> reflog of HEAD while another expires the reflog of refs/heads/master.
>
> The behavior before this patch was that the reflog for "HEAD" was
> modified while holding the reflog for "refs/heads/master". This is too
> strict and would make those two processes contend unnecessarily.
>
> I can't think of a reason that the current behavior is unsafe. But it's
> more restrictive than necessary, and more confusing than necessary. My
> guess is that it was unintended (i.e., a bug). It dates from
>
>     68db31cc28 (2007-05-09) git-update-ref: add --no-deref option for
> overwriting/detaching ref
>
> which initially added the REF_NODEREF constant and probably forgot that
> the new flag should be used in this invocation.
>
> However, another important question is whether other Git implementations
> have copied this unusual locking policy. If so, that would be a reason
> not to change it. I just pinged the libgit2 maintainer to find out their
> policy. Maybe you could find out about JGit?

>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
>