From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>, Taylor Blau <me@ttaylorr.com>,
git@vger.kernel.org
Subject: Re: What's cooking in git.git (Oct 2021, #02; Wed, 6)
Date: Thu, 07 Oct 2021 04:57:10 +0200 [thread overview]
Message-ID: <87wnmph73b.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <YV5aaD418SyZqS/1@coredump.intra.peff.net>
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@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;
}
next prev parent reply other threads:[~2021-10-07 3:00 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-07 0:24 What's cooking in git.git (Oct 2021, #02; Wed, 6) Junio C Hamano
2021-10-07 2:01 ` ab/make-sparse-for-real Ævar Arnfjörð Bjarmason
2021-10-07 2:24 ` What's cooking in git.git (Oct 2021, #02; Wed, 6) Jeff King
2021-10-07 2:38 ` Jeff King
2021-10-07 4:07 ` Taylor Blau
2021-10-08 3:55 ` Jeff King
2021-10-08 7:51 ` Johannes Schindelin
2021-10-08 21:32 ` Jeff King
2021-10-20 12:27 ` Johannes Schindelin
2021-10-20 14:30 ` Taylor Blau
2021-10-20 14:47 ` Junio C Hamano
2021-10-20 16:13 ` Jeff King
2022-08-16 9:05 ` Coverity, was " Johannes Schindelin
2022-08-17 0:57 ` Jeff King
2022-08-19 11:22 ` Johannes Schindelin
2021-10-07 7:42 ` Ævar Arnfjörð Bjarmason
2021-10-08 4:10 ` Jeff King
2021-10-08 20:03 ` Junio C Hamano
2021-10-08 20:19 ` Jeff King
2021-10-08 21:57 ` Junio C Hamano
2021-10-07 2:57 ` Ævar Arnfjörð Bjarmason [this message]
2021-10-07 4:15 ` Taylor Blau
2021-10-07 3:55 ` Taylor Blau
2021-10-07 18:02 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wnmph73b.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).