Brad King <brad.k...@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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
