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

Stefan Beller [off-list ref] writes:

On Fri, Feb 13, 2015 at 10:26 AM, Junio C Hamano [off-list ref] wrote:

quoted

Stefan Beller [off-list ref] writes:

quoted

On Fri, Feb 13, 2015 at 10:05 AM, Junio C Hamano [off-list ref] wrote:

quoted

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')?

On 02/13/2015 08:12 PM, Junio C Hamano wrote:

[...]
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.

Let's first talk about an ideal world if we had complete support for
symbolic references.

Yes, I agree with your principle. Moreover, suppose that S and S2 *both*
point at R, and there is a third symref S3 that points at symref S
(symbolic refs can point at other symbolic refs!):

X <- R <- S <- S3
        \
         S2

Now, if R updated from X to Y (regardless of whether the update is via R
directly or via one of the symrefs), then each of the four reflogs (R,
S, S2, and S3) should gain a new entry reflecting the update.

If S is reseated to point at R2 instead of R, then the reflogs for S and
for S3 should each gain new entries

What locks should we hold? In my opinion, we should hold the locks on
exactly those references (symbolic or regular) whose reflogs we want to
change. So in the first example, we should hold

    $GIT_DIR/$R.lock
    $GIT_DIR/$S.lock
    $GIT_DIR/$S2.lock, and
    $GIT_DIR/$S3.lock

Ideally, we should acquire all of the locks before making any modifications.


Now back to the real world. Currently, if R is changed *through* a
symbolic reference S, then the reflogs for both R and S are updated, but
not the reflogs for any other symbolic references that might point at R.
If R is changed directly, then no symref's reflogs are affected, except
for the special case that HEAD's reflog is changed if it points directly
at R. This limitation is a hack to avoid having to walk symrefs
backwards to find any symrefs that might be pointing at R.

If a symref is reseated, then its reflog is changed but not that of any
symrefs that might be pointed at it.

It might actually not be extremely expensive to follow symrefs
backwards. Symbolic references cannot be packed, so we would only have
to scan the loose references; we could ignore packed refs. But it would
still be a lot more expensive than just updating one file. I don't know
that it's worth it, given that symbolic references are used so sparingly.

I think that the rule about locks as expressed above can be carried over
the the real world:

    We should hold the locks on exactly those references (symbolic
    or regular) whose reflogs we plan to change. We should acquire all
    of the locks before making any changes.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu