[PATCH v3 00/10] update-index: migrate to parse-options API

[PATCH v3 00/10] update-index: migrate to parse-options API

[Jonathan Nieder]

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(-)

[PATCH 01/10] parse-options: Don't call parse_options_check() so much

[Jonathan Nieder]

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 parse_options_check(const struct option *opts)
 
 void parse_options_start(struct parse_opt_ctx_t *ctx,
 			 int argc, const char **argv, const char *prefix,
-			 int flags)
+			 int flags, const struct option *options)
 {
 	memset(ctx, 0, sizeof(*ctx));
 	ctx->argc = argc - 1;
@@ -350,6 +350,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
 	    (flags & PARSE_OPT_STOP_AT_NON_OPTION))
 		die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
+	parse_options_check(options);
 }
 
 static int usage_with_options_internal(struct parse_opt_ctx_t *,
@@ -362,8 +363,6 @@ 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;
 
@@ -452,7 +451,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 {
 	struct parse_opt_ctx_t ctx;
 
-	parse_options_start(&ctx, argc, argv, prefix, flags);
+	parse_options_start(&ctx, argc, argv, prefix, flags, options);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 		exit(129);
diff --git a/parse-options.h b/parse-options.h
index d982f0f..cfa03d5 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -180,7 +180,7 @@ struct parse_opt_ctx_t {
 
 extern void parse_options_start(struct parse_opt_ctx_t *ctx,
 				int argc, const char **argv, const char *prefix,
-				int flags);
+				int flags, const struct option *options);
 
 extern int parse_options_step(struct parse_opt_ctx_t *ctx,
 			      const struct option *options,
-- 
1.7.2.3

[PATCH 02/10] parse-options: clearer reporting of API misuse

[Jonathan Nieder]

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 incompatible flags "
-				      "LASTARG_DEFAULT and OPTARG", opts->long_name);
-			} else {
-				error("`-%c` uses incompatible flags "
-				      "LASTARG_DEFAULT and OPTARG", opts->short_name);
-			}
-			err |= 1;
-		}
+		    (opts->flags & PARSE_OPT_OPTARG))
+			optbug(opts, "uses incompatible flags "
+				"LASTARG_DEFAULT and OPTARG");
 	}
-
-	if (err)
-		exit(129);
 }
 
 void parse_options_start(struct parse_opt_ctx_t *ctx,
-- 
1.7.2.3

[PATCH 03/10] parse-options: move NODASH sanity checks to parse_options_check

[Jonathan Nieder]

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 supported for dashless options");
 	}
 }
 
-- 
1.7.2.3

[PATCH 04/10] parse-options: sanity check PARSE_OPT_NOARG flag

[Jonathan Nieder]

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

[PATCH 05/10] parse-options: do not infer PARSE_OPT_NOARG from option type

[Jonathan Nieder]

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

[PATCH 06/10] parse-options: never suppress arghelp if LITERAL_ARGHELP is set

[Jonathan Nieder]

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

[PATCH 07/10] parse-options: allow git commands to invent new option types

[Jonathan Nieder]

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,
 	OPTION_CALLBACK,
+	OPTION_LOWLEVEL_CALLBACK,
 	OPTION_FILENAME
 };
 
