Re: [PATCH v2 0/8] Multiple simultaneously locked ref updates

On 08/30/2013 08:11 PM, Brad King wrote:

I'm very interested in this area and I regret that I have been so invisible lately. I definitely like the way your changes are going.

I have been doing some work in the same neighborhood and our work overlaps somewhat. Namely, it is incorrect that delete_ref() currently deletes the loose ref before rewriting packed-refs, because it can cause a simultaneous reader to see the packed value for a moment in time, and the packed value might not even point to a valid object anymore. In fact, the current version is even worse: it deletes the loose reference before even locking the packed-refs file, so if the packed refs file cannot be rewritten (e.g., because it is locked by another process) then the reference is permanently left in a corrupt state.

On the other hand, writing the packed-refs file first would also be incorrect, because a pack-refs process could jump in before the loose refs file was deleted, read the loose value, and write it to the new packed-refs file. The result would be that the first process would think that it had deleted the reference but it would still exist (not such a catastrophe, but incorrect nevertheless).

The solution that I have been working on is to first lock *both* the loose and packed refs files, then rewrite the packed-refs file *but retain a lock on it*, then rewrite the loose-ref file and release its lock, then release the lock on the packed-refs file. By retaining the lock on the packed-refs file during the whole "transaction", we prevent another process from trying to pack the refs before our reference is completely deleted. This requires the file-locking API to be enhanced to allow a file to be replaced by its new version while still retaining a lock on the file.

Your code has the same bug as the original (it's not your fault!) so I think it will eventually have to be fixed to look something like

    acquire lock on packed-refs

    for reference in ref_updates:
        lock reference
        if old sha1 known:
            verify old sha1 is still current

    for reference in ref_updates:
        if reference should be created/modified:
            modify reference
            release lock on reference

    delete references from packed-refs file and activate new
            version of the file *but retain a lock on the
            packed-refs file*
    for reference in ref_updates:
        if reference should be deleted:
            delete loose version of reference
            release lock on reference

    release lock on packed-refs file

This is really all just for your information; there is certainly no obligation for you to fix this pre-existing problem. And I'm working on it anyway; if you happen to be interested you can view my current work-in-progress on GitHub (though it still doesn't work!):

    https://github.com/mhagger/git/tree/WIP-delete-ref-locking

Feedback would of course be welcome.

Michael