Re: What's cooking in git.git (Oct 2021, #02; Wed, 6)
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-10-07 03:00:48
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Wed, Oct 06 2021, Jeff King wrote:
quoted hunk ↗ jump to hunk
On Wed, Oct 06, 2021 at 05:24:14PM -0700, Junio C Hamano wrote:quoted
* 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 <redacted> --- 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; }