proposal for an OPTION_SUBARRAY (recursive parser)

proposal for an OPTION_SUBARRAY (recursive parser)

[Pierre Habouzit]

While working on OPTION_SUBARRAY I noticed a quite bad bug in how
abbrevations are dealt with, here is the fix, should be applied to next:
  [PATCH 1/4] parse-options: abbreviation engine fix.

Here is a better documentation of the struct option, only touch
comments, safe for next:
  [PATCH 2/4] Some better parse-options documentation.

Here is a 2 patch series that implement recursion and option relocation
of struct options when nested:
  [PATCH 3/4] Add OPTION_BASEOFFSET/OPTION_SUBARRAY.
  [PATCH 4/4] Implement OPTION_SUBARRAY handling.

Those two patch have nor real reason to go anywhere else than pu yet, I
post them for review and comments, and will probably base diff/log
migration on those soon.

[PATCH 1/4] parse-options: abbreviation engine fix.

[Pierre Habouzit]

When we had at least two long option then followed by another one that was a
prefix of both of them, then the abbreviation detector failed.

Fix the issue, add a test.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c          |   48 +++++++++++++++++++++++-----------------------
 t/t0040-parse-options.sh |   13 ++++++++++++
 test-parse-options.c     |    1 +
 3 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index cc09c98..d2e32c1 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -119,8 +119,8 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
                           const struct option *options)
 {
 	const char *arg_end = strchr(arg, '=');
-	const struct option *abbrev_option = NULL;
-	int abbrev_flags = 0;
+	const struct option *abbrev_option = NULL, *conflict_option = NULL;
+	int abbrev_flags = 0, conflict_flags = 0;
 
 	if (!arg_end)
 		arg_end = arg + strlen(arg);
@@ -132,42 +132,33 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
 		if (!options->long_name)
 			continue;
 
+		/* special case {n,no,no-} that always conflict */
+		if (!prefixcmp("no-", arg))
+			die("`--{n,no,no-}' cannot be abbreviated forms of options");
+
 		rest = skip_prefix(arg, options->long_name);
 		if (!rest) {
 			/* abbreviated? */
 			if (!strncmp(options->long_name, arg, arg_end - arg)) {
 is_abbreviated:
-				if (abbrev_option)
-					return error("Ambiguous option: %s "
-						"(could be --%s%s or --%s%s)",
-						arg,
-						(flags & OPT_UNSET) ?
-							"no-" : "",
-						options->long_name,
-						(abbrev_flags & OPT_UNSET) ?
-							"no-" : "",
-						abbrev_option->long_name);
-				if (!(flags & OPT_UNSET) && *arg_end)
-					p->opt = arg_end + 1;
+				conflict_option = abbrev_option;
+				conflict_flags = abbrev_flags;
 				abbrev_option = options;
 				abbrev_flags = flags;
 				continue;
 			}
-			/* negated and abbreviated very much? */
-			if (!prefixcmp("no-", arg)) {
-				flags |= OPT_UNSET;
-				goto is_abbreviated;
-			}
 			/* negated? */
 			if (strncmp(arg, "no-", 3))
 				continue;
 			flags |= OPT_UNSET;
-			rest = skip_prefix(arg + 3, options->long_name);
+			arg += 3;
+			rest = skip_prefix(arg, options->long_name);
 			/* abbreviated and negated? */
-			if (!rest && !prefixcmp(options->long_name, arg + 3))
-				goto is_abbreviated;
-			if (!rest)
+			if (!rest) {
+				if (!strncmp(options->long_name, arg, arg_end - arg))
+					goto is_abbreviated;
 				continue;
+			}
 		}
 		if (*rest) {
 			if (*rest != '=')
@@ -176,8 +167,17 @@ is_abbreviated:
 		}
 		return get_value(p, options, flags);
 	}
-	if (abbrev_option)
+	if (conflict_option)
+		return error("Ambiguous option: %s (could be --%s%s or --%s%s)",
+			arg, (conflict_flags & OPT_UNSET) ?  "no-" : "",
+			conflict_option->long_name,
+			(abbrev_flags & OPT_UNSET) ?  "no-" : "",
+			abbrev_option->long_name);
+	if (abbrev_option) {
+		if (!(abbrev_flags & OPT_UNSET) && *arg_end)
+			p->opt = arg_end + 1;
 		return get_value(p, abbrev_option, abbrev_flags);
+	}
 	return error("unknown option `%s'", arg);
 }
 
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ae49424..ee758e5 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -18,6 +18,7 @@ string options
     -s, --string <string>
                           get a string
     --string2 <str>       get another string
+    --st <st>             get another string (pervert ordering)
 
 EOF
 
@@ -90,4 +91,16 @@ test_expect_failure 'ambiguously abbreviated option' '
         test $? != 129
 '
 
+cat > expect << EOF
+boolean: 0
+integer: 2
+string: 123
+EOF
+
+test_expect_failure 'non ambiguous option (after two options it abbreviates)' '
+	test-parse-options --st 123 &&
+	test ! -s output.err &&
+	git diff expect output
+'
+
 test_done
diff --git a/test-parse-options.c b/test-parse-options.c
index 277cfe4..4d3e2ec 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -18,6 +18,7 @@ int main(int argc, const char **argv)
 		OPT_GROUP("string options"),
 		OPT_STRING('s', "string", &string, "string", "get a string"),
 		OPT_STRING(0, "string2", &string, "str", "get another string"),
+		OPT_STRING(0, "st", &string, "st", "get another string (pervert ordering)"),
 		OPT_END(),
 	};
 	int i;
-- 
1.5.3.5.1531.g59008

[PATCH 2/4] Some better parse-options documentation.

[Pierre Habouzit]

---
 parse-options.h |   37 +++++++++++++++++++++++++++++++++++--
 1 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/parse-options.h b/parse-options.h
