{"thread":{"id":"10668","subject":"proposal for an OPTION_SUBARRAY (recursive parser)","startedAt":"2007-11-05T12:03:20Z","lastAt":"2007-11-05T22:14:25Z","messageCount":17,"participants":["Pierre Habouzit","Johannes Schindelin","Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"58364","messageId":"1194264204-3475-1-git-send-email-madcoder@debian.org","threadId":"10668","inReplyTo":null,"subject":"proposal for an OPTION_SUBARRAY (recursive parser)","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T12:03:20Z","receivedAt":"2007-11-05T12:03:20Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"While working on OPTION_SUBARRAY I noticed a quite bad bug in how\nabbrevations are dealt with, here is the fix, should be applied to next:\n  [PATCH 1/4] parse-options: abbreviation engine fix.\n\nHere is a better documentation of the struct option, only touch\ncomments, safe for next:\n  [PATCH 2/4] Some better parse-options documentation.\n\nHere is a 2 patch series that implement recursion and option relocation\nof struct options when nested:\n  [PATCH 3/4] Add OPTION_BASEOFFSET/OPTION_SUBARRAY.\n  [PATCH 4/4] Implement OPTION_SUBARRAY handling.\n\nThose two patch have nor real reason to go anywhere else than pu yet, I\npost them for review and comments, and will probably base diff/log\nmigration on those soon.\n"},{"id":"299023","messageId":"1194264204-3475-2-git-send-email-madcoder@debian.org","threadId":"10668","inReplyTo":"1194264204-3475-1-git-send-email-madcoder@debian.org","subject":"[PATCH 1/4] parse-options: abbreviation engine fix.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T12:03:21Z","receivedAt":"2007-11-05T12:03:21Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"When we had at least two long option then followed by another one that was a\nprefix of both of them, then the abbreviation detector failed.\n\nFix the issue, add a test.\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n parse-options.c          |   48 +++++++++++++++++++++++-----------------------\n t/t0040-parse-options.sh |   13 ++++++++++++\n test-parse-options.c     |    1 +\n 3 files changed, 38 insertions(+), 24 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex cc09c98..d2e32c1 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -119,8 +119,8 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,\n                           const struct option *options)\n {\n \tconst char *arg_end = strchr(arg, '=');\n-\tconst struct option *abbrev_option = NULL;\n-\tint abbrev_flags = 0;\n+\tconst struct option *abbrev_option = NULL, *conflict_option = NULL;\n+\tint abbrev_flags = 0, conflict_flags = 0;\n \n \tif (!arg_end)\n \t\targ_end = arg + strlen(arg);\n@@ -132,42 +132,33 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,\n \t\tif (!options->long_name)\n \t\t\tcontinue;\n \n+\t\t/* special case {n,no,no-} that always conflict */\n+\t\tif (!prefixcmp(\"no-\", arg))\n+\t\t\tdie(\"`--{n,no,no-}' cannot be abbreviated forms of options\");\n+\n \t\trest = skip_prefix(arg, options->long_name);\n \t\tif (!rest) {\n \t\t\t/* abbreviated? */\n \t\t\tif (!strncmp(options->long_name, arg, arg_end - arg)) {\n is_abbreviated:\n-\t\t\t\tif (abbrev_option)\n-\t\t\t\t\treturn error(\"Ambiguous option: %s \"\n-\t\t\t\t\t\t\"(could be --%s%s or --%s%s)\",\n-\t\t\t\t\t\targ,\n-\t\t\t\t\t\t(flags & OPT_UNSET) ?\n-\t\t\t\t\t\t\t\"no-\" : \"\",\n-\t\t\t\t\t\toptions->long_name,\n-\t\t\t\t\t\t(abbrev_flags & OPT_UNSET) ?\n-\t\t\t\t\t\t\t\"no-\" : \"\",\n-\t\t\t\t\t\tabbrev_option->long_name);\n-\t\t\t\tif (!(flags & OPT_UNSET) && *arg_end)\n-\t\t\t\t\tp->opt = arg_end + 1;\n+\t\t\t\tconflict_option = abbrev_option;\n+\t\t\t\tconflict_flags = abbrev_flags;\n \t\t\t\tabbrev_option = options;\n \t\t\t\tabbrev_flags = flags;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\t/* negated and abbreviated very much? */\n-\t\t\tif (!prefixcmp(\"no-\", arg)) {\n-\t\t\t\tflags |= OPT_UNSET;\n-\t\t\t\tgoto is_abbreviated;\n-\t\t\t}\n \t\t\t/* negated? */\n \t\t\tif (strncmp(arg, \"no-\", 3))\n \t\t\t\tcontinue;\n \t\t\tflags |= OPT_UNSET;\n-\t\t\trest = skip_prefix(arg + 3, options->long_name);\n+\t\t\targ += 3;\n+\t\t\trest = skip_prefix(arg, options->long_name);\n \t\t\t/* abbreviated and negated? */\n-\t\t\tif (!rest && !prefixcmp(options->long_name, arg + 3))\n-\t\t\t\tgoto is_abbreviated;\n-\t\t\tif (!rest)\n+\t\t\tif (!rest) {\n+\t\t\t\tif (!strncmp(options->long_name, arg, arg_end - arg))\n+\t\t\t\t\tgoto is_abbreviated;\n \t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \t\tif (*rest) {\n \t\t\tif (*rest != '=')\n@@ -176,8 +167,17 @@ is_abbreviated:\n \t\t}\n \t\treturn get_value(p, options, flags);\n \t}\n-\tif (abbrev_option)\n+\tif (conflict_option)\n+\t\treturn error(\"Ambiguous option: %s (could be --%s%s or --%s%s)\",\n+\t\t\targ, (conflict_flags & OPT_UNSET) ?  \"no-\" : \"\",\n+\t\t\tconflict_option->long_name,\n+\t\t\t(abbrev_flags & OPT_UNSET) ?  \"no-\" : \"\",\n+\t\t\tabbrev_option->long_name);\n+\tif (abbrev_option) {\n+\t\tif (!(abbrev_flags & OPT_UNSET) && *arg_end)\n+\t\t\tp->opt = arg_end + 1;\n \t\treturn get_value(p, abbrev_option, abbrev_flags);\n+\t}\n \treturn error(\"unknown option `%s'\", arg);\n }\n \ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ae49424..ee758e5 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -18,6 +18,7 @@ string options\n     -s, --string <string>\n                           get a string\n     --string2 <str>       get another string\n+    --st <st>             get another string (pervert ordering)\n \n EOF\n \n@@ -90,4 +91,16 @@ test_expect_failure 'ambiguously abbreviated option' '\n         test $? != 129\n '\n \n+cat > expect << EOF\n+boolean: 0\n+integer: 2\n+string: 123\n+EOF\n+\n+test_expect_failure 'non ambiguous option (after two options it abbreviates)' '\n+\ttest-parse-options --st 123 &&\n+\ttest ! -s output.err &&\n+\tgit diff expect output\n+'\n+\n test_done\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 277cfe4..4d3e2ec 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -18,6 +18,7 @@ int main(int argc, const char **argv)\n \t\tOPT_GROUP(\"string options\"),\n \t\tOPT_STRING('s', \"string\", &string, \"string\", \"get a string\"),\n \t\tOPT_STRING(0, \"string2\", &string, \"str\", \"get another string\"),\n+\t\tOPT_STRING(0, \"st\", &string, \"st\", \"get another string (pervert ordering)\"),\n \t\tOPT_END(),\n \t};\n \tint i;\n-- \n1.5.3.5.1531.g59008\n\n"},{"id":"299024","messageId":"1194264204-3475-3-git-send-email-madcoder@debian.org","threadId":"10668","inReplyTo":"1194264204-3475-2-git-send-email-madcoder@debian.org","subject":"[PATCH 2/4] Some better parse-options documentation.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T12:03:22Z","receivedAt":"2007-11-05T12:03:22Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"---\n parse-options.h |   37 +++++++++++++++++++++++++++++++++++--\n 1 files changed, 35 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.h b/parse-options.h\nindex 3a470e5..65bce6e 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -22,6 +22,41 @@ enum parse_opt_option_flags {\n struct option;\n typedef int parse_opt_cb(const struct option *, const char *arg, int unset);\n \n+/*\n+ * `type`::\n+ *   holds the type of the option, you must have an OPTION_END last in your\n+ *   array.\n+ *\n+ * `short_name`::\n+ *   the character to use as a short option name, '\\0' if none.\n+ *\n+ * `long_name`::\n+ *   the long option name, without the leading dashes, NULL if none.\n+ *\n+ * `value`::\n+ *   stores pointers to the values to be filled.\n+ *\n+ * `argh`::\n+ *   token to explain the kind of argument this option wants. Keep it\n+ *   homogenous across the repository.\n+ *\n+ * `help`::\n+ *   the short help associated to what the option does.\n+ *   Must never be NULL (except for OPTION_END).\n+ *   OPTION_GROUP uses this pointer to store the group header.\n+ *\n+ * `flags`::\n+ *   mask of parse_opt_option_flags.\n+ *   PARSE_OPT_OPTARG: says that the argument is optionnal (not for BOOLEANs)\n+ *   PARSE_OPT_NOARG: says that this option takes no argument, for CALLBACKs\n+ *\n+ * `callback`::\n+ *   pointer to the callback to use for OPTION_CALLBACK.\n+ *\n+ * `defval`::\n+ *   default value to fill (*->value) with for PARSE_OPT_OPTARG.\n+ *   CALLBACKS can use it like they want.\n+ */\n struct option {\n \tenum parse_opt_type type;\n \tint short_name;\n@@ -32,8 +67,6 @@ struct option {\n \n \tint flags;\n \tparse_opt_cb *callback;\n-\t/* holds default value for PARSE_OPT_OPTARG,\n-\t   though callbacks can use it like they want */\n \tintptr_t defval;\n };\n \n-- \n1.5.3.5.1531.g59008\n\n"},{"id":"299025","messageId":"1194264204-3475-4-git-send-email-madcoder@debian.org","threadId":"10668","inReplyTo":"1194264204-3475-3-git-send-email-madcoder@debian.org","subject":"[PATCH 3/4] Add OPTION_BASEOFFSET/OPTION_SUBARRAY.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T12:03:23Z","receivedAt":"2007-11-05T12:03:23Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Currently make the implementation die() if SUBARRAYs are encountered.\nRefactor the rest of the code to be ready for recursion.\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n parse-options.c |  114 ++++++++++++++++++++++++++++++++++++++-----------------\n parse-options.h |    6 +++\n 2 files changed, 85 insertions(+), 35 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex d2e32c1..5cea511 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -8,6 +8,9 @@ struct optparse_t {\n \tconst char **argv;\n \tint argc;\n \tconst char *opt;\n+\n+\tconst struct option *abbrev_option, *conflict_option;\n+\tint abbrev_flags, conflict_flags;\n };\n \n static inline const char *get_arg(struct optparse_t *p)\n@@ -104,23 +107,35 @@ static int get_value(struct optparse_t *p,\n \t}\n }\n \n-static int parse_short_opt(struct optparse_t *p, const struct option *options)\n+static int parse_short_opt(struct optparse_t *p, const struct option *options,\n+                           int level)\n {\n-\tfor (; options->type != OPTION_END; options++) {\n-\t\tif (options->short_name == *p->opt) {\n+\tfor (;; options++) {\n+\t\tswitch (options->type) {\n+\t\tcase OPTION_END:\n+\t\t\treturn level > 0 ? -1 : error(\"unknown switch `%c'\", *p->opt);\n+\t\tcase OPTION_GROUP:\n+\t\tcase OPTION_BASEOFFSET:\n+\t\t\tcontinue;\n+\t\tcase OPTION_SUBARRAY:\n+\t\t\tdie(\"unsupported yet\");\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tif (options->short_name != *p->opt)\n+\t\t\t\tcontinue;\n \t\t\tp->opt = p->opt[1] ? p->opt + 1 : NULL;\n \t\t\treturn get_value(p, options, OPT_SHORT);\n \t\t}\n \t}\n-\treturn error(\"unknown switch `%c'\", *p->opt);\n }\n \n static int parse_long_opt(struct optparse_t *p, const char *arg,\n-                          const struct option *options)\n+                          const struct option *options, int level)\n {\n \tconst char *arg_end = strchr(arg, '=');\n-\tconst struct option *abbrev_option = NULL, *conflict_option = NULL;\n-\tint abbrev_flags = 0, conflict_flags = 0;\n+\n+\tif (level == 0)\n+\t\tp->conflict_option = p->abbrev_option = NULL;\n \n \tif (!arg_end)\n \t\targ_end = arg + strlen(arg);\n@@ -129,6 +144,17 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,\n \t\tconst char *rest;\n \t\tint flags = 0;\n \n+\t\tswitch (options->type) {\n+\t\tcase OPTION_GROUP:\n+\t\tcase OPTION_BASEOFFSET:\n+\t\t\tcontinue;\n+\t\tcase OPTION_SUBARRAY:\n+\t\t\tdie(\"unsupported yet\");\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tbreak;\n+\t\t}\n+\n \t\tif (!options->long_name)\n \t\t\tcontinue;\n \n@@ -138,13 +164,16 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,\n \n \t\trest = skip_prefix(arg, options->long_name);\n \t\tif (!rest) {\n+\t\t\t/* negated and abbreviated very much? */\n+\t\t\tif (!prefixcmp(\"no-\", arg))\n+\t\t\t\tdie(\"--n,--no,--no- are never proper abbreviated options\");\n \t\t\t/* abbreviated? */\n \t\t\tif (!strncmp(options->long_name, arg, arg_end - arg)) {\n is_abbreviated:\n-\t\t\t\tconflict_option = abbrev_option;\n-\t\t\t\tconflict_flags = abbrev_flags;\n-\t\t\t\tabbrev_option = options;\n-\t\t\t\tabbrev_flags = flags;\n+\t\t\t\tp->conflict_option = p->abbrev_option;\n+\t\t\t\tp->conflict_flags = p->abbrev_flags;\n+\t\t\t\tp->abbrev_option = options;\n+\t\t\t\tp->abbrev_flags = flags;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\t/* negated? */\n@@ -167,16 +196,18 @@ is_abbreviated:\n \t\t}\n \t\treturn get_value(p, options, flags);\n \t}\n-\tif (conflict_option)\n+\tif (level > 0)\n+\t\treturn -1;\n+\tif (p->conflict_option)\n \t\treturn error(\"Ambiguous option: %s (could be --%s%s or --%s%s)\",\n-\t\t\targ, (conflict_flags & OPT_UNSET) ?  \"no-\" : \"\",\n-\t\t\tconflict_option->long_name,\n-\t\t\t(abbrev_flags & OPT_UNSET) ?  \"no-\" : \"\",\n-\t\t\tabbrev_option->long_name);\n-\tif (abbrev_option) {\n-\t\tif (!(abbrev_flags & OPT_UNSET) && *arg_end)\n+\t\t\targ, (p->conflict_flags & OPT_UNSET) ?  \"no-\" : \"\",\n+\t\t\tp->conflict_option->long_name,\n+\t\t\t(p->abbrev_flags & OPT_UNSET) ?  \"no-\" : \"\",\n+\t\t\tp->abbrev_option->long_name);\n+\tif (p->abbrev_option) {\n+\t\tif (!(p->abbrev_flags & OPT_UNSET) && *arg_end)\n \t\t\tp->opt = arg_end + 1;\n-\t\treturn get_value(p, abbrev_option, abbrev_flags);\n+\t\treturn get_value(p, p->abbrev_option, p->abbrev_flags);\n \t}\n \treturn error(\"unknown option `%s'\", arg);\n }\n@@ -200,7 +231,7 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \t\t\tdo {\n \t\t\t\tif (*args.opt == 'h')\n \t\t\t\t\tusage_with_options(usagestr, options);\n-\t\t\t\tif (parse_short_opt(&args, options) < 0)\n+\t\t\t\tif (parse_short_opt(&args, options, 0) < 0)\n \t\t\t\t\tusage_with_options(usagestr, options);\n \t\t\t} while (args.opt);\n \t\t\tcontinue;\n@@ -216,7 +247,7 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \n \t\tif (!strcmp(arg + 2, \"help\"))\n \t\t\tusage_with_options(usagestr, options);\n-\t\tif (parse_long_opt(&args, arg + 2, options))\n+\t\tif (parse_long_opt(&args, arg + 2, options, 0))\n \t\t\tusage_with_options(usagestr, options);\n \t}\n \n@@ -228,27 +259,27 @@ int parse_options(int argc, const char **argv, const struct option *options,\n #define USAGE_OPTS_WIDTH 24\n #define USAGE_GAP         2\n \n-void usage_with_options(const char * const *usagestr,\n-                        const struct option *opts)\n+static void dump_options(const struct option *opts)\n {\n-\tfprintf(stderr, \"usage: %s\\n\", *usagestr++);\n-\twhile (*usagestr && **usagestr)\n-\t\tfprintf(stderr, \"   or: %s\\n\", *usagestr++);\n-\twhile (*usagestr)\n-\t\tfprintf(stderr, \"    %s\\n\", *usagestr++);\n-\n-\tif (opts->type != OPTION_GROUP)\n-\t\tfputc('\\n', stderr);\n-\n-\tfor (; opts->type != OPTION_END; opts++) {\n+\tfor (;; opts++) {\n \t\tsize_t pos;\n \t\tint pad;\n \n-\t\tif (opts->type == OPTION_GROUP) {\n+\t\tswitch (opts->type) {\n+\t\tcase OPTION_END:\n+\t\t\treturn;\n+\t\tcase OPTION_GROUP:\n \t\t\tfputc('\\n', stderr);\n \t\t\tif (*opts->help)\n \t\t\t\tfprintf(stderr, \"%s\\n\", opts->help);\n \t\t\tcontinue;\n+\t\tcase OPTION_BASEOFFSET:\n+\t\t\tcontinue;\n+\t\tcase OPTION_SUBARRAY:\n+\t\t\tdump_options((struct option *)opts->defval);\n+\t\t\tcontinue;\n+\t\tdefault:\n+\t\t\tbreak;\n \t\t}\n \n \t\tpos = fprintf(stderr, \"    \");\n@@ -295,8 +326,21 @@ void usage_with_options(const char * const *usagestr,\n \t\t}\n \t\tfprintf(stderr, \"%*s%s\\n\", pad + USAGE_GAP, \"\", opts->help);\n \t}\n-\tfputc('\\n', stderr);\n+}\n \n+void usage_with_options(const char * const *usagestr,\n+                        const struct option *opts)\n+{\n+\tfprintf(stderr, \"usage: %s\\n\", *usagestr++);\n+\twhile (*usagestr && **usagestr)\n+\t\tfprintf(stderr, \"   or: %s\\n\", *usagestr++);\n+\twhile (*usagestr)\n+\t\tfprintf(stderr, \"    %s\\n\", *usagestr++);\n+\n+\tif (opts->type != OPTION_GROUP)\n+\t\tfputc('\\n', stderr);\n+\tdump_options(opts);\n+\tfputc('\\n', stderr);\n \texit(129);\n }\n \ndiff --git a/parse-options.h b/parse-options.h\nindex 65bce6e..6668924 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -4,6 +4,8 @@\n enum parse_opt_type {\n \tOPTION_END,\n \tOPTION_GROUP,\n+\tOPTION_BASEOFFSET,\n+\tOPTION_SUBARRAY,\n \tOPTION_BOOLEAN,\n \tOPTION_STRING,\n \tOPTION_INTEGER,\n@@ -35,6 +37,7 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);\n  *\n  * `value`::\n  *   stores pointers to the values to be filled.\n+ *   BASEOFFSET use it to store the offset wrt which the struct was filled.\n  *\n  * `argh`::\n  *   token to explain the kind of argument this option wants. Keep it\n@@ -56,6 +59,9 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);\n  * `defval`::\n  *   default value to fill (*->value) with for PARSE_OPT_OPTARG.\n  *   CALLBACKS can use it like they want.\n+ *   SUBARRAYs use it to store the subarray address.\n+ *   BASEOFFSET use it to store the sizeof the struct used to fill the array.\n+ *              Any `value` that does not points into it is not relocated.\n  */\n struct option {\n \tenum parse_opt_type type;\n-- \n1.5.3.5.1531.g59008\n\n"},{"id":"299026","messageId":"1194264204-3475-5-git-send-email-madcoder@debian.org","threadId":"10668","inReplyTo":"1194264204-3475-4-git-send-email-madcoder@debian.org","subject":"[PATCH 4/4] Implement OPTION_SUBARRAY handling.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T12:03:24Z","receivedAt":"2007-11-05T12:03:24Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Basically, an OPTION_SUBARRAY is a pointer to a new `struct option` array.\nThis array should start with an OPTION_BASEOFFSET used to relocate ->value\npointers.\n\nThe sizeof() of the struct used for the BASEOFFSET entry is also stored so\nthat a subarray can also have global side effects (->values pointers that\nare not in the memory range of the BASE struct are not relocated).\n\nArbitrary nesting of subarrays is supported, though no checks that there is\na subarray loop is performed.\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n parse-options.c          |   78 +++++++++++++++++++++++++++++++++++++++------\n parse-options.h          |    5 +++\n t/t0040-parse-options.sh |   31 +++++++++++++++++-\n test-parse-options.c     |   25 ++++++++++++++-\n 4 files changed, 126 insertions(+), 13 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 5cea511..001d1e5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -4,6 +4,12 @@\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n+struct optreloc_t {\n+\tconst char *base;\n+\tconst char *end;\n+\tintptr_t offs;\n+};\n+\n struct optparse_t {\n \tconst char **argv;\n \tint argc;\n@@ -11,6 +17,8 @@ struct optparse_t {\n \n \tconst struct option *abbrev_option, *conflict_option;\n \tint abbrev_flags, conflict_flags;\n+\n+\tstruct optreloc_t reloc;\n };\n \n static inline const char *get_arg(struct optparse_t *p)\n@@ -39,10 +47,32 @@ static int opterror(const struct option *opt, const char *reason, int flags)\n \treturn error(\"option `%s' %s\", opt->long_name, reason);\n }\n \n+static void *reloc_value(struct optparse_t *p, const struct option *opt)\n+{\n+\tchar *value = opt->value;\n+\tif (value >= p->reloc.base && value < p->reloc.end)\n+\t\treturn value + p->reloc.offs;\n+\treturn value;\n+}\n+\n+static const struct option *reloc_option(struct optparse_t *p,\n+                                         const struct option *opt,\n+                                         struct option *buf)\n+{\n+\tchar *value = opt->value;\n+\tif (value >= p->reloc.base && value < p->reloc.end) {\n+\t\t*buf = *opt;\n+\t\tbuf->value = value + p->reloc.offs;\n+\t\treturn buf;\n+\t}\n+\treturn opt;\n+}\n+\n static int get_value(struct optparse_t *p,\n                      const struct option *opt, int flags)\n {\n \tconst char *s, *arg;\n+\tstruct option tmp;\n \targ = p->opt ? p->opt : (p->argc > 1 ? p->argv[1] : NULL);\n \n \tif (p->opt && (flags & OPT_UNSET))\n@@ -53,26 +83,27 @@ static int get_value(struct optparse_t *p,\n \t\tif (!(flags & OPT_SHORT) && p->opt)\n \t\t\treturn opterror(opt, \"takes no value\", flags);\n \t\tif (flags & OPT_UNSET)\n-\t\t\t*(int *)opt->value = 0;\n+\t\t\t*(int *)reloc_value(p, opt) = 0;\n \t\telse\n-\t\t\t(*(int *)opt->value)++;\n+\t\t\t(*(int *)reloc_value(p, opt))++;\n \t\treturn 0;\n \n \tcase OPTION_STRING:\n \t\tif (flags & OPT_UNSET) {\n-\t\t\t*(const char **)opt->value = (const char *)NULL;\n+\t\t\t*(const char **)reloc_value(p, opt) = (const char *)NULL;\n \t\t\treturn 0;\n \t\t}\n \t\tif (opt->flags & PARSE_OPT_OPTARG && (!arg || *arg == '-')) {\n-\t\t\t*(const char **)opt->value = (const char *)opt->defval;\n+\t\t\t*(const char **)reloc_value(p, opt) = (const char *)opt->defval;\n \t\t\treturn 0;\n \t\t}\n \t\tif (!arg)\n \t\t\treturn opterror(opt, \"requires a value\", flags);\n-\t\t*(const char **)opt->value = get_arg(p);\n+\t\t*(const char **)reloc_value(p, opt) = get_arg(p);\n \t\treturn 0;\n \n \tcase OPTION_CALLBACK:\n+\t\topt = reloc_option(p, opt, &tmp);\n \t\tif (flags & OPT_UNSET)\n \t\t\treturn (*opt->callback)(opt, NULL, 1);\n \t\tif (opt->flags & PARSE_OPT_NOARG) {\n@@ -88,16 +119,16 @@ static int get_value(struct optparse_t *p,\n \n \tcase OPTION_INTEGER:\n \t\tif (flags & OPT_UNSET) {\n-\t\t\t*(int *)opt->value = 0;\n+\t\t\t*(int *)reloc_value(p, opt) = 0;\n \t\t\treturn 0;\n \t\t}\n \t\tif (opt->flags & PARSE_OPT_OPTARG && (!arg || !isdigit(*arg))) {\n-\t\t\t*(int *)opt->value = opt->defval;\n+\t\t\t*(int *)reloc_value(p, opt) = opt->defval;\n \t\t\treturn 0;\n \t\t}\n \t\tif (!arg)\n \t\t\treturn opterror(opt, \"requires a value\", flags);\n-\t\t*(int *)opt->value = strtol(get_arg(p), (char **)&s, 10);\n+\t\t*(int *)reloc_value(p, opt) = strtol(get_arg(p), (char **)&s, 10);\n \t\tif (*s)\n \t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n \t\treturn 0;\n@@ -107,9 +138,24 @@ static int get_value(struct optparse_t *p,\n \t}\n }\n \n+static const struct option *prepare_recursion(struct optparse_t *p,\n+                                              const struct option *opt)\n+{\n+\tconst struct option *subarray = (const struct option *)opt->defval;\n+\tif (subarray->type != OPTION_BASEOFFSET)\n+\t\tdie(\"subarray does not begins with a relocation stanza\");\n+\tp->reloc.base = subarray->value;\n+\tp->reloc.end  = p->reloc.base + subarray->defval;\n+\tp->reloc.offs = (const char *)opt->value - p->reloc.base;\n+\treturn ++subarray;\n+}\n+\n static int parse_short_opt(struct optparse_t *p, const struct option *options,\n                            int level)\n {\n+\tstruct optreloc_t reloc;\n+\tint res;\n+\n \tfor (;; options++) {\n \t\tswitch (options->type) {\n \t\tcase OPTION_END:\n@@ -118,7 +164,11 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options,\n \t\tcase OPTION_BASEOFFSET:\n \t\t\tcontinue;\n \t\tcase OPTION_SUBARRAY:\n-\t\t\tdie(\"unsupported yet\");\n+\t\t\treloc = p->reloc;\n+\t\t\tres = parse_short_opt(p, prepare_recursion(p, options), level + 1);\n+\t\t\tp->reloc = reloc;\n+\t\t\tif (!res)\n+\t\t\t\treturn 0;\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tif (options->short_name != *p->opt)\n@@ -141,15 +191,21 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,\n \t\targ_end = arg + strlen(arg);\n \n \tfor (; options->type != OPTION_END; options++) {\n+\t\tstruct optreloc_t reloc;\n \t\tconst char *rest;\n-\t\tint flags = 0;\n+\t\tint res, flags = 0;\n \n \t\tswitch (options->type) {\n \t\tcase OPTION_GROUP:\n \t\tcase OPTION_BASEOFFSET:\n \t\t\tcontinue;\n \t\tcase OPTION_SUBARRAY:\n-\t\t\tdie(\"unsupported yet\");\n+\t\t\treloc = p->reloc;\n+\t\t\tres = parse_long_opt(p, arg, prepare_recursion(p, options),\n+\t\t\t                     level + 1);\n+\t\t\tp->reloc = reloc;\n+\t\t\tif (!res)\n+\t\t\t\treturn 0;\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tbreak;\ndiff --git a/parse-options.h b/parse-options.h\nindex 6668924..4f5a241 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -84,6 +84,11 @@ struct option {\n #define OPT_CALLBACK(s, l, v, a, h, f) \\\n \t{ OPTION_CALLBACK, (s), (l), (v), (a), (h), 0, (f) }\n \n+#define OPT_BASEOFFS(v)                 \\\n+\t{ OPTION_BASEOFFSET, 0, NULL, (v), NULL, NULL, 0, NULL, sizeof(*(v)) }\n+#define OPT_SUBARRAY(v, sub)            \\\n+\t{ OPTION_SUBARRAY, 0, NULL, (v), NULL, NULL, 0, NULL, (intptr_t)sub }\n+\n /* parse_options() will filter out the processed options and leave the\n  * non-option argments in argv[].\n  * Returns the number of arguments left in argv[].\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ee758e5..c7a754e 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -20,6 +20,10 @@ string options\n     --string2 <str>       get another string\n     --st <st>             get another string (pervert ordering)\n \n+test subarray\n+    --incr-reloc          increment a relocated integer\n+    --incr-fixed          increment a fixed integer\n+\n EOF\n \n test_expect_success 'test help' '\n@@ -32,6 +36,8 @@ cat > expect << EOF\n boolean: 2\n integer: 1729\n string: 123\n+diverted i: 0\n+fixed i: 0\n EOF\n \n test_expect_success 'short options' '\n@@ -43,6 +49,8 @@ cat > expect << EOF\n boolean: 2\n integer: 1729\n string: 321\n+diverted i: 0\n+fixed i: 0\n EOF\n \n test_expect_success 'long options' '\n@@ -56,6 +64,8 @@ cat > expect << EOF\n boolean: 1\n integer: 13\n string: 123\n+diverted i: 0\n+fixed i: 0\n arg 00: a1\n arg 01: b1\n arg 02: --boolean\n@@ -72,6 +82,8 @@ cat > expect << EOF\n boolean: 0\n integer: 2\n string: (not set)\n+diverted i: 0\n+fixed i: 0\n EOF\n \n test_expect_success 'unambiguously abbreviated option' '\n@@ -93,11 +105,28 @@ test_expect_failure 'ambiguously abbreviated option' '\n \n cat > expect << EOF\n boolean: 0\n+integer: 0\n+string: (not set)\n+diverted i: 1\n+fixed i: 2\n+EOF\n+\n+test_expect_success 'subarrays and partial relocation of options' '\n+\ttest-parse-options --incr-reloc --incr-fixed --incr-fixed > output 2> output.err &&\n+\ttest ! -s output.err &&\n+\tgit diff expect output\n+'\n+\n+test_done\n+cat > expect << EOF\n+boolean: 0\n integer: 2\n string: 123\n+diverted i: 0\n+fixed i: 0\n EOF\n \n-test_expect_failure 'non ambiguous option (after two options it abbreviates)' '\n+test_expect_failure 'non ambiguous option (after two options it abbreviates, across subarray)' '\n \ttest-parse-options --st 123 &&\n \ttest ! -s output.err &&\n \tgit diff expect output\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 4d3e2ec..5f740da 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -1,16 +1,34 @@\n #include \"cache.h\"\n #include \"parse-options.h\"\n \n+struct reloc_me_please {\n+\tint integer;\n+};\n+\n static int boolean = 0;\n static int integer = 0;\n static char *string = NULL;\n \n+static int reloc_integer = 0;\n+static int fixed_integer = 0;\n+\n+static const struct option subopts[] = {\n+\tOPT_BASEOFFS(&reloc_integer),\n+\tOPT_STRING(0, \"st\", &string, \"st\", \"get another string (pervert ordering)\"),\n+\tOPT_GROUP(\"test subarray\"),\n+\tOPT_BOOLEAN(0, \"incr-reloc\", &reloc_integer, \"increment a relocated integer\"),\n+\tOPT_BOOLEAN(0, \"incr-fixed\", &fixed_integer, \"increment a fixed integer\"),\n+\tOPT_END(),\n+};\n+\n int main(int argc, const char **argv)\n {\n \tconst char *usage[] = {\n \t\t\"test-parse-options <options>\",\n \t\tNULL\n \t};\n+\tint diverted_integer = 0;\n+\n \tstruct option options[] = {\n \t\tOPT_BOOLEAN('b', \"boolean\", &boolean, \"get a boolean\"),\n \t\tOPT_INTEGER('i', \"integer\", &integer, \"get a integer\"),\n@@ -18,7 +36,7 @@ int main(int argc, const char **argv)\n \t\tOPT_GROUP(\"string options\"),\n \t\tOPT_STRING('s', \"string\", &string, \"string\", \"get a string\"),\n \t\tOPT_STRING(0, \"string2\", &string, \"str\", \"get another string\"),\n-\t\tOPT_STRING(0, \"st\", &string, \"st\", \"get another string (pervert ordering)\"),\n+\t\tOPT_SUBARRAY(&diverted_integer, subopts),\n \t\tOPT_END(),\n \t};\n \tint i;\n@@ -28,9 +46,14 @@ int main(int argc, const char **argv)\n \tprintf(\"boolean: %d\\n\", boolean);\n \tprintf(\"integer: %d\\n\", integer);\n \tprintf(\"string: %s\\n\", string ? string : \"(not set)\");\n+\tprintf(\"diverted i: %d\\n\", diverted_integer);\n+\tprintf(\"fixed i: %d\\n\", fixed_integer);\n \n \tfor (i = 0; i < argc; i++)\n \t\tprintf(\"arg %02d: %s\\n\", i, argv[i]);\n \n+\tif (reloc_integer)\n+\t\tdie(\"reloc_integer should not ever change\");\n+\n \treturn 0;\n }\n-- \n1.5.3.5.1531.g59008\n\n"},{"id":"58369","messageId":"Pine.LNX.4.64.0711051230020.4362@racer.site","threadId":"10668","inReplyTo":"1194264204-3475-2-git-send-email-madcoder@debian.org","subject":"[PATCH] parse-options: abbreviation engine fix.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-05T12:34:00Z","receivedAt":"2007-11-05T12:34:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nWhen an option could be an ambiguous abbreviation of two options, the code \nused to error out.  Even if an exact match would have occured later.\n\nTest and original patch by Pierre Habouzit.\n\nSigned-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n---\n\n\tOn Mon, 5 Nov 2007, Pierre Habouzit wrote:\n\n\t> When we had at least two long option then followed by another \n\t> one that was a prefix of both of them, then the abbreviation \n\t> detector failed.\n\n\tYeah, I assumed that you would never do such a thing, but I agree \n\tthat with recursing option parsing it is much more likely.\n\n\tIt took me some time to understand your patch, and that the moving \n\tof the UNSET handling was unnecessary.\n\n\tIMHO this patch is easier to read.\n\n parse-options.c          |   33 +++++++++++++++++++++------------\n t/t0040-parse-options.sh |   13 +++++++++++++\n test-parse-options.c     |    1 +\n 3 files changed, 35 insertions(+), 12 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex cc09c98..15b32f7 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -119,8 +119,8 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,\n                           const struct option *options)\n {\n \tconst char *arg_end = strchr(arg, '=');\n-\tconst struct option *abbrev_option = NULL;\n-\tint abbrev_flags = 0;\n+\tconst struct option *abbrev_option = NULL, *ambiguous_option = NULL;\n+\tint abbrev_flags = 0, ambiguous_flags = 0;\n \n \tif (!arg_end)\n \t\targ_end = arg + strlen(arg);\n@@ -137,16 +137,16 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,\n \t\t\t/* abbreviated? */\n \t\t\tif (!strncmp(options->long_name, arg, arg_end - arg)) {\n is_abbreviated:\n-\t\t\t\tif (abbrev_option)\n-\t\t\t\t\treturn error(\"Ambiguous option: %s \"\n-\t\t\t\t\t\t\"(could be --%s%s or --%s%s)\",\n-\t\t\t\t\t\targ,\n-\t\t\t\t\t\t(flags & OPT_UNSET) ?\n-\t\t\t\t\t\t\t\"no-\" : \"\",\n-\t\t\t\t\t\toptions->long_name,\n-\t\t\t\t\t\t(abbrev_flags & OPT_UNSET) ?\n-\t\t\t\t\t\t\t\"no-\" : \"\",\n-\t\t\t\t\t\tabbrev_option->long_name);\n+\t\t\t\tif (abbrev_option) {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * If this is abbreviated, it is\n+\t\t\t\t\t * ambiguous. So when there is no\n+\t\t\t\t\t * exact match later, we need to\n+\t\t\t\t\t * error out.\n+\t\t\t\t\t */\n+\t\t\t\t\tambiguous_option = abbrev_option;\n+\t\t\t\t\tambiguous_flags = abbrev_flags;\n+\t\t\t\t}\n \t\t\t\tif (!(flags & OPT_UNSET) && *arg_end)\n \t\t\t\t\tp->opt = arg_end + 1;\n \t\t\t\tabbrev_option = options;\n@@ -176,6 +176,15 @@ is_abbreviated:\n \t\t}\n \t\treturn get_value(p, options, flags);\n \t}\n+\n+\tif (ambiguous_option)\n+\t\treturn error(\"Ambiguous option: %s \"\n+\t\t\t\"(could be --%s%s or --%s%s)\",\n+\t\t\targ,\n+\t\t\t(ambiguous_flags & OPT_UNSET) ?  \"no-\" : \"\",\n+\t\t\tambiguous_option->long_name,\n+\t\t\t(abbrev_flags & OPT_UNSET) ?  \"no-\" : \"\",\n+\t\t\tabbrev_option->long_name);\n \tif (abbrev_option)\n \t\treturn get_value(p, abbrev_option, abbrev_flags);\n \treturn error(\"unknown option `%s'\", arg);\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ae49424..ee758e5 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -18,6 +18,7 @@ string options\n     -s, --string <string>\n                           get a string\n     --string2 <str>       get another string\n+    --st <st>             get another string (pervert ordering)\n \n EOF\n \n@@ -90,4 +91,16 @@ test_expect_failure 'ambiguously abbreviated option' '\n         test $? != 129\n '\n \n+cat > expect << EOF\n+boolean: 0\n+integer: 2\n+string: 123\n+EOF\n+\n+test_expect_failure 'non ambiguous option (after two options it abbreviates)' '\n+\ttest-parse-options --st 123 &&\n+\ttest ! -s output.err &&\n+\tgit diff expect output\n+'\n+\n test_done\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 277cfe4..4d3e2ec 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -18,6 +18,7 @@ int main(int argc, const char **argv)\n \t\tOPT_GROUP(\"string options\"),\n \t\tOPT_STRING('s', \"string\", &string, \"string\", \"get a string\"),\n \t\tOPT_STRING(0, \"string2\", &string, \"str\", \"get another string\"),\n+\t\tOPT_STRING(0, \"st\", &string, \"st\", \"get another string (pervert ordering)\"),\n \t\tOPT_END(),\n \t};\n \tint i;\n-- \n1.5.3.5.1549.g91a3\n"},{"id":"58371","messageId":"Pine.LNX.4.64.0711051237420.4362@racer.site","threadId":"10668","inReplyTo":"Pine.LNX.4.64.0711051230020.4362@racer.site","subject":"Re: [PATCH] parse-options: abbreviation engine fix.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-05T12:38:43Z","receivedAt":"2007-11-05T12:38:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Nov 2007, Johannes Schindelin wrote:\n\n> +cat > expect << EOF\n> +boolean: 0\n> +integer: 2\n> +string: 123\n> +EOF\n> +\n> +test_expect_failure 'non ambiguous option (after two options it abbreviates)' '\n> +\ttest-parse-options --st 123 &&\n> +\ttest ! -s output.err &&\n> +\tgit diff expect output\n> +'\n> +\n\nAaargh!  Yet another instance of test_expect_failure being wrong, wrong, \nwrong.\n\nI'll come up with a replacement patch.\n\nCiao,\nDscho\n"},{"id":"58375","messageId":"20071105125914.GD25574@artemis.corp","threadId":"10668","inReplyTo":"Pine.LNX.4.64.0711051230020.4362@racer.site","subject":"Re: [PATCH] parse-options: abbreviation engine fix.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T12:59:14Z","receivedAt":"2007-11-05T12:59:14Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On lun, nov 05, 2007 at 12:34:00 +0000, Johannes Schindelin wrote:\n> \n> When an option could be an ambiguous abbreviation of two options, the code \n> used to error out.  Even if an exact match would have occured later.\n> \n> Test and original patch by Pierre Habouzit.\n> \n> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> ---\n> \n> \tOn Mon, 5 Nov 2007, Pierre Habouzit wrote:\n> \n> \t> When we had at least two long option then followed by another \n> \t> one that was a prefix of both of them, then the abbreviation \n> \t> detector failed.\n> \n> \tYeah, I assumed that you would never do such a thing, but I agree \n> \tthat with recursing option parsing it is much more likely.\n> \n> \tIt took me some time to understand your patch, and that the moving \n> \tof the UNSET handling was unnecessary.\n\n  yeah, that's because I dislike where it's done. Each time I read it,\nI'm under the impression that it clobbers p->opt if this case is not the\none used.\n\n  In fact, I believe that for code clarity we should do what you\nproposed earlier: disallow `=` in option names (that's rather sane\nanyway) and drop the `rest` variable altogether, just use your current\narg_end and prefixcmp/strncmps. That will incidentally also allow to\nremove skip_prefix while we're at it.\n\n  This way we could set p->opt first thing in the function and be done\nwith it instead of doing it two different ways in the function, hence\nmaking the reader assume one is wrong or at least questionable.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"58378","messageId":"Pine.LNX.4.64.0711051314350.4362@racer.site","threadId":"10668","inReplyTo":"Pine.LNX.4.64.0711051237420.4362@racer.site","subject":"[PATCH 1/3] parse-options: abbreviation engine fix.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-05T13:15:21Z","receivedAt":"2007-11-05T13:15:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nWhen an option could be an ambiguous abbreviation of two options, the code\nused to error out.  Even if an exact match would have occured later.\n\nTest and original patch by Pierre Habouzit.\n\nSigned-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n---\n parse-options.c          |   33 +++++++++++++++++++++------------\n t/t0040-parse-options.sh |   13 +++++++++++++\n test-parse-options.c     |    1 +\n 3 files changed, 35 insertions(+), 12 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex cc09c98..15b32f7 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -119,8 +119,8 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,\n                           const struct option *options)\n {\n \tconst char *arg_end = strchr(arg, '=');\n-\tconst struct option *abbrev_option = NULL;\n-\tint abbrev_flags = 0;\n+\tconst struct option *abbrev_option = NULL, *ambiguous_option = NULL;\n+\tint abbrev_flags = 0, ambiguous_flags = 0;\n \n \tif (!arg_end)\n \t\targ_end = arg + strlen(arg);\n@@ -137,16 +137,16 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,\n \t\t\t/* abbreviated? */\n \t\t\tif (!strncmp(options->long_name, arg, arg_end - arg)) {\n is_abbreviated:\n-\t\t\t\tif (abbrev_option)\n-\t\t\t\t\treturn error(\"Ambiguous option: %s \"\n-\t\t\t\t\t\t\"(could be --%s%s or --%s%s)\",\n-\t\t\t\t\t\targ,\n-\t\t\t\t\t\t(flags & OPT_UNSET) ?\n-\t\t\t\t\t\t\t\"no-\" : \"\",\n-\t\t\t\t\t\toptions->long_name,\n-\t\t\t\t\t\t(abbrev_flags & OPT_UNSET) ?\n-\t\t\t\t\t\t\t\"no-\" : \"\",\n-\t\t\t\t\t\tabbrev_option->long_name);\n+\t\t\t\tif (abbrev_option) {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * If this is abbreviated, it is\n+\t\t\t\t\t * ambiguous. So when there is no\n+\t\t\t\t\t * exact match later, we need to\n+\t\t\t\t\t * error out.\n+\t\t\t\t\t */\n+\t\t\t\t\tambiguous_option = abbrev_option;\n+\t\t\t\t\tambiguous_flags = abbrev_flags;\n+\t\t\t\t}\n \t\t\t\tif (!(flags & OPT_UNSET) && *arg_end)\n \t\t\t\t\tp->opt = arg_end + 1;\n \t\t\t\tabbrev_option = options;\n@@ -176,6 +176,15 @@ is_abbreviated:\n \t\t}\n \t\treturn get_value(p, options, flags);\n \t}\n+\n+\tif (ambiguous_option)\n+\t\treturn error(\"Ambiguous option: %s \"\n+\t\t\t\"(could be --%s%s or --%s%s)\",\n+\t\t\targ,\n+\t\t\t(ambiguous_flags & OPT_UNSET) ?  \"no-\" : \"\",\n+\t\t\tambiguous_option->long_name,\n+\t\t\t(abbrev_flags & OPT_UNSET) ?  \"no-\" : \"\",\n+\t\t\tabbrev_option->long_name);\n \tif (abbrev_option)\n \t\treturn get_value(p, abbrev_option, abbrev_flags);\n \treturn error(\"unknown option `%s'\", arg);\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ae49424..462fdf2 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -18,6 +18,7 @@ string options\n     -s, --string <string>\n                           get a string\n     --string2 <str>       get another string\n+    --st <st>             get another string (pervert ordering)\n \n EOF\n \n@@ -90,4 +91,16 @@ test_expect_failure 'ambiguously abbreviated option' '\n         test $? != 129\n '\n \n+cat > expect << EOF\n+boolean: 0\n+integer: 0\n+string: 123\n+EOF\n+\n+test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n+\ttest-parse-options --st 123 > output 2> output.err &&\n+\ttest ! -s output.err &&\n+\tgit diff expect output\n+'\n+\n test_done\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 277cfe4..4d3e2ec 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -18,6 +18,7 @@ int main(int argc, const char **argv)\n \t\tOPT_GROUP(\"string options\"),\n \t\tOPT_STRING('s', \"string\", &string, \"string\", \"get a string\"),\n \t\tOPT_STRING(0, \"string2\", &string, \"str\", \"get another string\"),\n+\t\tOPT_STRING(0, \"st\", &string, \"st\", \"get another string (pervert ordering)\"),\n \t\tOPT_END(),\n \t};\n \tint i;\n-- \n1.5.3.5.1549.g91a3\n"},{"id":"58379","messageId":"Pine.LNX.4.64.0711051315300.4362@racer.site","threadId":"10668","inReplyTo":"Pine.LNX.4.64.0711051237420.4362@racer.site","subject":"[PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-05T13:15:39Z","receivedAt":"2007-11-05T13:15:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe diff options should not need to be defined in every user of the\ndiffcore.  This provides the framework:\n\n\textern struct option *diff_options;\n\n\tstruct option options[] = {\n\t\tOPT_RECURSE(diff_options),\n\t\t...\n\t\tOPT_END(),\n\t};\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n parse-options.c          |   68 +++++++++++++++++++++++++++++++++++-----------\n parse-options.h          |    2 +\n t/t0040-parse-options.sh |   26 +++++++++++++++++\n test-parse-options.c     |   10 +++++++\n 4 files changed, 90 insertions(+), 16 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 15b32f7..ed8e951 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -4,6 +4,8 @@\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n+#define OPT_NOT_FOUND -2\n+\n struct optparse_t {\n \tconst char **argv;\n \tint argc;\n@@ -111,8 +113,14 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)\n \t\t\tp->opt = p->opt[1] ? p->opt + 1 : NULL;\n \t\t\treturn get_value(p, options, OPT_SHORT);\n \t\t}\n+\t\tif (options->type == OPTION_RECURSE) {\n+\t\t\tconst struct option *sub = options->value;\n+\t\t\tint result = parse_short_opt(p, sub);\n+\t\t\tif (result != OPT_NOT_FOUND)\n+\t\t\t\treturn result;\n+\t\t}\n \t}\n-\treturn error(\"unknown switch `%c'\", *p->opt);\n+\treturn OPT_NOT_FOUND;\n }\n \n static int parse_long_opt(struct optparse_t *p, const char *arg,\n@@ -129,6 +137,13 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,\n \t\tconst char *rest;\n \t\tint flags = 0;\n \n+\t\tif (options->type == OPTION_RECURSE) {\n+\t\t\tconst struct option *sub = options->value;\n+\t\t\tint result = parse_long_opt(p, arg, sub);\n+\t\t\tif (result != OPT_NOT_FOUND)\n+\t\t\t\treturn result;\n+\t\t}\n+\n \t\tif (!options->long_name)\n \t\t\tcontinue;\n \n@@ -187,14 +202,14 @@ is_abbreviated:\n \t\t\tabbrev_option->long_name);\n \tif (abbrev_option)\n \t\treturn get_value(p, abbrev_option, abbrev_flags);\n-\treturn error(\"unknown option `%s'\", arg);\n+\treturn OPT_NOT_FOUND;\n }\n \n int parse_options(int argc, const char **argv, const struct option *options,\n                   const char * const usagestr[], int flags)\n {\n \tstruct optparse_t args = { argv + 1, argc - 1, NULL };\n-\tint j = 0;\n+\tint j = 0, result;\n \n \tfor (; args.argc; args.argc--, args.argv++) {\n \t\tconst char *arg = args.argv[0];\n@@ -209,8 +224,13 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \t\t\tdo {\n \t\t\t\tif (*args.opt == 'h')\n \t\t\t\t\tusage_with_options(usagestr, options);\n-\t\t\t\tif (parse_short_opt(&args, options) < 0)\n+\t\t\t\tresult = parse_short_opt(&args, options);\n+\t\t\t\tif (result < 0) {\n+\t\t\t\t\tif (result == OPT_NOT_FOUND)\n+\t\t\t\t\t\terror(\"unknown switch `%c'\",\n+\t\t\t\t\t\t\t\targ[1]);\n \t\t\t\t\tusage_with_options(usagestr, options);\n+\t\t\t\t}\n \t\t\t} while (args.opt);\n \t\t\tcontinue;\n \t\t}\n@@ -225,8 +245,12 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \n \t\tif (!strcmp(arg + 2, \"help\"))\n \t\t\tusage_with_options(usagestr, options);\n-\t\tif (parse_long_opt(&args, arg + 2, options))\n+\t\tresult = parse_long_opt(&args, arg + 2, options);\n+\t\tif (result) {\n+\t\t\tif (result == OPT_NOT_FOUND)\n+\t\t\t\terror(\"unknown option `%s'\", arg + 2);\n \t\t\tusage_with_options(usagestr, options);\n+\t\t}\n \t}\n \n \tmemmove(argv + j, args.argv, args.argc * sizeof(*argv));\n@@ -237,22 +261,18 @@ int parse_options(int argc, const char **argv, const struct option *options,\n #define USAGE_OPTS_WIDTH 24\n #define USAGE_GAP         2\n \n-void usage_with_options(const char * const *usagestr,\n-                        const struct option *opts)\n+static void usage_show_options(const struct option *opts)\n {\n-\tfprintf(stderr, \"usage: %s\\n\", *usagestr++);\n-\twhile (*usagestr && **usagestr)\n-\t\tfprintf(stderr, \"   or: %s\\n\", *usagestr++);\n-\twhile (*usagestr)\n-\t\tfprintf(stderr, \"    %s\\n\", *usagestr++);\n-\n-\tif (opts->type != OPTION_GROUP)\n-\t\tfputc('\\n', stderr);\n-\n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tsize_t pos;\n \t\tint pad;\n \n+\t\tif (opts->type == OPTION_RECURSE) {\n+\t\t\tconst struct option *sub = opts->value;\n+\t\t\tusage_show_options(sub);\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tif (opts->type == OPTION_GROUP) {\n \t\t\tfputc('\\n', stderr);\n \t\t\tif (*opts->help)\n@@ -304,6 +324,22 @@ void usage_with_options(const char * const *usagestr,\n \t\t}\n \t\tfprintf(stderr, \"%*s%s\\n\", pad + USAGE_GAP, \"\", opts->help);\n \t}\n+}\n+\n+void usage_with_options(const char * const *usagestr,\n+                        const struct option *opts)\n+{\n+\tfprintf(stderr, \"usage: %s\\n\", *usagestr++);\n+\twhile (*usagestr && **usagestr)\n+\t\tfprintf(stderr, \"   or: %s\\n\", *usagestr++);\n+\twhile (*usagestr)\n+\t\tfprintf(stderr, \"    %s\\n\", *usagestr++);\n+\n+\tif (opts->type != OPTION_GROUP)\n+\t\tfputc('\\n', stderr);\n+\n+\tusage_show_options(opts);\n+\n \tfputc('\\n', stderr);\n \n \texit(129);\ndiff --git a/parse-options.h b/parse-options.h\nindex 3a470e5..9ec1e35 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -8,6 +8,7 @@ enum parse_opt_type {\n \tOPTION_STRING,\n \tOPTION_INTEGER,\n \tOPTION_CALLBACK,\n+\tOPTION_RECURSE,\n };\n \n enum parse_opt_flags {\n@@ -44,6 +45,7 @@ struct option {\n #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }\n #define OPT_CALLBACK(s, l, v, a, h, f) \\\n \t{ OPTION_CALLBACK, (s), (l), (v), (a), (h), 0, (f) }\n+#define OPT_RECURSE(options)        { OPTION_RECURSE, 0, NULL, options }\n \n /* parse_options() will filter out the processed options and leave the\n  * non-option argments in argv[].\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 462fdf2..9c3efaa 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -13,6 +13,8 @@ usage: test-parse-options <options>\n     -b, --boolean         get a boolean\n     -i, --integer <n>     get a integer\n     -j <n>                get a integer, too\n+    -n, --narf            what are we doing tonight?\n+    -z, --zort <n>        try to take over the world\n \n string options\n     -s, --string <string>\n@@ -29,6 +31,8 @@ test_expect_success 'test help' '\n '\n \n cat > expect << EOF\n+narf: 0\n+zort: 1\n boolean: 2\n integer: 1729\n string: 123\n@@ -40,6 +44,8 @@ test_expect_success 'short options' '\n \ttest ! -s output.err\n '\n cat > expect << EOF\n+narf: 0\n+zort: 1\n boolean: 2\n integer: 1729\n string: 321\n@@ -53,6 +59,8 @@ test_expect_success 'long options' '\n '\n \n cat > expect << EOF\n+narf: 0\n+zort: 1\n boolean: 1\n integer: 13\n string: 123\n@@ -69,6 +77,8 @@ test_expect_success 'intermingled arguments' '\n '\n \n cat > expect << EOF\n+narf: 0\n+zort: 1\n boolean: 0\n integer: 2\n string: (not set)\n@@ -92,6 +102,22 @@ test_expect_failure 'ambiguously abbreviated option' '\n '\n \n cat > expect << EOF\n+narf: 1\n+zort: 5\n+boolean: 0\n+integer: 3\n+string: (not set)\n+EOF\n+\n+test_expect_success 'recurse works' '\n+\ttest-parse-options --narf -z5 --int=3 > output 2> output.err &&\n+\ttest ! -s output.err &&\n+\tgit diff expect output\n+'\n+\n+cat > expect << EOF\n+narf: 0\n+zort: 1\n boolean: 0\n integer: 0\n string: 123\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 4d3e2ec..ee64fb3 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -1,6 +1,8 @@\n #include \"cache.h\"\n #include \"parse-options.h\"\n \n+static int narf = 0;\n+static int zort = 1;\n static int boolean = 0;\n static int integer = 0;\n static char *string = NULL;\n@@ -11,10 +13,16 @@ int main(int argc, const char **argv)\n \t\t\"test-parse-options <options>\",\n \t\tNULL\n \t};\n+\tstruct option sub[] = {\n+\t\tOPT_BOOLEAN('n', \"narf\", &narf, \"what are we doing tonight?\"),\n+\t\tOPT_INTEGER('z', \"zort\", &zort, \"try to take over the world\"),\n+\t\tOPT_END(),\n+\t};\n \tstruct option options[] = {\n \t\tOPT_BOOLEAN('b', \"boolean\", &boolean, \"get a boolean\"),\n \t\tOPT_INTEGER('i', \"integer\", &integer, \"get a integer\"),\n \t\tOPT_INTEGER('j', NULL, &integer, \"get a integer, too\"),\n+\t\tOPT_RECURSE(sub),\n \t\tOPT_GROUP(\"string options\"),\n \t\tOPT_STRING('s', \"string\", &string, \"string\", \"get a string\"),\n \t\tOPT_STRING(0, \"string2\", &string, \"str\", \"get another string\"),\n@@ -25,6 +33,8 @@ int main(int argc, const char **argv)\n \n \targc = parse_options(argc, argv, options, usage, 0);\n \n+\tprintf(\"narf: %d\\n\", narf);\n+\tprintf(\"zort: %d\\n\", zort);\n \tprintf(\"boolean: %d\\n\", boolean);\n \tprintf(\"integer: %d\\n\", integer);\n \tprintf(\"string: %s\\n\", string ? string : \"(not set)\");\n-- \n1.5.3.5.1549.g91a3\n"},{"id":"58380","messageId":"Pine.LNX.4.64.0711051315460.4362@racer.site","threadId":"10668","inReplyTo":"Pine.LNX.4.64.0711051237420.4362@racer.site","subject":"[PATCH 3/3] parseopt: do not list options with the same name twice","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-05T13:15:58Z","receivedAt":"2007-11-05T13:15:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe option parser will take the first exact match, and happily ignore\nit when a later option has the same name.  For example, when you have\nsomething like\n\n\tOPT_BOOLEAN('i', NULL, &troz, \"blah\"),\n\tOPT_BOOLEAN('i', NULL, &brain, \"blub\"),\n\nin your options array, a command line \"prog -i\" will increment the\nvariable \"troz\", and leave the variable \"brain\" alone.\n\nThis behavior is all good and well, especially since we plan to\nhave recursive options, e.g. for the diff options, and in some cases\noptions should be overridden (see for example format-patch's \"-n\").\n\nThis patch matches the usage to this behavior: whenever an option name\nwas overridden by another option, it is no longer shown.  In the\nexample above, that means that the usage would only show\n\n    -i    blah\n\nand not print anything about \"blub\".\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\n\tOkay, my plan was borked: the options struct is passed as a\n\tconst.  So this is plan B.\n\n parse-options.c      |   37 ++++++++++++++++++++++++++-----------\n test-parse-options.c |    2 ++\n 2 files changed, 28 insertions(+), 11 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex ed8e951..83a221b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1,5 +1,6 @@\n #include \"git-compat-util.h\"\n #include \"parse-options.h\"\n+#include \"path-list.h\"\n \n #define OPT_SHORT 1\n #define OPT_UNSET 2\n@@ -261,15 +262,18 @@ int parse_options(int argc, const char **argv, const struct option *options,\n #define USAGE_OPTS_WIDTH 24\n #define USAGE_GAP         2\n \n-static void usage_show_options(const struct option *opts)\n+static void usage_show_options(const struct option *opts,\n+\t\tchar *seen_short_names, struct path_list *seen_long_names)\n {\n \tfor (; opts->type != OPTION_END; opts++) {\n-\t\tsize_t pos;\n+\t\tconst char *before_option = \"    \";\n+\t\tsize_t pos = 0;\n \t\tint pad;\n \n \t\tif (opts->type == OPTION_RECURSE) {\n \t\t\tconst struct option *sub = opts->value;\n-\t\t\tusage_show_options(sub);\n+\t\t\tusage_show_options(sub,\n+\t\t\t\t\tseen_short_names, seen_long_names);\n \t\t\tcontinue;\n \t\t}\n \n@@ -280,13 +284,19 @@ static void usage_show_options(const struct option *opts)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tpos = fprintf(stderr, \"    \");\n-\t\tif (opts->short_name)\n-\t\t\tpos += fprintf(stderr, \"-%c\", opts->short_name);\n-\t\tif (opts->long_name && opts->short_name)\n-\t\t\tpos += fprintf(stderr, \", \");\n-\t\tif (opts->long_name)\n-\t\t\tpos += fprintf(stderr, \"--%s\", opts->long_name);\n+\t\tif (opts->short_name && !seen_short_names[opts->short_name]) {\n+\t\t\tpos += fprintf(stderr, \"%s-%c\",\n+\t\t\t\t\tbefore_option, opts->short_name);\n+\t\t\tseen_short_names[opts->short_name] = 1;\n+\t\t\tbefore_option = \", \";\n+\t\t}\n+\t\tif (opts->long_name && !path_list_has_path(seen_long_names,\n+\t\t\t\t\topts->long_name)) {\n+\t\t\tpos += fprintf(stderr, \"%s--%s\",\n+\t\t\t\t\tbefore_option, opts->long_name);\n+\t\t\tpath_list_insert(opts->long_name, seen_long_names);\n+\t\t} else if (*before_option != ',')\n+\t\t\tcontinue;\n \n \t\tswitch (opts->type) {\n \t\tcase OPTION_INTEGER:\n@@ -329,6 +339,9 @@ static void usage_show_options(const struct option *opts)\n void usage_with_options(const char * const *usagestr,\n                         const struct option *opts)\n {\n+\tchar seen_short_names[256];\n+\tstruct path_list seen_long_names = { NULL, 0, 0, 0 };\n+\n \tfprintf(stderr, \"usage: %s\\n\", *usagestr++);\n \twhile (*usagestr && **usagestr)\n \t\tfprintf(stderr, \"   or: %s\\n\", *usagestr++);\n@@ -338,7 +351,9 @@ void usage_with_options(const char * const *usagestr,\n \tif (opts->type != OPTION_GROUP)\n \t\tfputc('\\n', stderr);\n \n-\tusage_show_options(opts);\n+\tmemset(seen_short_names, 0, sizeof(seen_short_names));\n+\tusage_show_options(opts, seen_short_names, &seen_long_names);\n+\tpath_list_clear(&seen_long_names, 0);\n \n \tfputc('\\n', stderr);\n \ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex ee64fb3..56f6f24 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -16,6 +16,7 @@ int main(int argc, const char **argv)\n \tstruct option sub[] = {\n \t\tOPT_BOOLEAN('n', \"narf\", &narf, \"what are we doing tonight?\"),\n \t\tOPT_INTEGER('z', \"zort\", &zort, \"try to take over the world\"),\n+\t\tOPT_INTEGER('j', NULL, &boolean, \"ignored option\"),\n \t\tOPT_END(),\n \t};\n \tstruct option options[] = {\n@@ -27,6 +28,7 @@ int main(int argc, const char **argv)\n \t\tOPT_STRING('s', \"string\", &string, \"string\", \"get a string\"),\n \t\tOPT_STRING(0, \"string2\", &string, \"str\", \"get another string\"),\n \t\tOPT_STRING(0, \"st\", &string, \"st\", \"get another string (pervert ordering)\"),\n+\t\tOPT_INTEGER(0, \"string\", &boolean, \"ignored option\"),\n \t\tOPT_END(),\n \t};\n \tint i;\n-- \n1.5.3.5.1549.g91a3\n"},{"id":"58381","messageId":"Pine.LNX.4.64.0711051340490.4362@racer.site","threadId":"10668","inReplyTo":"Pine.LNX.4.64.0711051315300.4362@racer.site","subject":"Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-05T13:46:20Z","receivedAt":"2007-11-05T13:46:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Nov 2007, Johannes Schindelin wrote:\n\n> The diff options should not need to be defined in every user of the\n> diffcore.  This provides the framework:\n> \n> \textern struct option *diff_options;\n> \n> \tstruct option options[] = {\n> \t\tOPT_RECURSE(diff_options),\n> \t\t...\n> \t\tOPT_END(),\n> \t};\n\nAfter kicking this around a bit more on IRC, we had another idea.  Instead \nof introducing OPT_RECURSE(), do something like OPT__QUIET(), only this \ntime in diff.h:\n\n#define OPT__DIFF(opt) \\\n\tOPT_BOOLEAN('p', NULL, &opt.format_patch, \"show a patch\"), \\\n\t...\n\nPierre said this feels a bit \"80s\", so I'd like to hear other people's \nopinions.\n\nHmm?\n\nCiao,\nDscho\n"},{"id":"58387","messageId":"alpine.LFD.0.999.0711050755340.15101@woody.linux-foundation.org","threadId":"10668","inReplyTo":"Pine.LNX.4.64.0711051340490.4362@racer.site","subject":"Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-11-05T16:15:31Z","receivedAt":"2007-11-05T16:15:31Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 5 Nov 2007, Johannes Schindelin wrote:\n> \n> After kicking this around a bit more on IRC, we had another idea.  Instead \n> of introducing OPT_RECURSE(), do something like OPT__QUIET(), only this \n> time in diff.h: ....\n\nI think the preprocessor approach would tend to be simpler, which is an \nadvantage. But whichever approach is chosen, I think one important issue \nis to make sure that options that *hide* other options are correctly \nhandled in the help printout..\n\nWe have a few cases where a \"recursive\" option is hidden. For example, the \noption \"-n\" can mean different things in different contexts: in \ngit-format-patch, for example, the \"-n\" is handled *before* calling \nsetup_revisions() and allt he normal revision flags, which means that the \nformat-patch -specific meaning of \"-n\" overrides the normal \"revision\" \nmeaning. \n\nI suspect that in this kind of situation, it's actually easier to have a \nsingle linear array of options, because the option parser can just walk \nback and check \"oh, I already saw this short option, so I should not \noutput it as documentation because this version of it is hidden by the \nearlier one\". \n\nBut that's an implementation issue. The same certainly *can* be done with \na recursive setup, just passing a linked list of what the earlier levels \nwere (which is what we do in other places). And it's not like the \nrecursion is going to be very deep or complex.\n\n\t\tLinus\n"},{"id":"58388","messageId":"Pine.LNX.4.64.0711051623450.4362@racer.site","threadId":"10668","inReplyTo":"alpine.LFD.0.999.0711050755340.15101@woody.linux-foundation.org","subject":"Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-05T16:29:43Z","receivedAt":"2007-11-05T16:29:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Nov 2007, Linus Torvalds wrote:\n\n> On Mon, 5 Nov 2007, Johannes Schindelin wrote:\n> > \n> > After kicking this around a bit more on IRC, we had another idea.  \n> > Instead of introducing OPT_RECURSE(), do something like OPT__QUIET(), \n> > only this time in diff.h: ....\n> \n> I think the preprocessor approach would tend to be simpler, which is an \n> advantage. But whichever approach is chosen, I think one important issue \n> is to make sure that options that *hide* other options are correctly \n> handled in the help printout..\n\nYep. See my patch 3/3, which just used a char[256] for the short names, \nand a path-list for the long names.\n\n> But that's an implementation issue. The same certainly *can* be done \n> with a recursive setup, just passing a linked list of what the earlier \n> levels were (which is what we do in other places). And it's not like the \n> recursion is going to be very deep or complex.\n\nExactly.\n\nThe more pressing issue is that we have pointers in the option structure, \nwhich point back to the variables expected to hold the option values.\n\nThe recurse approach would need fixing up those (or some ugly copying of \na struct diff_options).\n\nBut the preprocessor approach means wasting space (since we basically have \nthe same options in different builtins), and it means that the callback \nfunctions needed to parse e.g. the diff colour names need to be public.  \nWhich is not the worst thing, of course.\n\nCiao,\nDscho\n"},{"id":"58390","messageId":"20071105165359.GC6205@artemis.corp","threadId":"10668","inReplyTo":"Pine.LNX.4.64.0711051623450.4362@racer.site","subject":"Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T16:53:59Z","receivedAt":"2007-11-05T16:53:59Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Nov 05, 2007 at 04:29:43PM +0000, Johannes Schindelin wrote:\n> Hi,\n> \n> On Mon, 5 Nov 2007, Linus Torvalds wrote:\n> \n> > On Mon, 5 Nov 2007, Johannes Schindelin wrote:\n> > > \n> > > After kicking this around a bit more on IRC, we had another idea.  \n> > > Instead of introducing OPT_RECURSE(), do something like OPT__QUIET(), \n> > > only this time in diff.h: ....\n> > \n> > I think the preprocessor approach would tend to be simpler, which is an \n> > advantage. But whichever approach is chosen, I think one important issue \n> > is to make sure that options that *hide* other options are correctly \n> > handled in the help printout..\n> \n> Yep. See my patch 3/3, which just used a char[256] for the short names, \n> and a path-list for the long names.\n> \n> > But that's an implementation issue. The same certainly *can* be done \n> > with a recursive setup, just passing a linked list of what the earlier \n> > levels were (which is what we do in other places). And it's not like the \n> > recursion is going to be very deep or complex.\n> \n> Exactly.\n> \n> The more pressing issue is that we have pointers in the option structure, \n> which point back to the variables expected to hold the option values.\n> \n> The recurse approach would need fixing up those (or some ugly copying of \n> a struct diff_options).\n> \n> But the preprocessor approach means wasting space (since we basically have \n> the same options in different builtins),\n\nThe \"lost\" space is the number of options x sizeof(struct option), the\nlatter being (if I'm correct):\n\non i386:  9 * 4 = 36 octets\non amd64: 4 x 2 + 8 * 4 + 8 (padding) + 8 * 2 = 64 octets.\n\nIt's not even near being an issue :)\n\n>                                          and it means that the callback \n> functions needed to parse e.g. the diff colour names need to be public.  \n> Which is not the worst thing, of course.\n\n  Well it's certainly less ugly than copying the diff_options or\nreseting it or anything like that. I don't care if we need to make a\ncouple of opt-parsing function public more than what we could have\nneeded.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"58428","messageId":"7vd4uonz30.fsf@gitster.siamese.dyndns.org","threadId":"10668","inReplyTo":"Pine.LNX.4.64.0711051340490.4362@racer.site","subject":"Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-05T21:48:19Z","receivedAt":"2007-11-05T21:48:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> After kicking this around a bit more on IRC, we had another idea.  Instead \n> of introducing OPT_RECURSE(), do something like OPT__QUIET(), only this \n> time in diff.h:\n>\n> #define OPT__DIFF(opt) \\\n> \tOPT_BOOLEAN('p', NULL, &opt.format_patch, \"show a patch\"), \\\n> \t...\n>\n> Pierre said this feels a bit \"80s\", so I'd like to hear other people's \n> opinions.\n>\n> Hmm?\n\nAs I am from \"80s\" ;-) I like the simpler \"macro\" one much\nbetter.  There aren't many things that can go wrong in the\napproach.\n"},{"id":"58434","messageId":"20071105221425.GQ8939@artemis.corp","threadId":"10668","inReplyTo":"7vd4uonz30.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T22:14:25Z","receivedAt":"2007-11-05T22:14:25Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Nov 05, 2007 at 09:48:19PM +0000, Junio C Hamano wrote:\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > After kicking this around a bit more on IRC, we had another idea.  Instead \n> > of introducing OPT_RECURSE(), do something like OPT__QUIET(), only this \n> > time in diff.h:\n> >\n> > #define OPT__DIFF(opt) \\\n> > \tOPT_BOOLEAN('p', NULL, &opt.format_patch, \"show a patch\"), \\\n> > \t...\n> >\n> > Pierre said this feels a bit \"80s\", so I'd like to hear other people's \n> > opinions.\n> >\n> > Hmm?\n> \n> As I am from \"80s\" ;-)\n\nHey grand'pa, how are your rheumatisms today :-P\n\n> I like the simpler \"macro\" one much better.  There aren't many things\n> that can go wrong in the approach.\n\nOkay, I suppose we will end up using the big macros then. Though I had a\nlook at diff_opt_parse and setup_revisions, the move isn't for tomorrow.\n\n  The current code is heavy (at best), and has some logic that parseopt\nis not yet ready to deal with (mainly the --not flags).\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"}]}