| Message ID | 20190429100525.32045-1-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] parse-options: don't emit "ambiguous option" for aliases | expand |
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > But due to the way the options parsing machinery works this resulted > in the rather absurd situation of: > > $ git clone --recurs [...] > error: ambiguous option: recurs (could be --recursive or --recurse-submodules) > > Add OPT_ALIAS() to express this link between two or more options and use > it in git-clone. Multiple aliases of an option could be written as > > OPT_ALIAS(0, "alias1", "original-name"), > OPT_ALIAS(0, "alias2", "original-name"), > ... > > The current implementation is not exactly optimal in this case. But we > can optimize it when it becomes a problem. So far we don't even have two > aliases of any option. Sounds good enough for any practical need I can forsee ;-) Thanks. > diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh > index 800b3ea5f5..cebc77fab0 100755 > --- a/t/t0040-parse-options.sh > +++ b/t/t0040-parse-options.sh > @@ -48,6 +48,12 @@ Standard options > -q, --quiet be quiet > --expect <string> expected output in the variable dump > > +Alias > + -A, --alias-source <string> > + get a string > + -Z, --alias-target <string> > + get a string > + > EOF This is not a new problem per-se, as there already is a line before the precontext of this hunk that shares the same issue, but to prevent future problems, I am very much tempted to apply the attached on top. -- >8 -- Subject: t0040: protect lines that are indented by spaces This block is byte-for-byte identical expected output, that contains a few lines that are indented in many spaces, which makes "git diff --check" unhappy and will break the test when "git am --whitespace=fix" is allowed to "correct" them. Protect the left edge with a marker character, and strip it with sed, which is used as a standard way to deal with this issue in our test suite. Signed-off-by: Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * Of course, if the right-edge need to be protected, we can do so as well. t/t0040-parse-options.sh | 94 ++++++++++++++++++++++++------------------------ 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index cebc77fab0..26373b5b72 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -7,53 +7,53 @@ test_description='our own option parser' . ./test-lib.sh -cat >expect <<\EOF -usage: test-tool parse-options <options> - - A helper function for the parse-options API. - - --yes get a boolean - -D, --no-doubt begins with 'no-' - -B, --no-fear be brave - -b, --boolean increment by one - -4, --or4 bitwise-or boolean with ...0100 - --neg-or4 same as --no-or4 - - -i, --integer <n> get a integer - -j <n> get a integer, too - -m, --magnitude <n> get a magnitude - --set23 set integer to 23 - -L, --length <str> get length of <str> - -F, --file <file> set file to <file> - -String options - -s, --string <string> - get a string - --string2 <str> get another string - --st <st> get another string (pervert ordering) - -o <str> get another string - --list <str> add str to list - -Magic arguments - --quux means --quux - -NUM set integer to NUM - + same as -b - --ambiguous positive ambiguity - --no-ambiguous negative ambiguity - -Standard options - --abbrev[=<n>] use <n> digits to display SHA-1s - -v, --verbose be verbose - -n, --dry-run dry run - -q, --quiet be quiet - --expect <string> expected output in the variable dump - -Alias - -A, --alias-source <string> - get a string - -Z, --alias-target <string> - get a string - +sed -e 's/^|//' >expect <<\EOF +|usage: test-tool parse-options <options> +| +| A helper function for the parse-options API. +| +| --yes get a boolean +| -D, --no-doubt begins with 'no-' +| -B, --no-fear be brave +| -b, --boolean increment by one +| -4, --or4 bitwise-or boolean with ...0100 +| --neg-or4 same as --no-or4 +| +| -i, --integer <n> get a integer +| -j <n> get a integer, too +| -m, --magnitude <n> get a magnitude +| --set23 set integer to 23 +| -L, --length <str> get length of <str> +| -F, --file <file> set file to <file> +| +|String options +| -s, --string <string> +| get a string +| --string2 <str> get another string +| --st <st> get another string (pervert ordering) +| -o <str> get another string +| --list <str> add str to list +| +|Magic arguments +| --quux means --quux +| -NUM set integer to NUM +| + same as -b +| --ambiguous positive ambiguity +| --no-ambiguous negative ambiguity +| +|Standard options +| --abbrev[=<n>] use <n> digits to display SHA-1s +| -v, --verbose be verbose +| -n, --dry-run dry run +| -q, --quiet be quiet +| --expect <string> expected output in the variable dump +| +|Alias +| -A, --alias-source <string> +| get a string +| -Z, --alias-target <string> +| get a string +| EOF test_expect_success 'test help' '
On Tue, May 7, 2019 at 10:43 AM Junio C Hamano <gitster@pobox.com> wrote: > -- >8 -- > Subject: t0040: protect lines that are indented by spaces > > This block is byte-for-byte identical expected output, that contains a > few lines that are indented in many spaces, which makes "git diff --check" > unhappy and will break the test when "git am --whitespace=fix" is > allowed to "correct" them. > > Protect the left edge with a marker character, and strip it with > sed, which is used as a standard way to deal with this issue in our > test suite. > > Signed-off-by: Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > * Of course, if the right-edge need to be protected, we can do so > as well. > > t/t0040-parse-options.sh | 94 ++++++++++++++++++++++++------------------------ > 1 file changed, 47 insertions(+), 47 deletions(-) > > diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh > index cebc77fab0..26373b5b72 100755 > --- a/t/t0040-parse-options.sh > +++ b/t/t0040-parse-options.sh > @@ -7,53 +7,53 @@ test_description='our own option parser' > > . ./test-lib.sh > > -cat >expect <<\EOF > -usage: test-tool parse-options <options> > - > - A helper function for the parse-options API. > - > - --yes get a boolean > - -D, --no-doubt begins with 'no-' > - -B, --no-fear be brave > - -b, --boolean increment by one > - -4, --or4 bitwise-or boolean with ...0100 > - --neg-or4 same as --no-or4 > - > - -i, --integer <n> get a integer > - -j <n> get a integer, too > - -m, --magnitude <n> get a magnitude > - --set23 set integer to 23 > - -L, --length <str> get length of <str> > - -F, --file <file> set file to <file> > - > -String options > - -s, --string <string> > - get a string > - --string2 <str> get another string > - --st <st> get another string (pervert ordering) > - -o <str> get another string > - --list <str> add str to list > - > -Magic arguments > - --quux means --quux > - -NUM set integer to NUM > - + same as -b > - --ambiguous positive ambiguity > - --no-ambiguous negative ambiguity > - > -Standard options > - --abbrev[=<n>] use <n> digits to display SHA-1s > - -v, --verbose be verbose > - -n, --dry-run dry run > - -q, --quiet be quiet > - --expect <string> expected output in the variable dump > - > -Alias > - -A, --alias-source <string> > - get a string > - -Z, --alias-target <string> > - get a string > - > +sed -e 's/^|//' >expect <<\EOF I think we already use qz_to_tab_space to protect leading spaces (t1450) or trailing ones (t4205). It's less strict than this though. Anyway, if you go with 's/^|//', maybe make it a helper function too because I'm pretty sure we have more text like this in the test suite. There's also the "tr -d Q" trick in t4038. But that's something to clean up if someone really have free time. > +|usage: test-tool parse-options <options> > +| > +| A helper function for the parse-options API. > +| > +| --yes get a boolean > +| -D, --no-doubt begins with 'no-' > +| -B, --no-fear be brave > +| -b, --boolean increment by one > +| -4, --or4 bitwise-or boolean with ...0100 > +| --neg-or4 same as --no-or4 > +| > +| -i, --integer <n> get a integer > +| -j <n> get a integer, too > +| -m, --magnitude <n> get a magnitude > +| --set23 set integer to 23 > +| -L, --length <str> get length of <str> > +| -F, --file <file> set file to <file> > +| > +|String options > +| -s, --string <string> > +| get a string > +| --string2 <str> get another string > +| --st <st> get another string (pervert ordering) > +| -o <str> get another string > +| --list <str> add str to list > +| > +|Magic arguments > +| --quux means --quux > +| -NUM set integer to NUM > +| + same as -b > +| --ambiguous positive ambiguity > +| --no-ambiguous negative ambiguity > +| > +|Standard options > +| --abbrev[=<n>] use <n> digits to display SHA-1s > +| -v, --verbose be verbose > +| -n, --dry-run dry run > +| -q, --quiet be quiet > +| --expect <string> expected output in the variable dump > +| > +|Alias > +| -A, --alias-source <string> > +| get a string > +| -Z, --alias-target <string> > +| get a string > +| > EOF > > test_expect_success 'test help' '
diff --git a/builtin/clone.c b/builtin/clone.c index 50bde99618..703b7247ad 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -98,10 +98,7 @@ static struct option builtin_clone_options[] = { N_("don't use local hardlinks, always copy")), OPT_BOOL('s', "shared", &option_shared, N_("setup as shared repository")), - { OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules, - N_("pathspec"), N_("initialize submodules in the clone"), - PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb, - (intptr_t)"." }, + OPT_ALIAS(0, "recursive", "recurse-submodules"), { OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules, N_("pathspec"), N_("initialize submodules in the clone"), PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." }, diff --git a/parse-options.c b/parse-options.c index acc3a93660..1b1cc2add7 100644 --- a/parse-options.c +++ b/parse-options.c @@ -261,6 +261,35 @@ static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p, return PARSE_OPT_UNKNOWN; } +static int has_string(const char *it, const char **array) +{ + while (*array) + if (!strcmp(it, *(array++))) + return 1; + return 0; +} + +static int is_alias(struct parse_opt_ctx_t *ctx, + const struct option *one_opt, + const struct option *another_opt) +{ + const char **group; + + if (!ctx->alias_groups) + return 0; + + if (!one_opt->long_name || !another_opt->long_name) + return 0; + + for (group = ctx->alias_groups; *group; group += 3) { + /* it and other are from the same family? */ + if (has_string(one_opt->long_name, group) && + has_string(another_opt->long_name, group)) + return 1; + } + return 0; +} + static enum parse_opt_result parse_long_opt( struct parse_opt_ctx_t *p, const char *arg, const struct option *options) @@ -296,7 +325,8 @@ static enum parse_opt_result parse_long_opt( if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) && !strncmp(long_name, arg, arg_end - arg)) { is_abbreviated: - if (abbrev_option) { + if (abbrev_option && + !is_alias(p, abbrev_option, options)) { /* * If this is abbreviated, it is * ambiguous. So when there is no @@ -445,6 +475,10 @@ static void parse_options_check(const struct option *opts) if (opts->callback) BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback"); break; + case OPTION_ALIAS: + BUG("OPT_ALIAS() should not remain at this point. " + "Are you using parse_options_step() directly?\n" + "That case is not supported yet."); default: ; /* ok. (usually accepts an argument) */ } @@ -456,11 +490,10 @@ static void parse_options_check(const struct option *opts) exit(128); } -void parse_options_start(struct parse_opt_ctx_t *ctx, - int argc, const char **argv, const char *prefix, - const struct option *options, int flags) +static void parse_options_start_1(struct parse_opt_ctx_t *ctx, + int argc, const char **argv, const char *prefix, + const struct option *options, int flags) { - memset(ctx, 0, sizeof(*ctx)); ctx->argc = argc; ctx->argv = argv; if (!(flags & PARSE_OPT_ONE_SHOT)) { @@ -482,6 +515,14 @@ void parse_options_start(struct parse_opt_ctx_t *ctx, parse_options_check(options); } +void parse_options_start(struct parse_opt_ctx_t *ctx, + int argc, const char **argv, const char *prefix, + const struct option *options, int flags) +{ + memset(ctx, 0, sizeof(*ctx)); + parse_options_start_1(ctx, argc, argv, prefix, options, flags); +} + static void show_negated_gitcomp(const struct option *opts, int nr_noopts) { int printed_dashdash = 0; @@ -574,6 +615,83 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx, return PARSE_OPT_COMPLETE; } +/* + * Scan and may produce a new option[] array, which should be used + * instead of the original 'options'. + * + * Right now this is only used to preprocess and substitue + * OPTION_ALIAS. + */ +static struct option *preprocess_options(struct parse_opt_ctx_t *ctx, + const struct option *options) +{ + struct option *newopt; + int i, nr, alias; + int nr_aliases = 0; + + for (nr = 0; options[nr].type != OPTION_END; nr++) { + if (options[nr].type == OPTION_ALIAS) + nr_aliases++; + } + + if (!nr_aliases) + return NULL; + + ALLOC_ARRAY(newopt, nr + 1); + COPY_ARRAY(newopt, options, nr + 1); + + /* each alias has two string pointers and NULL */ + CALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1)); + + for (alias = 0, i = 0; i < nr; i++) { + int short_name; + const char *long_name; + const char *source; + int j; + + if (newopt[i].type != OPTION_ALIAS) + continue; + + short_name = newopt[i].short_name; + long_name = newopt[i].long_name; + source = newopt[i].value; + + if (!long_name) + BUG("An alias must have long option name"); + + for (j = 0; j < nr; j++) { + const char *name = options[j].long_name; + + if (!name || strcmp(name, source)) + continue; + + if (options[j].type == OPTION_ALIAS) + BUG("No please. Nested aliases are not supported."); + + /* + * NEEDSWORK: this is a bit inconsistent because + * usage_with_options() on the original options[] will print + * help string as "alias of %s" but "git cmd -h" will + * print the original help string. + */ + memcpy(newopt + i, options + j, sizeof(*newopt)); + newopt[i].short_name = short_name; + newopt[i].long_name = long_name; + break; + } + + if (j == nr) + BUG("could not find source option '%s' of alias '%s'", + source, newopt[i].long_name); + ctx->alias_groups[alias * 3 + 0] = newopt[i].long_name; + ctx->alias_groups[alias * 3 + 1] = options[j].long_name; + ctx->alias_groups[alias * 3 + 2] = NULL; + alias++; + } + + return newopt; +} + static int usage_with_options_internal(struct parse_opt_ctx_t *, const char * const *, const struct option *, int, int); @@ -713,11 +831,16 @@ int parse_options(int argc, const char **argv, const char *prefix, int flags) { struct parse_opt_ctx_t ctx; + struct option *real_options; disallow_abbreviated_options = git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0); - parse_options_start(&ctx, argc, argv, prefix, options, flags); + memset(&ctx, 0, sizeof(ctx)); + real_options = preprocess_options(&ctx, options); + if (real_options) + options = real_options; + parse_options_start_1(&ctx, argc, argv, prefix, options, flags); switch (parse_options_step(&ctx, options, usagestr)) { case PARSE_OPT_HELP: case PARSE_OPT_ERROR: @@ -740,6 +863,8 @@ int parse_options(int argc, const char **argv, const char *prefix, } precompose_argv(argc, argv); + free(real_options); + free(ctx.alias_groups); return parse_options_end(&ctx); } @@ -834,6 +959,12 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx, fputc('\n', outfile); pad = USAGE_OPTS_WIDTH; } + if (opts->type == OPTION_ALIAS) { + fprintf(outfile, "%*s", pad + USAGE_GAP, ""); + fprintf_ln(outfile, _("alias of --%s"), + (const char *)opts->value); + continue; + } fprintf(outfile, "%*s%s\n", pad + USAGE_GAP, "", _(opts->help)); } fputc('\n', outfile); diff --git a/parse-options.h b/parse-options.h index 74cce4e7fc..9ed479f41d 100644 --- a/parse-options.h +++ b/parse-options.h @@ -7,6 +7,7 @@ enum parse_opt_type { OPTION_ARGUMENT, OPTION_GROUP, OPTION_NUMBER, + OPTION_ALIAS, /* options with no arguments */ OPTION_BIT, OPTION_NEGBIT, @@ -181,6 +182,9 @@ struct option { N_("no-op (backward compatibility)"), \ PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb } +#define OPT_ALIAS(s, l, source_long_name) \ + { OPTION_ALIAS, (s), (l), (source_long_name) } + /* * parse_options() will filter out the processed options and leave the * non-option arguments in argv[]. argv0 is assumed program name and @@ -256,6 +260,8 @@ struct parse_opt_ctx_t { const char *opt; int flags; const char *prefix; + const char **alias_groups; /* must be in groups of 3 elements! */ + struct option *updated_options; }; void parse_options_start(struct parse_opt_ctx_t *ctx, diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index cc88fba057..76cfb87588 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -149,6 +149,9 @@ int cmd__parse_options(int argc, const char **argv) OPT_CALLBACK(0, "expect", &expect, "string", "expected output in the variable dump", collect_expect), + OPT_GROUP("Alias"), + OPT_STRING('A', "alias-source", &string, "string", "get a string"), + OPT_ALIAS('Z', "alias-target", "alias-source"), OPT_END(), }; int i; diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 800b3ea5f5..cebc77fab0 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -48,6 +48,12 @@ Standard options -q, --quiet be quiet --expect <string> expected output in the variable dump +Alias + -A, --alias-source <string> + get a string + -Z, --alias-target <string> + get a string + EOF test_expect_success 'test help' ' @@ -224,6 +230,17 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' ' test-tool parse-options --expect="string: 123" --st 123 ' +test_expect_success 'Alias options do not contribute to abbreviation' ' + test-tool parse-options --alias-source 123 >output && + grep "^string: 123" output && + test-tool parse-options --alias-target 123 >output && + grep "^string: 123" output && + test_must_fail test-tool parse-options --alias && + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + test-tool parse-options --alias 123 >output && + grep "^string: 123" output +' + cat >typo.err <<\EOF error: did you mean `--boolean` (with two dashes ?) EOF
Change the option parsing machinery so that e.g. "clone --recurs ..." doesn't error out because "clone" understands both "--recursive" and "--recurse-submodules" to mean the same thing. Initially "clone" just understood --recursive until the --recurses-submodules alias was added in ccdd3da652 ("clone: Add the --recurse-submodules option as alias for --recursive", 2010-11-04). Since bb62e0a99f ("clone: teach --recurse-submodules to optionally take a pathspec", 2017-03-17) the longer form has been promoted to the default. But due to the way the options parsing machinery works this resulted in the rather absurd situation of: $ git clone --recurs [...] error: ambiguous option: recurs (could be --recursive or --recurse-submodules) Add OPT_ALIAS() to express this link between two or more options and use it in git-clone. Multiple aliases of an option could be written as OPT_ALIAS(0, "alias1", "original-name"), OPT_ALIAS(0, "alias2", "original-name"), ... The current implementation is not exactly optimal in this case. But we can optimize it when it becomes a problem. So far we don't even have two aliases of any option. A big chunk of code is actually from Junio C Hamano. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- OK it's working for real this time. test-parse-options.c is also updated to for testing OPT_ALIAS. builtin/clone.c | 5 +- parse-options.c | 143 ++++++++++++++++++++++++++++++++-- parse-options.h | 6 ++ t/helper/test-parse-options.c | 3 + t/t0040-parse-options.sh | 17 ++++ 5 files changed, 164 insertions(+), 10 deletions(-)