On Tue, Jan 16, 2018 at 2:46 AM, Gargi Sharma <gs051...@gmail.com> wrote: > 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. > Another discussion, here > (https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@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. > 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. > --- > 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! > 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! [...] > @@ -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.
