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

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

On 08/31/2013 02:19 PM, Michael Haggerty wrote:
> s/themeselves/themselves/

Fixed.

>> +	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.)

Fixed.

>> +	ret = strcmp(u1->ref_name, u2->ref_name);
> 
> Is there a need to compare more than ref_name?  If two entries are found
> with the same name, then ref_update_reject_duplicates() will error out

Junio mentioned possibility of auto-combining identical entries which would
need full ordering.  I think that can be added later so for now we can sort
only by ref name.  Thanks.

>> +		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.

Fixed.

>> +	/* 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.

Yes, thanks.

> 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.

I think the "goto cleanup" reorganization simplifies the code enough
to not need this.  After changing "updates" to an array of pointers
it needs to be separate so we can sort.  Also "delnames" needs to be
a separate array to pass to repack_without_refs.

>> +	/* 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.

Good idea.  Changed in next revision.

>> +			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)?

Yes.  Since we already have the lock at this point something must be
going pretty wrong if this fails so it is best to abort altogether.

>> +	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;".

Done, thanks.

>> +struct ref_update {
> 
> Please document this structure, especially the relationship between
> have_old and old_sha1.

Done.  I also moved it to the top of the header just under ref_lock
so it can be used by other APIs later.

Thanks,
-Brad