| Message ID | 20220819160411.1791200-5-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c1b117d31ca9b09444f14092c35d092f3330823e |
| Headers | show |
| Series | parse-options: handle subcommands | expand |
On Fri, Aug 19 2022, SZEDER Gábor wrote: > + for (int i = 0; i < argc; i++) See https://lore.kernel.org/git/CABPp-BHvQwct2WRRYGyzm=YVkjmwBqoe1DUtCicuQW=jrQ2hdA@mail.gmail.com/; but maybe nothing to to be done here... > + if (argc == 0 || strcmp(argv[0], "cmd")) { Nit: !argc > + error("'cmd' is mandatory"); > + usage_with_options(usage, test_flag_options); I think you want usage_msg_opt() instead. > +test_expect_success 'NO_INTERNAL_HELP works for -h' ' > + test_expect_code 129 test-tool parse-options-flags --no-internal-help cmd -h 2>err && > + cat err && Stray "cat", presumably in error.. > + grep "^error: unknown switch \`h$SQ" err && > + grep "^usage: " err > +' > + > +for help_opt in help help-all > +do > + test_expect_success "NO_INTERNAL_HELP works for --$help_opt" " > + test_expect_code 129 test-tool parse-options-flags --no-internal-help cmd --$help_opt 2>err && > + cat err && ditto.
SZEDER Gábor <szeder.dev@gmail.com> writes: > +static void print_args(int argc, const char **argv) > +{ > + for (int i = 0; i < argc; i++) > + printf("arg %02d: %s\n", i, argv[i]); > +} It is not November 2022 yet (cf. Documentation/CodingGuidelines). Curious why "%02d", not "%d", or autoscale for cases where argc is larger than 99, but I'll let it pass (iow no need to reroll only to "fix" it).
On Fri, Aug 19, 2022 at 11:18:20AM -0700, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > > > +static void print_args(int argc, const char **argv) > > +{ > > + for (int i = 0; i < argc; i++) > > + printf("arg %02d: %s\n", i, argv[i]); > > +} > > It is not November 2022 yet (cf. Documentation/CodingGuidelines). Oh, I've misunderstood Ævar's remarks about this in the previous round, and thought it's fair game. > Curious why "%02d", not "%d", or autoscale for cases where argc is > larger than 99, but I'll let it pass (iow no need to reroll only to > "fix" it). It doesn't matter for these tests, but 'test-tool parse-options' uses the same format to print args: $ ./t/helper/test-tool parse-options foo bar baz |tail -n3 arg 00: foo arg 01: bar arg 02: baz
On Fri, Aug 19, 2022 at 07:23:44PM +0200, Ævar Arnfjörð Bjarmason wrote: > > + error("'cmd' is mandatory"); > > + usage_with_options(usage, test_flag_options); > > I think you want usage_msg_opt() instead. As I responded to a similar remark in the previous round, parse-options uses the "error:" prefix in its error messages: $ ./t/helper/test-tool parse-options -U error: unknown switch `U' $ ./t/helper/test-tool parse-options --unknown error: unknown option `unknown' $ ./t/helper/test-tool parse-options -i error: switch `i' requires a value $ ./t/helper/test-tool parse-options --int error: option `integer' requires a value $ ./t/helper/test-tool parse-options -i foo error: switch `i' expects a numerical value $ ./t/helper/test-tool parse-options --int foo error: option `integer' expects a numerical value $ ./t/helper/test-tool parse-options --quiet=42 error: option `quiet' takes no value $ ./t/helper/test-tool parse-options --mode1 --mode2 error: option `mode2' is incompatible with --mode1 Subcommand-related error messages should be consistent with these, and use the "error:" prefix as well. Unfortunately, both usage_msg_opt() and usage_msg_optf() use the "fatal:" prefix instead, so I will not use those functions anywhere in this patch series. > > +test_expect_success 'NO_INTERNAL_HELP works for -h' ' > > + test_expect_code 129 test-tool parse-options-flags --no-internal-help cmd -h 2>err && > > + cat err && > > Stray "cat", presumably in error.. Leftover debugging :(
SZEDER Gábor <szeder.dev@gmail.com> writes: > On Fri, Aug 19, 2022 at 11:18:20AM -0700, Junio C Hamano wrote: >> SZEDER Gábor <szeder.dev@gmail.com> writes: >> >> > +static void print_args(int argc, const char **argv) >> > +{ >> > + for (int i = 0; i < argc; i++) >> > + printf("arg %02d: %s\n", i, argv[i]); >> > +} >> >> It is not November 2022 yet (cf. Documentation/CodingGuidelines). > > Oh, I've misunderstood Ævar's remarks about this in the previous > round, and thought it's fair game. If we make it a "fair game", when we find a platform that has problems with the syntax, we will have to find them and fix up many places. At least the number of the ones we let in by mistake are small and known, it may be still be manageable. It is how to be conservative. > It doesn't matter for these tests, but 'test-tool parse-options' uses > the same format to print args: As long as 99 is enough for us, I do not very much care. I just noticed an attempt to align that does not do a thorough job at it, and found it strange, that's all.
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index 48d3cf6692..88919785d3 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -192,3 +192,69 @@ int cmd__parse_options(int argc, const char **argv) return ret; } + +static void print_args(int argc, const char **argv) +{ + for (int i = 0; i < argc; i++) + printf("arg %02d: %s\n", i, argv[i]); +} + +static int parse_options_flags__cmd(int argc, const char **argv, + enum parse_opt_flags test_flags) +{ + const char *usage[] = { + "<...> cmd [options]", + NULL + }; + int opt = 0; + const struct option options[] = { + OPT_INTEGER('o', "opt", &opt, "an integer option"), + OPT_END() + }; + + argc = parse_options(argc, argv, NULL, options, usage, test_flags); + + printf("opt: %d\n", opt); + print_args(argc, argv); + + return 0; +} + +static enum parse_opt_flags test_flags = 0; +static const struct option test_flag_options[] = { + OPT_GROUP("flag-options:"), + OPT_BIT(0, "keep-dashdash", &test_flags, + "pass PARSE_OPT_KEEP_DASHDASH to parse_options()", + PARSE_OPT_KEEP_DASHDASH), + OPT_BIT(0, "stop-at-non-option", &test_flags, + "pass PARSE_OPT_STOP_AT_NON_OPTION to parse_options()", + PARSE_OPT_STOP_AT_NON_OPTION), + OPT_BIT(0, "keep-argv0", &test_flags, + "pass PARSE_OPT_KEEP_ARGV0 to parse_options()", + PARSE_OPT_KEEP_ARGV0), + OPT_BIT(0, "keep-unknown", &test_flags, + "pass PARSE_OPT_KEEP_UNKNOWN to parse_options()", + PARSE_OPT_KEEP_UNKNOWN), + OPT_BIT(0, "no-internal-help", &test_flags, + "pass PARSE_OPT_NO_INTERNAL_HELP to parse_options()", + PARSE_OPT_NO_INTERNAL_HELP), + OPT_END() +}; + +int cmd__parse_options_flags(int argc, const char **argv) +{ + const char *usage[] = { + "test-tool parse-options-flags [flag-options] cmd [options]", + NULL + }; + + argc = parse_options(argc, argv, NULL, test_flag_options, usage, + PARSE_OPT_STOP_AT_NON_OPTION); + + if (argc == 0 || strcmp(argv[0], "cmd")) { + error("'cmd' is mandatory"); + usage_with_options(usage, test_flag_options); + } + + return parse_options_flags__cmd(argc, argv, test_flags); +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 318fdbab0c..6e62282b60 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -51,6 +51,7 @@ static struct test_cmd cmds[] = { { "online-cpus", cmd__online_cpus }, { "pack-mtimes", cmd__pack_mtimes }, { "parse-options", cmd__parse_options }, + { "parse-options-flags", cmd__parse_options_flags }, { "parse-pathspec-file", cmd__parse_pathspec_file }, { "partial-clone", cmd__partial_clone }, { "path-utils", cmd__path_utils }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index bb79927163..d8e8403d70 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -41,6 +41,7 @@ int cmd__oidtree(int argc, const char **argv); int cmd__online_cpus(int argc, const char **argv); int cmd__pack_mtimes(int argc, const char **argv); int cmd__parse_options(int argc, const char **argv); +int cmd__parse_options_flags(int argc, const char **argv); int cmd__parse_pathspec_file(int argc, const char** argv); int cmd__partial_clone(int argc, const char **argv); int cmd__path_utils(int argc, const char **argv); diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index ed2fb620a9..8511ce24bb 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -456,4 +456,74 @@ test_expect_success '--end-of-options treats remainder as args' ' --end-of-options --verbose ' +test_expect_success 'KEEP_DASHDASH works' ' + test-tool parse-options-flags --keep-dashdash cmd --opt=1 -- --opt=2 --unknown >actual && + cat >expect <<-\EOF && + opt: 1 + arg 00: -- + arg 01: --opt=2 + arg 02: --unknown + EOF + test_cmp expect actual +' + +test_expect_success 'KEEP_ARGV0 works' ' + test-tool parse-options-flags --keep-argv0 cmd arg0 --opt=3 >actual && + cat >expect <<-\EOF && + opt: 3 + arg 00: cmd + arg 01: arg0 + EOF + test_cmp expect actual +' + +test_expect_success 'STOP_AT_NON_OPTION works' ' + test-tool parse-options-flags --stop-at-non-option cmd --opt=4 arg0 --opt=5 --unknown >actual && + cat >expect <<-\EOF && + opt: 4 + arg 00: arg0 + arg 01: --opt=5 + arg 02: --unknown + EOF + test_cmp expect actual +' + +test_expect_success 'KEEP_UNKNOWN works' ' + test-tool parse-options-flags --keep-unknown cmd --unknown=1 --opt=6 -u2 >actual && + cat >expect <<-\EOF && + opt: 6 + arg 00: --unknown=1 + arg 01: -u2 + EOF + test_cmp expect actual +' + +test_expect_success 'NO_INTERNAL_HELP works for -h' ' + test_expect_code 129 test-tool parse-options-flags --no-internal-help cmd -h 2>err && + cat err && + grep "^error: unknown switch \`h$SQ" err && + grep "^usage: " err +' + +for help_opt in help help-all +do + test_expect_success "NO_INTERNAL_HELP works for --$help_opt" " + test_expect_code 129 test-tool parse-options-flags --no-internal-help cmd --$help_opt 2>err && + cat err && + grep '^error: unknown option \`'$help_opt\' err && + grep '^usage: ' err + " +done + +test_expect_success 'KEEP_UNKNOWN | NO_INTERNAL_HELP works' ' + test-tool parse-options-flags --keep-unknown --no-internal-help cmd -h --help --help-all >actual && + cat >expect <<-\EOF && + opt: 0 + arg 00: -h + arg 01: --help + arg 02: --help-all + EOF + test_cmp expect actual +' + test_done
In 't0040-parse-options.sh' we thoroughly test the parsing of all types and forms of options, but in all those tests parse_options() is always invoked with a 0 flags parameter. Add a few tests to demonstrate how various 'enum parse_opt_flags' values are supposed to influence option parsing. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- t/helper/test-parse-options.c | 66 +++++++++++++++++++++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0040-parse-options.sh | 70 +++++++++++++++++++++++++++++++++++ 4 files changed, 138 insertions(+)