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

On Sat, May 17, 2014 at 6:33 AM, Michael Haggerty [off-list ref] wrote:

On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:

quoted

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.

Thanks. Fixed typo and deleted obsolete text.

quoted

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 <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
 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/