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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Feb 13, 2015, 16:26 UTC
Message-ID: <54DE259C.4030001@alum.mit.edu>
In-Reply-To: <CAGZ79kZgjRNS3zd4Tif6M66mjkP6-tDpy4FAtio8jiwqHxUtgw@mail.gmail.com>

On 02/12/2015 07:04 PM, Stefan Beller wrote:

Show 12 quoted lines

> On Thu, Feb 12, 2015 at 8:52 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> 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?
> 
> I could not really find reflog expiration for jgit for a while, but
> then I stumbled upon this:
> 
>     // TODO: implement reflog_expire(pm, repo);
> 
> so I think it's safe to change it in git.git for now.

Stefan: This locking policy doesn't only affect "reflog expire"; we also
need to lock the reflog when we add a new entry to it, for example when
updating refs/heads/master through HEAD. Do you know what JGit locks in
that scenario?

I had a discussion in IRC [1] with Carlos Martín Nieto about how libgit2 handles this:

* When libgit2 updates a reference through a symref, it locks the
pointed-to reference while updating the reflog for the symbolic ref. So,
for example, "git commit" would hold refs/heads/master.lock while
updating both logs/refs/heads/master and logs/HEAD. (This matches the
current Git behavior.)
* libgit2 doesn't support "reflog expire", though it does have an API
that allows users to overwrite the reflog with a specified list of
entries. That API locks the reference itself; i.e., HEAD.lock. This
disagrees with Git's current behavior.

I also realized that Git's current policy is probably not tenable if one process is re-seating a symref at the same time as another process is expiring its reflog. The "git reflog expire HEAD" might grab "refs/heads/master.lock" then start rewriting "logs/HEAD". Meanwhile, "git checkout foo" would grab "HEAD.lock" (not being blocked by the "expire" process), then rewrite it to "ref: refs/heads/foo", then grab "refs/heads/foo.lock" before updating "logs/HEAD". So both processes could be writing "logs/HEAD" at the same time.

In fact, it's even worse. "git checkout foo" and "git symbolic-ref HEAD refs/heads/foo" release "HEAD.lock" *before* updating logs/HEAD--in other words, they append to logs/HEAD without holding *any* lock.

What is the best way forward?

I think the current locking policy is untenable, even aside from the bug in "symbolic-ref".

Switching to holding only "HEAD.lock" while updating "logs/HEAD" is the right solution, but it would introduce an incompatibility with old versions of Git and libgit2 (and maybe JGit?) Probably nobody would notice, but who knows?

Therefore, I propose that we hold *both* "HEAD.lock" *and* "refs/heads/master.lock" when modifying "logs/HEAD" (even when running "reflog expire" or "reflog delete"). I think this will be compatible with old versions of Git and libgit2, and it will also fix some design problems mentioned above. Plus, it will prevent updates to the two reflogs from appearing out of order relative to each other.

Someday, when old clients have been retired, we can optionally switch to locking *only* "HEAD.lock" when modifying "logs/HEAD".

What do people think? Michael

[1] https://github.com/libgit2/libgit2/issues/2902

-- 
Michael Haggerty
mhagger@alum.mit.edu