refs: fix invalid old object IDs when migrating reflogs

Git fork

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>

authored by

Patrick Steinhardt and committed by

Junio C Hamano 7 months ago 465eff81 046c6732

+55 -13

6 changed files

expand all

unified split

refs

files-backend.c

reftable-backend.c

refs.c

refs.h

t1421-reflog-write.sh

t1460-refs-migrate.sh

+2 -1

refs.c

··· 1385 1386 assert(err); 1387 1388 - flags = REF_HAVE_OLD | REF_HAVE_NEW | REF_LOG_ONLY | REF_FORCE_CREATE_REFLOG | REF_NO_DEREF; 1389 1390 if (!transaction_refname_valid(refname, new_oid, flags, err)) 1391 return -1;

+8 -1

refs.h

··· 761 #define REF_SKIP_CREATE_REFLOG (1 << 12) 762 763 /* 764 * Bitmask of all of the flags that are allowed to be passed in to 765 * ref_transaction_update() and friends: 766 */ 767 #define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \ 768 (REF_NO_DEREF | REF_FORCE_CREATE_REFLOG | REF_SKIP_OID_VERIFICATION | \ 769 - REF_SKIP_REFNAME_VERIFICATION | REF_SKIP_CREATE_REFLOG) 770 771 /* 772 * Add a reference update to transaction. `new_oid` is the value that

··· 761 #define REF_SKIP_CREATE_REFLOG (1 << 12) 762 763 /* 764 + * When writing a REF_LOG_ONLY record, use the old and new object IDs provided 765 + * in the update instead of resolving the old object ID. The caller must also 766 + * set both REF_HAVE_OLD and REF_HAVE_NEW. 767 + */ 768 + #define REF_LOG_USE_PROVIDED_OIDS (1 << 13) 769 + 770 + /* 771 * Bitmask of all of the flags that are allowed to be passed in to 772 * ref_transaction_update() and friends: 773 */ 774 #define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \ 775 (REF_NO_DEREF | REF_FORCE_CREATE_REFLOG | REF_SKIP_OID_VERIFICATION | \ 776 + REF_SKIP_REFNAME_VERIFICATION | REF_SKIP_CREATE_REFLOG | REF_LOG_USE_PROVIDED_OIDS) 777 778 /* 779 * Add a reference update to transaction. `new_oid` is the value that

+15 -1

refs/files-backend.c

··· 3010 struct ref_lock *lock, 3011 struct strbuf *err) 3012 { 3013 if (update->new_target) { 3014 /* 3015 * We want to get the resolved OID for the target, to ensure ··· 3027 } 3028 } 3029 3030 - if (files_log_ref_write(refs, lock->ref_name, &lock->old_oid, 3031 &update->new_oid, update->committer_info, 3032 update->msg, update->flags, err)) { 3033 char *old_msg = strbuf_detach(err, NULL);

··· 3010 struct ref_lock *lock, 3011 struct strbuf *err) 3012 { 3013 + struct object_id *old_oid = &lock->old_oid; 3014 + 3015 + if (update->flags & REF_LOG_USE_PROVIDED_OIDS) { 3016 + if (!(update->flags & REF_HAVE_OLD) || 3017 + !(update->flags & REF_HAVE_NEW) || 3018 + !(update->flags & REF_LOG_ONLY)) { 3019 + strbuf_addf(err, _("trying to write reflog for '%s'" 3020 + "with incomplete values"), update->refname); 3021 + return REF_TRANSACTION_ERROR_GENERIC; 3022 + } 3023 + 3024 + old_oid = &update->old_oid; 3025 + } 3026 + 3027 if (update->new_target) { 3028 /* 3029 * We want to get the resolved OID for the target, to ensure ··· 3041 } 3042 } 3043 3044 + if (files_log_ref_write(refs, lock->ref_name, old_oid, 3045 &update->new_oid, update->committer_info, 3046 update->msg, update->flags, err)) { 3047 char *old_msg = strbuf_detach(err, NULL);

+14

refs/reftable-backend.c

··· 1096 if (ret) 1097 return REF_TRANSACTION_ERROR_GENERIC; 1098 1099 /* Verify that the new object ID is valid. */ 1100 if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) && 1101 !(u->flags & REF_SKIP_OID_VERIFICATION) &&

··· 1096 if (ret) 1097 return REF_TRANSACTION_ERROR_GENERIC; 1098 1099 + if (u->flags & REF_LOG_USE_PROVIDED_OIDS) { 1100 + if (!(u->flags & REF_HAVE_OLD) || 1101 + !(u->flags & REF_HAVE_NEW) || 1102 + !(u->flags & REF_LOG_ONLY)) { 1103 + strbuf_addf(err, _("trying to write reflog for '%s'" 1104 + "with incomplete values"), u->refname); 1105 + return REF_TRANSACTION_ERROR_GENERIC; 1106 + } 1107 + 1108 + if (queue_transaction_update(refs, tx_data, u, &u->old_oid, err)) 1109 + return REF_TRANSACTION_ERROR_GENERIC; 1110 + return 0; 1111 + } 1112 + 1113 /* Verify that the new object ID is valid. */ 1114 if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) && 1115 !(u->flags & REF_SKIP_OID_VERIFICATION) &&

