Re: [PATCH v2 04/12] refs: implement simple transactions for the packed-refs file
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Jun 20, 2013, 07:49 UTC
- Message-ID
- <51C2B41F.2050708@alum.mit.edu>
- In-Reply-To
- <7vfvwdzz6k.fsf@alter.siamese.dyndns.org>
On 06/19/2013 09:18 PM, Junio C Hamano wrote:
Show 57 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> Handle simple transactions for the packed-refs file at the
>> packed_ref_cache level via new functions lock_packed_refs(),
>> commit_packed_refs(), and rollback_packed_refs().
>>
>> Only allow the packed ref cache to be modified (via add_packed_ref())
>> while the packed refs file is locked.
>>
>> Change clone to add the new references within a transaction.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>> builtin/clone.c | 7 ++++-
>> refs.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++-----------
>> refs.h | 27 +++++++++++++++++--
>> 3 files changed, 98 insertions(+), 19 deletions(-)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 66bff57..b0c000a 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -489,17 +489,22 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
>> return local_refs;
>> }
>>
>> +static struct lock_file packed_refs_lock;
>> +
>> static void write_remote_refs(const struct ref *local_refs)
>> {
>> const struct ref *r;
>>
>> + lock_packed_refs(&packed_refs_lock, LOCK_DIE_ON_ERROR);
>> +
>> for (r = local_refs; r; r = r->next) {
>> if (!r->peer_ref)
>> continue;
>> add_packed_ref(r->peer_ref->name, r->old_sha1);
>> }
>>
>> - pack_refs(PACK_REFS_ALL);
>> + if (commit_packed_refs())
>> + die_errno("unable to overwrite old ref-pack file");
>> }
>
> The calling convention used here looks somewhat strange. You allow
> callers to specify which lock-file structure is used when locking,
> but when you are done, commit_packed_refs() does not take any
> parameter.
>
> lock_packed_refs() make the singleton in-core packed-ref-cache be
> aware of which lock it is under, so commit_packed_refs() does not
> need to be told (the singleton already knows what lockfile is in
> effect), so I am not saying the code is broken, though.
>
> Does the caller need to even have an access to this lock_file
> instance?No it doesn't. My reasoning was as follows:
lock_file instances can never be freed, because they are added to a linked list in the atexit handler but never removed. Therefore they have to be allocated statically (or allocated dynamically then leaked).
[I just noticed that lock_ref_sha1_basic() leaks a struct lock_file every time that it is called.]
I wanted to leave the way open to allow other packed refs caches to be locked. But since all packed refs caches are allocated dynamically, the lock_file instance cannot be part of struct packed_ref_cache. So I thought that the easiest approach is to rely on the caller, who presumably can know that only a finite number of locks are in use at once, to supply a usable lock_file instance.
But currently only the main packed ref cache can be locked, so it would be possible for lock_packed_refs() to use the static packlock instance for locking. I will change it to do so.
If/when we add support for locking other packed ref caches, we can change the API to use a caller-supplied lock_file. Or even better, implement a way to remove a lock_file instance from the atexit list; then lock_packed_refs() could use dynamically-allocated lock_file instances without having to leak them.
v3 to come.
Michael
-- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/