Re: [PATCH 01/39] pack: make packed_git_mru global a value instead of a pointer
- Date: Wed, 30 Aug 2017 15:44:48 -0400
- From: Jeff King <peff@xxxxxxxx>
- Subject: Re: [PATCH 01/39] pack: make packed_git_mru global a value instead of a pointer
On Tue, Aug 29, 2017 at 11:48:27PM -0700, Jonathan Nieder wrote:
> 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.
> 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. :)