Re: [PATCH v3 04/16] builtin/multi-pack-index.c: split sub-commands

[Jeff King <peff@peff.net>]

On Mon, Mar 29, 2021 at 04:38:33PM -0400, Taylor Blau wrote:

> On Mon, Mar 29, 2021 at 07:36:21AM -0400, Jeff King wrote:
> > This is definitely a harmless leak in the sense that we are going to
> > exit the program after midx_repack() returns anyway. But it might be
> > worth keeping things tidy, as we've recently seen a renewed effort to do
> > some leak-checking of the test suite. I _think_ we can just free the
> > options struct (even though we are still using the values themselves, we
> > don't care about the "struct options" anymore). But even if not, an
> > UNLEAK(options) annotation would do it.
> 
> I see what you're saying. Let me make sure that I got the right idea in
> mind after reading your email. I'm thinking of squashing the following
> diff into this patch. For what it's worth, it causes 'valgrind
> --leak-check=full ./git-multi-pack-index repack' to exit cleanly (when
> it didn't before).
> 
> Does this match your expectations?

Yes, though...

> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index 23e51dfeb4..a78640c061 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -56,9 +56,7 @@ static struct option common_opts[] = {
> 
>  static struct option *add_common_options(struct option *prev)
>  {
> -	struct option *with_common = parse_options_concat(common_opts, prev);
> -	free(prev);
> -	return with_common;
> +	return parse_options_concat(common_opts, prev);
>  }

This simplification is orthogonal to the leak, and I'd be OK if you
wanted to retain it as it was before (because it future-proofs against
adding more add_foo_options() later, though for now it is a useless
dup/free pair).

> @@ -123,6 +120,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)
>  		usage_with_options(builtin_multi_pack_index_repack_usage,
>  				   options);
> 
> +	FREE_AND_NULL(options);
> +

And this is the leak fix I care about. We'd want the same thing in the
later caller that adds another use of add_common_options(), of course.

-Peff