parse-options: don't emit "ambiguous option" for aliases

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/clone.c b/builtin/clone.c
index 50bde9961809b1d55c74fb6d3cbfbf5a86ddf5c7..703b7247ade1e58e493e8e7e910303977b22aa36 100644 (file)
--- 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")),
                    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)"." },
        { 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 cb24f1aa8ae980a366f6f700db15b68064e1f926..987e27cb91162ab03320ff9674f3ad52ffa829b4 100644 (file)
--- 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;
 }
 
        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)
 static enum parse_opt_result parse_long_opt(
        struct parse_opt_ctx_t *p, const char *arg,
        const struct option *options)


@@ -298,7 +327,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 (!(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
                                        /*
                                         * If this is abbreviated, it is
                                         * ambiguous. So when there is no


@@ -447,6 +477,10 @@ static void parse_options_check(const struct option *opts)
                        if (opts->callback)
                                BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
                        break;
                        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) */
                }
                default:
                        ; /* ok. (usually accepts an argument) */
                }


@@ -458,11 +492,10 @@ static void parse_options_check(const struct option *opts)
                exit(128);
 }
 
                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)) {
        ctx->argc = argc;
        ctx->argv = argv;
        if (!(flags & PARSE_OPT_ONE_SHOT)) {


@@ -484,6 +517,14 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
        parse_options_check(options);
 }
 
        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;
 static void show_negated_gitcomp(const struct option *opts, int nr_noopts)
 {
        int printed_dashdash = 0;


@@ -575,6 +616,83 @@ static int show_gitcomp(const struct option *opts)
        return PARSE_OPT_COMPLETE;
 }
 
        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);
 static int usage_with_options_internal(struct parse_opt_ctx_t *,
                                       const char * const *,
                                       const struct option *, int, int);


@@ -714,11 +832,16 @@ int parse_options(int argc, const char **argv, const char *prefix,
                  int flags)
 {
        struct parse_opt_ctx_t ctx;
                  int flags)
 {
        struct parse_opt_ctx_t ctx;

+       struct option *real_options;

 
        disallow_abbreviated_options =
                git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
 
 
        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:
        switch (parse_options_step(&ctx, options, usagestr)) {
        case PARSE_OPT_HELP:
        case PARSE_OPT_ERROR:


@@ -741,6 +864,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
        }
 
        precompose_argv(argc, argv);
        }
 
        precompose_argv(argc, argv);

+       free(real_options);
+       free(ctx.alias_groups);

        return parse_options_end(&ctx);
 }
 
        return parse_options_end(&ctx);
 }
 


@@ -835,6 +960,12 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
                        fputc('\n', outfile);
                        pad = USAGE_OPTS_WIDTH;
                }
                        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);
                fprintf(outfile, "%*s%s\n", pad + USAGE_GAP, "", _(opts->help));
        }
        fputc('\n', outfile);

diff --git a/parse-options.h b/parse-options.h
index cc9230adacb61f2a5066ac094c66bdfd18534354..bb49efc2da430ed933d8a2a1f6cdd732f7078ed4 100644 (file)
--- a/parse-options.h
+++ b/parse-options.h
@@ -7,6 +7,7 @@ enum parse_opt_type {
        OPTION_ARGUMENT,
        OPTION_GROUP,
        OPTION_NUMBER,
        OPTION_ARGUMENT,
        OPTION_GROUP,
        OPTION_NUMBER,

+       OPTION_ALIAS,

        /* options with no arguments */
        OPTION_BIT,
        OPTION_NEGBIT,
        /* options with no arguments */
        OPTION_BIT,
        OPTION_NEGBIT,


@@ -183,6 +184,9 @@ struct option {
          N_("no-op (backward compatibility)"),         \
          PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb }
 
          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
 /*
  * parse_options() will filter out the processed options and leave the
  * non-option arguments in argv[]. argv0 is assumed program name and


@@ -258,6 +262,8 @@ struct parse_opt_ctx_t {
        const char *opt;
        int flags;
        const char *prefix;
        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,
 };
 
 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 2232b2f79ecd7524b5763716fbca5a87bed06f5d..af82db06ac59c77c181351c76fff81ebe92f38c6 100644 (file)
--- 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_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;
                OPT_END(),
        };
        int i;

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 800b3ea5f5b64650efa95bfa05e8f90945966f85..cebc77fab0b254fc2e6f63e7eb68956b2b3dec9c 100755 (executable)
--- 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
 
     -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' '
 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-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
 cat >typo.err <<\EOF
 error: did you mean `--boolean` (with two dashes ?)
 EOF