Re: [PATCH v2 7/8] packfile: add kept-pack cache for find_kept_pack_entry()

[Taylor Blau <me@ttaylorr.com>]

On Wed, Feb 17, 2021 at 03:25:17PM -0500, Jeff King wrote:
> Would just doing:
>
>   if (cache.packs && cache.flags != flags)
> 	BUG("kept-pack-cache cannot handle multiple queries in a single process");
>
> be a better solution? That is not helping anyone towards a world where
> we gracefully handle back-and-forth queries. But it makes it abundantly
> clear when such a thing would become necessary.

I dunno. I can certainly see its merits, but I have to imagine that
anybody who cares enough about the performance will be able to find our
conversation here. Assuming that's the case, I would rather have the
kept-pack cache handle multiple queries before BUG()-ing.

> > > Is there any reason not to just embed the kept_pack_cache struct inside
> > > the object_store? It's one less pointer to deal with. I wonder if this
> > > is a holdover from an attempt to have multiple caches.
> > >
> > > (I also think it would be reasonable if we wanted to hide the definition
> > > of the cache struct from callers, but we don't seem do to that).
> >
> > Not a holdover, just designed to avoid adding too many extra fields to
> > the object-store. I don't feel strongly, but I do think hiding the
> > definition is a good idea, so I'll inline it.
>
> This response confuses me a bit. Hiding the definition from callers
> would mean _keeping_ it as a pointer, but putting the definition into
> packfile.c, where nobody outside that file could see it (at least that
> is what I meant by hiding).
>
> But inlining it to me implies embedding the struct (not a pointer to it)
> in "struct object_store", defining the struct at the point we define the
> struct field which uses it.
>
> I am fine with either, to be clear. I'm just confused which you are
> proposing to do. :)

Probably because I changed my mind in the middle of writing it ;). I'm
proposing embedding the definition of the struct into the definition of
object_store, and then operating on its fields (from within packfile.c).

Thanks,
Taylor