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@xxxxxx>
---
refs.c | 3 ++-
refs.h | 9 ++++++++-
refs/files-backend.c | 16 +++++++++++++++-
refs/reftable-backend.c | 14 ++++++++++++++
t/t1460-refs-migrate.sh | 22 +++++++++++++++-------
5 files changed, 54 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index c78d5be6e20..3d07ead92cb 100644
--- a/refs.c
+++ b/refs.c
@@ -1376,7 +1376,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 0faf3bc0422..8e5416178ae 100644
--- a/refs.h
+++ b/refs.h
@@ -759,13 +759,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 d519bb615fa..3ebe0323d4e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2976,6 +2976,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
@@ -2993,7 +3007,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 44af58ac50b..99fafd75ebe 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/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh
index 2ab97e1b7df..8191b08a79c 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 &&
+ cat reflogs | while read reflog
+ do
+ echo "REFLOG: $reflog" &&
+ test-tool -C "$repo" ref-store main for-each-reflog-ent "$reflog" ||
+ return 1
+ done
+}
+
# 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.465.gcb3da1c9e6.dirty