index 3a470e5..65bce6e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -22,6 +22,41 @@ enum parse_opt_option_flags {
 struct option;
 typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
 
+/*
+ * `type`::
+ *   holds the type of the option, you must have an OPTION_END last in your
+ *   array.
+ *
+ * `short_name`::
+ *   the character to use as a short option name, '\0' if none.
+ *
+ * `long_name`::
+ *   the long option name, without the leading dashes, NULL if none.
+ *
+ * `value`::
+ *   stores pointers to the values to be filled.
+ *
+ * `argh`::
+ *   token to explain the kind of argument this option wants. Keep it
+ *   homogenous across the repository.
+ *
+ * `help`::
+ *   the short help associated to what the option does.
+ *   Must never be NULL (except for OPTION_END).
+ *   OPTION_GROUP uses this pointer to store the group header.
+ *
+ * `flags`::
+ *   mask of parse_opt_option_flags.
+ *   PARSE_OPT_OPTARG: says that the argument is optionnal (not for BOOLEANs)
+ *   PARSE_OPT_NOARG: says that this option takes no argument, for CALLBACKs
+ *
+ * `callback`::
+ *   pointer to the callback to use for OPTION_CALLBACK.
+ *
+ * `defval`::
+ *   default value to fill (*->value) with for PARSE_OPT_OPTARG.
+ *   CALLBACKS can use it like they want.
+ */
 struct option {
 	enum parse_opt_type type;
 	int short_name;
@@ -32,8 +67,6 @@ struct option {
 
 	int flags;
 	parse_opt_cb *callback;
-	/* holds default value for PARSE_OPT_OPTARG,
-	   though callbacks can use it like they want */
 	intptr_t defval;
 };
 
-- 
1.5.3.5.1531.g59008

[PATCH 3/4] Add OPTION_BASEOFFSET/OPTION_SUBARRAY.

[Pierre Habouzit]

Currently make the implementation die() if SUBARRAYs are encountered.
Refactor the rest of the code to be ready for recursion.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |  114 ++++++++++++++++++++++++++++++++++++++-----------------
 parse-options.h |    6 +++
 2 files changed, 85 insertions(+), 35 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index d2e32c1..5cea511 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -8,6 +8,9 @@ struct optparse_t {
 	const char **argv;
 	int argc;
 	const char *opt;
+
+	const struct option *abbrev_option, *conflict_option;
+	int abbrev_flags, conflict_flags;
 };
 
 static inline const char *get_arg(struct optparse_t *p)
@@ -104,23 +107,35 @@ static int get_value(struct optparse_t *p,
 	}
 }
 
-static int parse_short_opt(struct optparse_t *p, const struct option *options)
+static int parse_short_opt(struct optparse_t *p, const struct option *options,
+                           int level)
 {
-	for (; options->type != OPTION_END; options++) {
-		if (options->short_name == *p->opt) {
+	for (;; options++) {
+		switch (options->type) {
+		case OPTION_END:
+			return level > 0 ? -1 : error("unknown switch `%c'", *p->opt);
+		case OPTION_GROUP:
+		case OPTION_BASEOFFSET:
+			continue;
+		case OPTION_SUBARRAY:
+			die("unsupported yet");
+			break;
+		default:
+			if (options->short_name != *p->opt)
+				continue;
 			p->opt = p->opt[1] ? p->opt + 1 : NULL;
 			return get_value(p, options, OPT_SHORT);
 		}
 	}
-	return error("unknown switch `%c'", *p->opt);
 }
 
 static int parse_long_opt(struct optparse_t *p, const char *arg,
-                          const struct option *options)
+                          const struct option *options, int level)
 {
 	const char *arg_end = strchr(arg, '=');
-	const struct option *abbrev_option = NULL, *conflict_option = NULL;
-	int abbrev_flags = 0, conflict_flags = 0;
+
+	if (level == 0)
+		p->conflict_option = p->abbrev_option = NULL;
 
 	if (!arg_end)
 		arg_end = arg + strlen(arg);
@@ -129,6 +144,17 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
 		const char *rest;
 		int flags = 0;
 
+		switch (options->type) {
+		case OPTION_GROUP:
+		case OPTION_BASEOFFSET:
+			continue;
+		case OPTION_SUBARRAY:
+			die("unsupported yet");
+			break;
+		default:
+			break;
+		}
+
 		if (!options->long_name)
 			continue;
 
@@ -138,13 +164,16 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
 
 		rest = skip_prefix(arg, options->long_name);
 		if (!rest) {
+			/* negated and abbreviated very much? */
+			if (!prefixcmp("no-", arg))
+				die("--n,--no,--no- are never proper abbreviated options");
 			/* abbreviated? */
 			if (!strncmp(options->long_name, arg, arg_end - arg)) {
 is_abbreviated:
-				conflict_option = abbrev_option;
-				conflict_flags = abbrev_flags;
-				abbrev_option = options;
-				abbrev_flags = flags;
+				p->conflict_option = p->abbrev_option;
+				p->conflict_flags = p->abbrev_flags;
+				p->abbrev_option = options;
+				p->abbrev_flags = flags;
 				continue;
 			}
 			/* negated? */
@@ -167,16 +196,18 @@ is_abbreviated:
 		}
 		return get_value(p, options, flags);
 	}
-	if (conflict_option)
+	if (level > 0)
+		return -1;
+	if (p->conflict_option)
 		return error("Ambiguous option: %s (could be --%s%s or --%s%s)",
-			arg, (conflict_flags & OPT_UNSET) ?  "no-" : "",
-			conflict_option->long_name,
-			(abbrev_flags & OPT_UNSET) ?  "no-" : "",
-			abbrev_option->long_name);
-	if (abbrev_option) {
-		if (!(abbrev_flags & OPT_UNSET) && *arg_end)
+			arg, (p->conflict_flags & OPT_UNSET) ?  "no-" : "",
+			p->conflict_option->long_name,
+			(p->abbrev_flags & OPT_UNSET) ?  "no-" : "",
+			p->abbrev_option->long_name);
+	if (p->abbrev_option) {
+		if (!(p->abbrev_flags & OPT_UNSET) && *arg_end)
 			p->opt = arg_end + 1;
-		return get_value(p, abbrev_option, abbrev_flags);
+		return get_value(p, p->abbrev_option, p->abbrev_flags);
 	}
 	return error("unknown option `%s'", arg);
 }
@@ -200,7 +231,7 @@ int parse_options(int argc, const char **argv, const struct option *options,
 			do {
 				if (*args.opt == 'h')
 					usage_with_options(usagestr, options);
-				if (parse_short_opt(&args, options) < 0)
+				if (parse_short_opt(&args, options, 0) < 0)
 					usage_with_options(usagestr, options);
 			} while (args.opt);
 			continue;
@@ -216,7 +247,7 @@ int parse_options(int argc, const char **argv, const struct option *options,
 
 		if (!strcmp(arg + 2, "help"))
 			usage_with_options(usagestr, options);
-		if (parse_long_opt(&args, arg + 2, options))
+		if (parse_long_opt(&args, arg + 2, options, 0))
 			usage_with_options(usagestr, options);
 	}
 
@@ -228,27 +259,27 @@ int parse_options(int argc, const char **argv, const struct option *options,
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2
 
-void usage_with_options(const char * const *usagestr,
-                        const struct option *opts)
+static void dump_options(const struct option *opts)
 {
-	fprintf(stderr, "usage: %s\n", *usagestr++);
-	while (*usagestr && **usagestr)
-		fprintf(stderr, "   or: %s\n", *usagestr++);
-	while (*usagestr)
-		fprintf(stderr, "    %s\n", *usagestr++);
-
-	if (opts->type != OPTION_GROUP)
-		fputc('\n', stderr);
-
-	for (; opts->type != OPTION_END; opts++) {
+	for (;; opts++) {
 		size_t pos;
 		int pad;
 
-		if (opts->type == OPTION_GROUP) {
+		switch (opts->type) {
+		case OPTION_END:
+			return;
+		case OPTION_GROUP:
 			fputc('\n', stderr);
 			if (*opts->help)
 				fprintf(stderr, "%s\n", opts->help);
 			continue;
+		case OPTION_BASEOFFSET:
+			continue;
+		case OPTION_SUBARRAY:
+			dump_options((struct option *)opts->defval);
+			continue;
+		default:
+			break;
 		}
 
 		pos = fprintf(stderr, "    ");
@@ -295,8 +326,21 @@ void usage_with_options(const char * const *usagestr,
 		}
 		fprintf(stderr, "%*s%s\n", pad + USAGE_GAP, "", opts->help);
 	}
-	fputc('\n', stderr);
+}
 
+void usage_with_options(const char * const *usagestr,
+                        const struct option *opts)
+{
+	fprintf(stderr, "usage: %s\n", *usagestr++);
+	while (*usagestr && **usagestr)
+		fprintf(stderr, "   or: %s\n", *usagestr++);
+	while (*usagestr)
+		fprintf(stderr, "    %s\n", *usagestr++);
+
+	if (opts->type != OPTION_GROUP)
+		fputc('\n', stderr);
+	dump_options(opts);
+	fputc('\n', stderr);
 	exit(129);
 }
 
diff --git a/parse-options.h b/parse-options.h
index 65bce6e..6668924 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -4,6 +4,8 @@
 enum parse_opt_type {
 	OPTION_END,
 	OPTION_GROUP,
+	OPTION_BASEOFFSET,
+	OPTION_SUBARRAY,
 	OPTION_BOOLEAN,
 	OPTION_STRING,
 	OPTION_INTEGER,
@@ -35,6 +37,7 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
  *
  * `value`::
  *   stores pointers to the values to be filled.
+ *   BASEOFFSET use it to store the offset wrt which the struct was filled.
  *
  * `argh`::
  *   token to explain the kind of argument this option wants. Keep it
@@ -56,6 +59,9 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
  * `defval`::
  *   default value to fill (*->value) with for PARSE_OPT_OPTARG.
  *   CALLBACKS can use it like they want.
+ *   SUBARRAYs use it to store the subarray address.
+ *   BASEOFFSET use it to store the sizeof the struct used to fill the array.
+ *              Any `value` that does not points into it is not relocated.
  */
 struct option {
 	enum parse_opt_type type;
-- 
1.5.3.5.1531.g59008

[PATCH 4/4] Implement OPTION_SUBARRAY handling.

[Pierre Habouzit]

Basically, an OPTION_SUBARRAY is a pointer to a new `struct option` array.
This array should start with an OPTION_BASEOFFSET used to relocate ->value
pointers.

The sizeof() of the struct used for the BASEOFFSET entry is also stored so
that a subarray can also have global side effects (->values pointers that
are not in the memory range of the BASE struct are not relocated).

Arbitrary nesting of subarrays is supported, though no checks that there is
a subarray loop is performed.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c          |   78 +++++++++++++++++++++++++++++++++++++++------
 parse-options.h          |    5 +++
 t/t0040-parse-options.sh |   31 +++++++++++++++++-
 test-parse-options.c     |   25 ++++++++++++++-
 4 files changed, 126 insertions(+), 13 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 5cea511..001d1e5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -4,6 +4,12 @@
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
+struct optreloc_t {
+	const char *base;
+	const char *end;
+	intptr_t offs;
+};
+
 struct optparse_t {
 	const char **argv;
 	int argc;
@@ -11,6 +17,8 @@ struct optparse_t {
 
 	const struct option *abbrev_option, *conflict_option;
 	int abbrev_flags, conflict_flags;
+
+	struct optreloc_t reloc;
 };
 
 static inline const char *get_arg(struct optparse_t *p)
@@ -39,10 +47,32 @@ static int opterror(const struct option *opt, const char *reason, int flags)
 	return error("option `%s' %s", opt->long_name, reason);
 }
 
+static void *reloc_value(struct optparse_t *p, const struct option *opt)
+{
+	char *value = opt->value;
+	if (value >= p->reloc.base && value < p->reloc.end)
+		return value + p->reloc.offs;
+	return value;
+}
+
+static const struct option *reloc_option(struct optparse_t *p,
+                                         const struct option *opt,
+                                         struct option *buf)
+{
+	char *value = opt->value;
+	if (value >= p->reloc.base && value < p->reloc.end) {
+		*buf = *opt;
+		buf->value = value + p->reloc.offs;
+		return buf;
+	}
+	return opt;
+}
+
 static int get_value(struct optparse_t *p,
                      const struct option *opt, int flags)
 {
 	const char *s, *arg;
+	struct option tmp;
 	arg = p->opt ? p->opt : (p->argc > 1 ? p->argv[1] : NULL);
 
 	if (p->opt && (flags & OPT_UNSET))
@@ -53,26 +83,27 @@ static int get_value(struct optparse_t *p,
 		if (!(flags & OPT_SHORT) && p->opt)
 			return opterror(opt, "takes no value", flags);
 		if (flags & OPT_UNSET)
-			*(int *)opt->value = 0;
+			*(int *)reloc_value(p, opt) = 0;
 		else
-			(*(int *)opt->value)++;
+			(*(int *)reloc_value(p, opt))++;
 		return 0;
 
 	case OPTION_STRING:
 		if (flags & OPT_UNSET) {
-			*(const char **)opt->value = (const char *)NULL;
+			*(const char **)reloc_value(p, opt) = (const char *)NULL;
 			return 0;
 		}
 		if (opt->flags & PARSE_OPT_OPTARG && (!arg || *arg == '-')) {
-			*(const char **)opt->value = (const char *)opt->defval;
+			*(const char **)reloc_value(p, opt) = (const char *)opt->defval;
 			return 0;
 		}
 		if (!arg)
 			return opterror(opt, "requires a value", flags);
-		*(const char **)opt->value = get_arg(p);
+		*(const char **)reloc_value(p, opt) = get_arg(p);
 		return 0;
 
 	case OPTION_CALLBACK:
+		opt = reloc_option(p, opt, &tmp);
 		if (flags & OPT_UNSET)
 			return (*opt->callback)(opt, NULL, 1);
 		if (opt->flags & PARSE_OPT_NOARG) {
@@ -88,16 +119,16 @@ static int get_value(struct optparse_t *p,
 
 	case OPTION_INTEGER:
 		if (flags & OPT_UNSET) {
-			*(int *)opt->value = 0;
+			*(int *)reloc_value(p, opt) = 0;
 			return 0;
 		}
 		if (opt->flags & PARSE_OPT_OPTARG && (!arg || !isdigit(*arg))) {
-			*(int *)opt->value = opt->defval;
+			*(int *)reloc_value(p, opt) = opt->defval;
 			return 0;
 		}
 		if (!arg)
 			return opterror(opt, "requires a value", flags);
-		*(int *)opt->value = strtol(get_arg(p), (char **)&s, 10);
+		*(int *)reloc_value(p, opt) = strtol(get_arg(p), (char **)&s, 10);
 		if (*s)
 			return opterror(opt, "expects a numerical value", flags);
 		return 0;
@@ -107,9 +138,24 @@ static int get_value(struct optparse_t *p,
 	}
 }
 
+static const struct option *prepare_recursion(struct optparse_t *p,
+                                              const struct option *opt)
+{
+	const struct option *subarray = (const struct option *)opt->defval;
+	if (subarray->type != OPTION_BASEOFFSET)
+		die("subarray does not begins with a relocation stanza");
+	p->reloc.base = subarray->value;
+	p->reloc.end  = p->reloc.base + subarray->defval;
+	p->reloc.offs = (const char *)opt->value - p->reloc.base;
+	return ++subarray;
+}
+
 static int parse_short_opt(struct optparse_t *p, const struct option *options,
                            int level)
 {
+	struct optreloc_t reloc;
+	int res;
+
 	for (;; options++) {
 		switch (options->type) {
 		case OPTION_END:
@@ -118,7 +164,11 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options,
 		case OPTION_BASEOFFSET:
 			continue;
 		case OPTION_SUBARRAY:
-			die("unsupported yet");
+			reloc = p->reloc;
+			res = parse_short_opt(p, prepare_recursion(p, options), level + 1);
+			p->reloc = reloc;
+			if (!res)
+				return 0;
 			break;
 		default:
 			if (options->short_name != *p->opt)
@@ -141,15 +191,21 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
 		arg_end = arg + strlen(arg);
 
 	for (; options->type != OPTION_END; options++) {
+		struct optreloc_t reloc;
 		const char *rest;
-		int flags = 0;
+		int res, flags = 0;
 
 		switch (options->type) {
 		case OPTION_GROUP:
 		case OPTION_BASEOFFSET:
 			continue;
 		case OPTION_SUBARRAY:
-			die("unsupported yet");
+			reloc = p->reloc;
+			res = parse_long_opt(p, arg, prepare_recursion(p, options),
+			                     level + 1);
+			p->reloc = reloc;
+			if (!res)
+				return 0;
 			break;
 		default:
 			break;
diff --git a/parse-options.h b/parse-options.h
index 6668924..4f5a241 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -84,6 +84,11 @@ struct option {
 #define OPT_CALLBACK(s, l, v, a, h, f) \
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), 0, (f) }
 
+#define OPT_BASEOFFS(v)                 \
+	{ OPTION_BASEOFFSET, 0, NULL, (v), NULL, NULL, 0, NULL, sizeof(*(v)) }
+#define OPT_SUBARRAY(v, sub)            \
+	{ OPTION_SUBARRAY, 0, NULL, (v), NULL, NULL, 0, NULL, (intptr_t)sub }
+
 /* parse_options() will filter out the processed options and leave the
  * non-option argments in argv[].
  * Returns the number of arguments left in argv[].
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ee758e5..c7a754e 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -20,6 +20,10 @@ string options
     --string2 <str>       get another string
     --st <st>             get another string (pervert ordering)
 
+test subarray
+    --incr-reloc          increment a relocated integer
+    --incr-fixed          increment a fixed integer
+
 EOF
 
 test_expect_success 'test help' '
@@ -32,6 +36,8 @@ cat > expect << EOF
 boolean: 2
 integer: 1729
 string: 123
+diverted i: 0
+fixed i: 0
 EOF
 
 test_expect_success 'short options' '
@@ -43,6 +49,8 @@ cat > expect << EOF
 boolean: 2
 integer: 1729
 string: 321
+diverted i: 0
+fixed i: 0
 EOF
 
 test_expect_success 'long options' '
@@ -56,6 +64,8 @@ cat > expect << EOF
 boolean: 1
 integer: 13
 string: 123
+diverted i: 0
+fixed i: 0
 arg 00: a1
 arg 01: b1
 arg 02: --boolean
@@ -72,6 +82,8 @@ cat > expect << EOF
 boolean: 0
 integer: 2
 string: (not set)
+diverted i: 0
+fixed i: 0
 EOF
 
 test_expect_success 'unambiguously abbreviated option' '
@@ -93,11 +105,28 @@ test_expect_failure 'ambiguously abbreviated option' '
 
 cat > expect << EOF
 boolean: 0
+integer: 0
+string: (not set)
+diverted i: 1
+fixed i: 2
+EOF
+
+test_expect_success 'subarrays and partial relocation of options' '
+	test-parse-options --incr-reloc --incr-fixed --incr-fixed > output 2> output.err &&
+	test ! -s output.err &&
+	git diff expect output
+'
+
+test_done
+cat > expect << EOF
+boolean: 0
 integer: 2
 string: 123
+diverted i: 0
+fixed i: 0
 EOF
 
-test_expect_failure 'non ambiguous option (after two options it abbreviates)' '
+test_expect_failure 'non ambiguous option (after two options it abbreviates, across subarray)' '
 	test-parse-options --st 123 &&
 	test ! -s output.err &&
 	git diff expect output
diff --git a/test-parse-options.c b/test-parse-options.c
index 4d3e2ec..5f740da 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -1,16 +1,34 @@
 #include "cache.h"
 #include "parse-options.h"
 
+struct reloc_me_please {
+	int integer;
+};
+
 static int boolean = 0;
 static int integer = 0;
 static char *string = NULL;
 
+static int reloc_integer = 0;
+static int fixed_integer = 0;
+
+static const struct option subopts[] = {
+	OPT_BASEOFFS(&reloc_integer),
+	OPT_STRING(0, "st", &string, "st", "get another string (pervert ordering)"),
+	OPT_GROUP("test subarray"),
+	OPT_BOOLEAN(0, "incr-reloc", &reloc_integer, "increment a relocated integer"),
+	OPT_BOOLEAN(0, "incr-fixed", &fixed_integer, "increment a fixed integer"),
+	OPT_END(),
+};
+
 int main(int argc, const char **argv)
 {
 	const char *usage[] = {
 		"test-parse-options <options>",
 		NULL
 	};
+	int diverted_integer = 0;
+
 	struct option options[] = {
 		OPT_BOOLEAN('b', "boolean", &boolean, "get a boolean"),
 		OPT_INTEGER('i', "integer", &integer, "get a integer"),
@@ -18,7 +36,7 @@ int main(int argc, const char **argv)
 		OPT_GROUP("string options"),
 		OPT_STRING('s', "string", &string, "string", "get a string"),
 		OPT_STRING(0, "string2", &string, "str", "get another string"),
-		OPT_STRING(0, "st", &string, "st", "get another string (pervert ordering)"),
+		OPT_SUBARRAY(&diverted_integer, subopts),
 		OPT_END(),
 	};
 	int i;
@@ -28,9 +46,14 @@ int main(int argc, const char **argv)
 	printf("boolean: %d\n", boolean);
 	printf("integer: %d\n", integer);
 	printf("string: %s\n", string ? string : "(not set)");
+	printf("diverted i: %d\n", diverted_integer);
+	printf("fixed i: %d\n", fixed_integer);
 
 	for (i = 0; i < argc; i++)
 		printf("arg %02d: %s\n", i, argv[i]);
 
+	if (reloc_integer)
+		die("reloc_integer should not ever change");
+
 	return 0;
 }
-- 
1.5.3.5.1531.g59008

[PATCH] parse-options: abbreviation engine fix.

[Johannes Schindelin]

When an option could be an ambiguous abbreviation of two options, the code 
used to error out.  Even if an exact match would have occured later.

Test and original patch by Pierre Habouzit.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---

	On Mon, 5 Nov 2007, Pierre Habouzit wrote:

	> When we had at least two long option then followed by another 
	> one that was a prefix of both of them, then the abbreviation 
	> detector failed.

	Yeah, I assumed that you would never do such a thing, but I agree 
	that with recursing option parsing it is much more likely.

	It took me some time to understand your patch, and that the moving 
	of the UNSET handling was unnecessary.

	IMHO this patch is easier to read.

 parse-options.c          |   33 +++++++++++++++++++++------------
 t/t0040-parse-options.sh |   13 +++++++++++++
 test-parse-options.c     |    1 +
 3 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index cc09c98..15b32f7 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -119,8 +119,8 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
                           const struct option *options)
 {
 	const char *arg_end = strchr(arg, '=');
-	const struct option *abbrev_option = NULL;
-	int abbrev_flags = 0;
+	const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
+	int abbrev_flags = 0, ambiguous_flags = 0;
 
 	if (!arg_end)
 		arg_end = arg + strlen(arg);
@@ -137,16 +137,16 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
 			/* abbreviated? */
 			if (!strncmp(options->long_name, arg, arg_end - arg)) {
 is_abbreviated:
-				if (abbrev_option)
-					return error("Ambiguous option: %s "
-						"(could be --%s%s or --%s%s)",
-						arg,
-						(flags & OPT_UNSET) ?
-							"no-" : "",
-						options->long_name,
-						(abbrev_flags & OPT_UNSET) ?
-							"no-" : "",
-						abbrev_option->long_name);
+				if (abbrev_option) {
+					/*
+					 * If this is abbreviated, it is
+					 * ambiguous. So when there is no
+					 * exact match later, we need to
+					 * error out.
+					 */
+					ambiguous_option = abbrev_option;
+					ambiguous_flags = abbrev_flags;
+				}
 				if (!(flags & OPT_UNSET) && *arg_end)
 					p->opt = arg_end + 1;
 				abbrev_option = options;
@@ -176,6 +176,15 @@ is_abbreviated:
 		}
 		return get_value(p, options, flags);
 	}
+
+	if (ambiguous_option)
+		return error("Ambiguous option: %s "
+			"(could be --%s%s or --%s%s)",
+			arg,
+			(ambiguous_flags & OPT_UNSET) ?  "no-" : "",
+			ambiguous_option->long_name,
+			(abbrev_flags & OPT_UNSET) ?  "no-" : "",
+			abbrev_option->long_name);
 	if (abbrev_option)
 		return get_value(p, abbrev_option, abbrev_flags);
 	return error("unknown option `%s'", arg);
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ae49424..ee758e5 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -18,6 +18,7 @@ string options
     -s, --string <string>
                           get a string
     --string2 <str>       get another string
+    --st <st>             get another string (pervert ordering)
 
 EOF
 
@@ -90,4 +91,16 @@ test_expect_failure 'ambiguously abbreviated option' '
         test $? != 129
 '
 
+cat > expect << EOF
+boolean: 0
+integer: 2
+string: 123
+EOF
+
+test_expect_failure 'non ambiguous option (after two options it abbreviates)' '
+	test-parse-options --st 123 &&
+	test ! -s output.err &&
+	git diff expect output
+'
+
 test_done
diff --git a/test-parse-options.c b/test-parse-options.c
index 277cfe4..4d3e2ec 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -18,6 +18,7 @@ int main(int argc, const char **argv)
 		OPT_GROUP("string options"),
 		OPT_STRING('s', "string", &string, "string", "get a string"),
 		OPT_STRING(0, "string2", &string, "str", "get another string"),
+		OPT_STRING(0, "st", &string, "st", "get another string (pervert ordering)"),
 		OPT_END(),
 	};
 	int i;
-- 
1.5.3.5.1549.g91a3

Re: [PATCH] parse-options: abbreviation engine fix.

[Johannes Schindelin]

Hi,

On Mon, 5 Nov 2007, Johannes Schindelin wrote:

> +cat > expect << EOF
> +boolean: 0
> +integer: 2
> +string: 123
> +EOF
> +
> +test_expect_failure 'non ambiguous option (after two options it abbreviates)' '
> +	test-parse-options --st 123 &&
> +	test ! -s output.err &&
> +	git diff expect output
> +'
> +

Aaargh!  Yet another instance of test_expect_failure being wrong, wrong, 
wrong.

I'll come up with a replacement patch.

Ciao,
Dscho

[PATCH 1/3] parse-options: abbreviation engine fix.

[Johannes Schindelin]

When an option could be an ambiguous abbreviation of two options, the code
used to error out.  Even if an exact match would have occured later.

Test and original patch by Pierre Habouzit.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 parse-options.c          |   33 +++++++++++++++++++++------------
 t/t0040-parse-options.sh |   13 +++++++++++++
 test-parse-options.c     |    1 +
 3 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index cc09c98..15b32f7 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -119,8 +119,8 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
                           const struct option *options)
 {
 	const char *arg_end = strchr(arg, '=');
-	const struct option *abbrev_option = NULL;
-	int abbrev_flags = 0;
+	const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
+	int abbrev_flags = 0, ambiguous_flags = 0;
 
 	if (!arg_end)
 		arg_end = arg + strlen(arg);
@@ -137,16 +137,16 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
 			/* abbreviated? */
 			if (!strncmp(options->long_name, arg, arg_end - arg)) {
 is_abbreviated:
-				if (abbrev_option)
-					return error("Ambiguous option: %s "
-						"(could be --%s%s or --%s%s)",
-						arg,
-						(flags & OPT_UNSET) ?
-							"no-" : "",
-						options->long_name,
-						(abbrev_flags & OPT_UNSET) ?
-							"no-" : "",
-						abbrev_option->long_name);
+				if (abbrev_option) {
+					/*
+					 * If this is abbreviated, it is
+					 * ambiguous. So when there is no
+					 * exact match later, we need to
+					 * error out.
+					 */
+					ambiguous_option = abbrev_option;
+					ambiguous_flags = abbrev_flags;
+				}
 				if (!(flags & OPT_UNSET) && *arg_end)
 					p->opt = arg_end + 1;
 				abbrev_option = options;
@@ -176,6 +176,15 @@ is_abbreviated:
 		}
 		return get_value(p, options, flags);
 	}
+
+	if (ambiguous_option)
+		return error("Ambiguous option: %s "
+			"(could be --%s%s or --%s%s)",
+			arg,
+			(ambiguous_flags & OPT_UNSET) ?  "no-" : "",
+			ambiguous_option->long_name,
+			(abbrev_flags & OPT_UNSET) ?  "no-" : "",
+			abbrev_option->long_name);
 	if (abbrev_option)
 		return get_value(p, abbrev_option, abbrev_flags);
 	return error("unknown option `%s'", arg);
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ae49424..462fdf2 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -18,6 +18,7 @@ string options
     -s, --string <string>
                           get a string
     --string2 <str>       get another string
+    --st <st>             get another string (pervert ordering)
 
 EOF
 
@@ -90,4 +91,16 @@ test_expect_failure 'ambiguously abbreviated option' '
         test $? != 129
 '
 
+cat > expect << EOF
+boolean: 0
+integer: 0
+string: 123
+EOF
+
+test_expect_success 'non ambiguous option (after two options it abbreviates)' '
+	test-parse-options --st 123 > output 2> output.err &&
+	test ! -s output.err &&
+	git diff expect output
+'
+
 test_done
diff --git a/test-parse-options.c b/test-parse-options.c
index 277cfe4..4d3e2ec 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -18,6 +18,7 @@ int main(int argc, const char **argv)
 		OPT_GROUP("string options"),
 		OPT_STRING('s', "string", &string, "string", "get a string"),
 		OPT_STRING(0, "string2", &string, "str", "get another string"),
+		OPT_STRING(0, "st", &string, "st", "get another string (pervert ordering)"),
 		OPT_END(),
 	};
 	int i;
-- 
1.5.3.5.1549.g91a3

[PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options

[Johannes Schindelin]

The diff options should not need to be defined in every user of the
diffcore.  This provides the framework:

	extern struct option *diff_options;

	struct option options[] = {
		OPT_RECURSE(diff_options),
		...
		OPT_END(),
	};

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 parse-options.c          |   68 +++++++++++++++++++++++++++++++++++-----------
 parse-options.h          |    2 +
 t/t0040-parse-options.sh |   26 +++++++++++++++++
 test-parse-options.c     |   10 +++++++
 4 files changed, 90 insertions(+), 16 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 15b32f7..ed8e951 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -4,6 +4,8 @@
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
+#define OPT_NOT_FOUND -2
+
 struct optparse_t {
 	const char **argv;
 	int argc;
@@ -111,8 +113,14 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)
 			p->opt = p->opt[1] ? p->opt + 1 : NULL;
 			return get_value(p, options, OPT_SHORT);
 		}
+		if (options->type == OPTION_RECURSE) {
+			const struct option *sub = options->value;
+			int result = parse_short_opt(p, sub);
+			if (result != OPT_NOT_FOUND)
+				return result;
+		}
 	}
-	return error("unknown switch `%c'", *p->opt);
+	return OPT_NOT_FOUND;
 }
 
 static int parse_long_opt(struct optparse_t *p, const char *arg,
@@ -129,6 +137,13 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
 		const char *rest;
 		int flags = 0;
 
+		if (options->type == OPTION_RECURSE) {
+			const struct option *sub = options->value;
+			int result = parse_long_opt(p, arg, sub);
+			if (result != OPT_NOT_FOUND)
+				return result;
+		}
+
 		if (!options->long_name)
 			continue;
 
@@ -187,14 +202,14 @@ is_abbreviated:
 			abbrev_option->long_name);
 	if (abbrev_option)
 		return get_value(p, abbrev_option, abbrev_flags);
-	return error("unknown option `%s'", arg);
+	return OPT_NOT_FOUND;
 }
 
 int parse_options(int argc, const char **argv, const struct option *options,
                   const char * const usagestr[], int flags)
 {
 	struct optparse_t args = { argv + 1, argc - 1, NULL };
-	int j = 0;
+	int j = 0, result;
 
 	for (; args.argc; args.argc--, args.argv++) {
 		const char *arg = args.argv[0];
@@ -209,8 +224,13 @@ int parse_options(int argc, const char **argv, const struct option *options,
 			do {
 				if (*args.opt == 'h')
 					usage_with_options(usagestr, options);
-				if (parse_short_opt(&args, options) < 0)
+				result = parse_short_opt(&args, options);
+				if (result < 0) {
+					if (result == OPT_NOT_FOUND)
+						error("unknown switch `%c'",
+								arg[1]);
 					usage_with_options(usagestr, options);
+				}
 			} while (args.opt);
 			continue;
 		}
@@ -225,8 +245,12 @@ int parse_options(int argc, const char **argv, const struct option *options,
 
 		if (!strcmp(arg + 2, "help"))
 			usage_with_options(usagestr, options);
-		if (parse_long_opt(&args, arg + 2, options))
+		result = parse_long_opt(&args, arg + 2, options);
+		if (result) {
+			if (result == OPT_NOT_FOUND)
+				error("unknown option `%s'", arg + 2);
 			usage_with_options(usagestr, options);
+		}
 	}
 
 	memmove(argv + j, args.argv, args.argc * sizeof(*argv));
@@ -237,22 +261,18 @@ int parse_options(int argc, const char **argv, const struct option *options,
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2
 
-void usage_with_options(const char * const *usagestr,
-                        const struct option *opts)
+static void usage_show_options(const struct option *opts)
 {
-	fprintf(stderr, "usage: %s\n", *usagestr++);
-	while (*usagestr && **usagestr)
-		fprintf(stderr, "   or: %s\n", *usagestr++);
-	while (*usagestr)
-		fprintf(stderr, "    %s\n", *usagestr++);
-
-	if (opts->type != OPTION_GROUP)
-		fputc('\n', stderr);
-
 	for (; opts->type != OPTION_END; opts++) {
 		size_t pos;
 		int pad;
 
+		if (opts->type == OPTION_RECURSE) {
+			const struct option *sub = opts->value;
+			usage_show_options(sub);
+			continue;
+		}
+
 		if (opts->type == OPTION_GROUP) {
 			fputc('\n', stderr);
 			if (*opts->help)
@@ -304,6 +324,22 @@ void usage_with_options(const char * const *usagestr,
 		}
 		fprintf(stderr, "%*s%s\n", pad + USAGE_GAP, "", opts->help);
 	}
+}
+
+void usage_with_options(const char * const *usagestr,
+                        const struct option *opts)
+{
+	fprintf(stderr, "usage: %s\n", *usagestr++);
+	while (*usagestr && **usagestr)
+		fprintf(stderr, "   or: %s\n", *usagestr++);
+	while (*usagestr)
+		fprintf(stderr, "    %s\n", *usagestr++);
+
+	if (opts->type != OPTION_GROUP)
+		fputc('\n', stderr);
+
+	usage_show_options(opts);
+
 	fputc('\n', stderr);
 
 	exit(129);
diff --git a/parse-options.h b/parse-options.h
index 3a470e5..9ec1e35 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -8,6 +8,7 @@ enum parse_opt_type {
 	OPTION_STRING,
 	OPTION_INTEGER,
 	OPTION_CALLBACK,
+	OPTION_RECURSE,
 };
 
 enum parse_opt_flags {
@@ -44,6 +45,7 @@ struct option {
 #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }
 #define OPT_CALLBACK(s, l, v, a, h, f) \
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), 0, (f) }
+#define OPT_RECURSE(options)        { OPTION_RECURSE, 0, NULL, options }
 
 /* parse_options() will filter out the processed options and leave the
  * non-option argments in argv[].
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 462fdf2..9c3efaa 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -13,6 +13,8 @@ usage: test-parse-options <options>
     -b, --boolean         get a boolean
     -i, --integer <n>     get a integer
     -j <n>                get a integer, too
+    -n, --narf            what are we doing tonight?
+    -z, --zort <n>        try to take over the world
 
 string options
     -s, --string <string>
@@ -29,6 +31,8 @@ test_expect_success 'test help' '
 '
 
 cat > expect << EOF
+narf: 0
+zort: 1
 boolean: 2
 integer: 1729
 string: 123
@@ -40,6 +44,8 @@ test_expect_success 'short options' '
 	test ! -s output.err
 '
 cat > expect << EOF
+narf: 0
+zort: 1
 boolean: 2
 integer: 1729
 string: 321
@@ -53,6 +59,8 @@ test_expect_success 'long options' '
 '
 
 cat > expect << EOF
+narf: 0
+zort: 1
 boolean: 1
 integer: 13
 string: 123
@@ -69,6 +77,8 @@ test_expect_success 'intermingled arguments' '
 '
 
 cat > expect << EOF
+narf: 0
+zort: 1
 boolean: 0
 integer: 2
 string: (not set)
@@ -92,6 +102,22 @@ test_expect_failure 'ambiguously abbreviated option' '
 '
 
 cat > expect << EOF
+narf: 1
+zort: 5
+boolean: 0
+integer: 3
+string: (not set)
+EOF
+
+test_expect_success 'recurse works' '
+	test-parse-options --narf -z5 --int=3 > output 2> output.err &&
+	test ! -s output.err &&
+	git diff expect output
+'
+
+cat > expect << EOF
+narf: 0
+zort: 1
 boolean: 0
 integer: 0
 string: 123
diff --git a/test-parse-options.c b/test-parse-options.c
index 4d3e2ec..ee64fb3 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -1,6 +1,8 @@
 #include "cache.h"
 #include "parse-options.h"
 
+static int narf = 0;
+static int zort = 1;
 static int boolean = 0;
 static int integer = 0;
 static char *string = NULL;
@@ -11,10 +13,16 @@ int main(int argc, const char **argv)
 		"test-parse-options <options>",
 		NULL
 	};
+	struct option sub[] = {
+		OPT_BOOLEAN('n', "narf", &narf, "what are we doing tonight?"),
+		OPT_INTEGER('z', "zort", &zort, "try to take over the world"),
+		OPT_END(),
+	};
 	struct option options[] = {
 		OPT_BOOLEAN('b', "boolean", &boolean, "get a boolean"),
 		OPT_INTEGER('i', "integer", &integer, "get a integer"),
 		OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
+		OPT_RECURSE(sub),
 		OPT_GROUP("string options"),
 		OPT_STRING('s', "string", &string, "string", "get a string"),
 		OPT_STRING(0, "string2", &string, "str", "get another string"),
@@ -25,6 +33,8 @@ int main(int argc, const char **argv)
 
 	argc = parse_options(argc, argv, options, usage, 0);
 
+	printf("narf: %d\n", narf);
+	printf("zort: %d\n", zort);
 	printf("boolean: %d\n", boolean);
 	printf("integer: %d\n", integer);
 	printf("string: %s\n", string ? string : "(not set)");
-- 
1.5.3.5.1549.g91a3

Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options

[Johannes Schindelin]

Hi,

On Mon, 5 Nov 2007, Johannes Schindelin wrote:

> The diff options should not need to be defined in every user of the
> diffcore.  This provides the framework:
> 
> 	extern struct option *diff_options;
> 
> 	struct option options[] = {
> 		OPT_RECURSE(diff_options),
> 		...
> 		OPT_END(),
> 	};

After kicking this around a bit more on IRC, we had another idea.  Instead 
of introducing OPT_RECURSE(), do something like OPT__QUIET(), only this 
time in diff.h:

#define OPT__DIFF(opt) \
	OPT_BOOLEAN('p', NULL, &opt.format_patch, "show a patch"), \
	...

Pierre said this feels a bit "80s", so I'd like to hear other people's 
opinions.

Hmm?

Ciao,
Dscho

Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options

[Linus Torvalds]

On Mon, 5 Nov 2007, Johannes Schindelin wrote:
> 
> After kicking this around a bit more on IRC, we had another idea.  Instead 
> of introducing OPT_RECURSE(), do something like OPT__QUIET(), only this 
> time in diff.h: ....

I think the preprocessor approach would tend to be simpler, which is an 
advantage. But whichever approach is chosen, I think one important issue 
is to make sure that options that *hide* other options are correctly 
handled in the help printout..

We have a few cases where a "recursive" option is hidden. For example, the 
option "-n" can mean different things in different contexts: in 
git-format-patch, for example, the "-n" is handled *before* calling 
setup_revisions() and allt he normal revision flags, which means that the 
format-patch -specific meaning of "-n" overrides the normal "revision" 
meaning. 

I suspect that in this kind of situation, it's actually easier to have a 
single linear array of options, because the option parser can just walk 
back and check "oh, I already saw this short option, so I should not 
output it as documentation because this version of it is hidden by the 
earlier one". 

But that's an implementation issue. The same certainly *can* be done with 
a recursive setup, just passing a linked list of what the earlier levels 
were (which is what we do in other places). And it's not like the 
recursion is going to be very deep or complex.

		Linus

Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options

[Johannes Schindelin]

Hi,

On Mon, 5 Nov 2007, Linus Torvalds wrote:

> On Mon, 5 Nov 2007, Johannes Schindelin wrote:
> > 
> > After kicking this around a bit more on IRC, we had another idea.  
> > Instead of introducing OPT_RECURSE(), do something like OPT__QUIET(), 
> > only this time in diff.h: ....
> 
> I think the preprocessor approach would tend to be simpler, which is an 
> advantage. But whichever approach is chosen, I think one important issue 
> is to make sure that options that *hide* other options are correctly 
> handled in the help printout..

Yep. See my patch 3/3, which just used a char[256] for the short names, 
and a path-list for the long names.

> But that's an implementation issue. The same certainly *can* be done 
> with a recursive setup, just passing a linked list of what the earlier 
> levels were (which is what we do in other places). And it's not like the 
> recursion is going to be very deep or complex.

Exactly.

The more pressing issue is that we have pointers in the option structure, 
which point back to the variables expected to hold the option values.

The recurse approach would need fixing up those (or some ugly copying of 
a struct diff_options).

But the preprocessor approach means wasting space (since we basically have 
the same options in different builtins), and it means that the callback 
functions needed to parse e.g. the diff colour names need to be public.  
Which is not the worst thing, of course.

Ciao,
Dscho

Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]

On Mon, Nov 05, 2007 at 04:29:43PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 5 Nov 2007, Linus Torvalds wrote:
> 
> > On Mon, 5 Nov 2007, Johannes Schindelin wrote:
> > > 
> > > After kicking this around a bit more on IRC, we had another idea.  
> > > Instead of introducing OPT_RECURSE(), do something like OPT__QUIET(), 
> > > only this time in diff.h: ....
> > 
> > I think the preprocessor approach would tend to be simpler, which is an 
> > advantage. But whichever approach is chosen, I think one important issue 
> > is to make sure that options that *hide* other options are correctly 
> > handled in the help printout..
> 
> Yep. See my patch 3/3, which just used a char[256] for the short names, 
> and a path-list for the long names.
> 
> > But that's an implementation issue. The same certainly *can* be done 
> > with a recursive setup, just passing a linked list of what the earlier 
> > levels were (which is what we do in other places). And it's not like the 
> > recursion is going to be very deep or complex.
> 
> Exactly.
> 
> The more pressing issue is that we have pointers in the option structure, 
> which point back to the variables expected to hold the option values.
> 
> The recurse approach would need fixing up those (or some ugly copying of 
> a struct diff_options).
> 
> But the preprocessor approach means wasting space (since we basically have 
> the same options in different builtins),

The "lost" space is the number of options x sizeof(struct option), the
latter being (if I'm correct):

on i386:  9 * 4 = 36 octets
on amd64: 4 x 2 + 8 * 4 + 8 (padding) + 8 * 2 = 64 octets.

It's not even near being an issue :)

>                                          and it means that the callback 
> functions needed to parse e.g. the diff colour names need to be public.  
> Which is not the worst thing, of course.

  Well it's certainly less ugly than copying the diff_options or
reseting it or anything like that. I don't care if we need to make a
couple of opt-parsing function public more than what we could have
needed.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> After kicking this around a bit more on IRC, we had another idea.  Instead 
> of introducing OPT_RECURSE(), do something like OPT__QUIET(), only this 
> time in diff.h:
>
> #define OPT__DIFF(opt) \
> 	OPT_BOOLEAN('p', NULL, &opt.format_patch, "show a patch"), \
> 	...
>
> Pierre said this feels a bit "80s", so I'd like to hear other people's 
> opinions.
>
> Hmm?

As I am from "80s" ;-) I like the simpler "macro" one much
better.  There aren't many things that can go wrong in the
approach.

Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On Mon, Nov 05, 2007 at 09:48:19PM +0000, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > After kicking this around a bit more on IRC, we had another idea.  Instead 
> > of introducing OPT_RECURSE(), do something like OPT__QUIET(), only this 
> > time in diff.h:
> >
> > #define OPT__DIFF(opt) \
> > 	OPT_BOOLEAN('p', NULL, &opt.format_patch, "show a patch"), \
> > 	...
> >
> > Pierre said this feels a bit "80s", so I'd like to hear other people's 
> > opinions.
> >
> > Hmm?
> 
> As I am from "80s" ;-)

Hey grand'pa, how are your rheumatisms today :-P

> I like the simpler "macro" one much better.  There aren't many things
> that can go wrong in the approach.

Okay, I suppose we will end up using the big macros then. Though I had a
look at diff_opt_parse and setup_revisions, the move isn't for tomorrow.

  The current code is heavy (at best), and has some logic that parseopt
is not yet ready to deal with (mainly the --not flags).

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 3/3] parseopt: do not list options with the same name twice

[Johannes Schindelin]

The option parser will take the first exact match, and happily ignore
it when a later option has the same name.  For example, when you have
something like

	OPT_BOOLEAN('i', NULL, &troz, "blah"),
	OPT_BOOLEAN('i', NULL, &brain, "blub"),

in your options array, a command line "prog -i" will increment the
variable "troz", and leave the variable "brain" alone.

This behavior is all good and well, especially since we plan to
have recursive options, e.g. for the diff options, and in some cases
options should be overridden (see for example format-patch's "-n").

This patch matches the usage to this behavior: whenever an option name
was overridden by another option, it is no longer shown.  In the
example above, that means that the usage would only show

    -i    blah

and not print anything about "blub".

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Okay, my plan was borked: the options struct is passed as a
	const.  So this is plan B.

 parse-options.c      |   37 ++++++++++++++++++++++++++-----------
 test-parse-options.c |    2 ++
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index ed8e951..83a221b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1,5 +1,6 @@
 #include "git-compat-util.h"
 #include "parse-options.h"
+#include "path-list.h"
 
 #define OPT_SHORT 1
 #define OPT_UNSET 2
@@ -261,15 +262,18 @@ int parse_options(int argc, const char **argv, const struct option *options,
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2
 
-static void usage_show_options(const struct option *opts)
+static void usage_show_options(const struct option *opts,
+		char *seen_short_names, struct path_list *seen_long_names)
 {
 	for (; opts->type != OPTION_END; opts++) {
-		size_t pos;
+		const char *before_option = "    ";
+		size_t pos = 0;
 		int pad;
 
 		if (opts->type == OPTION_RECURSE) {
 			const struct option *sub = opts->value;
-			usage_show_options(sub);
+			usage_show_options(sub,
+					seen_short_names, seen_long_names);
 			continue;
 		}
 
@@ -280,13 +284,19 @@ static void usage_show_options(const struct option *opts)
 			continue;
 		}
 
-		pos = fprintf(stderr, "    ");
-		if (opts->short_name)
-			pos += fprintf(stderr, "-%c", opts->short_name);
-		if (opts->long_name && opts->short_name)
-			pos += fprintf(stderr, ", ");
-		if (opts->long_name)
-			pos += fprintf(stderr, "--%s", opts->long_name);
+		if (opts->short_name && !seen_short_names[opts->short_name]) {
+			pos += fprintf(stderr, "%s-%c",
+					before_option, opts->short_name);
+			seen_short_names[opts->short_name] = 1;
+			before_option = ", ";
+		}
+		if (opts->long_name && !path_list_has_path(seen_long_names,
+					opts->long_name)) {
+			pos += fprintf(stderr, "%s--%s",
+					before_option, opts->long_name);
+			path_list_insert(opts->long_name, seen_long_names);
+		} else if (*before_option != ',')
+			continue;
 
 		switch (opts->type) {
 		case OPTION_INTEGER:
@@ -329,6 +339,9 @@ static void usage_show_options(const struct option *opts)
 void usage_with_options(const char * const *usagestr,
                         const struct option *opts)
 {
+	char seen_short_names[256];
+	struct path_list seen_long_names = { NULL, 0, 0, 0 };
+
 	fprintf(stderr, "usage: %s\n", *usagestr++);
 	while (*usagestr && **usagestr)
 		fprintf(stderr, "   or: %s\n", *usagestr++);
@@ -338,7 +351,9 @@ void usage_with_options(const char * const *usagestr,
 	if (opts->type != OPTION_GROUP)
 		fputc('\n', stderr);
 
-	usage_show_options(opts);
+	memset(seen_short_names, 0, sizeof(seen_short_names));
+	usage_show_options(opts, seen_short_names, &seen_long_names);
+	path_list_clear(&seen_long_names, 0);
 
 	fputc('\n', stderr);
 
diff --git a/test-parse-options.c b/test-parse-options.c
index ee64fb3..56f6f24 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -16,6 +16,7 @@ int main(int argc, const char **argv)
 	struct option sub[] = {
 		OPT_BOOLEAN('n', "narf", &narf, "what are we doing tonight?"),
 		OPT_INTEGER('z', "zort", &zort, "try to take over the world"),
+		OPT_INTEGER('j', NULL, &boolean, "ignored option"),
 		OPT_END(),
 	};
 	struct option options[] = {
@@ -27,6 +28,7 @@ int main(int argc, const char **argv)
 		OPT_STRING('s', "string", &string, "string", "get a string"),
 		OPT_STRING(0, "string2", &string, "str", "get another string"),
 		OPT_STRING(0, "st", &string, "st", "get another string (pervert ordering)"),
+		OPT_INTEGER(0, "string", &boolean, "ignored option"),
 		OPT_END(),
 	};
 	int i;
-- 
1.5.3.5.1549.g91a3

Re: [PATCH] parse-options: abbreviation engine fix.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]

On lun, nov 05, 2007 at 12:34:00 +0000, Johannes Schindelin wrote:
> 
> When an option could be an ambiguous abbreviation of two options, the code 
> used to error out.  Even if an exact match would have occured later.
> 
> Test and original patch by Pierre Habouzit.
> 
> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> ---
> 
> 	On Mon, 5 Nov 2007, Pierre Habouzit wrote:
> 
> 	> When we had at least two long option then followed by another 
> 	> one that was a prefix of both of them, then the abbreviation 
> 	> detector failed.
> 
> 	Yeah, I assumed that you would never do such a thing, but I agree 
> 	that with recursing option parsing it is much more likely.
> 
> 	It took me some time to understand your patch, and that the moving 
> 	of the UNSET handling was unnecessary.

  yeah, that's because I dislike where it's done. Each time I read it,
I'm under the impression that it clobbers p->opt if this case is not the
one used.

  In fact, I believe that for code clarity we should do what you
proposed earlier: disallow `=` in option names (that's rather sane
anyway) and drop the `rest` variable altogether, just use your current
arg_end and prefixcmp/strncmps. That will incidentally also allow to
remove skip_prefix while we're at it.

  This way we could set p->opt first thing in the function and be done
with it instead of doing it two different ways in the function, hence
making the reader assume one is wrong or at least questionable.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]