On Sun, Nov 12, 2017 at 09:54:35AM +0000, Jeff King wrote:
> > @@ -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