@@ -43,6 +44,10 @@ enum parse_opt_option_flags {
 struct option;
 typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
 
+struct parse_opt_ctx_t;
+typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
+				const struct option *opt, int unset);
+
 /*
  * `type`::
  *   holds the type of the option, you must have an OPTION_END last in your
@@ -87,7 +92,8 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
  *				useful for users of OPTION_NEGBIT.
  *
  * `callback`::
- *   pointer to the callback to use for OPTION_CALLBACK.
+ *   pointer to the callback to use for OPTION_CALLBACK or
+ *   OPTION_LOWLEVEL_CALLBACK.
  *
  * `defval`::
  *   default value to fill (*->value) with for PARSE_OPT_OPTARG.
-- 
1.7.2.3

[PATCH 08/10] parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTION

[Jonathan Nieder]

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

[PATCH 09/10] setup: save prefix (original cwd relative to toplevel) in startup_info

[Jonathan Nieder]

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

[PATCH 10/10] update-index: migrate to parse-options API

[Jonathan Nieder]

--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 update_index_usage[] =
-"git update-index [-q] [--add] [--replace] [--remove] [--unmerged] [--refresh] [--really-refresh] [--cacheinfo] [--chmod=(+|-)x] [--assume-unchanged] [--skip-worktree|--no-skip-worktree] [--info-only] [--force-remove] [--stdin] [--index-info] [--unresolve] [--again | -g] [--ignore-missing] [-z] [--verbose] [--] [<file>...]";
+static const char * const update_index_usage[] = {
+	"git update-index [options] [--] [<file>...]",
+	NULL
+};
 
 static unsigned char head_sha1[20];
 static unsigned char merge_head_sha1[20];
@@ -578,16 +581,211 @@ static int do_reupdate(int ac, const char **av,
 	return 0;
 }
 
+struct refresh_params {
+	unsigned int flags;
+	int *has_errors;
+};
+
+static int refresh(struct refresh_params *o, unsigned int flag)
+{
+	setup_work_tree();
+	*o->has_errors |= refresh_cache(o->flags | flag);
+	return 0;
+}
+
+static int refresh_callback(const struct option *opt,
+				const char *arg, int unset)
+{
+	return refresh(opt->value, 0);
+}
+
+static int really_refresh_callback(const struct option *opt,
+				const char *arg, int unset)
+{
+	return refresh(opt->value, REFRESH_REALLY);
+}
+
+static int chmod_callback(const struct option *opt,
+				const char *arg, int unset)
+{
+	char *flip = opt->value;
+	if ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])
+		return error("option 'chmod' expects \"+x\" or \"-x\"");
+	*flip = arg[0];
+	return 0;
+}
+
+static int resolve_undo_clear_callback(const struct option *opt,
+				const char *arg, int unset)
+{
+	resolve_undo_clear();
+	return 0;
+}
+
+static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,
+				const struct option *opt, int unset)
+{
+	unsigned char sha1[20];
+	unsigned int mode;
+
+	if (ctx->argc <= 3)
+		return error("option 'cacheinfo' expects three arguments");
+	if (strtoul_ui(*++ctx->argv, 8, &mode) ||
+	    get_sha1_hex(*++ctx->argv, sha1) ||
+	    add_cacheinfo(mode, sha1, *++ctx->argv, 0))
+		die("git update-index: --cacheinfo cannot add %s", *ctx->argv);
+	ctx->argc -= 3;
+	return 0;
+}
+
+static int stdin_cacheinfo_callback(struct parse_opt_ctx_t *ctx,
+			      const struct option *opt, int unset)
+{
+	int *line_termination = opt->value;
+
+	if (ctx->argc != 1)
+		return error("option '%s' must be the last argument", opt->long_name);
+	allow_add = allow_replace = allow_remove = 1;
+	read_index_info(*line_termination);
+	return 0;
+}
+
+static int stdin_callback(struct parse_opt_ctx_t *ctx,
+				const struct option *opt, int unset)
+{
+	int *read_from_stdin = opt->value;
+
+	if (ctx->argc != 1)
+		return error("option '%s' must be the last argument", opt->long_name);
+	*read_from_stdin = 1;
+	return 0;
+}
+
+static int unresolve_callback(struct parse_opt_ctx_t *ctx,
+				const struct option *opt, int flags)
+{
+	int *has_errors = opt->value;
+	const char *prefix = startup_info->prefix;
+
+	/* consume remaining arguments. */
+	*has_errors = do_unresolve(ctx->argc, ctx->argv,
+				prefix, prefix ? strlen(prefix) : 0);
+	if (*has_errors)
+		active_cache_changed = 0;
+
+	ctx->argv += ctx->argc - 1;
+	ctx->argc = 1;
+	return 0;
+}
+
+static int reupdate_callback(struct parse_opt_ctx_t *ctx,
+				const struct option *opt, int flags)
+{
+	int *has_errors = opt->value;
+	const char *prefix = startup_info->prefix;
+
+	/* consume remaining arguments. */
+	setup_work_tree();
+	*has_errors = do_reupdate(ctx->argc, ctx->argv,
+				prefix, prefix ? strlen(prefix) : 0);
+	if (*has_errors)
+		active_cache_changed = 0;
+
+	ctx->argv += ctx->argc - 1;
+	ctx->argc = 1;
+	return 0;
+}
+
 int cmd_update_index(int argc, const char **argv, const char *prefix)
 {
-	int i, newfd, entries, has_errors = 0, line_termination = '\n';
-	int allow_options = 1;
+	int newfd, entries, has_errors = 0, line_termination = '\n';
 	int read_from_stdin = 0;
 	int prefix_length = prefix ? strlen(prefix) : 0;
 	char set_executable_bit = 0;
-	unsigned int refresh_flags = 0;
+	struct refresh_params refresh_args = {0, &has_errors};
 	int lock_error = 0;
 	struct lock_file *lock_file;
+	struct parse_opt_ctx_t ctx;
+	int parseopt_state = PARSE_OPT_UNKNOWN;
+	struct option options[] = {
+		OPT_BIT('q', NULL, &refresh_args.flags,
+			"continue refresh even when index needs update",
+			REFRESH_QUIET),
+		OPT_BIT(0, "ignore-submodules", &refresh_args.flags,
+			"refresh: ignore submodules",
+			REFRESH_IGNORE_SUBMODULES),
+		OPT_SET_INT(0, "add", &allow_add,
+			"do not ignore new files", 1),
+		OPT_SET_INT(0, "replace", &allow_replace,
+			"let files replace directories and vice-versa", 1),
+		OPT_SET_INT(0, "remove", &allow_remove,
+			"notice files missing from worktree", 1),
+		OPT_BIT(0, "unmerged", &refresh_args.flags,
+			"refresh even if index contains unmerged entries",
+			REFRESH_UNMERGED),
+		{OPTION_CALLBACK, 0, "refresh", &refresh_args, NULL,
+			"refresh stat information",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+			refresh_callback},
+		{OPTION_CALLBACK, 0, "really-refresh", &refresh_args, NULL,
+			"like --refresh, but ignore assume-unchanged setting",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+			really_refresh_callback},
+		{OPTION_LOWLEVEL_CALLBACK, 0, "cacheinfo", NULL,
+			"<mode> <object> <path>",
+			"add the specified entry to the index",
+			PARSE_OPT_NOARG |	/* disallow --cacheinfo=<mode> form */
+			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
+			(parse_opt_cb *) cacheinfo_callback},
+		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, "(+/-)x",
+			"override the executable bit of the listed files",
+			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
+			chmod_callback},
+		{OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
+			"mark files as \"not changing\"",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
+		{OPTION_SET_INT, 0, "no-assume-unchanged", &mark_valid_only, NULL,
+			"clear assumed-unchanged bit",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},
+		{OPTION_SET_INT, 0, "skip-worktree", &mark_skip_worktree_only, NULL,
+			"mark files as \"index-only\"",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
+		{OPTION_SET_INT, 0, "no-skip-worktree", &mark_skip_worktree_only, NULL,
+			"clear skip-worktree bit",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},
+		OPT_SET_INT(0, "info-only", &info_only,
+			"add to index only; do not add content to object database", 1),
+		OPT_SET_INT(0, "force-remove", &force_remove,
+			"remove named paths even if present in worktree", 1),
+		OPT_SET_INT('z', NULL, &line_termination,
+			"with --stdin: input lines are terminated by null bytes", '\0'),
+		{OPTION_LOWLEVEL_CALLBACK, 0, "stdin", &read_from_stdin, NULL,
+			"read list of paths to be updated from standard input",
+			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			(parse_opt_cb *) stdin_callback},
+		{OPTION_LOWLEVEL_CALLBACK, 0, "index-info", &line_termination, NULL,
+			"add entries from standard input to the index",
+			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			(parse_opt_cb *) stdin_cacheinfo_callback},
+		{OPTION_LOWLEVEL_CALLBACK, 0, "unresolve", &has_errors, NULL,
+			"repopulate stages #2 and #3 for the listed paths",
+			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			(parse_opt_cb *) unresolve_callback},
+		{OPTION_LOWLEVEL_CALLBACK, 'g', "again", &has_errors, NULL,
+			"only update entries that differ from HEAD",
+			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			(parse_opt_cb *) reupdate_callback},
+		OPT_BIT(0, "ignore-missing", &refresh_args.flags,
+			"ignore files missing from worktree",
+			REFRESH_IGNORE_MISSING),
+		OPT_SET_INT(0, "verbose", &verbose,
+			"report actions to standard output", 1),
+		{OPTION_CALLBACK, 0, "clear-resolve-undo", NULL, NULL,
+			"(for porcelains) forget saved unresolved conflicts",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+			resolve_undo_clear_callback},
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
 
@@ -602,151 +800,48 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	if (entries < 0)
 		die("cache corrupted");
 
-	for (i = 1 ; i < argc; i++) {
-		const char *path = argv[i];
-		const char *p;
+	/*
+	 * Custom copy of parse_options() because we want to handle
+	 * filename arguments as they come.
+	 */
+	parse_options_start(&ctx, argc, argv, prefix,
+			    PARSE_OPT_STOP_AT_NON_OPTION, options);
+	while (ctx.argc) {
+		if (parseopt_state != PARSE_OPT_DONE)
+			parseopt_state = parse_options_step(&ctx, options,
+							    update_index_usage);
+		if (!ctx.argc)
+			break;
+		switch (parseopt_state) {
+		case PARSE_OPT_HELP:
+			exit(129);
+		case PARSE_OPT_NON_OPTION:
+		case PARSE_OPT_DONE:
+		{
+			const char *path = ctx.argv[0];
+			const char *p;
 
-		if (allow_options && *path == '-') {
-			if (!strcmp(path, "--")) {
-				allow_options = 0;
-				continue;
-			}
-			if (!strcmp(path, "-q")) {
-				refresh_flags |= REFRESH_QUIET;
-				continue;
-			}
-			if (!strcmp(path, "--ignore-submodules")) {
-				refresh_flags |= REFRESH_IGNORE_SUBMODULES;
-				continue;
-			}
-			if (!strcmp(path, "--add")) {
-				allow_add = 1;
-				continue;
-			}
-			if (!strcmp(path, "--replace")) {
-				allow_replace = 1;
-				continue;
-			}
-			if (!strcmp(path, "--remove")) {
-				allow_remove = 1;
-				continue;
-			}
-			if (!strcmp(path, "--unmerged")) {
-				refresh_flags |= REFRESH_UNMERGED;
-				continue;
-			}
-			if (!strcmp(path, "--refresh")) {
-				setup_work_tree();
-				has_errors |= refresh_cache(refresh_flags);
-				continue;
-			}
-			if (!strcmp(path, "--really-refresh")) {
-				setup_work_tree();
-				has_errors |= refresh_cache(REFRESH_REALLY | refresh_flags);
-				continue;
-			}
-			if (!strcmp(path, "--cacheinfo")) {
-				unsigned char sha1[20];
-				unsigned int mode;
-
-				if (i+3 >= argc)
-					die("git update-index: --cacheinfo <mode> <sha1> <path>");
-
-				if (strtoul_ui(argv[i+1], 8, &mode) ||
-				    get_sha1_hex(argv[i+2], sha1) ||
-				    add_cacheinfo(mode, sha1, argv[i+3], 0))
-					die("git update-index: --cacheinfo"
-					    " cannot add %s", argv[i+3]);
-				i += 3;
-				continue;
-			}
-			if (!strcmp(path, "--chmod=-x") ||
-			    !strcmp(path, "--chmod=+x")) {
-				if (argc <= i+1)
-					die("git update-index: %s <path>", path);
-				set_executable_bit = path[8];
-				continue;
-			}
-			if (!strcmp(path, "--assume-unchanged")) {
-				mark_valid_only = MARK_FLAG;
-				continue;
-			}
-			if (!strcmp(path, "--no-assume-unchanged")) {
-				mark_valid_only = UNMARK_FLAG;
-				continue;
-			}
-			if (!strcmp(path, "--no-skip-worktree")) {
-				mark_skip_worktree_only = UNMARK_FLAG;
-				continue;
-			}
-			if (!strcmp(path, "--skip-worktree")) {
-				mark_skip_worktree_only = MARK_FLAG;
-				continue;
-			}
-			if (!strcmp(path, "--info-only")) {
-				info_only = 1;
-				continue;
-			}
-			if (!strcmp(path, "--force-remove")) {
-				force_remove = 1;
-				continue;
-			}
-			if (!strcmp(path, "-z")) {
-				line_termination = 0;
-				continue;
-			}
-			if (!strcmp(path, "--stdin")) {
-				if (i != argc - 1)
-					die("--stdin must be at the end");
-				read_from_stdin = 1;
-				break;
-			}
-			if (!strcmp(path, "--index-info")) {
-				if (i != argc - 1)
-					die("--index-info must be at the end");
-				allow_add = allow_replace = allow_remove = 1;
-				read_index_info(line_termination);
-				break;
-			}
-			if (!strcmp(path, "--unresolve")) {
-				has_errors = do_unresolve(argc - i, argv + i,
-							  prefix, prefix_length);
-				if (has_errors)
-					active_cache_changed = 0;
-				goto finish;
-			}
-			if (!strcmp(path, "--again") || !strcmp(path, "-g")) {
-				setup_work_tree();
-				has_errors = do_reupdate(argc - i, argv + i,
-							 prefix, prefix_length);
-				if (has_errors)
-					active_cache_changed = 0;
-				goto finish;
-			}
-			if (!strcmp(path, "--ignore-missing")) {
-				refresh_flags |= REFRESH_IGNORE_MISSING;
-				continue;
-			}
-			if (!strcmp(path, "--verbose")) {
-				verbose = 1;
-				continue;
-			}
-			if (!strcmp(path, "--clear-resolve-undo")) {
-				resolve_undo_clear();
-				continue;
-			}
-			if (!strcmp(path, "-h") || !strcmp(path, "--help"))
-				usage(update_index_usage);
-			die("unknown option %s", path);
+			setup_work_tree();
+			p = prefix_path(prefix, prefix_length, path);
+			update_one(p, NULL, 0);
+			if (set_executable_bit)
+				chmod_path(set_executable_bit, p);
+			if (p < path || p > path + strlen(path))
+				free((char *)p);
+			ctx.argc--;
+			ctx.argv++;
+			break;
+		}
+		case PARSE_OPT_UNKNOWN:
+			if (ctx.argv[0][1] == '-')
+				error("unknown option '%s'", ctx.argv[0] + 2);
+			else
+				error("unknown switch '%c'", *ctx.opt);
+			usage_with_options(update_index_usage, options);
 		}
-		setup_work_tree();
-		p = prefix_path(prefix, prefix_length, path);
-		update_one(p, NULL, 0);
-		if (set_executable_bit)
-			chmod_path(set_executable_bit, p);
-		if (p < path || p > path + strlen(path))
-			free((char *)p);
 	}
+	argc = parse_options_end(&ctx);
+
 	if (read_from_stdin) {
 		struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;
 
@@ -770,10 +865,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		strbuf_release(&buf);
 	}
 
- finish:
 	if (active_cache_changed) {
 		if (newfd < 0) {
-			if (refresh_flags & REFRESH_QUIET)
+			if (refresh_args.flags & REFRESH_QUIET)
 				exit(128);
 			unable_to_lock_index_die(get_index_file(), lock_error);
 		}
-- 
1.7.2.3

Re: [PATCH 02/10] parse-options: clearer reporting of API misuse

[Jonathan Nieder]

Jonathan Nieder wrote:

> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -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 incompatible flags "
> -				      "LASTARG_DEFAULT and OPTARG", opts->long_name);
> -			} else {
> -				error("`-%c` uses incompatible flags "
> -				      "LASTARG_DEFAULT and OPTARG", opts->short_name);
> -			}
> -			err |= 1;
> -		}
> +		    (opts->flags & PARSE_OPT_OPTARG))
> +			optbug(opts, "uses incompatible flags "
> +				"LASTARG_DEFAULT and OPTARG");
>  	}
> -
> -	if (err)
> -		exit(129);

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.

[PATCH 02/10 v2] parse-options: clearer reporting of API misuse

[Jonathan Nieder]

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>
---
Jonathan Nieder wrote:
> Jonathan Nieder wrote:

>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -316,24 +323,12 @@ static void check_typos(const char *arg, const struct option *options)
[...]
>> +		    (opts->flags & PARSE_OPT_OPTARG))
>> +			optbug(opts, "uses incompatible flags "
>> +				"LASTARG_DEFAULT and OPTARG");
>>  	}
>> -
>> -	if (err)
>> -		exit(129);
>
> Hmph, this is simpler but it does not report all errors any more.
> So it would be better to do:

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 "
-				      "LASTARG_DEFAULT and OPTARG", opts->long_name);
-			} else {
-				error("`-%c` uses incompatible flags "
-				      "LASTARG_DEFAULT and OPTARG", opts->short_name);
-			}
-			err |= 1;
-		}
+		    (opts->flags & PARSE_OPT_OPTARG))
+			err |= optbug(opts, "uses incompatible flags "
+					"LASTARG_DEFAULT and OPTARG");
 	}
