{"thread":{"id":"19670","subject":"parse-options: ambiguous LASTARG_DEFAULT and OPTARG","startedAt":"2009-06-05T05:43:14Z","lastAt":"2009-06-12T21:25:57Z","messageCount":9,"participants":["Stephen Boyd","René Scharfe","Junio C Hamano","Pierre Habouzit"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"115474","messageId":"4A28B072.8030006@gmail.com","threadId":"19670","inReplyTo":null,"subject":"parse-options: ambiguous LASTARG_DEFAULT and OPTARG","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-06-05T05:43:14Z","receivedAt":"2009-06-05T05:43:14Z","isPatch":false,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Hi,\n\nThis in builtin-branch.c\n\n        {\n\t\tOPTION_CALLBACK, 0, \"merged\", &merge_filter_ref,\n\t\t\"commit\", \"print only merged branches\",\n\t\tPARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,\n\t\topt_parse_merge_filter, (intptr_t) \"HEAD\",\n\t},\n\nand the usage message for \"git-branch -h\" will print out\n\n    --merged <commit>\n\nwhen I'm expecting\n\n    --merged[=<commit>]\n\nThis is because the PARSE_OPT_OPTARG flag is not used. Is this correct?\nThe default value is still set correctly in some cases, but become\nambiguous in other cases. Take this for example\n\n    $ git branch --merged --verbose\n    fatal: malformed object name --verbose\n\nbut\n\n    $ git branch --verbose --merged\n\nworks fine.\n\nThe simple fix is to just add PARSE_OPT_OPTARG to the flags, and fix a\ntest or two. But I'm wondering if doing that will become problematic for\nend-users. Essentially you can no longer do git branch --merged master,\nyou must do git branch --merged=master.\n"},{"id":"115638","messageId":"4A2A4534.80604@lsrfire.ath.cx","threadId":"19670","inReplyTo":"4A28B072.8030006@gmail.com","subject":"Re: parse-options: ambiguous LASTARG_DEFAULT and OPTARG","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-06-06T10:30:12Z","receivedAt":"2009-06-06T10:30:12Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Stephen Boyd schrieb:\n> Hi,\n> \n> This in builtin-branch.c\n> \n>         {\n> \t\tOPTION_CALLBACK, 0, \"merged\", &merge_filter_ref,\n> \t\t\"commit\", \"print only merged branches\",\n> \t\tPARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,\n> \t\topt_parse_merge_filter, (intptr_t) \"HEAD\",\n> \t},\n> \n> and the usage message for \"git-branch -h\" will print out\n> \n>     --merged <commit>\n> \n> when I'm expecting\n> \n>     --merged[=<commit>]\n> \n> This is because the PARSE_OPT_OPTARG flag is not used. Is this correct?\n\n> The default value is still set correctly in some cases, but become\n> ambiguous in other cases. Take this for example\n> \n>     $ git branch --merged --verbose\n>     fatal: malformed object name --verbose\n> \n> but\n> \n>     $ git branch --verbose --merged\n> \n> works fine.\n> \n> The simple fix is to just add PARSE_OPT_OPTARG to the flags, and fix a\n> test or two. But I'm wondering if doing that will become problematic for\n> end-users. Essentially you can no longer do git branch --merged master,\n> you must do git branch --merged=master.\n\nPARSE_OPT_OPTARG overrides PARSE_OPT_LASTARG_DEFAULT, as Pierre noted in\ncommit 1cc6985c, which introduced the latter, so the two should not be\nused together.\n\nPARSE_OPT_LASTARG_DEFAULT uses the default value if the option is the\nlast one on the command line and requires an explicit argument if it's\nnot the last, as you found out above.  That's also what the code says \nand its name implies; the comment in parse-options.h (by yours truly) is \nprobably misleading because it doesn't mention this condition.\n\nI don't remember any other program having options with such a behaviour; \nI'm not sure how to stress that --merged needs to be the last option, as \nimplied by the help message.\n"},{"id":"115664","messageId":"4A2ACE32.8080504@gmail.com","threadId":"19670","inReplyTo":"4A2A4534.80604@lsrfire.ath.cx","subject":"Re: parse-options: ambiguous LASTARG_DEFAULT and OPTARG","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-06-06T20:14:42Z","receivedAt":"2009-06-06T20:14:42Z","isPatch":false,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"René Scharfe wrote:\n> PARSE_OPT_OPTARG overrides PARSE_OPT_LASTARG_DEFAULT, as Pierre noted in\n> commit 1cc6985c, which introduced the latter, so the two should not be\n> used together.\n\nOk, thanks. This means I used it wrong when I switched over show-branch\n:-/ I'll have to send a follow-up patch for that.\n\n> PARSE_OPT_LASTARG_DEFAULT uses the default value if the option is the\n> last one on the command line and requires an explicit argument if it's\n> not the last, as you found out above.  That's also what the code says\n> and its name implies; the comment in parse-options.h (by yours truly)\n> is probably misleading because it doesn't mention this condition.\n\nI was mislead. When I read it I thought I had to use the flag to say\nthat the default value will be used in the case when no argument is\ngiven. I completely ignored the LASTARG part (I thought it was\nreferencing the default arg). I think just adding what you said here to\nparse-options.h will help others to avoid this.\n\n> I don't remember any other program having options with such a\n> behaviour; I'm not sure how to stress that --merged needs to be the\n> last option, as implied by the help message.\n\n\"git tag --contains\" is the same. Figuring out a way to say that the\nsyntax changes when it's the last option versus in the middle is not\nobvious to me either.\n"},{"id":"115754","messageId":"1244417955-21226-1-git-send-email-bebarino@gmail.com","threadId":"19670","inReplyTo":"4A2ACE32.8080504@gmail.com","subject":"[PATCH] show-branch: don't use LASTARG_DEFAULT with OPTARG","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-06-07T23:39:15Z","receivedAt":"2009-06-07T23:39:15Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"5734365 (show-branch: migrate to parse-options API 2009-05-21)\nincorrectly set the --more option's flags to be\nPARSE_OPT_LASTARG_DEFAULT and PARSE_OPT_OPTARG. These two flags\nshouldn't be used together. An option taking a default should just set\nthe default value desired and parse options will take care of the rest.\n\nUpdate the header comment to better convey this information.\n\nSigned-off-by: Stephen Boyd <bebarino@gmail.com>\n---\n builtin-show-branch.c |    3 +--\n parse-options.h       |    7 +++++--\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-show-branch.c b/builtin-show-branch.c\nindex 9433811..01bea3b 100644\n--- a/builtin-show-branch.c\n+++ b/builtin-show-branch.c\n@@ -657,8 +657,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t    \"color '*!+-' corresponding to the branch\"),\n \t\t{ OPTION_INTEGER, 0, \"more\", &extra, \"n\",\n \t\t\t    \"show <n> more commits after the common ancestor\",\n-\t\t\t    PARSE_OPT_OPTARG | PARSE_OPT_LASTARG_DEFAULT,\n-\t\t\t    NULL, (intptr_t)1 },\n+\t\t\t    PARSE_OPT_OPTARG, NULL, (intptr_t)1 },\n \t\tOPT_SET_INT(0, \"list\", &extra, \"synonym to more=-1\", -1),\n \t\tOPT_BOOLEAN(0, \"no-name\", &no_name, \"suppress naming strings\"),\n \t\tOPT_BOOLEAN(0, \"current\", &with_current_branch,\ndiff --git a/parse-options.h b/parse-options.h\nindex b374ade..5653dba 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -71,8 +71,11 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);\n  *   PARSE_OPT_NONEG: says that this option cannot be negated\n  *   PARSE_OPT_HIDDEN: this option is skipped in the default usage, and\n  *                     shown only in the full usage.\n- *   PARSE_OPT_LASTARG_DEFAULT: if no argument is given, the default value\n- *                              is used.\n+ *   PARSE_OPT_LASTARG_DEFAULT: says that this option will take the default\n+ *\t\t\t\tvalue if no argument is given when the option\n+ *\t\t\t\tis last on the command line. If the option is\n+ *\t\t\t\tnot last it will require an argument.\n+ *\t\t\t\tShould not be used with PARSE_OPT_OPTARG.\n  *   PARSE_OPT_NODASH: this option doesn't start with a dash.\n  *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets\n  *\t\t\t\t(i.e. '<argh>') in the help message.\n-- \n1.6.3.2.202.g26c11\n"},{"id":"115822","messageId":"4A2D494E.3030803@lsrfire.ath.cx","threadId":"19670","inReplyTo":"1244417955-21226-1-git-send-email-bebarino@gmail.com","subject":"Re: [PATCH] show-branch: don't use LASTARG_DEFAULT with OPTARG","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-06-08T17:24:30Z","receivedAt":"2009-06-08T17:24:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Stephen Boyd schrieb:\n> 5734365 (show-branch: migrate to parse-options API 2009-05-21)\n> incorrectly set the --more option's flags to be\n> PARSE_OPT_LASTARG_DEFAULT and PARSE_OPT_OPTARG. These two flags\n> shouldn't be used together. An option taking a default should just set\n> the default value desired and parse options will take care of the rest.\n> \n> Update the header comment to better convey this information.\n\nThank you!\n"},{"id":"115849","messageId":"7vd49ewfsi.fsf@alter.siamese.dyndns.org","threadId":"19670","inReplyTo":"1244417955-21226-1-git-send-email-bebarino@gmail.com","subject":"Re: [PATCH] show-branch: don't use LASTARG_DEFAULT with OPTARG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-08T21:56:29Z","receivedAt":"2009-06-08T21:56:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Boyd <bebarino@gmail.com> writes:\n\n> 5734365 (show-branch: migrate to parse-options API 2009-05-21)\n> incorrectly set the --more option's flags to be\n> PARSE_OPT_LASTARG_DEFAULT and PARSE_OPT_OPTARG. These two flags\n> shouldn't be used together. An option taking a default should just set\n> the default value desired and parse options will take care of the rest.\n\nThanks.  Perhaps as a follow-up patch the runtime can check and barf when\nparse_options() is called and finds this combination?\n"},{"id":"115891","messageId":"1244535824-11970-1-git-send-email-madcoder@debian.org","threadId":"19670","inReplyTo":"7vd49ewfsi.fsf@alter.siamese.dyndns.org","subject":"[PATCH] parse-options: add parse_options_check to validate option specs.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2009-06-09T08:23:44Z","receivedAt":"2009-06-09T08:23:44Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"It only searches for now for the dreaded LASTARG_DEFAULT | OPTARG\ncombination, but can be extended to check for any other forbidden\ncombination.\n\nOptions are checked each time we call parse_options_start.\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n parse-options.c |   24 ++++++++++++++++++++++++\n 1 files changed, 24 insertions(+), 0 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e469fc0..34282ad 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -306,6 +306,28 @@ static void check_typos(const char *arg, const struct option *options)\n \t}\n }\n \n+static void parse_options_check(const struct option *opts)\n+{\n+\tint err = 0;\n+\n+\tfor (; opts->type != OPTION_END; opts++) {\n+\t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n+\t\t    (opts->flags & PARSE_OPT_OPTARG)) {\n+\t\t\tif (opts->long_name) {\n+\t\t\t\terror(\"`--%s` uses incompatible flags \"\n+\t\t\t\t      \"LASTARG_DEFAULT and OPTARG\", opts->long_name);\n+\t\t\t} else {\n+\t\t\t\terror(\"`-%c` uses incompatible flags \"\n+\t\t\t\t      \"LASTARG_DEFAULT and OPTARG\", opts->short_name);\n+\t\t\t}\n+\t\t\terr |= 1;\n+\t\t}\n+\t}\n+\n+\tif (err)\n+\t\texit(129);\n+}\n+\n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n \t\t\t int flags)\n@@ -331,6 +353,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n {\n \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n \n+\tparse_options_check(options);\n+\n \t/* we must reset ->opt, unknown short option leave it dangling */\n \tctx->opt = NULL;\n \n-- \n1.6.3.2.323.gcd28f\n"},{"id":"116175","messageId":"20090612193156.GC3129@artemis.corp","threadId":"19670","inReplyTo":"1244535824-11970-1-git-send-email-madcoder@debian.org","subject":"Re: [PATCH] parse-options: add parse_options_check to validate option specs.","fromName":"Pierre Habouzit","fromEmail":"madcoder@madism.org","sentAt":"2009-06-12T19:31:57Z","receivedAt":"2009-06-12T19:31:57Z","isPatch":true,"sender":{"key":"madcoder@madism.org","avatar":null},"body":"On Tue, Jun 09, 2009 at 10:23:44AM +0200, Pierre Habouzit wrote:\n> It only searches for now for the dreaded LASTARG_DEFAULT | OPTARG\n> combination, but can be extended to check for any other forbidden\n> combination.\n> \n> Options are checked each time we call parse_options_start.\n> \n> Signed-off-by: Pierre Habouzit <madcoder@debian.org>\n> ---\n>  parse-options.c |   24 ++++++++++++++++++++++++\n>  1 files changed, 24 insertions(+), 0 deletions(-)\n\nHas this patch been missed, or has it any kind of flaw that _I_ missed ?\n:)\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"116183","messageId":"4A32C7E5.5090604@lsrfire.ath.cx","threadId":"19670","inReplyTo":"20090612193156.GC3129@artemis.corp","subject":"Re: [PATCH] parse-options: add parse_options_check to validate option specs.","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-06-12T21:25:57Z","receivedAt":"2009-06-12T21:25:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Pierre Habouzit schrieb:\n> On Tue, Jun 09, 2009 at 10:23:44AM +0200, Pierre Habouzit wrote:\n>> It only searches for now for the dreaded LASTARG_DEFAULT | OPTARG\n>> combination, but can be extended to check for any other forbidden\n>> combination.\n>>\n>> Options are checked each time we call parse_options_start.\n>>\n>> Signed-off-by: Pierre Habouzit <madcoder@debian.org>\n>> ---\n>>  parse-options.c |   24 ++++++++++++++++++++++++\n>>  1 files changed, 24 insertions(+), 0 deletions(-)\n> \n> Has this patch been missed, or has it any kind of flaw that _I_ missed ?\n> :)\n> \n\nIt's in master (cb9d398c).\n"}]}