On Sun, May 25, 2025 at 05:09:43AM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx>
>
> 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.
> 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
