{"thread":{"id":"10669","subject":"[PATCH] parseopt: introduce OPT_RECURSE to specify shared options","startedAt":"2007-11-05T12:09:41Z","lastAt":"2007-11-05T13:53:19Z","messageCount":3,"participants":["Johannes Schindelin","Pierre Habouzit"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"58366","messageId":"Pine.LNX.4.64.0711051209061.4362@racer.site","threadId":"10669","inReplyTo":null,"subject":"[PATCH] parseopt: introduce OPT_RECURSE to specify shared options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-05T12:09:41Z","receivedAt":"2007-11-05T12:09:41Z","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\n\tThis is not yet clever enough to show the correct usage when\n\tsome sub option uses the same short or long name as one that\n\thas already been printed.\n\n\tAdd the superlevel options struct as another parameter to\n\tusage_show_options(), and write an unset_option() function\n\twhich takes a short name, a long name, and that superlevel\n\toptions struct, and unsets all options which match.\n\n\tThen teach usage_show_options() to skip those options which\n\thave a short name '\\0' and a long name NULL.\n\n\tBut I leave that as an exercise to the reader.\n\n parse-options.c          |   68 +++++++++++++++++++++++++++++++++++-----------\n parse-options.h          |    2 +\n t/t0040-parse-options.sh |   24 ++++++++++++++++\n test-parse-options.c     |   10 +++++++\n 4 files changed, 88 insertions(+), 16 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex cc09c98..66f79a1 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@@ -178,14 +193,14 @@ is_abbreviated:\n \t}\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@@ -200,8 +215,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@@ -216,8 +236,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@@ -228,22 +252,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@@ -295,6 +315,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 ae49424..de2d285 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@@ -28,6 +30,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@@ -39,6 +43,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@@ -52,6 +58,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@@ -68,6 +76,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@@ -90,4 +100,18 @@ test_expect_failure 'ambiguously abbreviated option' '\n         test $? != 129\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 test_done\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 277cfe4..24613f8 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@@ -24,6 +32,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":"58370","messageId":"20071105123923.GC25574@artemis.corp","threadId":"10669","inReplyTo":"Pine.LNX.4.64.0711051209061.4362@racer.site","subject":"Re: [PATCH] parseopt: introduce OPT_RECURSE to specify shared options","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T12:39:23Z","receivedAt":"2007-11-05T12:39:23Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Nov 05, 2007 at 12:09:41PM +0000, 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> \n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n> \n> \tThis is not yet clever enough to show the correct usage when\n> \tsome sub option uses the same short or long name as one that\n> \thas already been printed.\n> \n> \tAdd the superlevel options struct as another parameter to\n> \tusage_show_options(), and write an unset_option() function\n> \twhich takes a short name, a long name, and that superlevel\n> \toptions struct, and unsets all options which match.\n\n  Alternatively you can have a 256-bit bitfield to mark short options\nthat have already been seen and a hashtable of long options already\nprinted out, and while outputting an option, one should check that\neither the short option or the long option is new.\n\n  This is nicer as some of the struct option may live in .rodata\n\n>  parse-options.c          |   68 +++++++++++++++++++++++++++++++++++-----------\n>  parse-options.h          |    2 +\n>  t/t0040-parse-options.sh |   24 ++++++++++++++++\n>  test-parse-options.c     |   10 +++++++\n>  4 files changed, 88 insertions(+), 16 deletions(-)\n\n  Okay, we discussed this with Johannes on IRC. I came up with the\nrelocation thing because I feared that the msys port (and maybe other ?)\nthat are about to use (or already do) threads would step on each other\ntoes while recursing into a sub-array of options.\n\n  Johannes thinks that this never happens in our codebase, hence that my\npatches are an overkill.\n\n  The likely users of this feature are currently diff options (diff.c\ndiff_opt_parse) and revisions (builtin-log.c setup_revisions).\n\n  Using Johannes patch, we will have to export a global struct\ndiff_option (resp. struct rev_info) from diff.c (resp. revisions.c) and\nno function (or almost) would take struct diff_option (resp struct\nrev_info) as an argument because everyone would work on the global\nvariable[0].\n\n  With my patches, we can work like we do now, with a more functional\napproach.\n\n  I like the kind of code that I allow to write better (I tend to\ndislike big fat global variables), though it's obvious that Johannes\npatch is a lot simpler and I like that.\n\n\n  [0] actually we don't *need* to remove the struct diff_options argument\n      from many functions except from diff_opt_parse, it's just that for\n      real, everybody will work on the same global structure anyway.\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"58383","messageId":"20071105135319.GA6205@artemis.corp","threadId":"10669","inReplyTo":"20071105123923.GC25574@artemis.corp","subject":"Re: [PATCH] parseopt: introduce OPT_RECURSE to specify shared options","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T13:53:19Z","receivedAt":"2007-11-05T13:53:19Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Nov 05, 2007 at 12:39:23PM +0000, Pierre Habouzit wrote:\n>   I like the kind of code that I allow to write better (I tend to\n> dislike big fat global variables), though it's obvious that Johannes\n> patch is a lot simpler and I like that.\n\n  We discussed it further, and what came out is that instead of\nsupporting quite complicated recursion mechanisms (or even a non so\ncomplicated one), we can just define an\nOPT__DIFFOPTIONS(diffopts)/OPT__REVOPTIONS(rev) macro that would inline\nthe needed options.\n\n  That's an idea I had but dismissed. Though, maybe it's not _that_\nugly, is clearly simpler, and one can argue that it's in the logical\ncontinuation of OPT__QUIET and friends. What do you think ?\n\n  If it's the road we decide to take, then my documentation patch (2/4),\nthe parseopt fix (my 1/4 or johannes 1/3) and Johannes usage generator\nenhancement (his 3/3) are still to be taken.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"}]}