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
