Re: [PATCH v2 6/8] refs: add update_refs for multiple simultaneous updates

>> +	struct ref_update *u1 = (struct ref_update *)(r1);
>> +	struct ref_update *u2 = (struct ref_update *)(r2);
> 
> If you declare u1 and u2 to be "const struct ref_update *" (i.e., add
> "const"), then you have const correctness and don't need the explicit
> casts.  (And the parentheses around r1 and r2 are superfluous in any case.)

>> +		if (!strcmp(updates[i - 1].ref_name, updates[i].ref_name))
>> +			break;
> 
> The error handling code could be right here instead of the "break"
> statement, removing the need for the "if" conditional.

>> +	/* Allocate work space: */
>> +	updates = xmalloc(sizeof(struct ref_update) * n);
> 
> It seems preferred here to write
> 
> 	updates = xmalloc(sizeof(*updates) * n);
> 
> as this will continue to work if the type of updates is ever changed.

> Similarly for the next lines.
> 
>> +	types = xmalloc(sizeof(int) * n);
>> +	locks = xmalloc(sizeof(struct ref_lock *) * n);
>> +	delnames = xmalloc(sizeof(const char *) * n);
> 
> An alternative to managing separate arrays to hold types and locks would
> be to include the scratch space in struct ref_update and document it
> "for internal use only; need not be initialized by caller".  On the one
> hand it's ugly to cruft up the "interface" with internal implementation
> details; on the other hand there is precedent for this sort of thing
> (e.g., ref_lock::force_write or lock_file::on_list) and it would
> simplify the code.

>> +	/* Copy, sort, and reject duplicate refs: */
>> +	memcpy(updates, updates_orig, sizeof(struct ref_update) * n);
>> +	qsort(updates, n, sizeof(struct ref_update), ref_update_compare);
> 
> You could save some space and memory shuffling (during memcpy() and
> qsort()) if you would declare "updates" to be an array of pointers to
> "struct ref_update" rather than an array of structs.  Sorting could then
> be done by moving pointers around instead of moving the structs.  This
> would also make it easier for update_refs() to pass information about
> the references back to its caller, should that ever be needed.

>> +			ret |= update_ref_write(action,
>> +						updates[i].ref_name,
>> +						updates[i].new_sha1,
>> +						locks[i], onerr);
>> +			locks[i] = 0; /* freed by update_ref_write */
>> +		}
>> +
> 
> Hmmm, if one of the calls to update_ref_write() fails, would it be safer
> to abort the rest of the work (especially the reference deletions)?

>> +	free(updates);
>> +	free(types);
>> +	free(locks);
>> +	free(delnames);
>> +	return ret;
>> +}
> 
> There's a lot of duplicated cleanup code in the function.  If you put a
> label before the final for loop, and if you initialize the locks array
> to zeros (e.g., by using xcalloc()), then the three exits could all
> share the same code "ret = 1; goto cleanup;".