Re: [PATCH 00/10] Implement transactions for the packed ref store
- Date: Tue, 29 Aug 2017 11:30:08 -0700
- From: Brandon Williams <bmwill@xxxxxxxxxx>
- Subject: Re: [PATCH 00/10] Implement transactions for the packed ref store
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
> 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
> 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 . 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 :)