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

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

quoted

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

quoted

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