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

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.

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.

[...]

[...]

+		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 <redacted>

Thanks.