{"thread":{"id":"61787","subject":"[PATCH] Revert \"reflog expire: don't use lookup_commit_reference_gently()\"","startedAt":"2024-07-16T19:56:28Z","lastAt":"2024-07-17T14:56:23Z","messageCount":3,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"498802","messageId":"xmqqv8156rh2.fsf@gitster.g","threadId":"61787","inReplyTo":null,"subject":"[PATCH] Revert \"reflog expire: don't use lookup_commit_reference_gently()\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-16T19:56:25Z","receivedAt":"2024-07-16T19:56:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"During Git 2.35 timeframe, daf1d828 (reflog expire: don't use\nlookup_commit_reference_gently(), 2021-12-22) replaced a call to\nlookup_commit_reference_gently() with a call to lookup_commit().\n\nWhat it failed to consider was that our refs do not necessarily\npoint at commits (most notably, we have annotated and signed tags),\nand more importantly that lookup_commit() does not dereference a tag\nto return a commit; instead it returns NULL when a tag is given.\n\nSince the commit returned is used as a starting point for the\nreachability check, this ejected the commits that are reachable only\nby an annotated tag out of the set of reachable commits, breaking\nthe computation to correctly implement the \"--expire-unreachable\"\noption. We also started giving an error message that the API\nfunction expected to be fed a commit object.\n\nThis problem hasn't been reported or noticed for a long time,\nprobably because the \"refs/tags/\" hierarchy by default is not\ncovered by reflogs, as nobody usually moves tags.\n\nRevert the change to correctly find the commit pointed at by the ref\nto restore the previous behaviour, but do so only in a more modern\ncodebase, as we had significant code churn since then and it is not\ngrave enough to worry about for older maintenance tracks.\n\nSigned-off-by: Junio C Hamano \n---\n cf. \n\n reflog.c | 3 ++-\n t/t1410-reflog.sh | 8 ++++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/reflog.c b/reflog.c\nindex 0a1bc35e8c..2e45a17e12 100644\n--- a/reflog.c\n+++ b/reflog.c\n@@ -330,7 +330,8 @@ void reflog_expiry_prepare(const char *refname,\n \tif (!cb->cmd.expire_unreachable || is_head(refname)) {\n \t\tcb->unreachable_expire_kind = UE_HEAD;\n \t} else {\n-\t\tcommit = lookup_commit(the_repository, oid);\n+\t\tcommit = lookup_commit_reference_gently(the_repository,\n+\t\t\t\t\t\t\toid, 1);\n \t\tif (commit && is_null_oid(&commit->object.oid))\n \t\t\tcommit = NULL;\n \t\tcb->unreachable_expire_kind = commit ? UE_NORMAL : UE_ALWAYS;\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex d2f5f42e67..16816e82b2 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -146,6 +146,14 @@ test_expect_success rewind '\n \ttest_line_count = 5 output\n '\n \n+test_expect_success 'reflog expire should not barf on an annotated tag' '\n+\ttest_when_finished \"git tag -d v0.tag || :\" &&\n+\tgit -c core.logAllRefUpdates=always \\\n+\t\ttag -a -m \"tag name\" v0.tag main &&\n+\tgit reflog expire --dry-run refs/tags/v0.tag 2>err &&\n+\ttest_grep ! \"error: [Oo]bject .* not a commit\" err\n+'\n+\n test_expect_success 'corrupt and check' '\n \n \tcorrupt $F &&\n-- \n2.46.0-rc0-153-ge537c69d48\n\n"},{"id":"498811","messageId":"20240717054555.GA547635@coredump.intra.peff.net","threadId":"61787","inReplyTo":"xmqqv8156rh2.fsf@gitster.g","subject":"Re: [PATCH] Revert \"reflog expire: don't use lookup_commit_reference_gently()\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-17T05:45:55Z","receivedAt":"2024-07-17T05:46:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 16, 2024 at 12:56:25PM -0700, Junio C Hamano wrote:\n\n> During Git 2.35 timeframe, daf1d828 (reflog expire: don't use\n> lookup_commit_reference_gently(), 2021-12-22) replaced a call to\n> lookup_commit_reference_gently() with a call to lookup_commit().\n> \n> What it failed to consider was that our refs do not necessarily\n> point at commits (most notably, we have annotated and signed tags),\n> and more importantly that lookup_commit() does not dereference a tag\n> to return a commit; instead it returns NULL when a tag is given.\n> \n> Since the commit returned is used as a starting point for the\n> reachability check, this ejected the commits that are reachable only\n> by an annotated tag out of the set of reachable commits, breaking\n> the computation to correctly implement the \"--expire-unreachable\"\n> option. We also started giving an error message that the API\n> function expected to be fed a commit object.\n\nYikes. I guess this is at least only for reflog pruning, so you couldn't\ncorrupt a repo this way, only lose reflog entries. But still, I think\nyour revert here is the right thing.\n\nIt does make me wonder if there other gaps for reflogs that point to\nnon-commits. E.g., if I have a tag pointing to a blob, would we save its\nreflog entry if a reachable commit points to that blob?. I suspect not,\nas the full reachability check is very expensive.\n\nBut certainly your patch is not making anything worse there.\n\n-Peff\n"},{"id":"498832","messageId":"xmqqy1605ap7.fsf@gitster.g","threadId":"61787","inReplyTo":"20240717054555.GA547635@coredump.intra.peff.net","subject":"Re: [PATCH] Revert \"reflog expire: don't use lookup_commit_reference_gently()\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-17T14:56:20Z","receivedAt":"2024-07-17T14:56:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It does make me wonder if there other gaps for reflogs that point to\n> non-commits. E.g., if I have a tag pointing to a blob, would we save its\n> reflog entry if a reachable commit points to that blob?. I suspect not,\n> as the full reachability check is very expensive.\n\nreflog.c:should_expire_reflog_ent() says that anything that is not a\ncommit should be expired without doing the unreachable() check, and\nthe UE_ALWAYS flag used for that is set by the function in question.\nThis arrangement has stayed the same since daf1d828 (reflog expire:\ndon't use lookup_commit_reference_gently(), 2021-12-22).\n\nThanks.\n"}]}