Re: [PATCH v2 2/3] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed
Taylor Blau <me@ttaylorr.com> writes:
Show 26 quoted lines
> On Tue, May 20, 2025 at 09:23:09AM +0000, Taylor Blau via GitGitGadget wrote:
>> Signed-off-by: Taylor Blau <me@ttaylorr.com>
>
> 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.
Sounds like a plausible structure.