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

Christian Couder [off-list ref] writes:

On Tue, Jan 16, 2018 at 2:46 AM, Gargi Sharma [off-list ref] wrote:

quoted

Replace the custom calls to mru.[ch] with calls to list.h. This patch is the
final step in removing the mru API completely and inlining the logic.

You might want to say that this provides a significant code reduction
which shows that the mru API is not a very useful abstraction anymore.

quoted

Another discussion, here
(https://public-inbox.org/git/CAOCi2DGYQr4jFf5ObY2buyhNJeaAPQKF8tbojn2W0b18Eo+Wgw@mail.gmail.com/)
was on what has to be done with the next pointer of packed git type

I think using "pointer to a 'struct packed_git'" instead of "pointer
of packed git type" would be clearer here, but anyway see below.

quoted

inside the
packed_git structure. It can be removed _given_ that no one needs to
access the list in order and can be sent as another patch.

I don't think it's worth pointing to a discussion about a future
improvement in the commit message. You could perhaps even remove all
the above paragraph as this commit is valuable and self contained
enough by itself.

True. 

If it is summarizing conclusion of the earlier discussion, that is a
different matter, though.

It is perfectly OK to have it under "---" line, even if it is merely
voicing author's opinion, by the way.  It can serve as a good
discussion (re-)starter.

quoted

---

Missing sign-off?

quoted

Changes in v2:
        - Add a move to front function to the list API.
        - Remove memory leak.
        - Remove redundant remove operations on the list.

The commit has been built on top of ot/mru-on-list branch.

Nice!

quoted

 Makefile               |  1 -
 builtin/pack-objects.c | 12 ++++++------
 cache.h                |  9 +++++----
 list.h                 |  7 +++++++
 mru.c                  | 27 ---------------------------
 mru.h                  | 40 ----------------------------------------
 packfile.c             | 18 +++++++++---------
 sha1_file.c            |  1 -
 8 files changed, 27 insertions(+), 88 deletions(-)
 delete mode 100644 mru.c
 delete mode 100644 mru.h

Very nice!

Yes, nice reduction.

[...]

quoted

@@ -1030,8 +1029,9 @@ static int want_object_in_pack(const unsigned char *sha1,
                                *found_pack = p;
                        }
                        want = want_found_object(exclude, p);
-                       if (!exclude && want > 0)
-                               mru_mark(&packed_git_mru, entry);
+                       if (!exclude && want > 0) {
+                               list_move_to_front(&p->mru, &packed_git_mru);
+                       }

Style: we usually remove brackets when there is one line after the
if(...) line. (See the 2 lines that you delete.)

Otherwise the patch looks good to me.

Thanks,
Christian.