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

[Junio C Hamano <gitster@pobox.com>]

Brad King <brad.king@kitware.com> writes:

> Add 'struct ref_update' to encode the information needed to update or
> delete a ref (name, new sha1, optional old sha1, no-deref flag).  Add
> function 'update_refs' accepting an array of updates to perform.  First
> sort the input array to order locks consistently everywhere and reject
> multiple updates to the same ref.  Then acquire locks on all refs with
> verified old values.  Then update or delete all refs accordingly.  Fail
> if any one lock cannot be obtained or any one old value does not match.

OK.  The code releases the locks it acquired so far when it fails,
which is good.

> Though the refs themeselves cannot be modified together in a single

"themselves".

> atomic transaction, this function does enable some useful semantics.
> For example, a caller may create a new branch starting from the head of
> another branch and rewind the original branch at the same time.  This
> transfers ownership of commits between branches without risk of losing
> commits added to the original branch by a concurrent process, or risk of
> a concurrent process creating the new branch first.

> +static int ref_update_compare(const void *r1, const void *r2)
> +{
> +	struct ref_update *u1 = (struct ref_update *)(r1);
> +	struct ref_update *u2 = (struct ref_update *)(r2);
> +	int ret;

Let's have a blank line between the end of decls and the beginning
of the body here.

> +	ret = strcmp(u1->ref_name, u2->ref_name);
> +	if (ret)
> +		return ret;
> +	ret = hashcmp(u1->new_sha1, u2->new_sha1);
> +	if (ret)
> +		return ret;
> +	ret = hashcmp(u1->old_sha1, u2->old_sha1);
> +	if (ret)
> +		return ret;
> +	ret = u1->flags - u2->flags;
> +	if (ret)
> +		return ret;
> +	return u1->have_old - u2->have_old;
> +}

I notice that we are using an array of structures and letting qsort
swap 50~64 bytes of data, instead of sorting an array of pointers,
each element of which points at a structure.  This may not matter
unless we are asked to update thousands at once, so I think it is OK
for now.

> +static int ref_update_reject_duplicates(struct ref_update *updates, int n,
> +					enum action_on_err onerr)
> +{
> +	int i;
> +	for (i = 1; i < n; ++i)
> +		if (!strcmp(updates[i - 1].ref_name, updates[i].ref_name))
> +			break;

Optionally we could silently dedup multiple identical updates and
not fail it in ref-update-reject-duplicates.  But that does not have
to be done until we find people's script would benefit from such a
nicety.

By the way, unless there is a strong reason not to do so,
post-increment "i++" (and pre-decrement "--i", if you use it) is the
norm around here.  Especially in places like the third part of a
for(;;) loop where people are used to see "i++", breaking the idiom
makes readers wonder if there is something else going on.

> +	/* Perform updates first so live commits remain referenced: */
> +	for (i = 0; i < n; ++i)
> +		if (!is_null_sha1(updates[i].new_sha1)) {
> +			ret |= update_ref_write(action,
> +						updates[i].ref_name,
> +						updates[i].new_sha1,
> +						locks[i], onerr);
> +			locks[i] = 0; /* freed by update_ref_write */

I think what is assigned here is a NULL pointer.

Will locally tweak while queuing.  Thanks.