parse-options: ambiguous LASTARG_DEFAULT and OPTARG

René Scharfe· Jun 6, 2009, 10:30 UTC · re: Stephen Boyd · lore

Re: parse-options: ambiguous LASTARG_DEFAULT and OPTARG

Stephen Boyd schrieb:

> Hi,
> 
> This in builtin-branch.c
> 
>         {
> 		OPTION_CALLBACK, 0, "merged", &merge_filter_ref,
> 		"commit", "print only merged branches",
> 		PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,
> 		opt_parse_merge_filter, (intptr_t) "HEAD",
> 	},
> 
> and the usage message for "git-branch -h" will print out
> 
>     --merged <commit>
> 
> when I'm expecting
> 
>     --merged[=<commit>]
> 
> This is because the PARSE_OPT_OPTARG flag is not used. Is this correct?

> The default value is still set correctly in some cases, but become
> ambiguous in other cases. Take this for example
> 
>     $ git branch --merged --verbose
>     fatal: malformed object name --verbose
> 
> but
> 
>     $ git branch --verbose --merged
> 
> works fine.
> 
> The simple fix is to just add PARSE_OPT_OPTARG to the flags, and fix a
> test or two. But I'm wondering if doing that will become problematic for
> end-users. Essentially you can no longer do git branch --merged master,
> you must do git branch --merged=master.

Stephen Boyd· Jun 6, 2009, 20:14 UTC · re: René Scharfe · lore

Re: parse-options: ambiguous LASTARG_DEFAULT and OPTARG

René Scharfe wrote:

> PARSE_OPT_OPTARG overrides PARSE_OPT_LASTARG_DEFAULT, as Pierre noted in
> commit 1cc6985c, which introduced the latter, so the two should not be
> used together.

> PARSE_OPT_LASTARG_DEFAULT uses the default value if the option is the
> last one on the command line and requires an explicit argument if it's
> not the last, as you found out above.  That's also what the code says
> and its name implies; the comment in parse-options.h (by yours truly)
> is probably misleading because it doesn't mention this condition.

> I don't remember any other program having options with such a
> behaviour; I'm not sure how to stress that --merged needs to be the
> last option, as implied by the help message.

Stephen Boyd· Jun 7, 2009, 23:39 UTC · re: Stephen Boyd · lore

[PATCH] show-branch: don't use LASTARG_DEFAULT with OPTARG