+1 -3

t/t1421-reflog-write.sh

··· 101 EOF 102 103 git reflog write refs/heads/something $COMMIT_OID $COMMIT_OID second && 104 - # Note: the old object ID of the second reflog entry is broken. 105 - # This will be fixed in subsequent commits. 106 test_reflog_matches . refs/heads/something <<-EOF 107 $ZERO_OID $COMMIT_OID $SIGNATURE first 108 - $ZERO_OID $COMMIT_OID $SIGNATURE second 109 EOF 110 ) 111 '

··· 101 EOF 102 103 git reflog write refs/heads/something $COMMIT_OID $COMMIT_OID second && 104 test_reflog_matches . refs/heads/something <<-EOF 105 $ZERO_OID $COMMIT_OID $SIGNATURE first 106 + $COMMIT_OID $COMMIT_OID $SIGNATURE second 107 EOF 108 ) 109 '

+15 -7

t/t1460-refs-migrate.sh

··· 7 8 . ./test-lib.sh 9 10 # Migrate the provided repository from one format to the other and 11 # verify that the references and logs are migrated over correctly. 12 # Usage: test_migration <repo> <format> [<skip_reflog_verify> [<options...>]] ··· 28 --format='%(refname) %(objectname) %(symref)' >expect && 29 if ! $skip_reflog_verify 30 then 31 - git -C "$repo" reflog --all >expect_logs && 32 - git -C "$repo" reflog list >expect_log_list 33 fi && 34 35 git -C "$repo" refs migrate --ref-format="$format" "$@" && ··· 39 test_cmp expect actual && 40 if ! $skip_reflog_verify 41 then 42 - git -C "$repo" reflog --all >actual_logs && 43 - git -C "$repo" reflog list >actual_log_list && 44 - test_cmp expect_logs actual_logs && 45 - test_cmp expect_log_list actual_log_list 46 fi && 47 48 git -C "$repo" rev-parse --show-ref-format >actual && ··· 273 test_commit -C repo second && 274 printf "update refs/heads/ref-%d HEAD\n" $(test_seq 3000) >stdin && 275 git -C repo update-ref --stdin <stdin && 276 - test_migration repo reftable 277 ' 278 279 test_expect_success 'migrating from files format deletes backend files' '

··· 7 8 . ./test-lib.sh 9 10 + print_all_reflog_entries () { 11 + repo=$1 && 12 + test-tool -C "$repo" ref-store main for-each-reflog >reflogs && 13 + while read reflog 14 + do 15 + echo "REFLOG: $reflog" && 16 + test-tool -C "$repo" ref-store main for-each-reflog-ent "$reflog" || 17 + return 1 18 + done <reflogs 19 + } 20 + 21 # Migrate the provided repository from one format to the other and 22 # verify that the references and logs are migrated over correctly. 23 # Usage: test_migration <repo> <format> [<skip_reflog_verify> [<options...>]] ··· 39 --format='%(refname) %(objectname) %(symref)' >expect && 40 if ! $skip_reflog_verify 41 then 42 + print_all_reflog_entries "$repo" >expect_logs 43 fi && 44 45 git -C "$repo" refs migrate --ref-format="$format" "$@" && ··· 49 test_cmp expect actual && 50 if ! $skip_reflog_verify 51 then 52 + print_all_reflog_entries "$repo" >actual_logs && 53 + test_cmp expect_logs actual_logs 54 fi && 55 56 git -C "$repo" rev-parse --show-ref-format >actual && ··· 281 test_commit -C repo second && 282 printf "update refs/heads/ref-%d HEAD\n" $(test_seq 3000) >stdin && 283 git -C repo update-ref --stdin <stdin && 284 + test_migration repo reftable true 285 ' 286 287 test_expect_success 'migrating from files format deletes backend files' '