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

> 
> 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.

> 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?