Re: [PATCH Outreachy] mru: use double-linked list from list.h

From: Jeff King <peff@peff.net>
Date: Sep 28, 2017, 20:47 UTC
Message-ID: <20170928204705.7ixxspiflmhsdh7d@sigill.intra.peff.net>
In-Reply-To: <xmqq8tgz13x7.fsf@gitster.mtv.corp.google.com>

On Thu, Sep 28, 2017 at 08:03:00PM +0900, Junio C Hamano wrote:

Show 20 quoted lines

> > -	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).

Show 21 quoted lines

> > @@ -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