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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 19, 2018, 21:01 UTC
Message-ID: <xmqqefmlpnpj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAP8UFD0oBfmwx6r8rFMLzCyhEoy6QMQ-5RHJv=2WavYgE9FSMQ@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 22 quoted lines

> On Tue, Jan 16, 2018 at 2:46 AM, Gargi Sharma <gs051095@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+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.
>
>> 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?

Show 22 quoted lines

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

Show 20 quoted lines

>
> [...]
>
>> @@ -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.