Re: [PATCH/RFC 6/7] refs: add update_refs for multiple simultaneous updates

[Brad King <brad.king@kitware.com>]

On 08/29/2013 01:39 PM, Junio C Hamano wrote:
> Brad King <brad.king@kitware.com> writes:
>> +	for (i=0; i < n; ++i) {
> 
> Style:
> 
> 	for (i = 0; i < n; i++) {

Fixed.

> Is it asking for AB-BA deadlock?  If so, is the caller responsible
> for avoiding it?

Since we don't actually block waiting for locks we won't really
deadlock.  However, if two competing callers keep repeating they
might never finish.  In order to avoid this one must sort the refs
so that locks are always acquired in a consistent order.

For Git's internal API I think we can document this in a comment so
that update_refs does not have to sort.  Then we can add a new
ref_update_sort function to sort an array of struct ref_update.
The user-facing "update-ref --stdin" can then use ref_update_sort.

The sort logic may subsume duplicate ref update detection too.
After sorting a simple linear-time scan can detect duplicates.

>> +	unsigned char new_sha1[20];
>> +	unsigned char *old_sha1;
> 
> This asymmetry is rather curious and will later become problematic
> when we have more users of this API.  Maybe your callers happen have
> static storage to keep old_sha1[] outside this structure but do not
> have it for new_sha1[] for some unknown reason, which may not apply
> to later callers we may want to add.

I wasn't happy with the asymmetry either but forgot to raise it in
the cover letter.  We need a way to represent "old value not given"
as different from "old value is_null_sha1".

One approach is to use a bit in the "flags" member that already
stores REF_NODEREF.  However, that will be inconsistent with the
other ref update APIs that check whether old_sha1 is NULL.  We could
still do it and document the bit to mean "ignore old_sha1 member of
struct ref_update".

Another approach is to add a dedicated member to struct ref_update.

Comments?
-Brad