Re: [PATCH] mru: Replace mru.[ch] with list.h implementation

From: Jeff King <peff@peff.net>
Date: Nov 12, 2017, 09:59 UTC
Message-ID: <20171112095910.xwz43bzfbr5hz3tq@sigill.intra.peff.net>
In-Reply-To: <20171112095435.f4o662ygtt2taf5y@sigill.intra.peff.net>

On Sun, Nov 12, 2017 at 09:54:35AM +0000, Jeff King wrote:

Show 27 quoted lines

> > @@ -1566,6 +1566,7 @@ struct pack_window {
> >  
> >  extern struct packed_git {
> >  	struct packed_git *next;
> > +	struct list_head mru;
> >  	struct pack_window *windows;
> >  	off_t pack_size;
> >  	const void *index_data;
> 
> Sort of a side note, but seeing these two list pointers together makes
> me wonder what we should do with the list created by the "next" pointer.
> It seems like there are three options:
> 
>   1. Convert it to "struct list_head", too, for consistency.
> 
>   2. Leave it as-is. We never delete from the list nor do any fancy
>      manipulation, so it doesn't benefit from the reusable code.
> 
>   3. I wonder if we could drop it entirely, and just keep a single list
>      of packs, ordered by mru. I'm not sure if anybody actually cares
>      about accessing them in the "original" order. That order is
>      reverse-chronological (by prepare_packed_git()), but I think that
>      was mostly out of a sense that recent packs would be accessed more
>      than older ones (but having a real mru strategy replaces that
>      anyway).
> 
> What do you think?

Thinking on this a bit more, even if we want to go down any road except (2), it probably ought to come as a separate patch on top anyway. The changes you're making here are quite obviously a noop for visible behavior.

But dropping the "next" pointer (and the matching "don't clear the list" I mentioned later) would potentially mean examining the packs in a slightly different order. I _think_ that's fine, but it's possible there could be a subtle fallout. So it's better to keep it separate from the more pure refactoring in your patch.

-Peff