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

Brad King [off-list ref] 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.