parse-options: ambiguous LASTARG_DEFAULT and OPTARG

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 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.

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.

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.

Ok, thanks. This means I used it wrong when I switched over show-branch
:-/ I'll have to send a follow-up patch for that.

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 was mislead. When I read it I thought I had to use the flag to say
that the default value will be used in the case when no argument is
given. I completely ignored the LASTARG part (I thought it was
referencing the default arg). I think just adding what you said here to
parse-options.h will help others to avoid this.

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.

"git tag --contains" is the same. Figuring out a way to say that the
syntax changes when it's the last option versus in the middle is not
obvious to me either.

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.

Signed-off-by: Stephen Boyd <redacted>
---
 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

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!