On 12/05/2014 03:59 AM, ronnie sahlberg wrote: > On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty <mhagger@xxxxxxxxxxxx> 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@xxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html