refs: fix invalid old object IDs when migrating reflogs

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs.c b/refs.c
index f88928de746f078f34c0f41055e00824ab2927e5..946eb48941b59c1c433b30b4f648eecc349f7afa 100644 (file)
--- 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 253dd8f4d5d7c4cea7221e546599009596f19e78..090b4fdff4fbcb7ba29a8f9891ebb35e8c9ddb8a 100644 (file)
--- 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 85ab2ef2b94cac89b0d9832724e8acd16fe5ab2b..905555365b8ff0611843e52ea5def4ae2618d6c7 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3010,6 +3010,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
@@ -3027,7 +3041,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 44af58ac50b0b97fd6a3af4aaa48d952cb3052e1..99fafd75ebe8ff001d30b6d38885c76bbea45219 100644 (file)
--- 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 dd7ffa5241810570f52f657407080757177bf4bf..46df64c1761b40d18366c81a0fabc010e1d40750 100755 (executable)
--- 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 2ab97e1b7dfa7e9a00004db08d0bcd511b890054..0e1116a319d53dd9f953da4e4efb4b3eee292d3b 100755 (executable)
--- 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' '