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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> On Fri, Feb 13, 2015 at 10:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> On Fri, Feb 13, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>>> We convinced ourselves that not locking the symref is wrong, but
>>>> have we actually convinced us that not locking the underlying ref,
>>>> as long as we have a lock on the symref, is safe?
>>>>
>>>> To protect you, the holder of a lock on refs/remotes/origin/HEAD
>>>> that happens to point at refs/remotes/origin/next, from somebody who
>>>> is updating the underlying refs/remotes/origin/next directly without
>>>> going through the symbolic ref (e.g. receive-pack), wouldn't the
>>>> other party need to find any and all symbolic refs that point at the
>>>> underlying ref and take locks on them?
>>>
>>> As we're just modifying the ref log of HEAD in this case, we don't bother
>>> with where the HEAD points to. The other party may change
>>> refs/remotes/origin/next without us noticing, but we don't care here as
>>> all we do is rewriting the ref log for HEAD.
>>>
>>> If the other party were to modify HEAD (point it to some other branch, or
>>> forward the branch pointed to), they'd be expected to lock HEAD and
>>> would fail as we have the lock?
>>
>> I was not talking about HEAD in what you are responding to, though.
>> Don't we maintain a reflog on refs/remotes/origin/HEAD?  Is it OK to
>> allow its entries to become inconsistent with the underlying ref?
>
> Yes we do have a HEAD ref log, and that ref log would differ from
> the underlying ref log. I don't know if that is desired, but I
> would tend to not like it.

HEAD (or refs/remotes/origin/HEAD) reflog and reflog for
refs/heads/master (or refs/remotes/origin/next) would have to be
different as long as we allow symbolic refs to be repointed to
different refs.  If HEAD refers to 'next' today, and at the tip of
next sits commit X, the reflog for both of them would record the
fact that they were pointing at X.  If you repoint HEAD to point at
'master' (e.g. "git checkout master") whose tip is at commit Y, then
reflog for HEAD would record the fact that now HEAD points at Y, and
reflogs for 'master' or 'next' would not change merely because you
switched where HEAD points at.

And there is anything to like or not to like about it.

As we are trying to see a way to move forward to do the right thing
around reflog, I was wondering if locking only the symbolic ref is a
sensible endgame.  "The right thing" being:

   When a symbolic ref S points at underlying ref R, and if R's tip
   changes from X to Y, we would want to know from the reflog of S
   that S used to point at X and then changed to point at Y.

Replace S and R with either (HEAD, refs/heads/master) or
(refs/remotes/origin/HEAD, refs/remotes/origin/next) in the above
and we want both to be true.

How best to achieve that, and what is the set of right ref(s) to
take lock(s) on?  I am not very much interested in how incorrect
today's code might behave.  That is not the central point when
discussing what is the best way forward.

> So the other party updating the underlying ref would also need to lock
> HEAD then,

Yes, that is what I meant.  Your "also" can be read in two different
ways ("other party, too" or "HEAD, too"), though, and I think we
want both ;-).  That is why I hinted that it was iffy to state that
we only have to take the lock only on S and not on R, but only as a
workaround to keep older implementation out we take both---once they
get extinct we can get away with by taking a lock only on S.

When pushing to update 'master' and 'next' into a repository whose
'HEAD' points at 'master', we would want to take locks on 'next' (no
question), but is it sensible to take the lock on 'HEAD' and
deliberately leave 'master' unlocked?  Or is it more sensible to
take all locks on the underlying refs involved (i.e. 'next' and
'master') and in addition any symbolic refs that are pointing at
these refs involved (i.e. 'HEAD')?