lock_ref_for_update(): don't resolve symrefs

If a transaction includes a non-NODEREF update to a symbolic reference,
we don't have to look it up in lock_ref_for_update(). The reference will
be dereferenced anyway when the split-off update is processed.

This change requires that we store a backpointer from the split-off
update to its parent update, for two reasons:

* We still want to report the original reference name in error messages.
So if an error occurs when checking the split-off update's old_sha1,
walk the parent_update pointers back to find the original reference
name, and report that one.

* We still need to write the old_sha1 of the symref to its reflog. So
after we read the split-off update's reference value, walk the
parent_update pointers back and fill in their old_sha1 fields.

Aside from eliminating unnecessary reads, this change fixes a
subtle (though not very serious) race condition: in the old code, the
old_sha1 of the symref was resolved before the reference that it pointed
at was locked. So it was possible that the old_sha1 value logged to the
symref's reflog could be wrong if another process changed the downstream
reference before it was locked.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7bc18322aaae00aada50a2842de09ed4e8efecc0..a783513435a5b0f900d4e165cc1fb637151c9b65 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3376,14 +3376,32 @@ static int split_symref_update(struct ref_update *update,
                        update->new_sha1, update->old_sha1,
                        update->msg);
 
-       /* Change the symbolic ref update to log only: */
+       new_update->parent_update = update;
+
+       /*
+        * Change the symbolic ref update to log only. Also, it
+        * doesn't need to check its old SHA-1 value, as that will be
+        * done when new_update is processed.
+        */
        update->flags |= REF_LOG_ONLY | REF_NODEREF;
+       update->flags &= ~REF_HAVE_OLD;
 
        item->util = new_update;
 
        return 0;
 }
 
+/*
+ * Return the refname under which update was originally requested.
+ */
+static const char *original_update_refname(struct ref_update *update)
+{
+       while (update->parent_update)
+               update = update->parent_update;
+
+       return update->refname;
+}
+
 /*
  * Prepare for carrying out update:
  * - Lock the reference referred to by update.
@@ -3437,44 +3455,74 @@ static int lock_ref_for_update(struct ref_update *update,
        lock = update->lock;
 
        if (update->type & REF_ISSYMREF) {
-               if (read_ref_full(update->refname,
-                                 mustexist ? RESOLVE_REF_READING : 0,
-                                 lock->old_oid.hash, NULL)) {
-                       if (update->flags & REF_HAVE_OLD) {
-                               strbuf_addf(err, "cannot lock ref '%s': can't resolve old value",
-                                           update->refname);
+               if (update->flags & REF_NODEREF) {
+                       /*
+                        * We won't be reading the referent as part of
+                        * the transaction, so we have to read it here
+                        * to record and possibly check old_sha1:
+                        */
+                       if (read_ref_full(update->refname,
+                                         mustexist ? RESOLVE_REF_READING : 0,
+                                         lock->old_oid.hash, NULL)) {
+                               if (update->flags & REF_HAVE_OLD) {
+                                       strbuf_addf(err, "cannot lock ref '%s': "
+                                                   "can't resolve old value",
+                                                   update->refname);
+                                       return TRANSACTION_GENERIC_ERROR;
+                               } else {
+                                       hashclr(lock->old_oid.hash);
+                               }
+                       }
+                       if ((update->flags & REF_HAVE_OLD) &&
+                           hashcmp(lock->old_oid.hash, update->old_sha1)) {
+                               strbuf_addf(err, "cannot lock ref '%s': "
+                                           "is at %s but expected %s",
+                                           update->refname,
+                                           sha1_to_hex(lock->old_oid.hash),
+                                           sha1_to_hex(update->old_sha1));
                                return TRANSACTION_GENERIC_ERROR;
-                       } else {
-                               hashclr(lock->old_oid.hash);
                        }
-               }
-               if ((update->flags & REF_HAVE_OLD) &&
-                   hashcmp(lock->old_oid.hash, update->old_sha1)) {
-                       strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
-                                   update->refname,
-                                   sha1_to_hex(lock->old_oid.hash),
-                                   sha1_to_hex(update->old_sha1));
-                       return TRANSACTION_GENERIC_ERROR;
-               }
 
-               if (!(update->flags & REF_NODEREF)) {
+               } else {
+                       /*
+                        * Create a new update for the reference this
+                        * symref is pointing at. Also, we will record
+                        * and verify old_sha1 for this update as part
+                        * of processing the split-off update, so we
+                        * don't have to do it here.
+                        */
                        ret = split_symref_update(update, referent.buf, transaction,
                                                  affected_refnames, err);
                        if (ret)
                                return ret;
                }
-       } else if ((update->flags & REF_HAVE_OLD) &&
-                  hashcmp(lock->old_oid.hash, update->old_sha1)) {
-               if (is_null_sha1(update->old_sha1))
-                       strbuf_addf(err, "cannot lock ref '%s': reference already exists",
-                                   update->refname);
-               else
-                       strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
-                                   update->refname,
-                                   sha1_to_hex(lock->old_oid.hash),
-                                   sha1_to_hex(update->old_sha1));
+       } else {
+               struct ref_update *parent_update;
+
+               /*
+                * If this update is happening indirectly because of a
+                * symref update, record the old SHA-1 in the parent
+                * update:
+                */
+               for (parent_update = update->parent_update;
+                    parent_update;
+                    parent_update = parent_update->parent_update) {
+                       oidcpy(&parent_update->lock->old_oid, &lock->old_oid);
+               }
 
-               return TRANSACTION_GENERIC_ERROR;
+               if ((update->flags & REF_HAVE_OLD) &&
+                   hashcmp(lock->old_oid.hash, update->old_sha1)) {
+                       if (is_null_sha1(update->old_sha1))
+                               strbuf_addf(err, "cannot lock ref '%s': reference already exists",
+                                           original_update_refname(update));
+                       else
+                               strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
+                                           original_update_refname(update),
+                                           sha1_to_hex(lock->old_oid.hash),
+                                           sha1_to_hex(update->old_sha1));
+
+                       return TRANSACTION_GENERIC_ERROR;
+               }
        }
 
        if ((update->flags & REF_HAVE_NEW) &&

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index cccd76b28cd73dd7d0863153a05704953f901a4c..1bb3d87dc5bdb4f741938a99b850c5a99ee15e2f 100644 (file)
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -143,24 +143,41 @@ int should_autocreate_reflog(const char *refname);
  * not exist before update.
  */
 struct ref_update {
+
        /*
         * If (flags & REF_HAVE_NEW), set the reference to this value:
         */
        unsigned char new_sha1[20];
+
        /*
         * If (flags & REF_HAVE_OLD), check that the reference
         * previously had this value:
         */
        unsigned char old_sha1[20];
+
        /*
         * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
         * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and
         * REF_UPDATE_VIA_HEAD:
         */
        unsigned int flags;
+
        struct ref_lock *lock;
        unsigned int type;
        char *msg;
+
+       /*
+        * If this ref_update was split off of a symref update via
+        * split_symref_update(), then this member points at that
+        * update. This is used for two purposes:
+        * 1. When reporting errors, we report the refname under which
+        *    the update was originally requested.
+        * 2. When we read the old value of this reference, we
+        *    propagate it back to its parent update for recording in
+        *    the latter's reflog.
+        */
+       struct ref_update *parent_update;
+
        const char refname[FLEX_ARRAY];
 };