On Fri, Aug 19 2022, SZEDER Gábor wrote:
> '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@xxxxxxxxx>
> ---
> 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...
>
> 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()?
>
> - 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?
