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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Feb 23, 2026 at 02:12:15AM -0500, Jeff King wrote:
> On Sun, Feb 22, 2026 at 11:07:41PM +0000, brian m. carlson wrote:
> 
> > 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).

As pointed out in [1] the root cause is actually something different,
and we merely expose this now with the MRU-based iteration. But I
wouldn't mind if we eventually switched back to maintaining two lists,
or finding a different way for how to maintain the iteration order.

Patrick

[1]: <aZwTPfmyrFp-QAPq@pks.im>