-
 	if (err)
-		exit(129);
+		exit(128);
 }
 
 void parse_options_start(struct parse_opt_ctx_t *ctx,
-- 
1.7.2.3

[PATCH 03/10 v2] parse-options: move NODASH sanity checks to parse_options_check

[Jonathan Nieder]

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 |= optbug(opts, "uses feature "
+					"not supported for dashless options");
 	}
 	if (err)
 		exit(128);
-- 
1.7.2.3

[PATCH 04/10 v2] parse-options: sanity check PARSE_OPT_NOARG flag

[Jonathan Nieder]

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

[PATCH 02/10 v2 resend] parse-options: clearer reporting of API misuse

[Jonathan Nieder]

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

Jonathan Nieder wrote:
> Jonathan Nieder wrote:

>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -316,24 +323,12 @@ static void check_typos(const char *arg, const struct option *options)
[...]
>> +		    (opts->flags & PARSE_OPT_OPTARG))
>> +			optbug(opts, "uses incompatible flags "
>> +				"LASTARG_DEFAULT and OPTARG");
>>  	}
>> -
>> -	if (err)
>> -		exit(129);
>
> Hmph, this is simpler but it does not report all errors any more.
> So it would be better to do:

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 "
-				      "LASTARG_DEFAULT and OPTARG", opts->long_name);
-			} else {
-				error("`-%c` uses incompatible flags "
-				      "LASTARG_DEFAULT and OPTARG", opts->short_name);
-			}
-			err |= 1;
-		}
+		    (opts->flags & PARSE_OPT_OPTARG))
+			err |= optbug(opts, "uses incompatible flags "
+					"LASTARG_DEFAULT and OPTARG");
 	}
