| Message ID | 20190417124438.8191-1-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] parse-options: don't emit "ambiguous option" for aliases | expand |
On Wed, Apr 17, 2019 at 7:44 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > 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) > > Let's re-use the PARSE_OPT_NOCOMPLETE flag to mean "this option > doesn't contribute to abbreviation ambiguity". I was going to add a > new PARSE_OPT_NOABBREV flag, but it makes sense just to re-use > PARSE_OPT_NOCOMPLETE. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > > See > https://public-inbox.org/git/20190325202329.26033-1-avarab@gmail.com/ > for v1. There wasn't consensus for 1/2 there, but this used-to-be 2/2 > seems like a no-brainer bugfix. > > It conflicted with some recently-landed stuff in 'master', but now > cleanly applies to it and 'pu', and with pu's > GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS. > > builtin/clone.c | 4 ++-- > parse-options.c | 3 ++- > parse-options.h | 2 ++ > t/t0040-parse-options.sh | 5 +++++ > 4 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/builtin/clone.c b/builtin/clone.c > index 50bde99618..4dc26969a7 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = { > 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)"." }, > + PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE, What happens if someone adds --recursive-hard? --recursi then resolving to --recursive-hard sounds wrong. But on the other hand I can see it's a bit more work to teach parse-options OPT_ALIAS to say "--recursive is just an alias of --recurse-submodules" and chances of --recursive-hard coming up are probably very low. So I don't know but I thought I should point out. > + recurse_submodules_cb, (intptr_t)"." }, > { 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 cec74522e5..9899ce0171 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -292,7 +292,8 @@ static enum parse_opt_result parse_long_opt( > if (!rest) { > /* abbreviated? */ > if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) && > - !strncmp(long_name, arg, arg_end - arg)) { > + !strncmp(long_name, arg, arg_end - arg) && > + !(options->flags & PARSE_OPT_NOCOMPLETE)) { > is_abbreviated: > if (abbrev_option) { > /* > diff --git a/parse-options.h b/parse-options.h > index 74cce4e7fc..51c4b71ab0 100644 > --- a/parse-options.h > +++ b/parse-options.h > @@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx, > * Useful for options with multiple parameters. > * PARSE_OPT_NOCOMPLETE: by default all visible options are completable > * by git-completion.bash. This option suppresses that. > + * Will also skip this option when abbreviation is > + * considered. > * PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to > * complete an option as --name= not --name even if > * the option takes optional argument. > diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh > index b8f366c442..e8f0371830 100755 > --- a/t/t0040-parse-options.sh > +++ b/t/t0040-parse-options.sh > @@ -220,6 +220,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' ' > test-tool parse-options --expect="string: 123" --st 123 > ' > > +test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' ' > + test_when_finished "rm -rf A" && > + git clone --recurs . A > +' > + > cat >typo.err <<\EOF > error: did you mean `--boolean` (with two dashes ?) > EOF > -- > 2.21.0.593.g511ec345e18 >
Duy Nguyen <pclouds@gmail.com> writes: >> { 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)"." }, >> + PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE, > > What happens if someone adds --recursive-hard? --recursi then > resolving to --recursive-hard sounds wrong. That was exactly my initial reaction. Or "recurse-submodules" gets renamed away, and "recommend" gets added---now "--rec" is still not ambiguous as "recursive" is marked not to participate in the disambiguation (I think OPT_NOCOMPLETE should at least be renamed to OPT_NO_DISAMBIGUATION or something---if we were to use it for the purpose of marking an option as "not participating in disambiguation", but I am fairly negative on the approach). And my initial reaction was followed by "don't we want a more explicit link only between recursive and recurse-submodules?", which exactly matches what you said below ;-) > But on the other hand I can see it's a bit more work to teach > parse-options OPT_ALIAS to say "--recursive is just an alias of > --recurse-submodules" and chances of --recursive-hard coming up are > probably very low. The "bit more work" is something that is worth doing in this case, I think. Thanks.
On Thu, Apr 18, 2019 at 09:48:39AM +0900, Junio C Hamano wrote: > > But on the other hand I can see it's a bit more work to teach > > parse-options OPT_ALIAS to say "--recursive is just an alias of > > --recurse-submodules" and chances of --recursive-hard coming up are > > probably very low. > > The "bit more work" is something that is worth doing in this case, I > think. I had a look at it. Linking two options together is not exactly easy because the way we organize option data. And I also had difficulty defining the exact semantics of this OPT_ALIAS. So an alternative is simply outsource the ambiguity decision back to git-clone. If the same situation appears again elsewhere, we'll need to sit back and fix it for real. But this way we don't potentially introduce any new traps. parse_options_extended() also makes it easier to extend parseopt functionality without changing all call sites. One thing I have in mind, which could maybe use this interface, is to remove parse_options_{start,step,end} from public API, avoid expose the parse_options_step internal result code. The caller just need to pass a callback to handle unknown options. -- 8< -- diff --git a/builtin/clone.c b/builtin/clone.c index 50bde99618..041cd43ddc 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -145,6 +145,29 @@ static struct option builtin_clone_options[] = { OPT_END() }; +/* + * Avoid ambiguation error between --recursive and --recurse-submodule + * because they are the same. --recurs can be expanded to any of them + * and it still works. + */ +static int is_abbrev_ambiguous(const struct option *prev, + const struct option *next) +{ + const struct option *opts[] = { prev, next }; + int i, found = 0; + + for (i = 0; i < ARRAY_SIZE(opts); i++) { + if (!opts[i]->long_name) + continue; + if (!strcmp(opts[i]->long_name, "recursive")) + found |= 1 << 0; + if (!strcmp(opts[i]->long_name, "recurse-submodules")) + found |= 1 << 1; + } + + return found != 3; +} + static const char *get_repo_path_1(struct strbuf *path, int *is_bundle) { static char *suffix[] = { "/.git", "", ".git/.git", ".git" }; @@ -905,14 +928,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix) struct remote *remote; int err = 0, complete_refs_before_fetch = 1; int submodule_progress; + struct parseopt_options parseopts = { 0 }; struct argv_array ref_prefixes = ARGV_ARRAY_INIT; fetch_if_missing = 0; packet_trace_identity("clone"); - argc = parse_options(argc, argv, prefix, builtin_clone_options, - builtin_clone_usage, 0); + parseopts.usagestr = builtin_clone_usage; + parseopts.is_abbrev_ambiguous = is_abbrev_ambiguous; + argc = parse_options_extended(argc, argv, prefix, + builtin_clone_options, + &parseopts); if (argc > 2) usage_msg_opt(_("Too many arguments."), diff --git a/parse-options.c b/parse-options.c index cec74522e5..c0354e5a92 100644 --- a/parse-options.c +++ b/parse-options.c @@ -294,7 +294,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 && + p->is_abbrev_ambiguous(abbrev_option, options)) { /* * If this is abbreviated, it is * ambiguous. So when there is no @@ -450,6 +451,12 @@ static void parse_options_check(const struct option *opts) exit(128); } +static int abbrev_is_always_ambiguous(const struct option *prev, + const struct option *next) +{ + return 1; +} + void parse_options_start(struct parse_opt_ctx_t *ctx, int argc, const char **argv, const char *prefix, const struct option *options, int flags) @@ -466,6 +473,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx, ctx->prefix = prefix; ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0); ctx->flags = flags; + ctx->is_abbrev_ambiguous = abbrev_is_always_ambiguous; if ((flags & PARSE_OPT_KEEP_UNKNOWN) && (flags & PARSE_OPT_STOP_AT_NON_OPTION) && !(flags & PARSE_OPT_ONE_SHOT)) @@ -702,13 +710,17 @@ int parse_options_end(struct parse_opt_ctx_t *ctx) return ctx->cpidx + ctx->argc; } -int parse_options(int argc, const char **argv, const char *prefix, - const struct option *options, const char * const usagestr[], - int flags) +int parse_options_extended(int argc, const char **argv, const char *prefix, + const struct option *options, + const struct parseopt_options *parseopts) { struct parse_opt_ctx_t ctx; + const char * const * usagestr = parseopts->usagestr; + int flags = parseopts->flags; parse_options_start(&ctx, argc, argv, prefix, options, flags); + if (parseopts->is_abbrev_ambiguous) + ctx.is_abbrev_ambiguous = parseopts->is_abbrev_ambiguous; switch (parse_options_step(&ctx, options, usagestr)) { case PARSE_OPT_HELP: case PARSE_OPT_ERROR: @@ -734,6 +746,17 @@ int parse_options(int argc, const char **argv, const char *prefix, return parse_options_end(&ctx); } +int parse_options(int argc, const char **argv, const char *prefix, + const struct option *options, const char * const usagestr[], + int flags) +{ + struct parseopt_options parseopts = {0}; + + parseopts.flags = flags; + parseopts.usagestr = usagestr; + return parse_options_extended(argc, argv, prefix, options, &parseopts); +} + static int usage_argh(const struct option *opts, FILE *outfile) { const char *s; diff --git a/parse-options.h b/parse-options.h index 74cce4e7fc..31ebf4572c 100644 --- a/parse-options.h +++ b/parse-options.h @@ -198,6 +198,17 @@ int parse_options(int argc, const char **argv, const char *prefix, const struct option *options, const char * const usagestr[], int flags); +struct parseopt_options { + int flags; + const char * const * usagestr; + int (*is_abbrev_ambiguous)(const struct option *prev, + const struct option *next); +}; + +int parse_options_extended(int argc, const char **argv, const char *prefix, + const struct option *options, + const struct parseopt_options *parseopts); + NORETURN void usage_with_options(const char * const *usagestr, const struct option *options); @@ -256,6 +267,7 @@ struct parse_opt_ctx_t { const char *opt; int flags; const char *prefix; + int (*is_abbrev_ambiguous)(const struct option *, const struct option *); }; void parse_options_start(struct parse_opt_ctx_t *ctx, -- 8< -- > > Thanks.
Duy Nguyen <pclouds@gmail.com> writes: > So an alternative is simply outsource the ambiguity decision back to > git-clone. If the same situation appears again elsewhere, we'll need > to sit back and fix it for real. But this way we don't potentially > introduce any new traps. Sounds like a sensibly safe approach. > +/* > + * Avoid ambiguation error between --recursive and --recurse-submodule > + * because they are the same. --recurs can be expanded to any of them > + * and it still works. > + */ > +static int is_abbrev_ambiguous(const struct option *prev, > + const struct option *next) > +{ > + const struct option *opts[] = { prev, next }; By looking at its caller, I think the caller keeps track of the candidate it saw so far, and ask this function to see if the one it is looking at right now (i.e. "this one") should be flagged as conflicting with the other one (or "the other one"). So it probably makes more sense to call them <it, the_other> or <it, prev> [*1*]. Side note: *1* I would have used "this" instead of "it", but I vaguely recall there are those who want to use C++ aware static checkers and avoiding the identifier "this" is easy enough so ... > + int i, found = 0; > + > + for (i = 0; i < ARRAY_SIZE(opts); i++) { > + if (!opts[i]->long_name) > + continue; > + if (!strcmp(opts[i]->long_name, "recursive")) > + found |= 1 << 0; > + if (!strcmp(opts[i]->long_name, "recurse-submodules")) > + found |= 1 << 1; > + } > + > + return found != 3; > +} For any two options that share the prefix, unless they are "recursive" and "recurse-submodules" pair, we say "that's ambiguous". But when they are these two specifically singled out, we say it is OK. Makes sense. The above may be sufficient for this purpose, but I would have expected that these groups of aliases would be the ones in the table, not the <it, the_other> pair. IOW, with helpers like these: static const char *recurse_submodules[] = { "recurse-submodules", "recursive", NULL, }; static struct { const char **aliases; } disamb[] = { recurse_submodules, }; static int has_string(const char *it, const char **array) { while (*array) if (!strcmp(it, *(array++))) return 1; return 0; } the body would look something like: int j; for (j = 0; j < ARRAY_SIZE(disamb); j++) { /* it and other are from the same family? */ if (it->long_name && has_string(it->long_name, disamb[j].aliases) && other->long_name && has_string(other->long_name, disamb[j].aliases)) return 0; } return 1; Perhaps? I suspect that renaming the function (and the field in the context struct) to .is_alias() and reverse the polarity of its return value may make the flow of the logic at the caller side easier to follow? > diff --git a/parse-options.c b/parse-options.c > index cec74522e5..c0354e5a92 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -294,7 +294,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 && > + p->is_abbrev_ambiguous(abbrev_option, options)) { The above suggestion would make the guard here to if (abbrev_option && !p->is_alias(abbrev_option, options)) { That is, at this point, we know that the user may have used the given string to name the "abbrev_option" we earlier saw (and made sure it prefix matches the string), and now we are looking at another one, options[0], that also prefix matches the string. We usually flag this case as a problematic ambiguity inside the body of this if statement, but if one is an alias to the other, we do not do so. > /* > * If this is abbreviated, it is > * ambiguous. So when there is no
diff --git a/builtin/clone.c b/builtin/clone.c index 50bde99618..4dc26969a7 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = { 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)"." }, + PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE, + recurse_submodules_cb, (intptr_t)"." }, { 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 cec74522e5..9899ce0171 100644 --- a/parse-options.c +++ b/parse-options.c @@ -292,7 +292,8 @@ static enum parse_opt_result parse_long_opt( if (!rest) { /* abbreviated? */ if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) && - !strncmp(long_name, arg, arg_end - arg)) { + !strncmp(long_name, arg, arg_end - arg) && + !(options->flags & PARSE_OPT_NOCOMPLETE)) { is_abbreviated: if (abbrev_option) { /* diff --git a/parse-options.h b/parse-options.h index 74cce4e7fc..51c4b71ab0 100644 --- a/parse-options.h +++ b/parse-options.h @@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx, * Useful for options with multiple parameters. * PARSE_OPT_NOCOMPLETE: by default all visible options are completable * by git-completion.bash. This option suppresses that. + * Will also skip this option when abbreviation is + * considered. * PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to * complete an option as --name= not --name even if * the option takes optional argument. diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index b8f366c442..e8f0371830 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -220,6 +220,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' ' test-tool parse-options --expect="string: 123" --st 123 ' +test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' ' + test_when_finished "rm -rf A" && + git clone --recurs . A +' + 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) Let's re-use the PARSE_OPT_NOCOMPLETE flag to mean "this option doesn't contribute to abbreviation ambiguity". I was going to add a new PARSE_OPT_NOABBREV flag, but it makes sense just to re-use PARSE_OPT_NOCOMPLETE. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- See https://public-inbox.org/git/20190325202329.26033-1-avarab@gmail.com/ for v1. There wasn't consensus for 1/2 there, but this used-to-be 2/2 seems like a no-brainer bugfix. It conflicted with some recently-landed stuff in 'master', but now cleanly applies to it and 'pu', and with pu's GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS. builtin/clone.c | 4 ++-- parse-options.c | 3 ++- parse-options.h | 2 ++ t/t0040-parse-options.sh | 5 +++++ 4 files changed, 11 insertions(+), 3 deletions(-)