Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing
- From
Jeff King <peff@peff.net>
- Date
- Jul 18, 2022, 15:11 UTC
- Message-ID
- <YtV4KmrTBkmcx6m3@coredump.intra.peff.net>
- In-Reply-To
- <patch-6.6-4a581a4a6ce-20220713T130511Z-avarab@gmail.com>
On Wed, Jul 13, 2022 at 03:10:35PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 5 quoted lines
> There's several potential ways to fix those sort of leaks, we could > add a "nodup" mode to "struct strvec", which would work for the cases > where we push constant strings to it. But that wouldn't work as soon > as we used strvec_pushf(), or otherwise needed to duplicate or create > a string for that "struct strvec".
Right, I think this falls down when you have mixed const/non-const entries. You have to know which is which for strvec_clear().
> Let's instead make it the responsibility of the revisions API. If it's > going to clobber elements of argv it can also free() them, which it > will now do if instructed to do so via "free_removed_argv_elements".
OK, I think this is a reasonable and minimal fix for the "--" problem on its own.
But if you went just a little further and made the option "rev should own its own argv", then I think you can simplify life for callers even more. They could construct a strvec themselves and then hand it off to the rev_info, to be cleaned up when release_revisions() is called (and of course freeing the "--" when we overwrite it in the interim, as you do here).
Then all of the bisect callers from the previous patch could avoid having to deal with the strvec at all. They'd call bisect_rev_setup(), which would internally attach the memory to rev_info.
Show 7 quoted lines
> bisect.c | 6 ++++-- > builtin/submodule--helper.c | 5 ++++- > remote.c | 5 ++++- > revision.c | 2 ++ > revision.h | 3 ++- > t/t2020-checkout-detach.sh | 1 + > 6 files changed, 17 insertions(+), 5 deletions(-)
The patch itself works as advertised, I think.
-Peff