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

From: Taylor Blau <[email-masked]>
Date: Mon, 29 Mar 2021 16:38:33 -0400

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?

--- >8 ---

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);
 }

 static int cmd_multi_pack_index_write(int argc, const char **argv)
@@ -112,8 +110,7 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)
 		OPT_END(),
 	};

-	options = parse_options_dup(builtin_multi_pack_index_repack_options);
-	options = add_common_options(options);
+	options = add_common_options(builtin_multi_pack_index_repack_options);

 	argc = parse_options(argc, argv, NULL,
 			     options,
@@ -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);
+
 	return midx_repack(the_repository, opts.object_dir,
 			   (size_t)opts.batch_size, opts.flags);
 }