Re: [PATCH v2 11/20] builtin/commit-graph.c: let parse-options parse subcommands

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 19, 2022, 17:53 UTC
Message-ID: <220819.8635ds6run.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220819160411.1791200-12-szeder.dev@gmail.com>

On Fri, Aug 19 2022, SZEDER Gábor wrote:

Show 51 quoted lines

> 'git commit-graph' parses its subcommands with an if-else if
> statement.  parse-options has just learned to parse subcommands, so
> let's use that facility instead, with the benefits of shorter code,
> handling missing or unknown subcommands, and listing subcommands for
> Bash completion.
>
> Note that the functions implementing each subcommand only accept the
> 'argc' and '**argv' parameters, so add a (unused) '*prefix' parameter
> to make them match the type expected by parse-options, and thus avoid
> casting function pointers.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  builtin/commit-graph.c  | 30 +++++++++++++-----------------
>  t/t5318-commit-graph.sh |  4 ++--
>  2 files changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 51c4040ea6..1eb5492cbd 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -58,7 +58,7 @@ static struct option *add_common_options(struct option *to)
>  	return parse_options_concat(common_opts, to);
>  }
>  
> -static int graph_verify(int argc, const char **argv)
> +static int graph_verify(int argc, const char **argv, const char *prefix)
>  {
>  	struct commit_graph *graph = NULL;
>  	struct object_directory *odb = NULL;
> @@ -190,7 +190,7 @@ static int git_commit_graph_write_config(const char *var, const char *value,
>  	return 0;
>  }
>  
> -static int graph_write(int argc, const char **argv)
> +static int graph_write(int argc, const char **argv, const char *prefix)
>  {
>  	struct string_list pack_indexes = STRING_LIST_INIT_DUP;
>  	struct strbuf buf = STRBUF_INIT;
> @@ -307,26 +307,22 @@ static int graph_write(int argc, const char **argv)
>  
>  int cmd_commit_graph(int argc, const char **argv, const char *prefix)
>  {
> -	struct option *builtin_commit_graph_options = common_opts;
> +	parse_opt_subcommand_fn *fn = NULL;
> +	struct option builtin_commit_graph_options[] = {
> +		OPT_SUBCOMMAND("verify", &fn, graph_verify),
> +		OPT_SUBCOMMAND("write", &fn, graph_write),
> +		OPT_END(),
> +	};
> +	struct option *options = parse_options_concat(builtin_commit_graph_options, common_opts);

This looks like it'll leak if...

Show 8 quoted lines

>  
>  	git_config(git_default_config, NULL);
> -	argc = parse_options(argc, argv, prefix,
> -			     builtin_commit_graph_options,
> -			     builtin_commit_graph_usage,
> -			     PARSE_OPT_STOP_AT_NON_OPTION);
> -	if (!argc)
> -		goto usage;

We take this goto..

> +	argc = parse_options(argc, argv, prefix, options,
> +			     builtin_commit_graph_usage, 0);
> +	FREE_AND_NULL(options);

Why FREE_AND_NULL() over free()?

Show 6 quoted lines

>  
> -	error(_("unrecognized subcommand: %s"), argv[0]);
> -usage:
> -	usage_with_options(builtin_commit_graph_usage,
> -			   builtin_commit_graph_options);
> +	return fn(argc, argv, prefix);

The leak seems easily solved, let's just do the free() at the end?