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
