Re: What's cooking in git.git (Oct 2021, #02; Wed, 6)
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Oct 7, 2021, 02:57 UTC
- Message-ID
- <87wnmph73b.fsf@evledraar.gmail.com>
- In-Reply-To
- <YV5aaD418SyZqS/1@coredump.intra.peff.net>
On Wed, Oct 06 2021, Jeff King wrote:
Show 56 quoted lines
> 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@peff.net>
> ---
> 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; }