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

From: Jeff King <peff@peff.net>
Date: Sep 28, 2017, 22:42 UTC
Message-ID: <20170928224244.pi34zwifnornssqk@sigill.intra.peff.net>
In-Reply-To: <0102015ec7a3424b-529be659-bdb6-42c4-a48f-db264f33d53a-000000@eu-west-1.amazonses.com>

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

Show 12 quoted lines

> 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