From: Jonathan Nieder <jrnieder@gmail.com>
Date: Wed, 28 May 2014 19:31:39 GMT
Subject: Re: [PATCH v11 23/41] refs.c: change update_ref to use a transaction
Message-ID: <20140528193139.GW12314@google.com>
In-Reply-To: <1401222360-21175-24-git-send-email-sahlberg@google.com>

Ronnie Sahlberg wrote:

> --- a/refs.c
> +++ b/refs.c
> @@ -3474,11 +3474,28 @@ 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)) ||

(style) Extra parens.

> +	    (ref_transaction_commit(t, action, &err) && !(t = NULL))) {

No need for this assignment-in-if.

With the following squashed in,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git i/refs.c w/refs.c
index 568b358..fb462a3 100644
--- i/refs.c
+++ w/refs.c
@@ -3474,10 +3474,10 @@ int update_ref(const char *action, const char *refname,
 	struct strbuf err = STRBUF_INIT;
 
 	t = ref_transaction_begin(&err);
-	if ((!t ||
+	if (!t ||
 	    ref_transaction_update(t, refname, sha1, oldval, flags,
-				   !!oldval, &err)) ||
-	    (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
+				   !!oldval, &err) ||
+	    ref_transaction_commit(t, action, &err)) {
 		const char *str = "update_ref failed for ref '%s': %s";
 
 		ref_transaction_free(t);