Re: [PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index()

Eric Sunshine [off-list ref] writes:

On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau [off-list ref] wrote:

quoted

The function free_bitmap_index() is somewhat lax in what it frees. There
are two notable examples:

  - While it does call kh_destroy_oid_map on the "bitmaps" map (which
    maps) commit OIDs to their corresponding bitmaps, the bitmaps
    themselves are not freed. Note here that we recycle already-freed
    ewah_bitmaps into a pool, but these are handled correctly by
    ewah_pool_free().

The parentheses placement seems off; it's not clear what the intent
is. Perhaps either move the closing parenthesis to just before the
comma or drop them altogether.

Yeah, I think we can do without them and the sentence becomes
clearer (we can add a comma before "which", too).

quoted

  - We never bother to free the extended index's "positions" map, which
    we always allocate in load_bitmap().

Fix both of these.

Signed-off-by: Taylor Blau <redacted>

On Thu, Oct 21, 2021 at 11:32:16AM -0700, Junio C Hamano wrote:

Eric Sunshine [off-list ref] writes:

quoted

On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau [off-list ref] wrote:

quoted

The function free_bitmap_index() is somewhat lax in what it frees. There
are two notable examples:

  - While it does call kh_destroy_oid_map on the "bitmaps" map (which
    maps) commit OIDs to their corresponding bitmaps, the bitmaps
    themselves are not freed. Note here that we recycle already-freed
    ewah_bitmaps into a pool, but these are handled correctly by
    ewah_pool_free().

The parentheses placement seems off; it's not clear what the intent
is. Perhaps either move the closing parenthesis to just before the
comma or drop them altogether.

Yeah, I think we can do without them and the sentence becomes
clearer (we can add a comma before "which", too).

Yep, thanks both.

Thanks,
Taylor