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

From: Brad King <brad.king@kitware.com>
Date: Mar 10, 2014, 17:44 UTC
Message-ID: <531DF9FC.4070707@kitware.com>
In-Reply-To: <1394455603-2968-1-git-send-email-mhagger@alum.mit.edu>

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:

Show 9 quoted lines

> 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