Re: [PATCH Outreachy] mru: use double-linked list from list.h

[Jeff King <peff@peff.net>]

On Thu, Sep 28, 2017 at 08:38:39AM +0000, Olga Telezhnaya wrote:

> diff --git a/packfile.c b/packfile.c
> index f69a5c8d607af..ae3b0b2e9c09a 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -876,6 +876,7 @@ void prepare_packed_git(void)
>  	for (alt = alt_odb_list; alt; alt = alt->next)
>  		prepare_packed_git_one(alt->path, 0);
>  	rearrange_packed_git();
> +	INIT_LIST_HEAD(&packed_git_mru.list);
>  	prepare_packed_git_mru();
>  	prepare_packed_git_run_once = 1;
>  }

I was thinking on this hunk a bit more, and I think it's not quite
right.

The prepare_packed_git_mru() function will clear the mru list and then
re-add each item from the packed_git list. But by calling
INIT_LIST_HEAD() here, we're effectively clearing the packed_git_mru
list, and we end up leaking whatever was on the list before.

So for the first call to prepare_packed_git, we really need this
INIT_LIST_HEAD() call. But for subsequent calls (which come from
reprepare_packed_git()), we must not call it.

There are a few ways to work around it that I can think of:

  1. Check whether packed_git_mru.list.head is NULL, and only initialize
     in that case.

  2. Use a static initializer for packed_git_mru.list, so that we don't
     have do the first-time initializing here.

  3. Teach reprepare_packed_git() to do the mru_clear() call, so that we
     know the list is empty when we get here.

One final and more invasive option is to stop regenerating the
packed_git_mru list from scratch during each prepare_packed_git(). I did
it that way so that we start with the same order that
rearrange_packed_git() will give us, but I'm not sure how much value
that has in practice (it probably had a lot more when we didn't have the
mru, and the time-sorted pack order helped find recent objects more
quickly).

The alternative would be to just teach install_packed_git() to add each
newly-added pack to the mru list, and then never clear the list (and we
wouldn't need an mru_clear() at all, then).

-Peff