Subject: Re: [PATCH 00/26] Clean up update-refs --stdin and implement ref_transaction

From: Brad King <[email-masked]>
Date: Mon, 10 Mar 2014 13:44:28 -0400

Hi Michael,

This is excellent work.

I haven't reviewed every line of logic in detail but the changes
look correct at a high level.  The only exception is that the empty
<newvalue> is supposed to be accepted and treated as zero even in
"--stdin -z" mode.  See my response to that individual change.

On 03/10/2014 08:46 AM, Michael Haggerty wrote:
> The new API for dealing with reference transactions is
> 
>     ref_transaction *transaction = create_ref_transaction();
>     queue_create_ref(transaction, refname, new_sha1, ...);
>     queue_update_ref(transaction, refname, new_sha1, old_sha1, ...);
>     queue_delete_ref(transaction, refname, old_sha1, ...);
>     ...
>     if (commit_ref_transaction(transaction, msg, ...))
>         die(...);

The layout of this API looks good.

The name "queue" is not fully representative of the current behavior.
It implies that the order is meaningful but we currently allow at most
one update to a ref and sort them by refname.  Does your follow-up work
define behavior for multiple updates to one ref?  Can it collapse them
into a single update after checking internal consistency of the sequence?

> So most of the commits in this series are actually cleanups in
> builtin/update-ref.c.  I also spend some time making the error
> messages emitted by that command more uniform.

All good cleanups, thanks.

> Finally, now that refs.c owns the data structures for dealing with
> transactions, it is possible to make a few simplifications.

Yes, it is much nicer to keep the data structures private, especially
as it avoids the copy of the transaction made before sorting.

Thanks,
-Brad