refs: stop unsetting REF_HAVE_OLD for log-only updates

The `REF_HAVE_OLD` flag indicates whether a given ref update has its old
object ID set. If so, the value of that field is used to verify whether
the current state of the reference matches this expected state. It is
thus an important part of mitigating races with a concurrent process
that updates the same set of references.

When writing reflogs though we explicitly unset that flag. This is a
sensible thing to do: the old state of reflog entry updates may not
necessarily match the current on-disk state of its accompanying ref, but
it's only intended to signal what old object ID we want to write into
the new reflog entry. For example when migrating refs we end up writing
many reflog entries for a single reference, and most likely those reflog
entries will have many different old object IDs.

But unsetting this flag also removes a useful signal, namely that the
caller _did_ provide an old object ID for a given reflog entry. This
signal will become useful in a subsequent commit, where we add a new
flag that tells the transaction to use the provided old and new object
IDs to write a reflog entry. The `REF_HAVE_OLD` flag is then used as a
signal to verify that the caller really did provide an old object ID.

Stop unsetting the flag so that we can use it as this described signal
in a subsequent commit. Skip checking the old object ID for log-only
updates so that we don't expect it to match the current on-disk state.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs.c b/refs.c
index a5f9ffaa45d2289d91c7fe827e36f99fa740fbc5..f88928de746f078f34c0f41055e00824ab2927e5 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -1393,11 +1393,6 @@ int ref_transaction_update_reflog(struct ref_transaction *transaction,
        update = ref_transaction_add_update(transaction, refname, flags,
                                            new_oid, old_oid, NULL, NULL,
                                            committer_info, msg);
-       /*
-        * While we do set the old_oid value, we unset the flag to skip
-        * old_oid verification which only makes sense for refs.
-        */
-       update->flags &= ~REF_HAVE_OLD;
        update->index = index;
 
        /*
@@ -3318,6 +3313,9 @@ done:
 
 int ref_update_expects_existing_old_ref(struct ref_update *update)
 {
+       if (update->flags & REF_LOG_ONLY)
+               return 0;
+
        return (update->flags & REF_HAVE_OLD) &&
                (!is_null_oid(&update->old_oid) || update->old_target);
 }

diff --git a/refs/files-backend.c b/refs/files-backend.c
index ba018b0984a8a35daf972420d2c83209662ce0fd..85ab2ef2b94cac89b0d9832724e8acd16fe5ab2b 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2500,7 +2500,6 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,
         * done when new_update is processed.
         */
        update->flags |= REF_LOG_ONLY | REF_NO_DEREF;
-       update->flags &= ~REF_HAVE_OLD;
 
        return 0;
 }
@@ -2515,8 +2514,9 @@ static enum ref_transaction_error check_old_oid(struct ref_update *update,
                                                struct object_id *oid,
                                                struct strbuf *err)
 {
-       if (!(update->flags & REF_HAVE_OLD) ||
-                  oideq(oid, &update->old_oid))
+       if (update->flags & REF_LOG_ONLY ||
+           !(update->flags & REF_HAVE_OLD) ||
+           oideq(oid, &update->old_oid))
                return 0;
 
        if (is_null_oid(&update->old_oid)) {
@@ -3095,7 +3095,8 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
        for (i = 0; i < transaction->nr; i++) {
                struct ref_update *update = transaction->updates[i];
 
-               if ((update->flags & REF_HAVE_OLD) &&
+               if (!(update->flags & REF_LOG_ONLY) &&
+                   (update->flags & REF_HAVE_OLD) &&
                    !is_null_oid(&update->old_oid))
                        BUG("initial ref transaction with old_sha1 set");
 

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index f86887085191e8320900b11976a5dc80a75c560c..95a4dc3902f4ff25de91bae25c4008660144ae14 100644 (file)
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -802,7 +802,8 @@ enum ref_transaction_error ref_update_check_old_target(const char *referent,
 
 /*
  * Check if the ref must exist, this means that the old_oid or
- * old_target is non NULL.
+ * old_target is non NULL. Log-only updates never require the old state to
+ * match.
  */
 int ref_update_expects_existing_old_ref(struct ref_update *update);
 

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 4c3817f4ec1a887aa6e6feed2a1336ed2b29cbe2..44af58ac50b0b97fd6a3af4aaa48d952cb3052e1 100644 (file)
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1180,8 +1180,6 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
        if (ret > 0) {
                /* The reference does not exist, but we expected it to. */
                strbuf_addf(err, _("cannot lock ref '%s': "
-
-
                                   "unable to resolve reference '%s'"),
                            ref_update_original_update_refname(u), u->refname);
                return REF_TRANSACTION_ERROR_NONEXISTENT_REF;
@@ -1235,13 +1233,8 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
 
                        new_update->parent_update = u;
 
-                       /*
-                        * Change the symbolic ref update to log only. Also, it
-                        * doesn't need to check its old OID value, as that will be
-                        * done when new_update is processed.
-                        */
+                       /* Change the symbolic ref update to log only. */
                        u->flags |= REF_LOG_ONLY | REF_NO_DEREF;
-                       u->flags &= ~REF_HAVE_OLD;
                }
        }
 
@@ -1265,7 +1258,8 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
                ret = ref_update_check_old_target(referent->buf, u, err);
                if (ret)
                        return ret;
-       } else if ((u->flags & REF_HAVE_OLD) && !oideq(&current_oid, &u->old_oid)) {
+       } else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD &&
+                  !oideq(&current_oid, &u->old_oid)) {
                if (is_null_oid(&u->old_oid)) {
                        strbuf_addf(err, _("cannot lock ref '%s': "
                                           "reference already exists"),