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>

Patrick Steinhardt committed Aug 6, 2025 at 07:54 UTC 046c67325c66f9e29e3bbe249ee742477b7525ef
4 files changed +13 -19
refs.c
+3 -5
@@ -1393,11 +1393,6 @@ int ref_transaction_update_reflog(struct ref_transaction *transaction,
1393 update = ref_transaction_add_update(transaction, refname, flags,
1394 new_oid, old_oid, NULL, NULL,
1395 committer_info, msg);
1396 - /*
1397 - * While we do set the old_oid value, we unset the flag to skip
1398 - * old_oid verification which only makes sense for refs.
1399 - */
1400 - update->flags &= ~REF_HAVE_OLD;
1396 update->index = index;
1397
1398 /*
@@ -3318,6 +3313,9 @@ done:
3313
3314 int ref_update_expects_existing_old_ref(struct ref_update *update)
3315 {
3316 + if (update->flags & REF_LOG_ONLY)
3317 + return 0;
3318 +
3319 return (update->flags & REF_HAVE_OLD) &&
3320 (!is_null_oid(&update->old_oid) || update->old_target);
3321 }
refs/files-backend.c
+5 -4
@@ -2500,7 +2500,6 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,
2500 * done when new_update is processed.
2501 */
2502 update->flags |= REF_LOG_ONLY | REF_NO_DEREF;
2503 - update->flags &= ~REF_HAVE_OLD;
2503
2504 return 0;
2505 }
@@ -2515,8 +2514,9 @@ static enum ref_transaction_error check_old_oid(struct ref_update *update,
2514 struct object_id *oid,
2515 struct strbuf *err)
2516 {
2518 - if (!(update->flags & REF_HAVE_OLD) ||
2519 - oideq(oid, &update->old_oid))
2517 + if (update->flags & REF_LOG_ONLY ||
2518 + !(update->flags & REF_HAVE_OLD) ||
2519 + oideq(oid, &update->old_oid))
2520 return 0;
2521
2522 if (is_null_oid(&update->old_oid)) {
@@ -3095,7 +3095,8 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
3095 for (i = 0; i < transaction->nr; i++) {
3096 struct ref_update *update = transaction->updates[i];
3097
3098 - if ((update->flags & REF_HAVE_OLD) &&
3098 + if (!(update->flags & REF_LOG_ONLY) &&
3099 + (update->flags & REF_HAVE_OLD) &&
3100 !is_null_oid(&update->old_oid))
3101 BUG("initial ref transaction with old_sha1 set");
3102
refs/refs-internal.h
+2 -1
@@ -802,7 +802,8 @@ enum ref_transaction_error ref_update_check_old_target(const char *referent,
802
803 /*
804 * Check if the ref must exist, this means that the old_oid or
805 - * old_target is non NULL.
805 + * old_target is non NULL. Log-only updates never require the old state to
806 + * match.
807 */
808 int ref_update_expects_existing_old_ref(struct ref_update *update);
809
refs/reftable-backend.c
+3 -9
@@ -1180,8 +1180,6 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
1180 if (ret > 0) {
1181 /* The reference does not exist, but we expected it to. */
1182 strbuf_addf(err, _("cannot lock ref '%s': "
1183 -
1184 -
1183 "unable to resolve reference '%s'"),
1184 ref_update_original_update_refname(u), u->refname);
1185 return REF_TRANSACTION_ERROR_NONEXISTENT_REF;
@@ -1235,13 +1233,8 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
1233
1234 new_update->parent_update = u;
1235
1238 - /*
1239 - * Change the symbolic ref update to log only. Also, it
1240 - * doesn't need to check its old OID value, as that will be
1241 - * done when new_update is processed.
1242 - */
1236 + /* Change the symbolic ref update to log only. */
1237 u->flags |= REF_LOG_ONLY | REF_NO_DEREF;
1244 - u->flags &= ~REF_HAVE_OLD;
1238 }
1239 }
1240
@@ -1265,7 +1258,8 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
1258 ret = ref_update_check_old_target(referent->buf, u, err);
1259 if (ret)
1260 return ret;
1268 - } else if ((u->flags & REF_HAVE_OLD) && !oideq(&current_oid, &u->old_oid)) {
1261 + } else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD &&
1262 + !oideq(&current_oid, &u->old_oid)) {
1263 if (is_null_oid(&u->old_oid)) {
1264 strbuf_addf(err, _("cannot lock ref '%s': "
1265 "reference already exists"),