Christian Couder <[email protected]> writes:
> On Tue, Jan 16, 2018 at 2:46 AM, Gargi Sharma <[email protected]> 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.
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.
