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

[Jeff King <peff@peff.net>]

On Mon, Feb 23, 2026 at 10:36:25AM +0100, Patrick Steinhardt wrote:

> On Mon, Feb 23, 2026 at 04:25:23AM -0500, Jeff King wrote:
> > On Mon, Feb 23, 2026 at 09:46:00AM +0100, Patrick Steinhardt wrote:
> > 
> > > 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.
> > 
> > Maybe I don't understand what you're saying, but isn't the root cause
> > the same?
> > 
> > Code is iterating the list, and then during that iteration calls
> > find_pack_entry(). The fact that fsck only calls find_pack_entry() in
> > some subset of cases is immaterial, I'd think. The risk is always there
> > when iterating now.
> 
> It is, true. All I'm saying is that the problem runs a bit deeper, and
> that fixing the actual root cause would also fix the issue reported by
> brian.

Ah, OK, after reading your other email again, I see what you're saying.
The root cause (for you) is that it is unexpected for fsck to call
find_pack_entry() at all in this case. Which I agree is wrong, but I
just wouldn't haven't called it the "root". ;)

But I think we are both on the same page that there are two problems
worth looking at (fsck should not be looking up the object again, and we
should make iteration less susceptible to re-ordering bugs).

> So we might want to have another look at hardening packfile iteration
> either by reinstating the second list for iteration or by extending
> `repo_for_each_pack()` to also set the `skip_updating_mru` bit. Over
> time though I'd rather get rid of `repo_for_each_pack()`, and once that
> is the case and packed object iteration is neatly encapsulated in the
> backend the risk of only having the MRU will be significantly reduced.

OK. Of the two short term solutions, I prefer the double-list. IMHO
skip_updating_mru is a bit of a hack in the first place, because it
misses opportunities to update the MRU.

-Peff