{"thread":{"id":"34571","subject":"[PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times","startedAt":"2013-07-30T18:00:51Z","lastAt":"2013-08-18T09:27:11Z","messageCount":11,"participants":["Stefan Beller","Junio C Hamano","Stefano Lattarini","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"224308","messageId":"1375207251-4998-1-git-send-email-stefanbeller@googlemail.com","threadId":"34571","inReplyTo":null,"subject":"[PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-30T18:00:51Z","receivedAt":"2013-07-30T18:00:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As of b04ba2bb (parse-options: deprecate OPT_BOOLEAN, 2011-09-27),\nthe OPT_BOOLEAN was deprecated.\nWhile I am going to replace the OPT_BOOLEAN by the proposed OPT_BOOL or\nthe OPT_COUNTUP to keep existing behavior, this commit is actually a\nbug fix!\n\nIn line 499 we have:\n\tif (list + delete + verify > 1)\n\t\tusage_with_options(git_tag_usage, options);\nNow if we give one of the options twice, we'll get the usage information.\n(i.e. 'git tag --verify --verify <tagname>' and\n'git --delete --delete <tagname>' yield usage information and do not\ndo the intended command.)\n\nThis could have been fixed by rewriting the line to\n\tif (!!list + !!delete + !!verify > 1)\n\t\tusage_with_options(git_tag_usage, options);\nor as it happened in this patch by having the parameters not\ncounting up for each occurrence, but the OPT_BOOL just setting the\nvariables to either 0 if the option is not given or 1 if the option is\ngiven multiple times.\n\nHowever we could discuss if the negated options do make sense here, or if\nwe don't want to allow them here, as this seems valid (before and after\nthis patch):\n\tgit tag --no-verify --delete <tagname>\n\nSigned-off-by: Stefan Beller <stefanbeller@googlemail.com>\n---\n builtin/tag.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex b3942e4..d155c9d 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -442,12 +442,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstruct msg_arg msg = { 0, STRBUF_INIT };\n \tstruct commit_list *with_commit = NULL;\n \tstruct option options[] = {\n-\t\tOPT_BOOLEAN('l', \"list\", &list, N_(\"list tag names\")),\n+\t\tOPT_BOOL('l', \"list\", &list, N_(\"list tag names\")),\n \t\t{ OPTION_INTEGER, 'n', NULL, &lines, N_(\"n\"),\n \t\t\t\tN_(\"print <n> lines of each tag message\"),\n \t\t\t\tPARSE_OPT_OPTARG, NULL, 1 },\n-\t\tOPT_BOOLEAN('d', \"delete\", &delete, N_(\"delete tags\")),\n-\t\tOPT_BOOLEAN('v', \"verify\", &verify, N_(\"verify tags\")),\n+\t\tOPT_BOOL('d', \"delete\", &delete, N_(\"delete tags\")),\n+\t\tOPT_BOOL('v', \"verify\", &verify, N_(\"verify tags\")),\n \n \t\tOPT_GROUP(N_(\"Tag creation options\")),\n \t\tOPT_BOOL('a', \"annotate\", &annotate,\n@@ -455,7 +455,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK('m', \"message\", &msg, N_(\"message\"),\n \t\t\t     N_(\"tag message\"), parse_msg_arg),\n \t\tOPT_FILENAME('F', \"file\", &msgfile, N_(\"read message from file\")),\n-\t\tOPT_BOOLEAN('s', \"sign\", &opt.sign, N_(\"annotated and GPG-signed tag\")),\n+\t\tOPT_BOOL('s', \"sign\", &opt.sign, N_(\"annotated and GPG-signed tag\")),\n \t\tOPT_STRING(0, \"cleanup\", &cleanup_arg, N_(\"mode\"),\n \t\t\tN_(\"how to strip spaces and #comments from message\")),\n \t\tOPT_STRING('u', \"local-user\", &keyid, N_(\"key id\"),\n-- \n1.8.4.rc0.1.g8f6a3e5\n"},{"id":"224312","messageId":"7va9l3x34f.fsf@alter.siamese.dyndns.org","threadId":"34571","inReplyTo":"1375207251-4998-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-30T19:24:32Z","receivedAt":"2013-07-30T19:24:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <stefanbeller@googlemail.com> writes:\n\n> As of b04ba2bb (parse-options: deprecate OPT_BOOLEAN, 2011-09-27),\n> the OPT_BOOLEAN was deprecated.\n> While I am going to replace the OPT_BOOLEAN by the proposed OPT_BOOL or\n> the OPT_COUNTUP to keep existing behavior, this commit is actually a\n> bug fix!\n>\n> In line 499 we have:\n> \tif (list + delete + verify > 1)\n> \t\tusage_with_options(git_tag_usage, options);\n> Now if we give one of the options twice, we'll get the usage information.\n> (i.e. 'git tag --verify --verify <tagname>' and\n> 'git --delete --delete <tagname>' yield usage information and do not\n> do the intended command.)\n>\n> This could have been fixed by rewriting the line to\n> \tif (!!list + !!delete + !!verify > 1)\n> \t\tusage_with_options(git_tag_usage, options);\n> or as it happened in this patch by having the parameters not\n> counting up for each occurrence, but the OPT_BOOL just setting the\n> variables to either 0 if the option is not given or 1 if the option is\n> given multiple times.\n\nMakes twisted sort of sense ;-).\n\n> However we could discuss if the negated options do make sense\n> here, or if we don't want to allow them here, as this seems valid\n> (before and after this patch):\n>\n> \tgit tag --no-verify --delete <tagname>\n\nIt probably does not.  As you hinted in your earlier patch, we may\nwant to introduce a \"only can set to true\" boolean used solely to\nspecify these things.  They are disguised as \"options\", but are in\nfact command operation modes that are often mutually exclusive.\n\nFor these operation modes that are mutually exclusive, there are\nmultiple possible implementations:\n\n * One OPT_BOOL_NONEG per option; the code ensures the mutual\n   exclusion with \"if (list + delete + verify > 1)\";\n\n * One OPT_BIT per option in a single variable; the code ensures the\n   mutual exclusion with count_bits, which may be a lot more\n   cumbersome;\n\n * OPT_SET_INT that updates a single variable to enum; instead of\n   making it an error to give two conflicting modes, this would give\n   us the last-one-wins rule.\n\nUnlike usual \"options\", we generally do not want the last-one-wins\nsemantics for command operation modes, I think.\n\nPerhaps we would want something like this?\n\n-- >8 --\nSubject: [PATCH] parse-options: add OPT_CMDMODE()\n\nThis can be used to define a set of mutually exclusive \"command\nmode\" options, and automatically catch use of more than one from\nthat set as an error.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n parse-options.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n parse-options.h |  3 +++\n 2 files changed, 56 insertions(+), 5 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex c2cbca2..62e9b1c 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -43,8 +43,42 @@ static void fix_filename(const char *prefix, const char **file)\n \t*file = xstrdup(prefix_filename(prefix, strlen(prefix), *file));\n }\n \n+static int opt_command_mode_error(const struct option *opt,\n+\t\t\t\t  const struct option *all_opts,\n+\t\t\t\t  int flags)\n+{\n+\tconst struct option *that;\n+\tstruct strbuf message = STRBUF_INIT;\n+\tstruct strbuf that_name = STRBUF_INIT;\n+\n+\t/*\n+\t * Find the other option that was used to set the variable\n+\t * already, and report that this is not compatible with it.\n+\t */\n+\tfor (that = all_opts; that->type != OPTION_END; that++) {\n+\t\tif (that == opt ||\n+\t\t    that->type != OPTION_CMDMODE ||\n+\t\t    that->value != opt->value ||\n+\t\t    that->defval != *(int *)opt->value)\n+\t\t\tcontinue;\n+\n+\t\tif (that->long_name)\n+\t\t\tstrbuf_addf(&that_name, \"--%s\", that->long_name);\n+\t\telse\n+\t\t\tstrbuf_addf(&that_name, \"-%c\", that->short_name);\n+\t\tstrbuf_addf(&message, \": incompatible with %s\", that_name.buf);\n+\t\tstrbuf_release(&that_name);\n+\t\topterror(opt, message.buf, flags);\n+\t\tstrbuf_release(&message);\n+\t\treturn -1;\n+\t}\n+\treturn opterror(opt, \": incompatible with something else\", flags);\n+}\n+\n static int get_value(struct parse_opt_ctx_t *p,\n-\t\t     const struct option *opt, int flags)\n+\t\t     const struct option *opt,\n+\t\t     const struct option *all_opts,\n+\t\t     int flags)\n {\n \tconst char *s, *arg;\n \tconst int unset = flags & OPT_UNSET;\n@@ -83,6 +117,16 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\t*(int *)opt->value = unset ? 0 : opt->defval;\n \t\treturn 0;\n \n+\tcase OPTION_CMDMODE:\n+\t\t/*\n+\t\t * Giving the same mode option twice, although is unnecessary,\n+\t\t * is not a grave error, so let it pass.\n+\t\t */\n+\t\tif (*(int *)opt->value && *(int *)opt->value != opt->defval)\n+\t\t\treturn opt_command_mode_error(opt, all_opts, flags);\n+\t\t*(int *)opt->value = opt->defval;\n+\t\treturn 0;\n+\n \tcase OPTION_SET_PTR:\n \t\t*(void **)opt->value = unset ? NULL : (void *)opt->defval;\n \t\treturn 0;\n@@ -143,12 +187,13 @@ static int get_value(struct parse_opt_ctx_t *p,\n \n static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *options)\n {\n+\tconst struct option *all_opts = options;\n \tconst struct option *numopt = NULL;\n \n \tfor (; options->type != OPTION_END; options++) {\n \t\tif (options->short_name == *p->opt) {\n \t\t\tp->opt = p->opt[1] ? p->opt + 1 : NULL;\n-\t\t\treturn get_value(p, options, OPT_SHORT);\n+\t\t\treturn get_value(p, options, all_opts, OPT_SHORT);\n \t\t}\n \n \t\t/*\n@@ -177,6 +222,7 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *optio\n static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,\n                           const struct option *options)\n {\n+\tconst struct option *all_opts = options;\n \tconst char *arg_end = strchr(arg, '=');\n \tconst struct option *abbrev_option = NULL, *ambiguous_option = NULL;\n \tint abbrev_flags = 0, ambiguous_flags = 0;\n@@ -253,7 +299,7 @@ is_abbreviated:\n \t\t\t\tcontinue;\n \t\t\tp->opt = rest + 1;\n \t\t}\n-\t\treturn get_value(p, options, flags ^ opt_flags);\n+\t\treturn get_value(p, options, all_opts, flags ^ opt_flags);\n \t}\n \n \tif (ambiguous_option)\n@@ -265,18 +311,20 @@ is_abbreviated:\n \t\t\t(abbrev_flags & OPT_UNSET) ?  \"no-\" : \"\",\n \t\t\tabbrev_option->long_name);\n \tif (abbrev_option)\n-\t\treturn get_value(p, abbrev_option, abbrev_flags);\n+\t\treturn get_value(p, abbrev_option, all_opts, abbrev_flags);\n \treturn -2;\n }\n \n static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n \t\t\t    const struct option *options)\n {\n+\tconst struct option *all_opts = options;\n+\n \tfor (; options->type != OPTION_END; options++) {\n \t\tif (!(options->flags & PARSE_OPT_NODASH))\n \t\t\tcontinue;\n \t\tif (options->short_name == arg[0] && arg[1] == '\\0')\n-\t\t\treturn get_value(p, options, OPT_SHORT);\n+\t\t\treturn get_value(p, options, all_opts, OPT_SHORT);\n \t}\n \treturn -2;\n }\ndiff --git a/parse-options.h b/parse-options.h\nindex c378b75..2404e06 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -13,6 +13,7 @@ enum parse_opt_type {\n \tOPTION_COUNTUP,\n \tOPTION_SET_INT,\n \tOPTION_SET_PTR,\n+\tOPTION_CMDMODE,\n \t/* options with arguments (usually) */\n \tOPTION_STRING,\n \tOPTION_INTEGER,\n@@ -130,6 +131,8 @@ struct option {\n #define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)\n #define OPT_SET_PTR(s, l, v, h, p)  { OPTION_SET_PTR, (s), (l), (v), NULL, \\\n \t\t\t\t      (h), PARSE_OPT_NOARG, NULL, (p) }\n+#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n+\t\t\t\t      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h) }\n #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }\n #define OPT_STRING_LIST(s, l, v, a, h) \\\n-- \n1.8.4-rc0-153-g9820077\n\n\n... and then \"git tag\" may become like so.\n\n builtin/tag.c | 27 ++++++++++++---------------\n 1 file changed, 12 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex af3af3f..d8ae5aa 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -436,18 +436,18 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstruct ref_lock *lock;\n \tstruct create_tag_options opt;\n \tchar *cleanup_arg = NULL;\n-\tint annotate = 0, force = 0, lines = -1, list = 0,\n-\t\tdelete = 0, verify = 0;\n+\tint annotate = 0, force = 0, lines = -1;\n+\tint cmdmode = 0;\n \tconst char *msgfile = NULL, *keyid = NULL;\n \tstruct msg_arg msg = { 0, STRBUF_INIT };\n \tstruct commit_list *with_commit = NULL;\n \tstruct option options[] = {\n-\t\tOPT_BOOLEAN('l', \"list\", &list, N_(\"list tag names\")),\n+\t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n \t\t{ OPTION_INTEGER, 'n', NULL, &lines, N_(\"n\"),\n \t\t\t\tN_(\"print <n> lines of each tag message\"),\n \t\t\t\tPARSE_OPT_OPTARG, NULL, 1 },\n-\t\tOPT_BOOLEAN('d', \"delete\", &delete, N_(\"delete tags\")),\n-\t\tOPT_BOOLEAN('v', \"verify\", &verify, N_(\"verify tags\")),\n+\t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete tags\"), 'd'),\n+\t\tOPT_CMDMODE('v', \"verify\", &cmdmode, N_(\"verify tags\"), 'v'),\n \n \t\tOPT_GROUP(N_(\"Tag creation options\")),\n \t\tOPT_BOOLEAN('a', \"annotate\", &annotate,\n@@ -489,22 +489,19 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t}\n \tif (opt.sign)\n \t\tannotate = 1;\n-\tif (argc == 0 && !(delete || verify))\n-\t\tlist = 1;\n+\tif (argc == 0 && !cmdmode)\n+\t\tcmdmode = 'l';\n \n-\tif ((annotate || msg.given || msgfile || force) &&\n-\t    (list || delete || verify))\n+\tif ((annotate || msg.given || msgfile || force) && (cmdmode != 0))\n \t\tusage_with_options(git_tag_usage, options);\n \n-\tif (list + delete + verify > 1)\n-\t\tusage_with_options(git_tag_usage, options);\n \tfinalize_colopts(&colopts, -1);\n-\tif (list && lines != -1) {\n+\tif (cmdmode == 'l' && lines != -1) {\n \t\tif (explicitly_enable_column(colopts))\n \t\t\tdie(_(\"--column and -n are incompatible\"));\n \t\tcolopts = 0;\n \t}\n-\tif (list) {\n+\tif (cmdmode == 'l') {\n \t\tint ret;\n \t\tif (column_active(colopts)) {\n \t\t\tstruct column_options copts;\n@@ -523,9 +520,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--contains option is only allowed with -l.\"));\n \tif (points_at.nr)\n \t\tdie(_(\"--points-at option is only allowed with -l.\"));\n-\tif (delete)\n+\tif (cmdmode == 'd')\n \t\treturn for_each_tag_name(argv, delete_tag);\n-\tif (verify)\n+\tif (cmdmode == 'v')\n \t\treturn for_each_tag_name(argv, verify_tag);\n \n \tif (msg.given || msgfile) {\n"},{"id":"224323","messageId":"51F82E83.30203@googlemail.com","threadId":"34571","inReplyTo":"7va9l3x34f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-30T21:22:11Z","receivedAt":"2013-07-30T21:22:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 07/30/13 21:24, Junio C Hamano wrote:\n> Stefan Beller <stefanbeller@googlemail.com> writes:\n> \n>> As of b04ba2bb (parse-options: deprecate OPT_BOOLEAN, 2011-09-27),\n>> the OPT_BOOLEAN was deprecated.\n>> While I am going to replace the OPT_BOOLEAN by the proposed OPT_BOOL or\n>> the OPT_COUNTUP to keep existing behavior, this commit is actually a\n>> bug fix!\n>>\n>> In line 499 we have:\n>> \tif (list + delete + verify > 1)\n>> \t\tusage_with_options(git_tag_usage, options);\n>> Now if we give one of the options twice, we'll get the usage information.\n>> (i.e. 'git tag --verify --verify <tagname>' and\n>> 'git --delete --delete <tagname>' yield usage information and do not\n>> do the intended command.)\n>>\n>> This could have been fixed by rewriting the line to\n>> \tif (!!list + !!delete + !!verify > 1)\n>> \t\tusage_with_options(git_tag_usage, options);\n>> or as it happened in this patch by having the parameters not\n>> counting up for each occurrence, but the OPT_BOOL just setting the\n>> variables to either 0 if the option is not given or 1 if the option is\n>> given multiple times.\n> \n> Makes twisted sort of sense ;-).\n> \n>> However we could discuss if the negated options do make sense\n>> here, or if we don't want to allow them here, as this seems valid\n>> (before and after this patch):\n>>\n>> \tgit tag --no-verify --delete <tagname>\n> \n> It probably does not.  As you hinted in your earlier patch, we may\n> want to introduce a \"only can set to true\" boolean used solely to\n> specify these things.  They are disguised as \"options\", but are in\n> fact command operation modes that are often mutually exclusive.\n> \n> For these operation modes that are mutually exclusive, there are\n> multiple possible implementations:\n> \n>  * One OPT_BOOL_NONEG per option; the code ensures the mutual\n>    exclusion with \"if (list + delete + verify > 1)\";\n> \n>  * One OPT_BIT per option in a single variable; the code ensures the\n>    mutual exclusion with count_bits, which may be a lot more\n>    cumbersome;\n> \n>  * OPT_SET_INT that updates a single variable to enum; instead of\n>    making it an error to give two conflicting modes, this would give\n>    us the last-one-wins rule.\n> \n> Unlike usual \"options\", we generally do not want the last-one-wins\n> semantics for command operation modes, I think.\n> \n> Perhaps we would want something like this?\n> \n> -- >8 --\n> Subject: [PATCH] parse-options: add OPT_CMDMODE()\n> \n> This can be used to define a set of mutually exclusive \"command\n> mode\" options, and automatically catch use of more than one from\n> that set as an error.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  parse-options.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n>  parse-options.h |  3 +++\n>  2 files changed, 56 insertions(+), 5 deletions(-)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index c2cbca2..62e9b1c 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -43,8 +43,42 @@ static void fix_filename(const char *prefix, const char **file)\n>  \t*file = xstrdup(prefix_filename(prefix, strlen(prefix), *file));\n>  }\n>  \n> +static int opt_command_mode_error(const struct option *opt,\n> +\t\t\t\t  const struct option *all_opts,\n> +\t\t\t\t  int flags)\n> +{\n> +\tconst struct option *that;\n> +\tstruct strbuf message = STRBUF_INIT;\n> +\tstruct strbuf that_name = STRBUF_INIT;\n> +\n> +\t/*\n> +\t * Find the other option that was used to set the variable\n> +\t * already, and report that this is not compatible with it.\n> +\t */\n> +\tfor (that = all_opts; that->type != OPTION_END; that++) {\n> +\t\tif (that == opt ||\n> +\t\t    that->type != OPTION_CMDMODE ||\n> +\t\t    that->value != opt->value ||\n> +\t\t    that->defval != *(int *)opt->value)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (that->long_name)\n> +\t\t\tstrbuf_addf(&that_name, \"--%s\", that->long_name);\n> +\t\telse\n> +\t\t\tstrbuf_addf(&that_name, \"-%c\", that->short_name);\n> +\t\tstrbuf_addf(&message, \": incompatible with %s\", that_name.buf);\n> +\t\tstrbuf_release(&that_name);\n> +\t\topterror(opt, message.buf, flags);\n> +\t\tstrbuf_release(&message);\n> +\t\treturn -1;\n> +\t}\n> +\treturn opterror(opt, \": incompatible with something else\", flags);\n> +}\n> +\n>  static int get_value(struct parse_opt_ctx_t *p,\n> -\t\t     const struct option *opt, int flags)\n> +\t\t     const struct option *opt,\n> +\t\t     const struct option *all_opts,\n> +\t\t     int flags)\n>  {\n>  \tconst char *s, *arg;\n>  \tconst int unset = flags & OPT_UNSET;\n> @@ -83,6 +117,16 @@ static int get_value(struct parse_opt_ctx_t *p,\n>  \t\t*(int *)opt->value = unset ? 0 : opt->defval;\n>  \t\treturn 0;\n>  \n> +\tcase OPTION_CMDMODE:\n> +\t\t/*\n> +\t\t * Giving the same mode option twice, although is unnecessary,\n> +\t\t * is not a grave error, so let it pass.\n> +\t\t */\n> +\t\tif (*(int *)opt->value && *(int *)opt->value != opt->defval)\n> +\t\t\treturn opt_command_mode_error(opt, all_opts, flags);\n> +\t\t*(int *)opt->value = opt->defval;\n> +\t\treturn 0;\n> +\n>  \tcase OPTION_SET_PTR:\n>  \t\t*(void **)opt->value = unset ? NULL : (void *)opt->defval;\n>  \t\treturn 0;\n> @@ -143,12 +187,13 @@ static int get_value(struct parse_opt_ctx_t *p,\n>  \n>  static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *options)\n>  {\n> +\tconst struct option *all_opts = options;\n>  \tconst struct option *numopt = NULL;\n>  \n>  \tfor (; options->type != OPTION_END; options++) {\n>  \t\tif (options->short_name == *p->opt) {\n>  \t\t\tp->opt = p->opt[1] ? p->opt + 1 : NULL;\n> -\t\t\treturn get_value(p, options, OPT_SHORT);\n> +\t\t\treturn get_value(p, options, all_opts, OPT_SHORT);\n>  \t\t}\n>  \n>  \t\t/*\n> @@ -177,6 +222,7 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *optio\n>  static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,\n>                            const struct option *options)\n>  {\n> +\tconst struct option *all_opts = options;\n>  \tconst char *arg_end = strchr(arg, '=');\n>  \tconst struct option *abbrev_option = NULL, *ambiguous_option = NULL;\n>  \tint abbrev_flags = 0, ambiguous_flags = 0;\n> @@ -253,7 +299,7 @@ is_abbreviated:\n>  \t\t\t\tcontinue;\n>  \t\t\tp->opt = rest + 1;\n>  \t\t}\n> -\t\treturn get_value(p, options, flags ^ opt_flags);\n> +\t\treturn get_value(p, options, all_opts, flags ^ opt_flags);\n>  \t}\n>  \n>  \tif (ambiguous_option)\n> @@ -265,18 +311,20 @@ is_abbreviated:\n>  \t\t\t(abbrev_flags & OPT_UNSET) ?  \"no-\" : \"\",\n>  \t\t\tabbrev_option->long_name);\n>  \tif (abbrev_option)\n> -\t\treturn get_value(p, abbrev_option, abbrev_flags);\n> +\t\treturn get_value(p, abbrev_option, all_opts, abbrev_flags);\n>  \treturn -2;\n>  }\n>  \n>  static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n>  \t\t\t    const struct option *options)\n>  {\n> +\tconst struct option *all_opts = options;\n> +\n>  \tfor (; options->type != OPTION_END; options++) {\n>  \t\tif (!(options->flags & PARSE_OPT_NODASH))\n>  \t\t\tcontinue;\n>  \t\tif (options->short_name == arg[0] && arg[1] == '\\0')\n> -\t\t\treturn get_value(p, options, OPT_SHORT);\n> +\t\t\treturn get_value(p, options, all_opts, OPT_SHORT);\n>  \t}\n>  \treturn -2;\n>  }\n> diff --git a/parse-options.h b/parse-options.h\n> index c378b75..2404e06 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -13,6 +13,7 @@ enum parse_opt_type {\n>  \tOPTION_COUNTUP,\n>  \tOPTION_SET_INT,\n>  \tOPTION_SET_PTR,\n> +\tOPTION_CMDMODE,\n>  \t/* options with arguments (usually) */\n>  \tOPTION_STRING,\n>  \tOPTION_INTEGER,\n> @@ -130,6 +131,8 @@ struct option {\n>  #define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)\n>  #define OPT_SET_PTR(s, l, v, h, p)  { OPTION_SET_PTR, (s), (l), (v), NULL, \\\n>  \t\t\t\t      (h), PARSE_OPT_NOARG, NULL, (p) }\n> +#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n> +\t\t\t\t      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n>  #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h) }\n>  #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }\n>  #define OPT_STRING_LIST(s, l, v, a, h) \\\n> \n\nYour approach seems more like what we really want, however I'd have\nsome points:\n * Is it a good idea to have so many different OPT_MODE or\n   OPTION_MODE defines? In my attempts I tried to reuse existing\n   OPTION_s to not pollute the parsing infrastructure with more\n   lines of code. ;)\n\n * You can only have one OPTION_CMDMODE in one argv vector right?\n   I searched through the commands (... > 1) and did not find any\n   places, where we'd want to have multiple 'groups' of exclusive \n   commands, such as (either A or B) and/or (either C or D)\n   This cmd_mode would just all a (either A, B, C or D), but that\n   should be good for now.\n\n * This command mode could also be used for builtin/branch:\n\tif (!!delete + !!rename + !!force_create + !!list + !!new_upstream + !!unset_upstream > 1)\n\t\tusage_with_options(builtin_branch_usage, options);  \n   as well as commit:\n\tif (!!also + !!only + !!all + !!interactive > 1)\n\t\tdie(_(\"Only one of --include/--only/--all/--interactive/--patch can be used.\"));\n   as well as for checkout:\n\tif ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)\n\t\tdie(_(\"-b, -B and --orphan are mutually exclusive\"));\n   So if we'd introduce this command mode, I'd be happy to supply patches\n   for branch, commit and checkout to use the new exclusive mechanism.\n \nSo I think I like it.\nReviewed-by: Stefan Beller <stefanbeller@googlemail.com>\n\n\n"},{"id":"224324","messageId":"51F83010.2060804@googlemail.com","threadId":"34571","inReplyTo":"7va9l3x34f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-30T21:28:48Z","receivedAt":"2013-07-30T21:28:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 07/30/13 21:24, Junio C Hamano wrote:\n> \n> ... and then \"git tag\" may become like so.\n> \n>  builtin/tag.c | 27 ++++++++++++---------------\n>  1 file changed, 12 insertions(+), 15 deletions(-)\n> \n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index af3af3f..d8ae5aa 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -436,18 +436,18 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \tstruct ref_lock *lock;\n>  \tstruct create_tag_options opt;\n>  \tchar *cleanup_arg = NULL;\n> -\tint annotate = 0, force = 0, lines = -1, list = 0,\n> -\t\tdelete = 0, verify = 0;\n> +\tint annotate = 0, force = 0, lines = -1;\n> +\tint cmdmode = 0;\n>  \tconst char *msgfile = NULL, *keyid = NULL;\n>  \tstruct msg_arg msg = { 0, STRBUF_INIT };\n>  \tstruct commit_list *with_commit = NULL;\n>  \tstruct option options[] = {\n> -\t\tOPT_BOOLEAN('l', \"list\", &list, N_(\"list tag names\")),\n> +\t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n>  \t\t{ OPTION_INTEGER, 'n', NULL, &lines, N_(\"n\"),\n>  \t\t\t\tN_(\"print <n> lines of each tag message\"),\n>  \t\t\t\tPARSE_OPT_OPTARG, NULL, 1 },\n> -\t\tOPT_BOOLEAN('d', \"delete\", &delete, N_(\"delete tags\")),\n> -\t\tOPT_BOOLEAN('v', \"verify\", &verify, N_(\"verify tags\")),\n> +\t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete tags\"), 'd'),\n> +\t\tOPT_CMDMODE('v', \"verify\", &cmdmode, N_(\"verify tags\"), 'v'),\n>  \n>  \t\tOPT_GROUP(N_(\"Tag creation options\")),\n>  \t\tOPT_BOOLEAN('a', \"annotate\", &annotate,\n> @@ -489,22 +489,19 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \tif (opt.sign)\n>  \t\tannotate = 1;\n> -\tif (argc == 0 && !(delete || verify))\n> -\t\tlist = 1;\n> +\tif (argc == 0 && !cmdmode)\n> +\t\tcmdmode = 'l';\n>  \n> -\tif ((annotate || msg.given || msgfile || force) &&\n> -\t    (list || delete || verify))\n> +\tif ((annotate || msg.given || msgfile || force) && (cmdmode != 0))\n>  \t\tusage_with_options(git_tag_usage, options);\n>  \n> -\tif (list + delete + verify > 1)\n> -\t\tusage_with_options(git_tag_usage, options);\n>  \tfinalize_colopts(&colopts, -1);\n> -\tif (list && lines != -1) {\n> +\tif (cmdmode == 'l' && lines != -1) {\n>  \t\tif (explicitly_enable_column(colopts))\n>  \t\t\tdie(_(\"--column and -n are incompatible\"));\n>  \t\tcolopts = 0;\n>  \t}\n> -\tif (list) {\n> +\tif (cmdmode == 'l') {\n>  \t\tint ret;\n>  \t\tif (column_active(colopts)) {\n>  \t\t\tstruct column_options copts;\n> @@ -523,9 +520,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \t\tdie(_(\"--contains option is only allowed with -l.\"));\n>  \tif (points_at.nr)\n>  \t\tdie(_(\"--points-at option is only allowed with -l.\"));\n> -\tif (delete)\n> +\tif (cmdmode == 'd')\n>  \t\treturn for_each_tag_name(argv, delete_tag);\n> -\tif (verify)\n> +\tif (cmdmode == 'v')\n>  \t\treturn for_each_tag_name(argv, verify_tag);\n>  \n>  \tif (msg.given || msgfile) {\n\n\nHere is just another idea: \n\tif (cmdmode == 'v')\nThis may be hard to read, (What is 'v'? I cannot remember \nall the alphabet ;)) So maybe we could have an enum instead of\nthe last parameter? \nOPT_CMDMODE( short, long, variable, description, enum)\n\nAlso the variable would then only need to be an enum accepting variable,\nand not an integer accepting all integer range, so we'd also catch \ntypos or wrong values in such a case:\n> +\tif (argc == 0 && !cmdmode)\n> +\t\tcmdmode = 'l'; // maybe 'l' is a typo and not existing?\n\n"},{"id":"224326","messageId":"7vmwp3vgaq.fsf@alter.siamese.dyndns.org","threadId":"34571","inReplyTo":"51F82E83.30203@googlemail.com","subject":"Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-30T22:22:53Z","receivedAt":"2013-07-30T22:22:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <stefanbeller@googlemail.com> writes:\n\n> Your approach seems more like what we really want, however I'd have\n> some points:\n>  * Is it a good idea to have so many different OPT_MODE or\n>    OPTION_MODE defines? In my attempts I tried to reuse existing\n>    OPTION_s to not pollute the parsing infrastructure with more\n>    lines of code. ;)\n>\n>  * You can only have one OPTION_CMDMODE in one argv vector right?\n\nThat is not what I intended, at least.\n\n\tint one = 0, two = 0;\n\tstruct option options[] = {\n        OPT_CMDMODE('a', NULL, &one, N_(\"set one to a\"), 'a'),\n        OPT_CMDMODE('b', NULL, &one, N_(\"set one to b\"), 'b'),\n        OPT_CMDMODE('c', NULL, &two, N_(\"set two to c\"), 'c'),\n        OPT_CMDMODE('d', NULL, &two, N_(\"set two to d\"), 'd'),\n        OPT_END()\n        }\n\nshould give you two independent sets of modes, one and two.\n\nThe only reason I needed to add an extra parameter to get_value()\nwas so that I can tell the former two and the latter two belong to\ndifferent groups, and that is done by looking at the address of the\nvariable.  In opt_command_mode_error(), opt->value == that->value\nis used as a condition to see if the other option is possibly the\none that was used previously, which conflicted with us.\n"},{"id":"224327","messageId":"7vfvuvvg0r.fsf@alter.siamese.dyndns.org","threadId":"34571","inReplyTo":"51F83010.2060804@googlemail.com","subject":"Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-30T22:28:52Z","receivedAt":"2013-07-30T22:28:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <stefanbeller@googlemail.com> writes:\n\n> Here is just another idea: \n> \tif (cmdmode == 'v')\n> This may be hard to read, (What is 'v'? I cannot remember \n> all the alphabet ;)) So maybe we could have an enum instead of\n> the last parameter? \n> OPT_CMDMODE( short, long, variable, description, enum)\n\nI actually was thinking about going totally in the opposite\ndirection.\n\nPeople who grew up with the old Unix tradition getopt(3) are used to\ncode like this:\n\n        while ((opt = getopt(ac, av, \"abcde\")) != -1) {\n                switch (opt) {\n                case 'a': perform_a(); break;\n                case 'b': perform_b(); break;\n                ...\n                }\n        }\n\nIn other words, the \"enum\" is most convenient if it matches the\n\"short\" option, so instead of having to repeat ourselves over and\nover, like I did in that illustration patch for builtin/tag.c, e.g.\n\n\t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete tags\"), 'd'),\n\t\tOPT_CMDMODE('v', \"verify\", &cmdmode, N_(\"verify tags\"), 'v'),\n\nwe could just do\n\n#define OPT_CMDMODE(s, l, v, h) \\\n    { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (s) }\n"},{"id":"224347","messageId":"51F8E81E.6000705@googlemail.com","threadId":"34571","inReplyTo":"7vfvuvvg0r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-31T10:34:06Z","receivedAt":"2013-07-31T10:34:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 07/31/13 00:28, Junio C Hamano wrote:\n> \n> we could just do\n> \n> #define OPT_CMDMODE(s, l, v, h) \\\n>     { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n>       (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (s) }\n> \n\nI agree that's a better proposal than mine.\n\n"},{"id":"224409","messageId":"7vbo5itjfl.fsf@alter.siamese.dyndns.org","threadId":"34571","inReplyTo":"51F8E81E.6000705@googlemail.com","subject":"Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-31T23:10:22Z","receivedAt":"2013-07-31T23:10:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <stefanbeller@googlemail.com> writes:\n\n> On 07/31/13 00:28, Junio C Hamano wrote:\n>> \n>> we could just do\n>> \n>> #define OPT_CMDMODE(s, l, v, h) \\\n>>     { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n>>       (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (s) }\n>> \n>\n> I agree that's a better proposal than mine.\n\nBy the way, I haven't convinced myself that it is a good idea in\ngeneral to encourage more use of command mode options, so I am a bit\nreluctant to add this before knowing which direction in the longer\nterm we are going.\n\n - Some large-ish Git subcommands, like \"git submodule\", use the\n   mode word (e.g. \"git submodule status\") to specify the operation\n   mode (youe could consider \"status\" a subsubcommand that\n   \"submodule\" subcommand takes).  These commands typically began\n   their life from day one with the mode words.\n\n - On the other hand, many Git subcommands, like \"git tag\", have\n   \"the primary operation mode\" (e.g. \"create a new one\" is the\n   primary operation mode for \"git tag\"), and use command mode\n   options to specify other operation modes (e.g. \"--delete\").\n   These commands started as single purpose commands (i.e. to\n   perform their \"primary operation\") but have organically grown\n   over time and acquired command mode options to invoke their\n   secondary operations.\n\nAs an end user, you need to learn which style each command takes,\nwhich is an unnecessary burden at the UI level.  In the longer term,\nwe may want to consider picking a single style, and migrating\neverybody to it.  If I have to vote today, I would say we should\nteach \"git submodule\" to also take command mode options (e.g. \"git\nsubmodule --status\" will be understood the same way as \"git\nsubmodule status\"), make them issue warnings when mode words are\nused and encourage users to use command mode options instead, and\noptionally remove the support of mode words at a large version bump\nlike 3.0.\n\nOne clear advantage mode words have over command mode options is\nthat there is no room for end user confusion.  The first word after\n\"git subcmd\" is the mode word, and you will not even dream of asking\n\"what would 'git submodule add del foo' do?\" as it is nonsensical.\nThe command mode options, on the other hand, gives too much useless\nflexibility to ask for nonsense, e.g. \"git tag --delete --verify\",\n\"git tag --no-delete --delete\", etc., and extra code needs to detect\nand reject combinations.  But commands that took mode options cannot\nbe easily migrated to take mode words without hurting existing users\nand scripts (e.g. \"git tag delete master\" can never be a request to\ndelete the tag 'master', as it is a request to create a tag whose\nname is 'delete' that points at the same object as 'master' points\nat).\n"},{"id":"225371","messageId":"520F9051.4040600@gmail.com","threadId":"34571","inReplyTo":"7vbo5itjfl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2013-08-17T15:01:37Z","receivedAt":"2013-08-17T15:01:37Z","isPatch":true,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"(Going through old mail today, sorry for the late reply)\n\nOn 08/01/2013 12:10 AM, Junio C Hamano wrote:> Stefan Beller \n<stefanbeller@googlemail.com> writes:\n >\n >> On 07/31/13 00:28, Junio C Hamano wrote:\n >>>\n >>> we could just do\n >>>\n >>> #define OPT_CMDMODE(s, l, v, h) \\\n >>>      { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n >>>        (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (s) }\n >>>\n >>\n >> I agree that's a better proposal than mine.\n >\n > By the way, I haven't convinced myself that it is a good idea in\n > general to encourage more use of command mode options, so I am a bit\n > reluctant to add this before knowing which direction in the longer\n > term we are going.\n >\n >   - Some large-ish Git subcommands, like \"git submodule\", use the\n >     mode word (e.g. \"git submodule status\") to specify the operation\n >     mode (youe could consider \"status\" a subsubcommand that\n >     \"submodule\" subcommand takes).  These commands typically began\n >     their life from day one with the mode words.\n >\n >   - On the other hand, many Git subcommands, like \"git tag\", have\n >     \"the primary operation mode\" (e.g. \"create a new one\" is the\n >     primary operation mode for \"git tag\"), and use command mode\n >     options to specify other operation modes (e.g. \"--delete\").\n >     These commands started as single purpose commands (i.e. to\n >     perform their \"primary operation\") but have organically grown\n >     over time and acquired command mode options to invoke their\n >     secondary operations.\n >\n > As an end user, you need to learn which style each command takes,\n > which is an unnecessary burden at the UI level.  In the longer term,\n > we may want to consider picking a single style, and migrating\n > everybody to it.  If I have to vote today, I would say we should\n > teach \"git submodule\" to also take command mode options (e.g. \"git\n > submodule --status\" will be understood the same way as \"git\n > submodule status\"), make them issue warnings when mode words are\n > used and encourage users to use command mode options instead, and\n > optionally remove the support of mode words at a large version bump\n > like 3.0.\n >\n > One clear advantage mode words have over command mode options is\n > that there is no room for end user confusion.  The first word after\n > \"git subcmd\" is the mode word, and you will not even dream of asking\n > \"what would 'git submodule add del foo' do?\" as it is nonsensical.\n > The command mode options, on the other hand, gives too much useless\n > flexibility to ask for nonsense, e.g. \"git tag --delete --verify\",\n > \"git tag --no-delete --delete\", etc., and extra code needs to detect\n > and reject combinations.  But commands that took mode options cannot\n > be easily migrated to take mode words without hurting existing users\n > and scripts (e.g. \"git tag delete master\" can never be a request to\n > delete the tag 'master', as it is a request to create a tag whose\n > name is 'delete' that points at the same object as 'master' points\n > at).\n >\nWhy not encourage the use of a standardized '--action' option instead?\nThis can work with lesser compatibility headaches for both the commands\ntaking mode options and the commands taking mode words:\n\n   \"git submodule init\"   becomes  \"git submodule --action=init\"\n   \"git tag --delete TAG\" becomes  \"git tag --action=delete TAGNAME\"\n\nCommands that have a \"primary operation mode\" can keep the use\nof --action optional, while commands that have no such sensible\nprimary mode can make it mandatory (again, this gives more compatibility \nwith the existing syntax).  And the old syntax\ncan be supported in parallel to the new one for a potentially\nindefinite time; albeit, if I were to propose a timetable, I'd\ngot for:\n\n   - Git 2.0: the new syntax is introduced\n   - Git 2.x: any use the old syntax starts to trigger warnings\n     (which can be silenced with a config option)\n   - Git 3.0: any use the old syntax starts to trigger fatal\n     errors (which can be turned into mandatory warnings with\n     a config option)\n   - Git 4.0: any use the old syntax starts to trigger\n     mandatory fatal errors.\n   - Git 4.x: Remove any handling of the old syntax.\n\nJust my 2 cents,\n   Stefano\n"},{"id":"225384","messageId":"20130817203458.GB2904@elie.Belkin","threadId":"34571","inReplyTo":"520F9051.4040600@gmail.com","subject":"Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-17T20:34:58Z","receivedAt":"2013-08-17T20:34:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefano Lattarini wrote:\n\n> Why not encourage the use of a standardized '--action' option instead?\n\nBecause it's an unpleasant UI. :)\n\n> This can work with lesser compatibility headaches for both the commands\n> taking mode options and the commands taking mode words:\n>\n>   \"git submodule init\"   becomes  \"git submodule --action=init\"\n>   \"git tag --delete TAG\" becomes  \"git tag --action=delete TAGNAME\"\n\nThat looks like a bad change in both cases --- it involves more\ntyping without much upside to go along with it.  But\n\n\t\"git submodule init\"   gains synonym \"git submodule --init\"\n\t\"git tag --delete TAG\" stays as      \"git tag --delete TAG\"\n\nlooks fine to me.\n\nIn the long run, we could require that for new commands the 'action'\noption must come immediately after the git command name if that makes\nthings easier to learn.\n\nThanks for some food for thought.\n\nMy two cents,\nJonathan\n"},{"id":"225386","messageId":"xmqqioz3e4cw.fsf@gitster.dls.corp.google.com","threadId":"34571","inReplyTo":"20130817203458.GB2904@elie.Belkin","subject":"Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-18T09:27:11Z","receivedAt":"2013-08-18T09:27:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Stefano Lattarini wrote:\n>\n>> Why not encourage the use of a standardized '--action' option instead?\n>\n> Because it's an unpleasant UI. :)\n>\n>> This can work with lesser compatibility headaches for both the commands\n>> taking mode options and the commands taking mode words:\n>>\n>>   \"git submodule init\"   becomes  \"git submodule --action=init\"\n>>   \"git tag --delete TAG\" becomes  \"git tag --action=delete TAGNAME\"\n>\n> That looks like a bad change in both cases --- it involves more\n> typing without much upside to go along with it.  But\n>\n> \t\"git submodule init\"   gains synonym \"git submodule --init\"\n> \t\"git tag --delete TAG\" stays as      \"git tag --delete TAG\"\n>\n> looks fine to me.\n\nI agree 100% with the above that illustrates why --action=<name> is\na bad idea.  As I already said, adding action-option like --init, if\ndoing so might help people, I am not opposed to it.\n\nThanks.\n"}]}