Re: [PATCH v10 18/44] branch.c: use ref transaction for all ref updates

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

On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
> Change create_branch to use a ref transaction when creating the new branch.
> ref_transaction_create will check that the ref does not already exist and fail
> otherwise meaning that we no longer need to keep a lock on the ref during the
> setup_tracking. This simplifies the code since we can now do the transaction
> in one single step.
> 
> If the forcing flag is false then use ref_transaction_create since this will
> fail if the ref already exist. Otherwise use ref_transaction_update.

s/exist/exists/

And the references to ref_transaction_create() in the commit message are
obsolete.

> 
> This also fixes a race condition in the old code where two concurrent
> create_branch could race since the lock_any_ref_for_update/write_ref_sha1
> did not protect against the ref already existing. I.e. one thread could end up
> overwriting a branch even if the forcing flag is false.
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  branch.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/branch.c b/branch.c
> index 660097b..8bf7588 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -226,7 +226,6 @@ void create_branch(const char *head,
>  		   int force, int reflog, int clobber_head,
>  		   int quiet, enum branch_track track)
>  {
> -	struct ref_lock *lock = NULL;
>  	struct commit *commit;
>  	unsigned char sha1[20];
>  	char *real_ref, msg[PATH_MAX + 20];
> @@ -285,15 +284,6 @@ void create_branch(const char *head,
>  		die(_("Not a valid branch point: '%s'."), start_name);
>  	hashcpy(sha1, commit->object.sha1);
>  
> -	if (!dont_change_ref) {
> -		lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
> -		if (!lock)
> -			die_errno(_("Failed to lock ref for update"));
> -	}
> -
> -	if (reflog)
> -		log_all_ref_updates = 1;
> -
>  	if (forcing)
>  		snprintf(msg, sizeof msg, "branch: Reset to %s",
>  			 start_name);
> @@ -301,13 +291,24 @@ void create_branch(const char *head,
>  		snprintf(msg, sizeof msg, "branch: Created from %s",
>  			 start_name);
>  
> +	if (reflog)
> +		log_all_ref_updates = 1;
> +
> +	if (!dont_change_ref) {
> +		struct ref_transaction *transaction;
> +		struct strbuf err = STRBUF_INIT;
> +
> +		transaction = ref_transaction_begin();
> +		if (!transaction ||
> +		    ref_transaction_update(transaction, ref.buf, sha1,
> +					   null_sha1, 0, !forcing, &err) ||
> +		    ref_transaction_commit(transaction, msg, &err))
> +			die("%s", err.buf);
> +	}
> +
>  	if (real_ref && track)
>  		setup_tracking(ref.buf + 11, real_ref, track, quiet);
>  
> -	if (!dont_change_ref)
> -		if (write_ref_sha1(lock, sha1, msg) < 0)
> -			die_errno(_("Failed to write ref"));
> -
>  	strbuf_release(&ref);
>  	free(real_ref);
>  }
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/