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

Reply via email to