Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing

[Jeff King <peff@peff.net>]

On Wed, Jul 13, 2022 at 03:10:35PM +0200, Ævar Arnfjörð Bjarmason wrote:

> 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.

>  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