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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux