[PATCH v4 9/9] refs: fix invalid old object IDs when migrating reflogs
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Aug 4, 2025, 09:46 UTC
- Message-ID
- <20250804-pks-reflog-append-v4-9-13213fef7200@pks.im>
- In-Reply-To
- <20250804-pks-reflog-append-v4-0-13213fef7200@pks.im>
When migrating reflog entries between different storage formats we end up with invalid old object IDs for the migrated entries: instead of writing the old object ID of the to-be-migrated entry, we end up with the all-zeroes object ID.
The root cause of this issue is that we don't know to use the old object ID provided by the caller. Instead, we manually resolve the old object ID by resolving the current value of its matching reference. But as that reference does not yet exist in the target ref storage we always end up resolving it to all-zeroes.
This issue got unnoticed as there is no user-facing command that would even show the old object ID. While `git log -g` knows to show the new object ID, we don't have any formatting directive to show the old object ID.
Fix the bug by introducing a new flag `REF_LOG_USE_PROVIDED_OIDS`. If set, backends are instructed to use the old and new object IDs provided by the caller, without doing any manual resolving. Set this flag in `ref_transaction_update_reflog()`.
Amend our tests in t1460-refs-migrate to use our test tool to read reflog entries. This test tool prints out both old and new object ID of each reflog entry, which fixes the test gap. Furthermore it also prints the full identity used to write the reflog, which provides test coverage for the previous commit in this patch series that fixed the identity for migrated reflogs.
Signed-off-by: Patrick Steinhardt <ps@pks.im> --- refs.c | 3 ++- refs.h | 9 ++++++++- refs/files-backend.c | 16 +++++++++++++++- refs/reftable-backend.c | 14 ++++++++++++++ t/t1421-reflog-write.sh | 4 +--- t/t1460-refs-migrate.sh | 22 +++++++++++++++------- 6 files changed, 55 insertions(+), 13 deletions(-)
diff --git a/refs.c b/refs.c index f88928de74..946eb48941 100644 --- a/refs.c +++ b/refs.c @@ -1385,7 +1385,8 @@ int ref_transaction_update_reflog(struct ref_transaction *transaction, assert(err); - flags = REF_HAVE_OLD | REF_HAVE_NEW | REF_LOG_ONLY | REF_FORCE_CREATE_REFLOG | REF_NO_DEREF; + flags = REF_HAVE_OLD | REF_HAVE_NEW | REF_LOG_ONLY | REF_FORCE_CREATE_REFLOG | REF_NO_DEREF | + REF_LOG_USE_PROVIDED_OIDS; if (!transaction_refname_valid(refname, new_oid, flags, err)) return -1; diff --git a/refs.h b/refs.h index 253dd8f4d5..090b4fdff4 100644 --- a/refs.h +++ b/refs.h @@ -760,13 +760,20 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs, */ #define REF_SKIP_CREATE_REFLOG (1 << 12) +/* + * When writing a REF_LOG_ONLY record, use the old and new object IDs provided + * in the update instead of resolving the old object ID. The caller must also + * set both REF_HAVE_OLD and REF_HAVE_NEW. + */ +#define REF_LOG_USE_PROVIDED_OIDS (1 << 13) + /* * Bitmask of all of the flags that are allowed to be passed in to * ref_transaction_update() and friends: */ #define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \ (REF_NO_DEREF | REF_FORCE_CREATE_REFLOG | REF_SKIP_OID_VERIFICATION | \ - REF_SKIP_REFNAME_VERIFICATION | REF_SKIP_CREATE_REFLOG) + REF_SKIP_REFNAME_VERIFICATION | REF_SKIP_CREATE_REFLOG | REF_LOG_USE_PROVIDED_OIDS) /* * Add a reference update to transaction. `new_oid` is the value that diff --git a/refs/files-backend.c b/refs/files-backend.c index c37fbfd138..851b1b33f4 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3026,6 +3026,20 @@ static int parse_and_write_reflog(struct files_ref_store *refs, struct ref_lock *lock, struct strbuf *err) { + struct object_id *old_oid = &lock->old_oid; + + if (update->flags & REF_LOG_USE_PROVIDED_OIDS) { + if (!(update->flags & REF_HAVE_OLD) || + !(update->flags & REF_HAVE_NEW) || + !(update->flags & REF_LOG_ONLY)) { + strbuf_addf(err, _("trying to write reflog for '%s'" + "with incomplete values"), update->refname); + return REF_TRANSACTION_ERROR_GENERIC; + } + + old_oid = &update->old_oid; + } + if (update->new_target) { /* * We want to get the resolved OID for the target, to ensure @@ -3043,7 +3057,7 @@ static int parse_and_write_reflog(struct files_ref_store *refs, } } - if (files_log_ref_write(refs, lock->ref_name, &lock->old_oid, + if (files_log_ref_write(refs, lock->ref_name, old_oid, &update->new_oid, update->committer_info, update->msg, update->flags, err)) { char *old_msg = strbuf_detach(err, NULL); diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 44af58ac50..99fafd75eb 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1096,6 +1096,20 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor if (ret) return REF_TRANSACTION_ERROR_GENERIC; + if (u->flags & REF_LOG_USE_PROVIDED_OIDS) { + if (!(u->flags & REF_HAVE_OLD) || + !(u->flags & REF_HAVE_NEW) || + !(u->flags & REF_LOG_ONLY)) { + strbuf_addf(err, _("trying to write reflog for '%s'" + "with incomplete values"), u->refname); + return REF_TRANSACTION_ERROR_GENERIC; + } + + if (queue_transaction_update(refs, tx_data, u, &u->old_oid, err)) + return REF_TRANSACTION_ERROR_GENERIC; + return 0; + } + /* Verify that the new object ID is valid. */ if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) && !(u->flags & REF_SKIP_OID_VERIFICATION) && diff --git a/t/t1421-reflog-write.sh b/t/t1421-reflog-write.sh index dd7ffa5241..46df64c176 100755 --- a/t/t1421-reflog-write.sh +++ b/t/t1421-reflog-write.sh @@ -101,11 +101,9 @@ test_expect_success 'simple writes' ' EOF git reflog write refs/heads/something $COMMIT_OID $COMMIT_OID second && - # Note: the old object ID of the second reflog entry is broken. - # This will be fixed in subsequent commits. test_reflog_matches . refs/heads/something <<-EOF $ZERO_OID $COMMIT_OID $SIGNATURE first - $ZERO_OID $COMMIT_OID $SIGNATURE second + $COMMIT_OID $COMMIT_OID $SIGNATURE second EOF ) ' diff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh index 2ab97e1b7d..0e1116a319 100755 --- a/t/t1460-refs-migrate.sh +++ b/t/t1460-refs-migrate.sh @@ -7,6 +7,17 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +print_all_reflog_entries () { + repo=$1 && + test-tool -C "$repo" ref-store main for-each-reflog >reflogs && + while read reflog + do + echo "REFLOG: $reflog" && + test-tool -C "$repo" ref-store main for-each-reflog-ent "$reflog" || + return 1 + done <reflogs +} + # Migrate the provided repository from one format to the other and # verify that the references and logs are migrated over correctly. # Usage: test_migration <repo> <format> [<skip_reflog_verify> [<options...>]] @@ -28,8 +39,7 @@ test_migration () { --format='%(refname) %(objectname) %(symref)' >expect && if ! $skip_reflog_verify then - git -C "$repo" reflog --all >expect_logs && - git -C "$repo" reflog list >expect_log_list + print_all_reflog_entries "$repo" >expect_logs fi && git -C "$repo" refs migrate --ref-format="$format" "$@" && @@ -39,10 +49,8 @@ test_migration () { test_cmp expect actual && if ! $skip_reflog_verify then - git -C "$repo" reflog --all >actual_logs && - git -C "$repo" reflog list >actual_log_list && - test_cmp expect_logs actual_logs && - test_cmp expect_log_list actual_log_list + print_all_reflog_entries "$repo" >actual_logs && + test_cmp expect_logs actual_logs fi && git -C "$repo" rev-parse --show-ref-format >actual && @@ -273,7 +281,7 @@ test_expect_success 'multiple reftable blocks with multiple entries' ' test_commit -C repo second && printf "update refs/heads/ref-%d HEAD\n" $(test_seq 3000) >stdin && git -C repo update-ref --stdin <stdin && - test_migration repo reftable + test_migration repo reftable true ' test_expect_success 'migrating from files format deletes backend files' '
-- 2.50.1.723.g3e08bea96f.dirty