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

From: Jeff King <peff@peff.net>
Date: Jul 17, 2024, 05:45 UTC
Message-ID: <20240717054555.GA547635@coredump.intra.peff.net>
In-Reply-To: <xmqqv8156rh2.fsf@gitster.g>

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

Show 15 quoted lines

> 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