Re: [PATCH v2 2/3] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed

On Tue, May 20, 2025 at 09:23:09AM +0000, Taylor Blau via GitGitGadget wrote:

This commit forges my Signed-off-by, but I am happy with the result here.

I do think the series is structured a little awkwardly as a result of adding this patch to it. That this and the previous patch have the subject "pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed" make the series not quite as clear as it could be.

I think there are a couple of things going on:

  - This patch is a bug fix that could be applied independently of the
    first one. The rationale there would be that we shouldn't be leaking
    the EWAH bitmaps in 'b->bitmaps', but we are as a result of NULL'ing
    the pointer in the "failed" label. That patch can stand alone.

  - The first patch (yours) is no longer fixing a leak, at least after
    this patch. But it does delay reading the bitmap until we have
    validated its XOR offset for sanity, which is a good thing mostly
    from a performance perspective.

I would probably swap the two patches around so that yours applies on top of mine, and then rewords the patch message in yours to reflect that it is no longer fixing a leak.

That all said, if you feel strongly that the structure is fine/better as-is, I'd be more than happy to discuss it further.

Thanks, Taylor