On Tue, May 20, 2025 at 09:23:09AM +0000, Taylor Blau via GitGitGadget wrote:
> Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
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
