Subject: Re: [PATCH 13/16] packfile: ensure `close_pack_revindex()` frees in-memory revindex

From: Jeff King <[email-masked]>
Date: Tue, 7 Apr 2026 02:29:54 -0400

On Sun, Mar 29, 2026 at 05:41:23PM -0400, Taylor Blau wrote:

> The following commit will introduce a case where we write a MIDX bitmap
> over packs that do not themselves have on-disk *.rev files.
> 
> This case is supported within Git, and we will simply fall back to
> generating the revindex in memory. But we don't ever release that
> memory, causing a leak that is exposed by a test introduced in the
> following commit.
> 
> (As far as I could find, we never free()'d memory allocated as a
> byproduct of creating an in-memory revindex, likely because that code
> predates the leak-checking niceties we have in the test suite now.)

Hmmm, I'm surprised we don't trigger this leak during the course of
odb_free(). But I guess we do that fairly rarely. It mostly happens via
repo_clear(), and we never do that for the "main" repo (just for
submodules). And it's unlikely that we'd have loaded a revindex for the
submodule for most operations (mostly we need them for generating packs
or when using bitmaps).

So I can't see an obvious way that this would be triggered in practice
with the current code. But I agree it is a leak waiting to happen, as
nobody ever clears p->revindex.

> diff --git a/packfile.c b/packfile.c
> index d4de9f3ffe8..97f7662e079 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -420,6 +420,8 @@ void close_pack_index(struct packed_git *p)
>  
>  static void close_pack_revindex(struct packed_git *p)
>  {
> +	FREE_AND_NULL(p->revindex);
> +
>  	if (!p->revindex_map)
>  		return;

The fact that this is called "close" made me wonder if some code might
want to just relinquish access to on-disk resources, and then pay the
cost when we have to unexpectedly re-compute the in-memory revindex.

But this function and its siblings are really about resetting the whole
state of the packed_git struct. So the behavior you're proposing matches
that.

-Peff