Re: [PATCH 01/39] pack: make packed_git_mru global a value instead of a pointer
- From
Jeff King <peff@peff.net>
- Date
- Aug 30, 2017, 19:44 UTC
- Message-ID
- <20170830194448.u653vadhgzmkru3y@sigill.intra.peff.net>
- In-Reply-To
- <20170830064827.GB153983@aiede.mtv.corp.google.com>
On Tue, Aug 29, 2017 at 11:48:27PM -0700, Jonathan Nieder wrote:
Show 13 quoted lines
> The MRU cache that keeps track of recently used packs is represented > using two global variables: > > struct mru packed_git_mru_storage; > struct mru *packed_git_mru = &packed_git_mru_storage; > > Callers never assign to the packed_git_mru pointer, though, so we can > simplify by eliminating it and using &packed_git_mru_storage (renamed > to &packed_git_mru) directly. This variable is only used by the > packfile subsystem, making this a relatively uninvasive change (and > any new unadapted callers would trigger a compile error). > > Noticed while moving these globals to the object_store struct.
Sounds reasonable. I think I had originally wanted to hide the implementation and make the MRU more opaque, hence exposing only the pointer. But since I ended up just letting people directly walk over the struct pointers to do iteration, it needs to expose the struct internals anyway, and this layer of indirection isn't useful.
Show 5 quoted lines
> --- > builtin/pack-objects.c | 4 ++-- > cache.h | 4 ++-- > packfile.c | 12 +++++------- > 3 files changed, 9 insertions(+), 11 deletions(-)
The patch looks good to me.
As an aside, the mru code could probably be simplified a bit by reusing the list implementation from list.h (both were added around the same time, and it wasn't worth creating a dependency then, but I think list.h is useful and here to stay at this point).
It's definitely not critical to put that into this already-large series, though. Maybe we can use Junio's #leftoverbits tag. :)
-Peff