Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file

[Michael Haggerty <mhagger@alum.mit.edu>]

On 12/05/2014 03:59 AM, ronnie sahlberg wrote:
> On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> We don't actually need the locking functionality, because we already
>> hold the lock on the reference itself,
> 
> No. You do need the lock.
> The ref is locked only during transaction_commit()
> 
> If you don't want to lock the reflog file and instead rely on the lock
> on the ref itself you will need to
> rework your patches so that the lock on the ref is taken already
> during, for example, transaction_update_ref() instead.
> 
> But without doing those changes and moving the ref locking from
> _commit() to _update_ref() you will risk reflog corruption/surprises
> if two operations collide and both rewrite the reflog without any lock held.

Ronnie, I don't understand your comments.

It is a statement of fact (to the best of my knowledge) that reflogs are
supposed to be modified only under a lock on the corresponding
reference, namely "$GIT_DIR/refs/$refname.lock". We do not require
reflog writers to hold "$GIT_DIR/logs/$refname.lock".

In this function, "$GIT_DIR/logs/$refname.lock" happens to be the name
of the temporary file being used to stage the new contents of the
reflog. But that is more or less a coincidence; we could call the
temporary file whatever we want because it has no locking implications.
However, what we want to do with the file in this code path (write a new
version then rename the new version on top of the old version, deleting
the temporary file if the program is interrupted) is the same as what we
do with lockfiles, so we use the lockfile code because it is convenient.

This patch series has nothing to do with ref_transaction_commit() or any
of the transaction machinery. It has to do with expire_reflog(), which
is invoked outside of any transaction.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu