On Thu, Sep 28, 2017 at 08:03:00PM +0900, Junio C Hamano wrote:
> > - for (entry = packed_git_mru.head; entry; entry = entry->next) {
> > + list_for_each(pos, &packed_git_mru.list) {
> > + struct mru *entry = list_entry(pos, struct mru, list);
> > struct packed_git *p = entry->item;
> > off_t offset;
>
> I was a bit surprised to see a change outside mru.[ch] like this
> one. The reason why I was surprised was because I expected mru.[ch]
> would offer its own API that encapsulates enumeration like this one
> and this patch would just be reimplementing that API using the list
> API, instead of rewriting the users of mru API to directly access
> the list API.
>
> Alas, there is no such mru API that lets a mru user to iterate over
> elements, so the original of the above code were using mru's
> implementation detail directly.
>
> We probably want to invent mru_for_each() that hides the fact that
> mru is implemented in terms of list_head from the users of mru API
> and use it here.
I agree that the caller would be a little shorter with an mru-specific
iterator (e.g., we could probably do the list_entry() part
automatically).
But I also think this patch may be a stepping stone to dropping "struct
mru" entirely, and just pushing a "struct list_head mru" into the
packed_git object itself (or of course any object you like). At which
point we'd just directly use the list iterators anyway.
(One could argue that if that's our end goal, we could go straight
there. But I think this middle state has value, because the individual
steps are easier to verify).
> > @@ -8,18 +10,15 @@
> > *
> > * // Create a list. Zero-initialization is required.
> > * static struct mru cache;
> > - * mru_append(&cache, item);
> > - * ...
> > + * INIT_LIST_HEAD(&cache.list);
>
> "Zero-initialization is required." is no longer true, it seems, and
> the comment above needs to be updated, right?
>
> More importantly, this leaks to the user of the API the fact that
> mru is internally implemented in terms of the list API, which is
> not necessary (when we want to update the implementation later, we'd
> need to update all the users again). Perhaps you'd want
>
> INIT_MRU(cache);
>
> which is #define'd in this file, perhaps like so:
>
> #define INIT_MRU(mru) INIT_LIST_HEAD(&((mru).list))
I'd make the same claims here as above (both that I agree your proposed
interface looks nicer, but also that I think we eventually do want to
expose that this is tightly coupled with list.h).
-Peff
