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
