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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 15, 2014, 22:58 UTC
Message-ID: <20140515225804.GQ26471@google.com>
In-Reply-To: <1400174999-26786-19-git-send-email-sahlberg@google.com>

Ronnie Sahlberg wrote:

Show 5 quoted lines

> 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.

Previously the ref lock was also held during setup_tracking, which sets up configuration for the branch in .git/config. So in principle they were one transaction and this patch would change that.

The race:

	checkout -B master origin/master
	--------------------------------
	                        update-ref -d refs/heads/master
	                        -------------------------------
	                                                checkout -B master other/master
	                                                -------------------------------
	create ref
	                        delete ref
	                                                create ref

	configure tracking                              configure tracking

Since setup_tracking is not a single transaction, if the two processes are lucky enough to not try to write to .git/config at the same time then the resulting configuration could have branch.master.merge set by the first checkout -b and branch.master.remote set by the second.

But trying to "checkout -b" the same branch in two terminals concurrently is a somewhat insane thing to do, so I don't mind breaking it.

[...]

> 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 existsing. I.e. one thread could end up

s/existsing/existing/

> overwriting a branch even if the forcing flag is false.

Good catch.

> --- a/branch.c
> +++ b/branch.c

[...]

> @@ -301,13 +291,25 @@ void create_branch(const char *head,

[...]

Show 6 quoted lines

> +		if (!transaction ||
> +			ref_transaction_update(transaction, ref.buf, sha1,
> +					       null_sha1, 0, !forcing) ||
> +			ref_transaction_commit(transaction, msg, &err))
> +				die_errno(_("%s: failed to write ref: %s"),
> +					  ref.buf, err.buf);

errno is not guaranteed valid here.  The usual

				die("%s", err.buf);

should do the trick.

With the changes mentioned above (commit message typofix, die()
message),
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.