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

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 19, 2018, 18:26 UTC
Message-ID: <CAP8UFD0oBfmwx6r8rFMLzCyhEoy6QMQ-5RHJv=2WavYgE9FSMQ@mail.gmail.com>
In-Reply-To: <1516067185-1087-1-git-send-email-gs051095@gmail.com>

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.

Show 7 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!

Show 11 quoted lines

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

[...]

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