Re: [PATCH v20 30/48] refs.c: change update_ref to use a transaction

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Change the update_ref helper function to use a ref transaction internally.
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  refs.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 8c695ba..4bdccc5 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3524,11 +3524,31 @@ int update_ref(const char *action, const char *refname,
>  	       const unsigned char *sha1, const unsigned char *oldval,
>  	       int flags, enum action_on_err onerr)
>  {
> -	struct ref_lock *lock;
> -	lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
> -	if (!lock)
> +	struct ref_transaction *t;
> +	struct strbuf err = STRBUF_INIT;
> +
> +	t = ref_transaction_begin(&err);
> +	if (!t ||
> +	    ref_transaction_update(t, refname, sha1, oldval, flags,
> +				   !!oldval, &err) ||
> +	    ref_transaction_commit(t, action, &err)) {
> +		const char *str = "update_ref failed for ref '%s': %s";
> +
> +		ref_transaction_free(t);
> +		switch (onerr) {
> +		case UPDATE_REFS_MSG_ON_ERR:
> +			error(str, refname, err.buf);
> +			break;
> +		case UPDATE_REFS_DIE_ON_ERR:
> +			die(str, refname, err.buf);
> +			break;
> +		case UPDATE_REFS_QUIET_ON_ERR:
> +			break;
> +		}
> +		strbuf_release(&err);
>  		return 1;
> -	return update_ref_write(action, refname, sha1, lock, NULL, onerr);
> +	}
> +	return 0;
>  }
>  

Should this function be scheduled for the "take strbuf *err argument"
treatment instead of continuing to use an action_on_err parameter?
(Maybe you've changed this later in the patch series?)

I'm not saying this change has to be part of the current patch series,
but let's consider it for the future.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu