[PATCH] Revert "reflog expire: don't use lookup_commit_reference_gently()"

[PATCH] Revert "reflog expire: don't use lookup_commit_reference_gently()"

[Junio C Hamano]

During Git 2.35 timeframe, daf1d828 (reflog expire: don't use
lookup_commit_reference_gently(), 2021-12-22) replaced a call to
lookup_commit_reference_gently() with a call to lookup_commit().

What it failed to consider was that our refs do not necessarily
point at commits (most notably, we have annotated and signed tags),
and more importantly that lookup_commit() does not dereference a tag
to return a commit; instead it returns NULL when a tag is given.

Since the commit returned is used as a starting point for the
reachability check, this ejected the commits that are reachable only
by an annotated tag out of the set of reachable commits, breaking
the computation to correctly implement the "--expire-unreachable"
option.  We also started giving an error message that the API
function expected to be fed a commit object.

This problem hasn't been reported or noticed for a long time,
probably because the "refs/tags/" hierarchy by default is not
covered by reflogs, as nobody usually moves tags.

Revert the change to correctly find the commit pointed at by the ref
to restore the previous behaviour, but do so only in a more modern
codebase, as we had significant code churn since then and it is not
grave enough to worry about for older maintenance tracks.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
  cf. <patch-v3-6.9-cfa80e84c6d-20211222T040557Z-avarab@gmail.com>

 reflog.c          | 3 ++-
 t/t1410-reflog.sh | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/reflog.c b/reflog.c
index 0a1bc35e8c..2e45a17e12 100644
--- a/reflog.c
+++ b/reflog.c
@@ -330,7 +330,8 @@ void reflog_expiry_prepare(const char *refname,
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
 		cb->unreachable_expire_kind = UE_HEAD;
 	} else {
-		commit = lookup_commit(the_repository, oid);
+		commit = lookup_commit_reference_gently(the_repository,
+							oid, 1);
 		if (commit && is_null_oid(&commit->object.oid))
 			commit = NULL;
 		cb->unreachable_expire_kind = commit ? UE_NORMAL : UE_ALWAYS;
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index d2f5f42e67..16816e82b2 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -146,6 +146,14 @@ test_expect_success rewind '
 	test_line_count = 5 output
 '
 
+test_expect_success 'reflog expire should not barf on an annotated tag' '
+	test_when_finished "git tag -d v0.tag || :" &&
+	git -c core.logAllRefUpdates=always \
+		tag -a -m "tag name" v0.tag main &&
+	git reflog expire --dry-run refs/tags/v0.tag 2>err &&
+	test_grep ! "error: [Oo]bject .* not a commit" err
+'
+
 test_expect_success 'corrupt and check' '
 
 	corrupt $F &&
-- 
2.46.0-rc0-153-ge537c69d48

Re: [PATCH] Revert "reflog expire: don't use lookup_commit_reference_gently()"

[Jeff King]

On Tue, Jul 16, 2024 at 12:56:25PM -0700, Junio C Hamano wrote:

> During Git 2.35 timeframe, daf1d828 (reflog expire: don't use
> lookup_commit_reference_gently(), 2021-12-22) replaced a call to
> lookup_commit_reference_gently() with a call to lookup_commit().
> 
> What it failed to consider was that our refs do not necessarily
> point at commits (most notably, we have annotated and signed tags),
> and more importantly that lookup_commit() does not dereference a tag
> to return a commit; instead it returns NULL when a tag is given.
> 
> Since the commit returned is used as a starting point for the
> reachability check, this ejected the commits that are reachable only
> by an annotated tag out of the set of reachable commits, breaking
> the computation to correctly implement the "--expire-unreachable"
> option.  We also started giving an error message that the API
> function expected to be fed a commit object.

Yikes. I guess this is at least only for reflog pruning, so you couldn't
corrupt a repo this way, only lose reflog entries. But still, I think
your revert here is the right thing.

It does make me wonder if there other gaps for reflogs that point to
non-commits. E.g., if I have a tag pointing to a blob, would we save its
reflog entry if a reachable commit points to that blob?. I suspect not,
as the full reachability check is very expensive.

But certainly your patch is not making anything worse there.

-Peff

Re: [PATCH] Revert "reflog expire: don't use lookup_commit_reference_gently()"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It does make me wonder if there other gaps for reflogs that point to
> non-commits. E.g., if I have a tag pointing to a blob, would we save its
> reflog entry if a reachable commit points to that blob?. I suspect not,
> as the full reachability check is very expensive.

reflog.c:should_expire_reflog_ent() says that anything that is not a
commit should be expired without doing the unreachable() check, and
the UE_ALWAYS flag used for that is set by the function in question.
This arrangement has stayed the same since daf1d828 (reflog expire:
don't use lookup_commit_reference_gently(), 2021-12-22).

Thanks.