From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from fout-a4-smtp.messagingengine.com (fout-a4-smtp.messagingengine.com [103.168.172.147]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5277A28A1DD for ; Fri, 25 Jul 2025 06:58:56 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=103.168.172.147 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1753426738; cv=none; b=QcZ1mNzxbwKS0JBi2rlEOgqxDeQHNMYGoeB4V2FzhEYfTuYuolRrFx8NPiPfwIA/xSAP9Cqkxs4hBUgfpXOJRoVbCqubT9VjbKLF/5tOoPCcRDPS4rjTuxvVzlgg3zq1SBBxwq1agd5KZwTsAz7XyhfXn/1E/wy1rv5VfZMArZk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1753426738; c=relaxed/simple; bh=FhP6pAWsvGYiKDDEK0NmKXMhlX6ESVQnz/vhqWdNJOs=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=lKqhYFEUgZ2b7RAqb6cIG0SkfPiJOpbwZ4iP+FNytV7rMLX80ywDAukMrQj3tANyiRkjGMda8geWv3jn031DYZEO8Xd2rpR1Yp493rQTktKdPpkBasoCiuiea5gtaDRSDUYLI8SFeKgGHv+l0EXxnlsQwJtqX8A5sqGp+s5itow= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im; spf=pass smtp.mailfrom=pks.im; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b=nx/m5jcr; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=GEx1suyf; arc=none smtp.client-ip=103.168.172.147 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=pks.im Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b="nx/m5jcr"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="GEx1suyf" Received: from phl-compute-08.internal (phl-compute-08.phl.internal [10.202.2.48]) by mailfout.phl.internal (Postfix) with ESMTP id 9111DEC0531; Fri, 25 Jul 2025 02:58:55 -0400 (EDT) Received: from phl-mailfrontend-01 ([10.202.2.162]) by phl-compute-08.internal (MEProxy); Fri, 25 Jul 2025 02:58:55 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-transfer-encoding:content-type:content-type:date:date :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to; s=fm2; t=1753426735; x=1753513135; bh=ujQX8lnG6Tum41etK0Q+03eTNR/Jtq/Uc3+CaaHyHCY=; b= nx/m5jcrVCxtN9feUIuKfa6hCgClwE7z7m7/IPOfeFIP6Wt8PDhvdDG+Xby9hJlP XYBeE8CKse86iyy19zId04fOefZ0R1H8kkzl+9JXNH6j2Q3EWWFabtP5SXJYuWCA 7wbPAZKgi07BieuuFo8/gnCSBq/sE78fkJLp0CVs4xwH9xr3DDayQqPxToWEqKuP cajY3VNbGVRg1Z5SWV3APiU4jKbJFKf4c3oCPy6A/gvU9M5uhxVnSvLVC/OmLCYh VduUa3fPz299RozcnmoY4UqaKIju6+ZRyGhBmV5nyWpvDyFbtRLsbA6g98r6X3/w +zgGKCnuFIL6yeERalmycw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm2; t=1753426735; x= 1753513135; bh=ujQX8lnG6Tum41etK0Q+03eTNR/Jtq/Uc3+CaaHyHCY=; b=G Ex1suyf3H5rTBlErDE4kqU+62wPPa9KJqRqzz0LkY1VXWSNl2qMbESUrKFhf+sj5 0zMqTRpwZaEJSVW/mwra5xTNvCc7xg/1Zu+2okEXEAbIdLY1RLjSt6nRbAK8D3Uq QPiF5F6VZaXZ+cjfN5Mw7TqdLhxd9a094igGZAZNrlBpfZ//wssTLZ6NthqAYp4q F1EPVthVLHrIuxc6M0rFw7WX8SuVbwWLeeQVwbYS9MYfi/N5csrycLyeSt8WNcvs RatDILYOk28b54umcEb23RrKlKSjKM6BR5ZISlUrAm7ROTwF/7QBwdvDh5XmR8SZ V37unZQZ2EoSbglJiDocw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeffedrtdefgdekvdekjecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpuffrtefokffrpgfnqfghnecuuegr ihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenucfjug hrpefhfffugggtgffkfhgjvfevofesthejredtredtjeenucfhrhhomheprfgrthhrihgt khcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecuggftrfgrthhtvghrnh epffeuiedujedvkeehuedvkeefffeivdeuleetkeduheejteekgedvudfgtdfgieelnecu vehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkh hsrdhimhdpnhgspghrtghpthhtohepiedpmhhouggvpehsmhhtphhouhhtpdhrtghpthht ohepjhhlthhosghlvghrsehgmhgrihhlrdgtohhmpdhrtghpthhtohepghhithhsthgvrh esphhosghogidrtghomhdprhgtphhtthhopehtohhonhesihhothgtlhdrtghomhdprhgt phhtthhopehgihhtsehvghgvrhdrkhgvrhhnvghlrdhorhhgpdhrtghpthhtohepkhgrrh hthhhikhdrudekkeesghhmrghilhdrtghomhdprhgtphhtthhopehsiigvuggvrhdruggv vhesghhmrghilhdrtghomh X-ME-Proxy: Feedback-ID: i197146af:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri, 25 Jul 2025 02:58:54 -0400 (EDT) Received: by mail (OpenSMTPD) with ESMTPSA id 74418f0f (TLSv1.3:TLS_CHACHA20_POLY1305_SHA256:256:NO); Fri, 25 Jul 2025 06:58:53 +0000 (UTC) From: Patrick Steinhardt Date: Fri, 25 Jul 2025 08:58:30 +0200 Subject: [PATCH v2 8/8] refs: fix invalid old object IDs when migrating reflogs Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20250725-pks-reflog-append-v2-8-e4e7cbe3f578@pks.im> References: <20250725-pks-reflog-append-v2-0-e4e7cbe3f578@pks.im> In-Reply-To: <20250725-pks-reflog-append-v2-0-e4e7cbe3f578@pks.im> To: git@vger.kernel.org Cc: Karthik Nayak , Justin Tobler , Junio C Hamano , =?utf-8?q?SZEDER_G=C3=A1bor?= , Toon Claes X-Mailer: b4 0.14.2 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 --- 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 f88928de746..946eb48941b 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 253dd8f4d5d..090b4fdff4f 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 8b42fe18901..b891a326a13 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2974,6 +2974,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 @@ -2991,7 +3005,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/t1421-reflog-write.sh b/t/t1421-reflog-write.sh index 0cd2c4f4f31..a09b5cd44e9 100755 --- a/t/t1421-reflog-write.sh +++ b/t/t1421-reflog-write.sh @@ -89,11 +89,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 2ab97e1b7df..0e1116a319d 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 [ []] @@ -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