login
Search
This adapts "git update-index" to use the parse-options API (with resulting perks like nice "-h" output). Doing so reveals some potential improvements to parse-options infrastructure, too. See [1] for the previous version. This version incorporates the last few suggestions by Stephen. The iffiest bit is still handling of the --cacheinfo option. Thanks to Stephen and Junio for advice. Patches applies to maint, for no particular reason. [1] http://thread.gmane.org/gmane.comp.version-control.git/159386/focus=162463 Jonathan Nieder (7): parse-options: clearer reporting of API misuse parse-options: move NODASH sanity checks to parse_options_check parse-options: sanity check PARSE_OPT_NOARG flag parse-options: never suppress arghelp if LITERAL_ARGHELP is set parse-options: allow git commands to invent new option types parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTION update-index: migrate to parse-options API Nguyễn Thái Ngọc Duy (1): setup: save prefix (original cwd relative to toplevel) in startup_info Stephen Boyd (2): parse-options: Don't call parse_options_check() so much parse-options: do not infer PARSE_OPT_NOARG from option type builtin/blame.c | 2 +- builtin/shortlog.c | 2 +- builtin/update-index.c | 392 ++++++++++++++++++++++++++++++------------------ cache.h | 1 + parse-options.c | 85 +++++------ parse-options.h | 11 +- setup.c | 4 +- 7 files changed, 299 insertions(+), 198 deletions(-) --
From: Stephen Boyd <bebarino@gmail.com>
parse_options_check() is being called for each invocation of
parse_options_step() which can be quite a bit for some commands. The
commit introducing this function cb9d398 (parse-options: add
parse_options_check to validate option specs., 2009-06-09) had the
correct motivation and explicitly states that parse_options_check()
should be called from parse_options_start(). However, the implementation
differs from the motivation. Fix it.
Signed-off-by: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
I would prefer to put "options" before flags, but seemed better to
just get this out there. Unchanged.
builtin/blame.c | 2 +-
builtin/shortlog.c | 2 +-
parse-options.c | 7 +++----
parse-options.h | 2 +-
4 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 1015354..adf344c 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2299,7 +2299,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
dashdash_pos = 0;
parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
- PARSE_OPT_KEEP_ARGV0);
+ PARSE_OPT_KEEP_ARGV0, options);
for (;;) {
switch (parse_options_step(&ctx, options, blame_opt_usage)) {
case PARSE_OPT_HELP:
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 2135b0d..8473391 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -269,7 +269,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
shortlog_init(&log);
init_revisions(&rev, prefix);
parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
- PARSE_OPT_KEEP_ARGV0);
+ PARSE_OPT_KEEP_ARGV0, options);
for (;;) {
switch (parse_options_step(&ctx, options, shortlog_usage)) {
diff --git a/parse-options.c b/parse-options.c
index 0fa79bc..196ba71 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -338,7 +338,7 @@ static void ...It might be better to put options before flags, i.e. to use the same order as in parse_options(). René --
Jonathan mentioned that too. Sounds good to me. Here's an updated patch.
--->8----8<----
Subject: [PATCH] parse-options: Don't call parse_options_check() so much
parse_options_check() is being called for each invocation of
parse_options_step which can be quite a bit for some commands. The
commit introducing this function cb9d398 (parse-options: add
parse_options_check to validate option specs., 2009-06-09) had the
correct motivation and explicitly states that parse_options_check()
should be called from parse_options_start(). However, the implementation
differs from the motivation. Fix it.
Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
builtin/blame.c | 4 ++--
builtin/shortlog.c | 4 ++--
parse-options.c | 7 +++----
parse-options.h | 2 +-
4 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index cb25ec9..aa30ec5 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2325,8 +2325,8 @@ int cmd_blame(int argc, const char **argv, const
char *prefix)
save_commit_buffer = 0;
dashdash_pos = 0;
- parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
- PARSE_OPT_KEEP_ARGV0);
+ parse_options_start(&ctx, argc, argv, prefix, options,
+ PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
for (;;) {
switch (parse_options_step(&ctx, options, blame_opt_usage)) {
case PARSE_OPT_HELP:
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 2135b0d..1a21e4b 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -268,8 +268,8 @@ int cmd_shortlog(int argc, const char **argv, const
char *prefix)
git_config(git_default_config, NULL);
shortlog_init(&log);
init_revisions(&rev, prefix);
- parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
- PARSE_OPT_KEEP_ARGV0);
+ parse_options_start(&ctx, argc, argv, prefix, options,
+ PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
for (;;) {
switch (parse_options_step(&ctx, ...The PARSE_OPT_LASTARG_DEFAULT flag is meant for options like
--contains that (1) traditionally had a mandatory argument and
(2) have some better behavior to use when appearing in the final
position. It makes no sense to combine this with OPTARG, so ever
since v1.6.4-rc0~71 (parse-options: add parse_options_check to
validate option specs, 2009-07-09) this mistake is flagged with
error: `--option` uses incompatible flags LASTARG_DEFAULT and OPTARG
and an exit status representing an error in commandline usage.
Unfortunately that which might confuse scripters calling such an
erroneous program into thinking the _script_ contains an error.
Clarify that it is an internal error by dying with a message beginning
"fatal: BUG: ..." and status 128.
While at it, clean up parse_options_check to prepare for more checks.
Long term, it would be nicer to make such checks happen at compile
time.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
parse-options.c | 25 ++++++++++---------------
1 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 196ba71..12f100b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -11,6 +11,13 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
#define OPT_SHORT 1
#define OPT_UNSET 2
+static NORETURN void optbug(const struct option *opt, const char *reason)
+{
+ if (opt->long_name)
+ die("BUG: option '%s' %s", opt->long_name, reason);
+ die("BUG: switch '%c' %s", opt->short_name, reason);
+}
+
static int opterror(const struct option *opt, const char *reason, int flags)
{
if (flags & OPT_SHORT)
@@ -316,24 +323,12 @@ 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 ...Hmph, this is simpler but it does not report all errors any more.
So it would be better to do:
int err = 0;
for (; opts->type != OPTION_END; opts++) {
if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&
(opts->flags & PARSE_OPT_OPTARG))
err |= optbug(opts, "uses incompatible flags "
"LASTARG_DEFAULT and OPTARG");
}
if (err)
exit(128);
Sorry about that.
--
The PARSE_OPT_LASTARG_DEFAULT flag is meant for options like
--contains that (1) traditionally had a mandatory argument and
(2) have some better behavior to use when appearing in the final
position. It makes no sense to combine this with OPTARG, so ever
since v1.6.4-rc0~71 (parse-options: add parse_options_check to
validate option specs, 2009-07-09) this mistake is flagged with
error: `--option` uses incompatible flags LASTARG_DEFAULT and OPTARG
and an exit status representing an error in commandline usage.
Unfortunately that which might confuse scripters calling such an
erroneous program into thinking the _script_ contains an error.
Clarify that it is an internal error by dying with a message beginning
"error: BUG: ..." and status 128.
While at it, clean up parse_options_check to prepare for more checks.
Long term, it would be nicer to make such checks happen at compile
time.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Like this. (Looks okay now. Famous last words...)
parse-options.c | 23 +++++++++++------------
1 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 196ba71..97d7ff7 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -11,6 +11,13 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
#define OPT_SHORT 1
#define OPT_UNSET 2
+static int optbug(const struct option *opt, const char *reason)
+{
+ if (opt->long_name)
+ return error("BUG: option '%s' %s", opt->long_name, reason);
+ return error("BUG: switch '%c' %s", opt->short_name, reason);
+}
+
static int opterror(const struct option *opt, const char *reason, int flags)
{
if (flags & OPT_SHORT)
@@ -320,20 +327,12 @@ static void parse_options_check(const struct option *opts)
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 "
- ...The PARSE_OPT_LASTARG_DEFAULT flag is meant for options like
--contains that (1) traditionally had a mandatory argument and
(2) have some better behavior to use when appearing in the final
position. It makes no sense to combine this with OPTARG, so ever
since v1.6.4-rc0~71 (parse-options: add parse_options_check to
validate option specs, 2009-07-09) this mistake is flagged with
error: `--option` uses incompatible flags LASTARG_DEFAULT and OPTARG
and an exit status representing an error in commandline usage.
Unfortunately that which might confuse scripters calling such an
erroneous program into thinking the _script_ contains an error.
Clarify that it is an internal error by dying with a message beginning
"error: BUG: ..." and status 128.
While at it, clean up parse_options_check to prepare for more checks.
Long term, it would be nicer to make such checks happen at compile
time.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
[Resending because vger doesn't seem to have accepted the last copy.
Sorry for the patch flood.]
Like this. (Looks okay now. Famous last words...)
parse-options.c | 23 +++++++++++------------
1 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 196ba71..97d7ff7 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -11,6 +11,13 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
#define OPT_SHORT 1
#define OPT_UNSET 2
+static int optbug(const struct option *opt, const char *reason)
+{
+ if (opt->long_name)
+ return error("BUG: option '%s' %s", opt->long_name, reason);
+ return error("BUG: switch '%c' %s", opt->short_name, reason);
+}
+
static int opterror(const struct option *opt, const char *reason, int flags)
{
if (flags & OPT_SHORT)
@@ -320,20 +327,12 @@ static void parse_options_check(const struct option *opts)
for (; opts->type != OPTION_END; opts++) {
if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&
- (opts->flags & ...A dashless switch (like '(' passed to 'git grep') cannot be negated,
cannot be attached to an argument, and cannot have a long form.
Currently parse-options runs the related sanity checks when the
dashless option is used; better to always check them at the start of
option parsing, so mistakes can be caught more quickly.
The error message at the new call site is less specific about the
nature of the error, for simplicity. On the other hand, it is more
specific in that it prints which switch was problematic. Before:
fatal: BUG: dashless options can't be long
After:
fatal: BUG: switch '(' uses feature not supported for dashless options
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
parse-options.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 12f100b..c825620 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -288,13 +288,6 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,
for (; options->type != OPTION_END; options++) {
if (!(options->flags & PARSE_OPT_NODASH))
continue;
- if ((options->flags & PARSE_OPT_OPTARG) ||
- !(options->flags & PARSE_OPT_NOARG))
- die("BUG: dashless options don't support arguments");
- if (!(options->flags & PARSE_OPT_NONEG))
- die("BUG: dashless options don't support negation");
- if (options->long_name)
- die("BUG: dashless options can't be long");
if (options->short_name == arg[0] && arg[1] == '\0')
return get_value(p, options, OPT_SHORT);
}
@@ -328,6 +321,13 @@ static void parse_options_check(const struct option *opts)
(opts->flags & PARSE_OPT_OPTARG))
optbug(opts, "uses incompatible flags "
"LASTARG_DEFAULT and OPTARG");
+ if (opts->flags & PARSE_OPT_NODASH &&
+ ((opts->flags & PARSE_OPT_OPTARG) ||
+ !(opts->flags & PARSE_OPT_NOARG) ||
+ !(opts->flags & PARSE_OPT_NONEG) ||
+ opts->long_name))
+ optbug(opts, "uses feature "
+ "not ...A dashless switch (like '(' passed to 'git grep') cannot be negated,
cannot be attached to an argument, and cannot have a long form.
Currently parse-options runs the related sanity checks when the
dashless option is used; better to always check them at the start of
option parsing, so mistakes can be caught more quickly.
The error message at the new call site is less specific about the
nature of the error, for simplicity. On the other hand, it prints
which switch was problematic. Before:
fatal: BUG: dashless options can't be long
After:
error: BUG: switch '(' uses feature not supported for dashless options
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Change since v1:
- rebase on top of patch 2 v2.
parse-options.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 97d7ff7..9f6d305 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -288,13 +288,6 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,
for (; options->type != OPTION_END; options++) {
if (!(options->flags & PARSE_OPT_NODASH))
continue;
- if ((options->flags & PARSE_OPT_OPTARG) ||
- !(options->flags & PARSE_OPT_NOARG))
- die("BUG: dashless options don't support arguments");
- if (!(options->flags & PARSE_OPT_NONEG))
- die("BUG: dashless options don't support negation");
- if (options->long_name)
- die("BUG: dashless options can't be long");
if (options->short_name == arg[0] && arg[1] == '\0')
return get_value(p, options, OPT_SHORT);
}
@@ -330,6 +323,13 @@ static void parse_options_check(const struct option *opts)
(opts->flags & PARSE_OPT_OPTARG))
err |= optbug(opts, "uses incompatible flags "
"LASTARG_DEFAULT and OPTARG");
+ if (opts->flags & PARSE_OPT_NODASH &&
+ ((opts->flags & PARSE_OPT_OPTARG) ||
+ !(opts->flags & PARSE_OPT_NOARG) ||
+ !(opts->flags & PARSE_OPT_NONEG) ||
+ opts->long_name))
+ err |= ...Some option types cannot use an argument --- boolean options that
would set a bit or flag or increment a counter, for example. If
configured in the flag word to accept an argument anyway, the result
is an argument that is advertised in "program -h" output only to be
rejected by parse-options::get_value.
Luckily all current users of these option types use PARSE_OPT_NOARG
and do not use PARSE_OPT_OPTARG. Add a check to ensure that that
remains true. The check is run once for each invocation of
parse_option_start().
Improved-by: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
parse-options.c | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index c825620..e4d0b82 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -328,6 +328,19 @@ static void parse_options_check(const struct option *opts)
opts->long_name))
optbug(opts, "uses feature "
"not supported for dashless options");
+ switch (opts->type) {
+ case OPTION_BOOLEAN:
+ case OPTION_BIT:
+ case OPTION_NEGBIT:
+ case OPTION_SET_INT:
+ case OPTION_SET_PTR:
+ case OPTION_NUMBER:
+ if ((opts->flags & PARSE_OPT_OPTARG) ||
+ !(opts->flags & PARSE_OPT_NOARG))
+ optbug(opts, "should not accept an argument");
+ default:
+ ; /* ok. (usually accepts an argument) */
+ }
}
}
--
1.7.2.3
--
Some option types cannot use an argument --- boolean options that
would set a bit or flag or increment a counter, for example. If
configured in the flag word to accept an argument anyway, the result
is an argument that is advertised in "program -h" output only to be
rejected by parse-options::get_value.
Luckily all current users of these option types use PARSE_OPT_NOARG
and do not use PARSE_OPT_OPTARG. Add a check to ensure that that
remains true. The check is run once for each invocation of
parse_option_start().
Improved-by: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Changes since v1:
- adapt for updated patch 2/10
parse-options.c | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 9f6d305..74ab0c8 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -330,6 +330,19 @@ static void parse_options_check(const struct option *opts)
opts->long_name))
err |= optbug(opts, "uses feature "
"not supported for dashless options");
+ switch (opts->type) {
+ case OPTION_BOOLEAN:
+ case OPTION_BIT:
+ case OPTION_NEGBIT:
+ case OPTION_SET_INT:
+ case OPTION_SET_PTR:
+ case OPTION_NUMBER:
+ if ((opts->flags & PARSE_OPT_OPTARG) ||
+ !(opts->flags & PARSE_OPT_NOARG))
+ err |= optbug(opts, "should not accept an argument");
+ default:
+ ; /* ok. (usually accepts an argument) */
+ }
}
if (err)
exit(128);
--
1.7.2.3
--
From: Stephen Boyd <bebarino@gmail.com>
Simplify the "takes no value" error path by relying on PARSE_OPT_NOARG
being set correctly. That is:
- if the PARSE_OPT_NOARG flag is set, reject --opt=value
regardless of the option type;
- if the PARSE_OPT_NOARG flag is unset, accept --opt=value
regardless of the option type.
This way, the accepted usage more closely matches the usage advertised
with --help-all.
No functional change intended, since the NOARG flag is only used
with "boolean-only" option types in existing parse_options callers.
Signed-off-by: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
parse-options.c | 19 ++-----------------
1 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index e4d0b82..684d330 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -62,23 +62,8 @@ static int get_value(struct parse_opt_ctx_t *p,
return opterror(opt, "takes no value", flags);
if (unset && (opt->flags & PARSE_OPT_NONEG))
return opterror(opt, "isn't available", flags);
-
- if (!(flags & OPT_SHORT) && p->opt) {
- switch (opt->type) {
- case OPTION_CALLBACK:
- if (!(opt->flags & PARSE_OPT_NOARG))
- break;
- /* FALLTHROUGH */
- case OPTION_BOOLEAN:
- case OPTION_BIT:
- case OPTION_NEGBIT:
- case OPTION_SET_INT:
- case OPTION_SET_PTR:
- return opterror(opt, "takes no value", flags);
- default:
- break;
- }
- }
+ if (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))
+ return opterror(opt, "takes no value", flags);
switch (opt->type) {
case OPTION_BIT:
--
1.7.2.3
--
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.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/parse-options.c b/parse-options.c index 684d330..b640ac5 100644 --- a/parse-options.c +++ b/parse-options.c @@ -533,7 +533,8 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx, if (opts->type == OPTION_NUMBER) pos += fprintf(outfile, "-NUM"); - if (!(opts->flags & PARSE_OPT_NOARG)) + if ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) || + !(opts->flags & PARSE_OPT_NOARG)) pos += usage_argh(opts, outfile); if (pos <= USAGE_OPTS_WIDTH) -- 1.7.2.3 --
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. --
Just to clarify: the NOARG was not meant to affect the usage message but the actual accepted usage. The idea was that git update-index --cacheinfo=100644 87a8767c87b file.c should be rejected, because if it is accepted that would tempt people to try git update-index --cacheinfo=100644 -q 87a8767c87b file.c which fails. That is, the argument to --cacheinfo is not <mode>, since --cacheinfo takes _three_ arguments and therefore the sticked I agree with your conclusion. Let's drop this patch, and I'll look into adding the check and a PARSE_OPT_NOSTICKED flag. --
Sorry, I don't quite understand why we should reject the sticked form when we don't advertise its usage anywhere (man pages or usageh). Maybe I'm just not thinking right since I'm' optimistic people won't do that -q thing. But if you really want to do it we don't really need to add a new flag right? We can just die in the cacheinfo callback if the context's opt pointer is set. If this ever becomes a common thing we can add the flag later. Putting a comment like "if only we had PARSE_OPT_NOSTICKED..." would be fine too. I'll be fine with or without the flag though. --
parse-options provides a variety of option behaviors, including
OPTION_CALLBACK, which should take care of just about any sane
behavior. All supported behaviors obey the following constraint:
A --foo option can only accept (and base its behavior on)
one argument, which would be the following command-line
argument in the "unsticked" form.
Alas, some existing git commands have options that do not obey that
constraint. For example, update-index --cacheinfo takes three
arguments, and update-index --resolve takes all later parameters as
arguments.
Introduces an OPTION_LOWLEVEL_CALLBACK backdoor to parse-options so
such option types can be supported without tempting inventors of other
commands through mention in the public API. Commands can set the
callback field to a function accepting three arguments: the option
parsing context, the option itself, and a flag indicating whether the
the option was negated. When the option is encountered, that function
is called to take over from get_value(). The return value should be
zero for success, -1 for usage errors.
Thanks to Stephen Boyd for API guidance.
Improved-by: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
parse-options.c | 3 +++
parse-options.h | 8 +++++++-
2 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index b640ac5..4c58e7f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -66,6 +66,9 @@ static int get_value(struct parse_opt_ctx_t *p,
return opterror(opt, "takes no value", flags);
switch (opt->type) {
+ case OPTION_LOWLEVEL_CALLBACK:
+ return (*(parse_opt_ll_cb *)opt->callback)(p, opt, unset);
+
case OPTION_BIT:
if (unset)
*(int *)opt->value &= ~opt->defval;
diff --git a/parse-options.h b/parse-options.h
index cfa03d5..ab1bdf0 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -17,6 +17,7 @@ enum parse_opt_type {
OPTION_STRING,
OPTION_INTEGER,
...Introduce a PARSE_OPT_NON_OPTION state, so parse_option_step()
callers can easily distinguish between non-options and other
reasons for option parsing termination (like "--").
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
parse-options.c | 3 ++-
parse-options.h | 1 +
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 4c58e7f..4b000e6 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -369,7 +369,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
if (parse_nodash_opt(ctx, arg, options) == 0)
continue;
if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
- break;
+ return PARSE_OPT_NON_OPTION;
ctx->out[ctx->cpidx++] = ctx->argv[0];
continue;
}
@@ -451,6 +451,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
switch (parse_options_step(&ctx, options, usagestr)) {
case PARSE_OPT_HELP:
exit(129);
+ case PARSE_OPT_NON_OPTION:
case PARSE_OPT_DONE:
break;
default: /* PARSE_OPT_UNKNOWN */
diff --git a/parse-options.h b/parse-options.h
index ab1bdf0..b897d6b 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -167,6 +167,7 @@ extern NORETURN void usage_msg_opt(const char *msg,
enum {
PARSE_OPT_HELP = -1,
PARSE_OPT_DONE,
+ PARSE_OPT_NON_OPTION,
PARSE_OPT_UNKNOWN
};
--
1.7.2.3
--
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Save the path from the original cwd to the cwd at the end of the
setup procedure in the startup_info struct introduced in e37c1329
(2010-08-05). The value cannot vary from thread to thread anyway,
since the cwd is global.
So now in your builtin command, instead of passing prefix around,
when you want to convert a user-supplied path to a cwd-relative
path, you can use startup_info->prefix directly.
Caveat: As with the return value from setup_git_directory_gently(),
startup_info->prefix would be NULL when the original cwd is not a
subdir of the toplevel.
Longer term, this would allow the prefix to be reused when several
noncooperating functions require access to the same repository (for
example, when accessing configuration before running a builtin).
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
cache.h | 1 +
setup.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/cache.h b/cache.h
index 33decd9..222d9cf 100644
--- a/cache.h
+++ b/cache.h
@@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
/* git.c */
struct startup_info {
int have_repository;
+ const char *prefix;
};
extern struct startup_info *startup_info;
diff --git a/setup.c b/setup.c
index a3b76de..833db12 100644
--- a/setup.c
+++ b/setup.c
@@ -512,8 +512,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
const char *prefix;
prefix = setup_git_directory_gently_1(nongit_ok);
- if (startup_info)
+ if (startup_info) {
startup_info->have_repository = !nongit_ok || !*nongit_ok;
+ startup_info->prefix = prefix;
+ }
return prefix;
}
--
1.7.2.3
--
--refresh and --really-refresh accept flags (like -q) and modify an error indicator. It might make sense to make the error indicator global, but just pass the flags and a pointer to the error indicator in a struct instead. --cacheinfo wants 3 arguments. Use the OPTION_LOWLEVEL_CALLBACK extension to grab them and PARSE_OPT_NOARG to disallow the "sticked" --cacheinfo=foo form. (The resulting message $ git update-index --cacheinfo=foo error: option `cacheinfo' takes no value is unfortunately incorrect.) --assume-unchanged and --no-assume-unchanged probably should use the OPT_UYN feature; but use a callback for now so the existing MARK_FLAG and UNMARK_FLAG values can be used. --stdin and --index-info are still constrained to be the last argument (implemented using the OPTION_LOWLEVEL_CALLBACK extension). --unresolve and --again consume all arguments that come after them (also using OPTION_LOWLEVEL_CALLBACK). The order of options matters. Each path on the command line is affected only by the options that come before it. A custom argument-parsing loop with parse_options_step() brings that about. In exchange for all the fuss, we get the usual perks: support for un-sticked options, better usage error messages, more useful -h output, and argument parsing code that should be easier to tweak in the future. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- builtin/update-index.c | 392 ++++++++++++++++++++++++++++++------------------ 1 files changed, 243 insertions(+), 149 deletions(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index 62d9f3f..869f5b1 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -10,6 +10,7 @@ #include "builtin.h" #include "refs.h" #include "resolve-undo.h" +#include "parse-options.h" /* * Default to not allowing changes to the list of files. The @@ -397,8 +398,10 @@ static void read_index_info(int line_termination) strbuf_release(&uq); } -static const char ...
