From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 30 Oct 2025 08:58:49 GMT
Subject: Re: [PATCH 7/8] packfile: always add packfiles to MRU when adding a pack
Message-ID: <aQMoyQAAeMD5WPxu@pks.im>
In-Reply-To: <aQKiT9JA+3zF4DHA@nand.local>

On Wed, Oct 29, 2025 at 07:25:03PM -0400, Taylor Blau wrote:
> On Tue, Oct 28, 2025 at 12:08:37PM +0100, Patrick Steinhardt wrote:
> > When adding a packfile to it store we add it both to the list and map of
> > packfiles, but we don't append it to the most-recently-used list of
> > packs. We do know to add the packfile to the MRU list as soon as we
> > access any of its objects, but in between we're being inconistent. It
> > doesn't help that there are some subsystems that _do_ add the packfile
> > to the MRU after having added it, which only adds to the confusion.
> >
> > Refactor the code so that we unconditionally add packfiles to the MRU
> > when adding them to a packfile store.
> 
> Reading this, I thought that the MRU cache lazily added packs only upon
> a successful object lookup, but looking more closely,
> packfile_store_prepare_mru() adds all of the known packs to the MRU
> cache eagerly.

Hm, you're right, this description is quite misleading.

Overall it's a mixed bag. We do add packfiles to the MRU initially via
`packfile_store_prepare_mru()` as you mention. But there are direct and
indirect callers of `packfile_store_add_pack()` that don't:

  - "builtin/index-pack.c"
  - "builtin/fast-import.c"
  - "http.c"

In all of these cases we expect that the objects will be read, so there
is no reason to not have them in the MRU as far as I can see.

Will rewrite the commit message, thanks!

Patrick