Update the header comment to better convey this information.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 builtin-show-branch.c |    3 +--
 parse-options.h       |    7 +++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 9433811..01bea3b 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -657,8 +657,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			    "color '*!+-' corresponding to the branch"),
 		{ OPTION_INTEGER, 0, "more", &extra, "n",
 			    "show <n> more commits after the common ancestor",
-			    PARSE_OPT_OPTARG | PARSE_OPT_LASTARG_DEFAULT,
-			    NULL, (intptr_t)1 },
+			    PARSE_OPT_OPTARG, NULL, (intptr_t)1 },
 		OPT_SET_INT(0, "list", &extra, "synonym to more=-1", -1),
 		OPT_BOOLEAN(0, "no-name", &no_name, "suppress naming strings"),
 		OPT_BOOLEAN(0, "current", &with_current_branch,
diff --git a/parse-options.h b/parse-options.h
index b374ade..5653dba 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -71,8 +71,11 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
  *   PARSE_OPT_NONEG: says that this option cannot be negated
  *   PARSE_OPT_HIDDEN: this option is skipped in the default usage, and
  *                     shown only in the full usage.
- *   PARSE_OPT_LASTARG_DEFAULT: if no argument is given, the default value
- *                              is used.
+ *   PARSE_OPT_LASTARG_DEFAULT: says that this option will take the default
+ *				value if no argument is given when the option
+ *				is last on the command line. If the option is
+ *				not last it will require an argument.
+ *				Should not be used with PARSE_OPT_OPTARG.
  *   PARSE_OPT_NODASH: this option doesn't start with a dash.
  *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets
  *				(i.e. '<argh>') in the help message.

-- 
1.6.3.2.202.g26c11

René Scharfe· Jun 8, 2009, 17:24 UTC · re: Stephen Boyd · lore

Re: [PATCH] show-branch: don't use LASTARG_DEFAULT with OPTARG

Stephen Boyd schrieb:

> 5734365 (show-branch: migrate to parse-options API 2009-05-21)
> incorrectly set the --more option's flags to be
> PARSE_OPT_LASTARG_DEFAULT and PARSE_OPT_OPTARG. These two flags
> shouldn't be used together. An option taking a default should just set
> the default value desired and parse options will take care of the rest.
> 
> Update the header comment to better convey this information.

Thank you!

Junio C Hamano· Jun 8, 2009, 21:56 UTC · re: Stephen Boyd · lore

Re: [PATCH] show-branch: don't use LASTARG_DEFAULT with OPTARG

Stephen Boyd <bebarino@gmail.com> writes:

> 5734365 (show-branch: migrate to parse-options API 2009-05-21)
> incorrectly set the --more option's flags to be
> PARSE_OPT_LASTARG_DEFAULT and PARSE_OPT_OPTARG. These two flags
> shouldn't be used together. An option taking a default should just set
> the default value desired and parse options will take care of the rest.

Pierre Habouzit· Jun 9, 2009, 08:23 UTC · re: Junio C Hamano · lore

[PATCH] parse-options: add parse_options_check to validate option specs.

Options are checked each time we call parse_options_start.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index e469fc0..34282ad 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -306,6 +306,28 @@ static void check_typos(const char *arg, const struct option *options)
 	}
 }
 
+static void parse_options_check(const struct option *opts)
+{
+	int err = 0;
+
+	for (; opts->type != OPTION_END; opts++) {
+		if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&
+		    (opts->flags & PARSE_OPT_OPTARG)) {
+			if (opts->long_name) {
+				error("`--%s` uses incompatible flags "
+				      "LASTARG_DEFAULT and OPTARG", opts->long_name);
+			} else {
+				error("`-%c` uses incompatible flags "
+				      "LASTARG_DEFAULT and OPTARG", opts->short_name);
+			}
+			err |= 1;
+		}
+	}
+
+	if (err)
+		exit(129);
+}
+
 void parse_options_start(struct parse_opt_ctx_t *ctx,
 			 int argc, const char **argv, const char *prefix,
 			 int flags)
@@ -331,6 +353,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 {
 	int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
 
+	parse_options_check(options);
+
 	/* we must reset ->opt, unknown short option leave it dangling */
 	ctx->opt = NULL;
 

-- 
1.6.3.2.323.gcd28f

Re: [PATCH] parse-options: add parse_options_check to validate option specs.

On Tue, Jun 09, 2009 at 10:23:44AM +0200, Pierre Habouzit wrote:

> It only searches for now for the dreaded LASTARG_DEFAULT | OPTARG
> combination, but can be extended to check for any other forbidden
> combination.
> 
> Options are checked each time we call parse_options_start.
> 
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>  parse-options.c |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

René Scharfe· Jun 12, 2009, 21:25 UTC · re: Pierre Habouzit · lore

Re: [PATCH] parse-options: add parse_options_check to validate option specs.

Pierre Habouzit schrieb:

> On Tue, Jun 09, 2009 at 10:23:44AM +0200, Pierre Habouzit wrote:
>> It only searches for now for the dreaded LASTARG_DEFAULT | OPTARG
>> combination, but can be extended to check for any other forbidden
>> combination.
>>
>> Options are checked each time we call parse_options_start.
>>
>> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
>> ---
>>  parse-options.c |   24 ++++++++++++++++++++++++
>>  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> Has this patch been missed, or has it any kind of flaw that _I_ missed ?
> :)
> 

It's in master (cb9d398c).