On 06/19/2013 09:18 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html