From: Stephen Boyd <bebarino@gmail.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: git@vger.kernel.org, "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
"Pierre Habouzit" <madcoder@debian.org>
Subject: Re: [PATCH 06/10] parse-options: never suppress arghelp if LITERAL_ARGHELP is set
Date: Fri, 03 Dec 2010 01:16:15 -0800 [thread overview]
Message-ID: <4CF8B55F.8020203@gmail.com> (raw)
In-Reply-To: <20101201233136.GG31815@burratino>
On 12/01/10 15:31, Jonathan Nieder wrote:
> The PARSE_OPT_LITERAL_ARGHELP flag allows a program to override the
> standard "<argument> for mandatory, [argument] for optional" markup in
> its help message. Extend it to override the usual "no text for
> disallowed", too (for the PARSE_OPT_NOARG | PARSE_OPT_LITERAL_ARGHELP
> case, which was previously meaningless), to be more intuitive.
>
> The motivation is to allow update-index to correctly advertise
>
> --cacheinfo <mode> <object> <path>
> add the specified entry to the index
>
> while abusing PARSE_OPT_NOARG to disallow the "sticked form"
>
> --cacheinfo=<mode> <object> <path>
>
> Noticed-by: Stephen Boyd <bebarino@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> This seems like the intuitive thing to do, but the motivating use case
> is iffy. Might be better to introduce a PARSE_OPT_NOSTICKED flag.
parse-options should accept both forms of --cacheinfo above if the
option isn't marked PARSE_OPT_NOARG. Marking it NOARG to get rid of the
equals sign in the usage seems wrong when both the equals sign and no
equals sign can be accepted. The fault lies in the implementation of the
low-level callback. It needs to handle more of the parsing.
If --cacheinfo is marked PARSE_OPT_NOARG it should show
--cacheinfo
If --cacheinfo is marked PARSE_OPT_OPTARG it should show
--cacheinfo[=<argh>]
If --cacheinfo isn't marked either of the above it should show
--cacheinfo <argh>
which looks like what you want and it also says it takes an argument
unconditionally (perfect?). On top of that, the equals sign is optional
(or at least should be). At the point of get_value() p->opt will contain
whatever is after the equals sign for the option that is parsed. For
options which take no value (NOARG) they'll fail the test if they have
anything with an equals sign after it.
By not marking the cacheinfo option NOARG, we've successfully gotten
what we want so far. Now we just need to make the cacheinfo callback a
bit more like get_arg() and have it look at the context argument. If
p->opt is set, we should pull <mode> out of that and make sure we have
two more arguments. If p->opt isn't set, we should make sure we have 3
arguments and basically do what is already there.
I know this is a bit more code, but it also means that we don't have
this approach bite someone else down the line when they make assumptions
about options marked as NOARG not taking arguments.
We should probably add another check like "if argh is set and
PARSE_OPT_NOARG is true error out" so this can't be done.
next prev parent reply other threads:[~2010-12-03 9:16 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-12-01 23:27 [PATCH v3 00/10] update-index: migrate to parse-options API Jonathan Nieder
2010-12-01 23:28 ` [PATCH 01/10] parse-options: Don't call parse_options_check() so much Jonathan Nieder
2010-12-05 18:14 ` René Scharfe
2010-12-06 7:57 ` Stephen Boyd
2010-12-01 23:29 ` [PATCH 02/10] parse-options: clearer reporting of API misuse Jonathan Nieder
2010-12-02 4:57 ` Jonathan Nieder
2010-12-02 6:01 ` [PATCH 02/10 v2] " Jonathan Nieder
2010-12-02 6:13 ` [PATCH 02/10 v2 resend] " Jonathan Nieder
2010-12-01 23:29 ` [PATCH 03/10] parse-options: move NODASH sanity checks to parse_options_check Jonathan Nieder
2010-12-02 6:05 ` [PATCH 03/10 v2] " Jonathan Nieder
2010-12-01 23:30 ` [PATCH 04/10] parse-options: sanity check PARSE_OPT_NOARG flag Jonathan Nieder
2010-12-02 6:08 ` [PATCH 04/10 v2] " Jonathan Nieder
2010-12-01 23:30 ` [PATCH 05/10] parse-options: do not infer PARSE_OPT_NOARG from option type Jonathan Nieder
2010-12-01 23:31 ` [PATCH 06/10] parse-options: never suppress arghelp if LITERAL_ARGHELP is set Jonathan Nieder
2010-12-03 9:16 ` Stephen Boyd [this message]
2010-12-03 9:40 ` Jonathan Nieder
2010-12-03 17:53 ` Stephen Boyd
2010-12-01 23:32 ` [PATCH 07/10] parse-options: allow git commands to invent new option types Jonathan Nieder
2010-12-01 23:32 ` [PATCH 08/10] parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTION Jonathan Nieder
2010-12-01 23:33 ` [PATCH 09/10] setup: save prefix (original cwd relative to toplevel) in startup_info Jonathan Nieder
2010-12-01 23:34 ` [PATCH 10/10] update-index: migrate to parse-options API Jonathan Nieder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4CF8B55F.8020203@gmail.com \
--to=bebarino@gmail.com \
--cc=git@vger.kernel.org \
--cc=jrnieder@gmail.com \
--cc=madcoder@debian.org \
--cc=pclouds@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).