On Fri, Aug 19 2022, Ævar Arnfjörð Bjarmason wrote:
> 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..
Sorry, I'm being an idiot and misreading this, the "goto" isn't here
anymore, du'h!
>> + argc = parse_options(argc, argv, prefix, options,
>> + builtin_commit_graph_usage, 0);
>> + FREE_AND_NULL(options);
>
> Why FREE_AND_NULL() over free()?
But this question still stands :)
