Re: [PATCH 00/10] Implement transactions for the packed ref store

[Brandon Williams <bmwill@google.com>]

On 08/29, Michael Haggerty wrote:
> This should be the second-to-last patch series in the quest for
> mmapped packed-refs.
> 
> Before this patch series, there is still more coupling than necessary
> between `files_ref_store` and `packed_ref_store`:
> 
> * `files_ref_store` adds packed references (e.g., during `git clone`
>   and `git pack-refs`) by inserting them into the `packed_ref_store`'s
>   internal `ref_cache`, then calling `commit_packed_refs()`. But
>   `files_ref_store` shouldn't even *know* that `packed_ref_store` uses
>   a `ref_cache`, let alone muck about in it.
> 
> * `files_ref_store` deletes packed references by calling
>   `repack_without_refs()`.
> 
> Instead, implement reference transactions and `delete_refs()` for
> `packed_ref_store`, and change `files_ref_store` to use these standard
> methods rather than internal `packed_ref_store` methods.
> 
> This patch series finishes up the encapsulation of `packed_ref_store`.
> At the end of the series, the outside world only interacts with it via
> the refs API plus a couple of locking-related functions. That will
> make it easy for the next patch series to change the internal workings
> of `packed_ref_store` without affecting `files_ref_store`.
> 
> Along the way, we fix some longstanding problems with reference
> updates. Quoting from patch [08/10]:
> 
>     First, the old code didn't obtain the `packed-refs` lock until
>     `files_transaction_finish()`. This means that a failure to acquire the
>     `packed-refs` lock (e.g., due to contention with another process)
>     wasn't detected until it was too late (problems like this are supposed
>     to be detected in the "prepare" phase). The new code acquires the
>     `packed-refs` lock in `files_transaction_prepare()`, the same stage of
>     the processing when the loose reference locks are being acquired,
>     removing another reason why the "prepare" phase might succeed and the
>     "finish" phase might nevertheless fail.
> 
>     Second, the old code deleted the loose version of a reference before
>     deleting any packed version of the same reference. This left a moment
>     when another process might think that the packed version of the
>     reference is current, which is incorrect. (Even worse, the packed
>     version of the reference can be arbitrarily old, and might even point
>     at an object that has since been garbage-collected.)
> 
>     Third, if a reference deletion fails to acquire the `packed-refs` lock
>     altogether, then the old code might leave the repository in the
>     incorrect state (possibly corrupt) described in the previous
>     paragraph.
> 
>     Now we activate the new "packed-refs" file (sans any references that
>     are being deleted) *before* deleting the corresponding loose
>     references. But we hold the "packed-refs" lock until after the loose
>     references have been finalized, thus preventing a simultaneous
>     "pack-refs" process from packing the loose version of the reference in
>     the time gap, which would otherwise defeat our attempt to delete it.
> 
> This patch series is also available as branch
> `packed-ref-transactions` in my fork on GitHub [1]. A draft of the
> patch series to change `packed_ref_store` to use mmap and get rid of
> its `ref_cache` is available as branch `mmap-packed-refs` from the
> same source.

Overall the patches look sane to me, though I don't believe I'm
qualified in this area to give you a complete thumbs up since I don't
understand the refs code super well yet.  I do like reading patch from
you as you do a great job of laying out what you are doing in code,
comments and commit messages, something I'm trying to get better at :)

-- 
Brandon Williams