Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates
From: Michael Haggerty <hidden>
Date: 2016-06-15 23:00:50
On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
quoted hunk ↗ jump to hunk
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. 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 overwriting a branch even if the forcing flag is false. Signed-off-by: Ronnie Sahlberg <redacted> --- branch.c | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-)diff --git a/branch.c b/branch.c index 660097b..23cde1e 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,34 @@ 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; + char *err = NULL; + + transaction = ref_transaction_begin(); + if (forcing) { + if (!transaction || + ref_transaction_update(transaction, ref.buf, sha1, + NULL, 0, 0) || + ref_transaction_commit(transaction, msg, &err)) + die_errno(_("%s: failed to write ref: %s"), + ref.buf, err); + } else { + if (!transaction || + ref_transaction_create(transaction, ref.buf, sha1, + 0) || + ref_transaction_commit(transaction, msg, &err)) + die_errno(_("%s: failed to write ref: %s"), + ref.buf, err); + }
You've got some indentation problems above.
But actually, there seems like a lot of duplicated code here. Couldn't
you instead do a single block with have_old set based on forcing:
ref_transaction_update(transaction, ref.buf, sha1,
null_sha1, 0, !forcing)
?
+ }
+
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/