| Message ID | 20220819160411.1791200-12-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1c3b05170a02adf894a33bc888b0fb730ee7f236 |
| Headers | show |
| Series | parse-options: handle subcommands | expand |
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.. > + 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?
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 :)
On Fri, Aug 19, 2022 at 07:53:08PM +0200, Ævar Arnfjörð Bjarmason wrote: > > + argc = parse_options(argc, argv, prefix, options, > > + builtin_commit_graph_usage, 0); > > + FREE_AND_NULL(options); > > Why FREE_AND_NULL() over free()? Heh, it's interesting that you should ask that ;) I followed the existing pattern for now, just like you did in 84e4484f12 (commit-graph: use parse_options_concat(), 2021-08-23), where you made graph_verify() and graph_write() use FREE_AND_NULL() to release their concatenated options arrays. But yeah, a plain free() should work just fine in these cases.
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); 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; read_replace_refs = 0; save_commit_buffer = 0; - if (!strcmp(argv[0], "verify")) - return graph_verify(argc, argv); - else if (argc && !strcmp(argv[0], "write")) - return graph_write(argc, argv); + argc = parse_options(argc, argv, prefix, options, + builtin_commit_graph_usage, 0); + FREE_AND_NULL(options); - error(_("unrecognized subcommand: %s"), argv[0]); -usage: - usage_with_options(builtin_commit_graph_usage, - builtin_commit_graph_options); + return fn(argc, argv, prefix); } diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index be0b5641ff..7e040eb1ed 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -12,12 +12,12 @@ test_expect_success 'usage' ' test_expect_success 'usage shown without sub-command' ' test_expect_code 129 git commit-graph 2>err && - ! grep error: err + grep usage: err ' test_expect_success 'usage shown with an error on unknown sub-command' ' cat >expect <<-\EOF && - error: unrecognized subcommand: unknown + error: unknown subcommand: `unknown'\'' EOF test_expect_code 129 git commit-graph unknown 2>stderr && grep error stderr >actual &&
'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(-)