{"thread":{"id":"56912","subject":"[PATCH] parse-options API: simplify OPT_ALIAS()","startedAt":"2021-11-15T22:52:44Z","lastAt":"2021-11-15T22:52:44Z","messageCount":1,"participants":["Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"441170","messageId":"patch-1.1-b09bf423694-20211115T220334Z-avarab@gmail.com","threadId":"56912","inReplyTo":null,"subject":"[PATCH] parse-options API: simplify OPT_ALIAS()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-15T22:03:43Z","receivedAt":"2021-11-15T22:52:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove OPT_ALIAS() while satisfying the use-case that brought it into\nexistence. This is mostly a code removal, but also a code\nsimplification.\n\nThe reason we've got OPT_ALIAS is detailed in\n5c387428f10 (parse-options: don't emit \"ambiguous option\" for aliases,\n2019-04-29), i.e. wanting to have \"git clone\" understand the\nincomplete \"--recurs\" form without complaining that \"--recursive\" and\n\"--recurse-submodules\" are ambiguous.\n\nOnly the \"clone\" caller cared about that use-case, the other four,\nincluding two recently added by me in 98e2d9d6f7d (upload-pack:\ndocument and rename --advertise-refs, 2021-08-05), only cared about\nthe small amount of typing saved by re-declaring the relevant OPT_*()\nargument again with mostly the same parameters.\n\nFor the more verbose caller in \"clone\" let's provide a macro inline to\nclearly show what is and isn't the same, and for the rest duplicate\nthe definition inline. The description of the aliased options was\n\"alias of --%s\", which is being carried forward here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/clone.c               |  12 ++--\n builtin/log.c                 |   2 +-\n builtin/receive-pack.c        |   2 +-\n builtin/upload-pack.c         |   2 +-\n parse-options.c               | 111 +---------------------------------\n parse-options.h               |   6 +-\n t/helper/test-parse-options.c |   3 +-\n 7 files changed, 17 insertions(+), 121 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex fb377b27657..1234ee6e1d9 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -107,10 +107,14 @@ static struct option builtin_clone_options[] = {\n \t\t    N_(\"don't use local hardlinks, always copy\")),\n \tOPT_BOOL('s', \"shared\", &option_shared,\n \t\t    N_(\"setup as shared repository\")),\n-\t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &option_recurse_submodules,\n-\t  N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n-\t  PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)\".\" },\n-\tOPT_ALIAS(0, \"recursive\", \"recurse-submodules\"),\n+#define OPT_RECURSE_F(l, d, f) \\\n+\t{ OPTION_CALLBACK, 0, (l), &option_recurse_submodules, \\\n+\t  N_(\"pathspec\"), (d), \\\n+\t  PARSE_OPT_OPTARG | (f), recurse_submodules_cb, (intptr_t)\".\" }\n+\tOPT_RECURSE_F(\"recurse-submodules\",\n+\t\t      N_(\"initialize submodules in the clone\"), 0),\n+\tOPT_RECURSE_F(\"recursive\", N_(\"alias of --recurse-submodules\"),\n+\t\t      PARSE_OPT_AMBIGUOUS_ALIAS),\n \tOPT_INTEGER('j', \"jobs\", &max_jobs,\n \t\t    N_(\"number of submodules cloned in parallel\")),\n \tOPT_STRING(0, \"template\", &option_template, N_(\"template-directory\"),\ndiff --git a/builtin/log.c b/builtin/log.c\nindex f75d87e8d7f..4dfdc6e1671 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -179,7 +179,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\tOPT__QUIET(&quiet, N_(\"suppress diff output\")),\n \t\tOPT_BOOL(0, \"source\", &source, N_(\"show source\")),\n \t\tOPT_BOOL(0, \"use-mailmap\", &mailmap, N_(\"use mail map file\")),\n-\t\tOPT_ALIAS(0, \"mailmap\", \"use-mailmap\"),\n+\t\tOPT_BOOL(0, \"mailmap\", &mailmap, N_(\"alias of --use-mailmap\")),\n \t\tOPT_STRING_LIST(0, \"decorate-refs\", &decorate_refs_include,\n \t\t\t\tN_(\"pattern\"), N_(\"only decorate refs that match <pattern>\")),\n \t\tOPT_STRING_LIST(0, \"decorate-refs-exclude\", &decorate_refs_exclude,\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 49b846d9605..e4c825af1a9 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2480,7 +2480,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tOPT__QUIET(&quiet, N_(\"quiet\")),\n \t\tOPT_HIDDEN_BOOL(0, \"stateless-rpc\", &stateless_rpc, NULL),\n \t\tOPT_HIDDEN_BOOL(0, \"http-backend-info-refs\", &advertise_refs, NULL),\n-\t\tOPT_ALIAS(0, \"advertise-refs\", \"http-backend-info-refs\"),\n+\t\tOPT_HIDDEN_BOOL(0, \"advertise-refs\", &advertise_refs, NULL),\n \t\tOPT_HIDDEN_BOOL(0, \"reject-thin-pack-for-testing\", &reject_thin, NULL),\n \t\tOPT_END()\n \t};\ndiff --git a/builtin/upload-pack.c b/builtin/upload-pack.c\nindex 125af53885f..5ab20fcb1c9 100644\n--- a/builtin/upload-pack.c\n+++ b/builtin/upload-pack.c\n@@ -24,7 +24,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"quit after a single request/response exchange\")),\n \t\tOPT_HIDDEN_BOOL(0, \"http-backend-info-refs\", &advertise_refs,\n \t\t\t\tN_(\"serve up the info/refs for git-http-backend\")),\n-\t\tOPT_ALIAS(0, \"advertise-refs\", \"http-backend-info-refs\"),\n+\t\tOPT_HIDDEN_BOOL(0, \"advertise-refs\", &advertise_refs, NULL),\n \t\tOPT_BOOL(0, \"strict\", &strict,\n \t\t\t N_(\"do not try <directory>/.git/ if <directory> is no Git directory\")),\n \t\tOPT_INTEGER(0, \"timeout\", &timeout,\ndiff --git a/parse-options.c b/parse-options.c\nindex fc5b43ff0b2..0b5c71e923b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -389,7 +389,8 @@ static enum parse_opt_result parse_long_opt(\n \t\tdie(\"disallowed abbreviated or ambiguous option '%.*s'\",\n \t\t    (int)(arg_end - arg), arg);\n \n-\tif (ambiguous_option) {\n+\tif (ambiguous_option &&\n+\t    !(abbrev_option->flags & PARSE_OPT_AMBIGUOUS_ALIAS)) {\n \t\terror(_(\"ambiguous option: %s \"\n \t\t\t\"(could be --%s%s or --%s%s)\"),\n \t\t\targ,\n@@ -484,10 +485,6 @@ static void parse_options_check(const struct option *opts)\n \t\t\tif (opts->callback)\n \t\t\t\tBUG(\"OPTION_LOWLEVEL_CALLBACK needs no high level callback\");\n \t\t\tbreak;\n-\t\tcase OPTION_ALIAS:\n-\t\t\tBUG(\"OPT_ALIAS() should not remain at this point. \"\n-\t\t\t    \"Are you using parse_options_step() directly?\\n\"\n-\t\t\t    \"That case is not supported yet.\");\n \t\tdefault:\n \t\t\t; /* ok. (usually accepts an argument) */\n \t\t}\n@@ -594,7 +591,7 @@ static int show_gitcomp(const struct option *opts, int show_all)\n \t\tif (!opts->long_name)\n \t\t\tcontinue;\n \t\tif (!show_all &&\n-\t\t\t(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE | PARSE_OPT_FROM_ALIAS)))\n+\t\t\t(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE | PARSE_OPT_AMBIGUOUS_ALIAS)))\n \t\t\tcontinue;\n \n \t\tswitch (opts->type) {\n@@ -628,97 +625,6 @@ static int show_gitcomp(const struct option *opts, int show_all)\n \treturn PARSE_OPT_COMPLETE;\n }\n \n-/*\n- * Scan and may produce a new option[] array, which should be used\n- * instead of the original 'options'.\n- *\n- * Right now this is only used to preprocess and substitute\n- * OPTION_ALIAS.\n- *\n- * The returned options should be freed using free_preprocessed_options.\n- */\n-static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,\n-\t\t\t\t\t const struct option *options)\n-{\n-\tstruct option *newopt;\n-\tint i, nr, alias;\n-\tint nr_aliases = 0;\n-\n-\tfor (nr = 0; options[nr].type != OPTION_END; nr++) {\n-\t\tif (options[nr].type == OPTION_ALIAS)\n-\t\t\tnr_aliases++;\n-\t}\n-\n-\tif (!nr_aliases)\n-\t\treturn NULL;\n-\n-\tALLOC_ARRAY(newopt, nr + 1);\n-\tCOPY_ARRAY(newopt, options, nr + 1);\n-\n-\t/* each alias has two string pointers and NULL */\n-\tCALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1));\n-\n-\tfor (alias = 0, i = 0; i < nr; i++) {\n-\t\tint short_name;\n-\t\tconst char *long_name;\n-\t\tconst char *source;\n-\t\tstruct strbuf help = STRBUF_INIT;\n-\t\tint j;\n-\n-\t\tif (newopt[i].type != OPTION_ALIAS)\n-\t\t\tcontinue;\n-\n-\t\tshort_name = newopt[i].short_name;\n-\t\tlong_name = newopt[i].long_name;\n-\t\tsource = newopt[i].value;\n-\n-\t\tif (!long_name)\n-\t\t\tBUG(\"An alias must have long option name\");\n-\t\tstrbuf_addf(&help, _(\"alias of --%s\"), source);\n-\n-\t\tfor (j = 0; j < nr; j++) {\n-\t\t\tconst char *name = options[j].long_name;\n-\n-\t\t\tif (!name || strcmp(name, source))\n-\t\t\t\tcontinue;\n-\n-\t\t\tif (options[j].type == OPTION_ALIAS)\n-\t\t\t\tBUG(\"No please. Nested aliases are not supported.\");\n-\n-\t\t\tmemcpy(newopt + i, options + j, sizeof(*newopt));\n-\t\t\tnewopt[i].short_name = short_name;\n-\t\t\tnewopt[i].long_name = long_name;\n-\t\t\tnewopt[i].help = strbuf_detach(&help, NULL);\n-\t\t\tnewopt[i].flags |= PARSE_OPT_FROM_ALIAS;\n-\t\t\tbreak;\n-\t\t}\n-\n-\t\tif (j == nr)\n-\t\t\tBUG(\"could not find source option '%s' of alias '%s'\",\n-\t\t\t    source, newopt[i].long_name);\n-\t\tctx->alias_groups[alias * 3 + 0] = newopt[i].long_name;\n-\t\tctx->alias_groups[alias * 3 + 1] = options[j].long_name;\n-\t\tctx->alias_groups[alias * 3 + 2] = NULL;\n-\t\talias++;\n-\t}\n-\n-\treturn newopt;\n-}\n-\n-static void free_preprocessed_options(struct option *options)\n-{\n-\tint i;\n-\n-\tif (!options)\n-\t\treturn;\n-\n-\tfor (i = 0; options[i].type != OPTION_END; i++) {\n-\t\tif (options[i].flags & PARSE_OPT_FROM_ALIAS)\n-\t\t\tfree((void *)options[i].help);\n-\t}\n-\tfree(options);\n-}\n-\n static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n \t\t\t\t\t\t\t const char * const *,\n \t\t\t\t\t\t\t const struct option *,\n@@ -867,15 +773,11 @@ int parse_options(int argc, const char **argv,\n \t\t  enum parse_opt_flags flags)\n {\n \tstruct parse_opt_ctx_t ctx;\n-\tstruct option *real_options;\n \n \tdisallow_abbreviated_options =\n \t\tgit_env_bool(\"GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\", 0);\n \n \tmemset(&ctx, 0, sizeof(ctx));\n-\treal_options = preprocess_options(&ctx, options);\n-\tif (real_options)\n-\t\toptions = real_options;\n \tparse_options_start_1(&ctx, argc, argv, prefix, options, flags);\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n@@ -899,7 +801,6 @@ int parse_options(int argc, const char **argv,\n \t}\n \n \tprecompose_argv_prefix(argc, argv, NULL);\n-\tfree_preprocessed_options(real_options);\n \tfree(ctx.alias_groups);\n \treturn parse_options_end(&ctx);\n }\n@@ -1048,12 +949,6 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \t\t\tfputc('\\n', outfile);\n \t\t\tpad = USAGE_OPTS_WIDTH;\n \t\t}\n-\t\tif (opts->type == OPTION_ALIAS) {\n-\t\t\tfprintf(outfile, \"%*s\", pad + USAGE_GAP, \"\");\n-\t\t\tfprintf_ln(outfile, _(\"alias of --%s\"),\n-\t\t\t\t   (const char *)opts->value);\n-\t\t\tcontinue;\n-\t\t}\n \t\tfprintf(outfile, \"%*s%s\\n\", pad + USAGE_GAP, \"\", _(opts->help));\n \t}\n \tfputc('\\n', outfile);\ndiff --git a/parse-options.h b/parse-options.h\nindex 275fb440818..3ad0c7dbafd 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -10,7 +10,6 @@ enum parse_opt_type {\n \tOPTION_END,\n \tOPTION_GROUP,\n \tOPTION_NUMBER,\n-\tOPTION_ALIAS,\n \t/* options with no arguments */\n \tOPTION_BIT,\n \tOPTION_NEGBIT,\n@@ -44,7 +43,7 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_LASTARG_DEFAULT = 1 << 4,\n \tPARSE_OPT_NODASH = 1 << 5,\n \tPARSE_OPT_LITERAL_ARGHELP = 1 << 6,\n-\tPARSE_OPT_FROM_ALIAS = 1 << 7,\n+\tPARSE_OPT_AMBIGUOUS_ALIAS = 1 << 7,\n \tPARSE_OPT_NOCOMPLETE = 1 << 9,\n \tPARSE_OPT_COMP_ARG = 1 << 10,\n \tPARSE_OPT_CMDMODE = 1 << 11,\n@@ -197,9 +196,6 @@ struct option {\n \t  N_(\"no-op (backward compatibility)\"),\t\t\\\n \t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb }\n \n-#define OPT_ALIAS(s, l, source_long_name) \\\n-\t{ OPTION_ALIAS, (s), (l), (source_long_name) }\n-\n /*\n  * parse_options() will filter out the processed options and leave the\n  * non-option arguments in argv[]. argv0 is assumed program name and\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex 48d3cf6692d..e0a3d9e8f51 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -153,7 +153,8 @@ int cmd__parse_options(int argc, const char **argv)\n \t\t\t     collect_expect),\n \t\tOPT_GROUP(\"Alias\"),\n \t\tOPT_STRING('A', \"alias-source\", &string, \"string\", \"get a string\"),\n-\t\tOPT_ALIAS('Z', \"alias-target\", \"alias-source\"),\n+\t\t{ OPTION_STRING, 'Z', \"alias-target\", &string, \"string\",\n+\t\t  \"alias of --alias-source\", PARSE_OPT_AMBIGUOUS_ALIAS },\n \t\tOPT_END(),\n \t};\n \tint i;\n-- \n2.34.0.rc2.809.g11e21d44b24\n\n"}]}