Re: [PATCH] pack-bitmap: remove checks before bitmap_free

From: Patrick Steinhardt <ps@pks.im>
Date: May 26, 2025, 06:49 UTC
Message-ID: <aDQO4Vkj7POztMnC@pks.im>
In-Reply-To: <pull.1977.git.git.1748149783383.gitgitgadget@gmail.com>

On Sun, May 25, 2025 at 05:09:43AM +0000, Lidong Yan via GitGitGadget wrote:

Show 7 quoted lines

> From: Lidong Yan <502024330056@smail.nju.edu.cn>
> 
> In pack-bitmap.c:find_boundary_objects, we build a roots_bitmap and
> cascade it to cb.base. However, I’m wondering why we only free
> roots_bitmap when the cascade succeeds. It seems we could safely remove
> this check and always free roots_bitmap afterward, which might provide
> some performance benefits.

This commit message isn't quite a convincing one. As author of a patch the onus falls on you to explain why the change is sensible, but even more importantly it also falls on you to explain why it is correct.

It is of course fine to ask for help and input, but in that case you should probably mark the patch accordingly, for example with the RFC tag.

Show 13 quoted lines

> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index ac6d62b980c..8727f316de9 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,
>  			bitmap_set(roots_bitmap, pos);
>  		}
>  
> -		if (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))
> -			bitmap_free(roots_bitmap);
> +		cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);
> +		bitmap_free(roots_bitmap);
>  	}

We know that `roots_bitmap` is always allocated via `bitmap_new()`, so it won't ever be a `NULL` pointer and should in theory always be free'd. Furthermore, we know that the pointer never escapes the local scope, either.

The next question would thus be: what does `cascade_pseudo_merges_1()` do with the bitmap? Are there situations where it does free it for us, or where it moves ownership of that bitmap? So let's go down the call chain:

  - `cascade_pseudo_merges_1()` passes it on to
    `cascade_pseudo_merges()`.

  - `cascade_pseudo_merges()` passes it on to `apply_pseudo_merge()`.

`apply_pseudo_merge()` itself then checks whether the pseudo-merge is a subset of the `roots_bitmap` and, if not, ORs the pseudo-merge into it.

None of these operations move around ownership or free the bitmap, so this looks like a true memory leak in case `cascade_pseudo_merges_1()` returns non-zero. Which would raise another question: when exactly does it return non-zero, and can we trigger the memory leak via a test?

Information like this should ideally be part of the commit message itself. It helps reviewers to figure out _why_ a change is correct and, if anybody were to dig into history, would also help them to have enough context.

Thanks!

Patrick