Re: [PATCH v2 04/12] refs: implement simple transactions for the packed-refs file

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/19/2013 09:18 PM, Junio C Hamano wrote:
> 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/