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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 1, 2013, 06:08 UTC
Message-ID: <xmqqob8dul99.fsf@gitster.dls.corp.google.com>
In-Reply-To: <edaddbd4e303866f789f1a4f755a9da77590aeef.1377885441.git.brad.king@kitware.com>

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

Show 7 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 14 quoted lines

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

Show 7 quoted lines

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

Show 8 quoted lines

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