On Wed, Oct 06 2021, Jeff King wrote:
> On Wed, Oct 06, 2021 at 05:24:14PM -0700, Junio C Hamano wrote:
>
>> * tb/repack-write-midx (2021-10-01) 9 commits
>> (merged to 'next' on 2021-10-06 at ccdd5aaf2a)
>> + builtin/repack.c: pass `--refs-snapshot` when writing bitmaps
>> + builtin/repack.c: make largest pack preferred
>> + builtin/repack.c: support writing a MIDX while repacking
>> + builtin/repack.c: extract showing progress to a variable
>> + builtin/repack.c: rename variables that deal with non-kept packs
>> + builtin/repack.c: keep track of existing packs unconditionally
>> + midx: preliminary support for `--refs-snapshot`
>> + builtin/multi-pack-index.c: support `--stdin-packs` mode
>> + midx: expose `write_midx_file_only()` publicly
>>
>> "git repack" has been taught to generate multi-pack reachability
>> bitmaps.
>>
>> Will merge to 'master'.
>
> Sorry not to catch this before it hit 'next', but there's a small leak
> in the test helper. This patch can go on top to fix it.
>
> -- >8 --
> Subject: [PATCH] test-read-midx: fix leak of bitmap_index struct
>
> In read_midx_preferred_pack(), we open the bitmap index but never free
> it. This isn't a big deal since this is just a test helper, and we exit
> immediately after, but since we're trying to keep our leak-checking tidy
> now, it's worth fixing.
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> t/helper/test-read-midx.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
> index 0038559129..9d6fa7a377 100644
> --- a/t/helper/test-read-midx.c
> +++ b/t/helper/test-read-midx.c
> @@ -85,11 +85,15 @@ static int read_midx_preferred_pack(const char *object_dir)
> return 1;
>
> bitmap = prepare_bitmap_git(the_repository);
> - if (!(bitmap && bitmap_is_midx(bitmap)))
> + if (!bitmap)
> return 1;
> -
> + if (!bitmap_is_midx(bitmap)) {
> + free_bitmap_index(bitmap);
> + return 1;
> + }
>
> printf("%s\n", midx->pack_names[midx_preferred_pack(bitmap)]);
> + free_bitmap_index(bitmap);
> return 0;
> }
Thanks, I think it's no big deal, those tests seem to leak a lot
already. Here's a patch that might be generally applicable and makes a
few more of its tests pass.
The s/free/free_chunkfile/g seems like a good bug fix, and that "m =
NULL" pattern seems odd, don't we always want to free in that scenario?
It passes all tests...
This brings t5319-multi-pack-index.sh down from 64 to 57 failures in
"next", I didn't try in combination with your patch.
diff --git a/midx.c b/midx.c
index 7e06e859756..d24b1e6a9d4 100644
--- a/midx.c
+++ b/midx.c
@@ -179,12 +179,13 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
trace2_data_intmax("midx", the_repository, "load/num_packs", m->num_packs);
trace2_data_intmax("midx", the_repository, "load/num_objects", m->num_objects);
+ free_chunkfile(cf);
return m;
cleanup_fail:
free(m);
free(midx_name);
- free(cf);
+ free_chunkfile(cf);
if (midx_map)
munmap(midx_map, midx_size);
if (0 <= fd)
@@ -1602,7 +1603,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
* Remaining tests assume that we have objects, so we can
* return here.
*/
- return verify_midx_error;
+ goto cleanup;
}
if (flags & MIDX_PROGRESS)
@@ -1679,8 +1680,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
midx_display_sparse_progress(progress, i + 1);
}
stop_progress(&progress);
-
+cleanup:
free(pairs);
+ free(m);
return verify_midx_error;
}
@@ -1927,11 +1929,9 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
}
result = write_midx_internal(object_dir, NULL, NULL, NULL, NULL, flags);
- m = NULL;
cleanup:
- if (m)
- close_midx(m);
+ close_midx(m);
free(include_pack);
return result;
}
