Christian Couder <christian.couder@xxxxxxxxx> writes: > On Tue, Jan 16, 2018 at 2:46 AM, Gargi Sharma <gs051095@xxxxxxxxx> 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+Wgw@xxxxxxxxxxxxxx/) >> 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. 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. >> --- Missing sign-off? >> 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! Yes, nice reduction. > > [...] > >> @@ -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.