-
 	if (err)
-		exit(129);
+		exit(128);
 }
 
 void parse_options_start(struct parse_opt_ctx_t *ctx,
-- 
1.7.2.3

Re: [PATCH 06/10] parse-options: never suppress arghelp if LITERAL_ARGHELP is set

[Stephen Boyd]

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.

Re: [PATCH 06/10] parse-options: never suppress arghelp if LITERAL_ARGHELP is set

[Jonathan Nieder]

Stephen Boyd wrote:
> On 12/01/10 15:31, Jonathan Nieder wrote:

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

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
form sends a wrong message.

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

I agree with your conclusion.  Let's drop this patch, and I'll look
into adding the check and a PARSE_OPT_NOSTICKED flag.

Re: [PATCH 06/10] parse-options: never suppress arghelp if LITERAL_ARGHELP is set

[Stephen Boyd]

On 12/03/10 01:40, Jonathan Nieder wrote:
> 
> 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
> form sends a wrong message.
> 

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.

Re: [PATCH 01/10] parse-options: Don't call parse_options_check() so much

[René Scharfe]

Am 02.12.2010 00:28, schrieb Jonathan Nieder:
> 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.

Good idea.

>  void parse_options_start(struct parse_opt_ctx_t *ctx,
>  			 int argc, const char **argv, const char *prefix,
> -			 int flags)
> +			 int flags, const struct option *options)

