Re: [PATCH v2 3/5] commit-graph: use parse_options_concat()

[Taylor Blau <me@ttaylorr.com>]

On Sun, Jul 18, 2021 at 09:58:07AM +0200, Ævar Arnfjörð Bjarmason wrote:
> Make use of the parse_options_concat() so we don't need to copy/paste
> common options like --object-dir. This is inspired by a similar change
> to "checkout" in 2087182272
> (checkout: split options[] array in three pieces, 2019-03-29).
>
> A minor behavior change here is that now we're going to list both
> --object-dir and --progress first, before we'd list --progress along
> with other options.

This is very reminiscent to the patch I sent to do the same in the
`multi-pack-index` builtin, probably because you were the person to
recommend I do that cleanup in the first place ;).

I got some good advice from Peff in [1] went I sent that patch, which
I'll try to summarize here, since I think a few pieces of it could be
applied to clean up this patch a little.

[1]: https://lore.kernel.org/git/YGG7tWBzo5NGl2+g@coredump.intra.peff.net/

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/commit-graph.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index baead04a03b..ff125adf2d5 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -44,6 +44,21 @@ static struct opts_commit_graph {
>  	int enable_changed_paths;
>  } opts;
>
> +static struct option *add_common_options(struct option *prevopts)
> +{
> +	struct option options[] = {
> +		OPT_STRING(0, "object-dir", &opts.obj_dir,
> +			   N_("dir"),
> +			   N_("the object directory to store the graph")),
> +		OPT_BOOL(0, "progress", &opts.progress,
> +			 N_("force progress reporting")),
> +		OPT_END()
> +	};

Not from Peff's mail, but is there any reason to non statically allocate
this?

The only reason I could think of is that `opts` is heap allocated, but
it's not, so I think we could probably mark `options` as static here
(and perhaps rename it to `common_opts` while we're at it, if for no
other reason than to be consistent with what's in the multi-pack-index
builtin).

> +	struct option *newopts = parse_options_concat(options, prevopts);
> +	free(prevopts);

Since we're not concatenating more than one layer on top of the common
options (unlike in `checkout`), this can be simplified to just return
parse_options_concat without freeing prevopts.

We should be careful to make sure we free the return value from
parse_options_concat eventually, though.

> +	return newopts;
> +}
> +
>  static struct object_directory *find_odb(struct repository *r,
>  					 const char *obj_dir)
>  {
> @@ -77,20 +92,18 @@ static int graph_verify(int argc, const char **argv)
>  	int flags = 0;
>
>  	static struct option builtin_commit_graph_verify_options[] = {
> -		OPT_STRING(0, "object-dir", &opts.obj_dir,
> -			   N_("dir"),
> -			   N_("the object directory to store the graph")),
>  		OPT_BOOL(0, "shallow", &opts.shallow,
>  			 N_("if the commit-graph is split, only verify the tip file")),
> -		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
>  		OPT_END(),
>  	};
> +	struct option *options = parse_options_dup(builtin_commit_graph_verify_options);
> +	options = add_common_options(options);

Likewise down here (and in the other callers, too) this dup is pointless
if we're going to immediately free it after calling
parse_options_concat. So we can drop that, too.

Here's something to consider squashing in on top:

--- >8 ---

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index ff125adf2d..00b0721789 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -44,19 +44,18 @@ static struct opts_commit_graph {
 	int enable_changed_paths;
 } opts;

-static struct option *add_common_options(struct option *prevopts)
+static struct option common_opts[] = {
+	OPT_STRING(0, "object-dir", &opts.obj_dir,
+		   N_("dir"),
+		   N_("the object directory to store the graph")),
+	OPT_BOOL(0, "progress", &opts.progress,
+		 N_("force progress reporting")),
+	OPT_END()
+};
+
+static struct option *add_common_options(struct option *to)
 {
-	struct option options[] = {
-		OPT_STRING(0, "object-dir", &opts.obj_dir,
-			   N_("dir"),
-			   N_("the object directory to store the graph")),
-		OPT_BOOL(0, "progress", &opts.progress,
-			 N_("force progress reporting")),
-		OPT_END()
-	};
-	struct option *newopts = parse_options_concat(options, prevopts);
-	free(prevopts);
-	return newopts;
+	return parse_options_concat(common_opts, to);
 }

 static struct object_directory *find_odb(struct repository *r,
@@ -96,8 +95,7 @@ static int graph_verify(int argc, const char **argv)
 			 N_("if the commit-graph is split, only verify the tip file")),
 		OPT_END(),
 	};
-	struct option *options = parse_options_dup(builtin_commit_graph_verify_options);
-	options = add_common_options(options);
+	struct option *options = add_common_options(builtin_commit_graph_verify_options);

 	trace2_cmd_mode("verify");

@@ -120,6 +118,7 @@ static int graph_verify(int argc, const char **argv)
 		die_errno(_("Could not open commit-graph '%s'"), graph_name);

 	FREE_AND_NULL(graph_name);
+	FREE_AND_NULL(options);

 	if (open_ok)
 		graph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);
@@ -245,8 +244,7 @@ static int graph_write(int argc, const char **argv)
 			0, write_option_max_new_filters),
 		OPT_END(),
 	};
-	struct option *options = parse_options_dup(builtin_commit_graph_write_options);
-	options = add_common_options(options);
+	struct option *options = add_common_options(builtin_commit_graph_write_options);

 	opts.progress = isatty(2);
 	opts.enable_changed_paths = -1;
@@ -316,6 +314,7 @@ static int graph_write(int argc, const char **argv)
 		result = 1;

 cleanup:
+	FREE_AND_NULL(options);
 	string_list_clear(&pack_indexes, 0);
 	strbuf_release(&buf);
 	return result;
@@ -323,8 +322,7 @@ static int graph_write(int argc, const char **argv)

 int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 {
-	struct option *no_options = parse_options_dup(NULL);
-	struct option *builtin_commit_graph_options = add_common_options(no_options);
+	struct option *builtin_commit_graph_options = common_opts;

 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix,