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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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@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...
>
>>  
>>  	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 :)