It might be better to put options before flags, i.e. to use the same
order as in parse_options().

René

Re: [PATCH 01/10] parse-options: Don't call parse_options_check() so much

[Stephen Boyd]

On 12/05/10 10:14, René Scharfe wrote:
> Am 02.12.2010 00:28, schrieb Jonathan Nieder:
>> From: Stephen Boyd <bebarino@gmail.com>
>>  void parse_options_start(struct parse_opt_ctx_t *ctx,
>>  			 int argc, const char **argv, const char *prefix,
>> -			 int flags)
>> +			 int flags, const struct option *options)
> 
> It might be better to put options before flags, i.e. to use the same
> order as in parse_options().
> 

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, options, shortlog_usage)) {
diff --git a/parse-options.c b/parse-options.c
index 0fa79bc..9bbbc6a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -338,7 +338,7 @@ static void parse_options_check(const struct option
*opts)

 void parse_options_start(struct parse_opt_ctx_t *ctx,
 			 int argc, const char **argv, const char *prefix,
-			 int flags)
+			 const struct option *options, int flags)
 {
 	memset(ctx, 0, sizeof(*ctx));
 	ctx->argc = argc - 1;
@@ -350,6 +350,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
 	    (flags & PARSE_OPT_STOP_AT_NON_OPTION))
 		die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
+	parse_options_check(options);
 }

 static int usage_with_options_internal(struct parse_opt_ctx_t *,
@@ -362,8 +363,6 @@ 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;

@@ -452,7 +451,7 @@ int parse_options(int argc, const char **argv, const
char *prefix,
 {
 	struct parse_opt_ctx_t ctx;

-	parse_options_start(&ctx, argc, argv, prefix, flags);
+	parse_options_start(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 		exit(129);
diff --git a/parse-options.h b/parse-options.h
index ae8647d..7b53414 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -180,7 +180,7 @@ struct parse_opt_ctx_t {

 extern void parse_options_start(struct parse_opt_ctx_t *ctx,
 				int argc, const char **argv, const char *prefix,
-				int flags);
+				const struct option *options, int flags);

 extern int parse_options_step(struct parse_opt_ctx_t *ctx,
 			      const struct option *options,
--