Re: [PATCH] fsck: do not loop infinitely when processing packs

From: Jeff King <peff@peff.net>
Date: Feb 23, 2026, 07:12 UTC
Message-ID: <20260223071215.GA136463@coredump.intra.peff.net>
In-Reply-To: <aZuMPcMYwFi4Sch5@fruit.crustytoothpaste.net>

On Sun, Feb 22, 2026 at 11:07:41PM +0000, brian m. carlson wrote:

Show 10 quoted lines

> I noticed that the code here seems to have come in with the 2.53 cycle,
> so we may want to cherry-pick it to `maint` at some point if it seems
> like the problem occurs often.  From what I can tell, it only occurs
> when one explicitly invokes `git fsck`[0] and not on transfer, so it
> shouldn't cause a DoS against server implementations.
> 
> Of course, we should wait for Patrick, who authored this code, to chime
> in and lend his expertise here.  I must admit I'm not very familiar with
> this area, although I had recently seen the MRU code when working on
> pack index v3 (and then I thought, "is this actually the problem?").

The problem seems to bisect to c31bad4f7d (packfile: track packs via the MRU list exclusively, 2025-10-30), which is not terribly surprising, as it was one of the known risks of collapsing the two lists into one.

Your solution is using the tool provided by that commit for its edge case:

    Note that there is one important edge case: `for_each_packed_object()`
    uses the MRU list to iterate through packs, and then it lists each
    object in those packs. This would have the effect that we now sort the
    current pack towards the front, thus modifying the list of packfiles we
    are iterating over, with the consequence that we'll see an infinite
    loop. This edge case is worked around by introducing a new field that
    allows us to skip updating the MRU.

So in that sense it is the right thing. But it really makes me wonder if we are going back to keeping two lists (one MRU and one in some stable order). Or at the very least providing _some_ iteration method that is guaranteed to be stable (whether a linked list or a function), so that iterating code is not subject to this subtle dependency by default.

Having to identify each potential spot and set a "btw, don't switch the pack list order!" flag seems error-prone. And also loses efficiency when you are iterating a pack and accessing objects in it (since we can't push that pack to the front of the MRU then, even though we'd expect there to be high locality with our iteration).

-Peff