From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 01 Apr 2014 19:26:25 GMT
Subject: Re: [PATCH v2 24/27] ref_transaction_commit(): Introduce temporary variables
Message-ID: <xmqqy4zozw9q.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1395683820-17304-25-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Use temporary variables in the for-loop blocks to simplify expressions
> in the rest of the loop.

Shouldn't the summary of the change "simplify expressions"?  Use of
temporary variables is a means to the end.  If you have enough room
to say "achieve X by doing Y", please do so; otherwise "achieve X"
is more important part than "do Y".

Other than that, this looks good.

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 2b80f6d..d51566c 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3432,10 +3432,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  
>  	/* Acquire all locks while verifying old values */
>  	for (i = 0; i < n; i++) {
> -		locks[i] = update_ref_lock(updates[i]->refname,
> -					   (updates[i]->have_old ?
> -					    updates[i]->old_sha1 : NULL),
> -					   updates[i]->flags,
> +		struct ref_update *update = updates[i];
> +
> +		locks[i] = update_ref_lock(update->refname,
> +					   (update->have_old ?
> +					    update->old_sha1 : NULL),
> +					   update->flags,
>  					   &types[i], onerr);
>  		if (!locks[i]) {
>  			ret = 1;
> @@ -3444,16 +3446,19 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  	}
>  
>  	/* Perform updates first so live commits remain referenced */
> -	for (i = 0; i < n; i++)
> -		if (!is_null_sha1(updates[i]->new_sha1)) {
> +	for (i = 0; i < n; i++) {
> +		struct ref_update *update = updates[i];
> +
> +		if (!is_null_sha1(update->new_sha1)) {
>  			ret = update_ref_write(msg,
> -					       updates[i]->refname,
> -					       updates[i]->new_sha1,
> +					       update->refname,
> +					       update->new_sha1,
>  					       locks[i], onerr);
>  			locks[i] = NULL; /* freed by update_ref_write */
>  			if (ret)
>  				goto cleanup;
>  		}
> +	}
>  
>  	/* Perform deletes now that updates are safely completed */
>  	for (i = 0; i < n; i++)