From: Jeff King Date: Mon, 06 Nov 2023 17:20:24 GMT Subject: Re: [PATCH 1/4] test-bloom: stop setting up Git directory twice Message-ID: <20231106172024.GA10414@coredump.intra.peff.net> In-Reply-To: On Mon, Nov 06, 2023 at 11:45:53AM +0100, Patrick Steinhardt wrote: > We're setting up the Git directory twice in the `test-tool bloom` > helper, once at the beginning of `cmd_bloom()` and once in the local > subcommand implementation `get_bloom_filter_for_commit()`. This can lead > to memory leaks as we'll overwrite variables of `the_repository` with > newly allocated data structures. On top of that it's simply unnecessary. > > Fix this by only setting up the Git directory once. Makes sense. This situation was created by 094a685cd7 (t: make test-bloom initialize repository, 2020-07-29), which added the setup call at the start of the program. That commit closed the door to running test-bloom outside of a repository for sub-commands that could handle it (perhaps the murmur3 one, but I didn't test). So there are two possible directions here: - drop the call in cmd__bloom() and make sure all of the relevant sub-command functions do the setup. - drop the one command-specific one (i.e., your patch) In practice I don't think anybody cares about running this test-helper outside of a repository, and having to do setup in each sub-command is an extra maintenance burden. So I think your patch is the best direction. -Peff