Re: [PATCH 1/4] test-bloom: stop setting up Git directory twice

From: Jeff King <peff@peff.net>
Date: Nov 6, 2023, 17:20 UTC
Message-ID: <20231106172024.GA10414@coredump.intra.peff.net>
In-Reply-To: <fb1cc73ed3395ba3de72aba70d7679dd85ebb034.1699267422.git.ps@pks.im>

On Mon, Nov 06, 2023 at 11:45:53AM +0100, Patrick Steinhardt wrote:

Show 7 quoted lines

> 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