{"thread":{"id":"14096","subject":"Convert 'git blame' to parse_options()","startedAt":"2008-06-23T05:15:41Z","lastAt":"2008-06-26T09:37:32Z","messageCount":82,"participants":["Linus Torvalds","Junio C Hamano","Pierre Habouzit","Johannes Schindelin","Jeff King","Johannes Sixt","Andreas Ericsson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"80673","messageId":"alpine.LFD.1.10.0806222207220.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":null,"subject":"Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T05:15:41Z","receivedAt":"2008-06-23T05:15:41Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nWhy? I did some more size statistics, and 'cmd_blame()' was the biggest \nfunction in all of git. Admittedly my script for finding sizes is probably \nbroken, but it does look very messy.\n\nJust cleaning up the option parsing actually cuts down the size a bit, and \nwhile parse_options() isn't really perfect for what cmd_blame() wants, \nit's manageable.\n\nI think I'll want to add a PARSE_OPT_IGNORE_UNRECOGNIZED flag, and also \nmake it not write the resulting array into argv[0..] but back into \nargv[1..] (so that you can use parse_options() as a _filter_ for options \nparsing and make it easier to do partial conversions), but in the meantime \nthis mostly works.\n\nSigned-off-by: Linus Torvalds \n---\n\nThat builtin-blame option parsing is really ugly, trying to then move \nunrecognized options to be parsed by a secondary\n\n\tsetup_revisions(unk, argv, &revs, NULL);\n\nwhich parses the rest. This is _really_ not what parse_options() was \nwritten with in mind, and to give strictly the same behaviour as before \nyou really do need some kind of PARSE_OPT_IGNORE_UNRECOGNIZED flag, but I \nthink this is already a good enough halfway point.\n\nHmm?\n\nBtw, making the \"struct options\" array be 'static' avoids having to build \nit up at run-time on the stack, which is why it's usually a good idea. Of \ncourse, it means that the option variables themselves have to be static, \nwhich has its own set of downsides, so it's a balancing act.\n\n builtin-blame.c | 177 +++++++++++++++++++++++++++----------------------------\n 1 files changed, 86 insertions(+), 91 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex b451f6c..f04bd93 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -18,24 +18,11 @@\n #include \"cache-tree.h\"\n #include \"path-list.h\"\n #include \"mailmap.h\"\n+#include \"parse-options.h\"\n \n-static char blame_usage[] =\n-\"git-blame [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-p] [-w] [-L n,m] [-S ] [-M] [-C] [-C] [--contents ] [--incremental] [commit] [--] file\\n\"\n-\" -c Use the same output mode as git-annotate (Default: off)\\n\"\n-\" -b Show blank SHA-1 for boundary commits (Default: off)\\n\"\n-\" -l Show long commit SHA1 (Default: off)\\n\"\n-\" --root Do not treat root commits as boundaries (Default: off)\\n\"\n-\" -t Show raw timestamp (Default: off)\\n\"\n-\" -f, --show-name Show original filename (Default: auto)\\n\"\n-\" -n, --show-number Show original linenumber (Default: off)\\n\"\n-\" -s Suppress author name and timestamp (Default: off)\\n\"\n-\" -p, --porcelain Show in a format designed for machine consumption\\n\"\n-\" -w Ignore whitespace differences\\n\"\n-\" -L n,m Process only line range n,m, counting from 1\\n\"\n-\" -M, -C Find line movements within and across files\\n\"\n-\" --incremental Show blame entries as we find them, incrementally\\n\"\n-\" --contents file Use 's contents as the final image\\n\"\n-\" -S revs-file Use revisions from revs-file instead of calling git-rev-list\\n\";\n+static char blame_usage[] = \"git-blame [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-p] [-w] [-L n,m] [-S ] [-M] [-C] [-C] [--contents ] [--incremental] [commit] [--] file\";\n+\n+static const char *blame_opt_usage[] = { blame_usage, NULL };\n \n static int longest_file;\n static int longest_author;\n@@ -2122,6 +2109,50 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con\n \treturn commit;\n }\n \n+static int blame_copy_callback(const struct option *option, const char *arg, int unset)\n+{\n+\tint *opt = option->value;\n+\n+\t/*\n+\t * -C enables copy from removed files;\n+\t * -C -C enables copy from existing files, but only\n+\t * when blaming a new file;\n+\t * -C -C -C enables copy from existing files for\n+\t * everybody\n+\t */\n+\tif (*opt & PICKAXE_BLAME_COPY_HARDER)\n+\t\t*opt |= PICKAXE_BLAME_COPY_HARDEST;\n+\tif (*opt & PICKAXE_BLAME_COPY)\n+\t\t*opt |= PICKAXE_BLAME_COPY_HARDER;\n+\t*opt |= PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE;\n+\n+\tif (arg)\n+\t\tblame_copy_score = parse_score(arg);\n+\treturn 0;\n+}\n+\n+static int blame_move_callback(const struct option *option, const char *arg, int unset)\n+{\n+\tint *opt = option->value;\n+\n+\t*opt |= PICKAXE_BLAME_MOVE;\n+\n+\tif (arg)\n+\t\tblame_move_score = parse_score(arg);\n+\treturn 0;\n+}\n+\n+static int blame_bottomtop_callback(const struct option *option, const char *arg, int unset)\n+{\n+\tconst char **bottomtop = option->value;\n+\tif (!arg)\n+\t\treturn -1;\n+\tif (*bottomtop)\n+\t\tdie(\"More than one '-L n,m' option given\");\n+\t*bottomtop = arg;\n+\treturn 0;\n+}\n+\n int cmd_blame(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n@@ -2129,94 +2160,58 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tstruct scoreboard sb;\n \tstruct origin *o;\n \tstruct blame_entry *ent;\n-\tint i, seen_dashdash, unk, opt;\n+\tint i, seen_dashdash, unk;\n \tlong bottom, top, lno;\n-\tint output_option = 0;\n-\tint show_stats = 0;\n-\tconst char *revs_file = NULL;\n \tconst char *final_commit_name = NULL;\n \tenum object_type type;\n-\tconst char *bottomtop = NULL;\n-\tconst char *contents_from = NULL;\n+\n+\tstatic const char *bottomtop = NULL;\n+\tstatic int output_option = 0, opt = 0;\n+\tstatic int show_stats = 0;\n+\tstatic const char *revs_file = NULL;\n+\tstatic const char *contents_from = NULL;\n+\tstatic const struct option options[] = {\n+\t\tOPT_BOOLEAN(0, \"incremental\", &incremental, \"Show blame entries as we find them, incrementally\"),\n+\t\tOPT_BOOLEAN('b', NULL, &blank_boundary, \"Show blank SHA-1 for boundary commits (Default: off)\"),\n+\t\tOPT_BOOLEAN(0, \"root\", &show_root, \"Do not treat root commits as boundaries (Default: off)\"),\n+\t\tOPT_BOOLEAN(0, \"show-stats\", &show_stats, \"Show work cost statistics\"),\n+\t\tOPT_BIT(0, \"score-debug\", &output_option, \"Show output score for blame entries\", OUTPUT_SHOW_SCORE),\n+\t\tOPT_BIT('f', \"show-name\", &output_option, \"Show original filename (Default: auto)\", OUTPUT_SHOW_NAME),\n+\t\tOPT_BIT('n', \"show-number\", &output_option, \"Show original linenumber (Default: off)\", OUTPUT_SHOW_NUMBER),\n+\t\tOPT_BIT('p', \"porcelain\", &output_option, \"Show in a format designed for machine consumption\", OUTPUT_PORCELAIN),\n+\t\tOPT_BIT('c', NULL, &output_option, \"Use the same output mode as git-annotate (Default: off)\", OUTPUT_ANNOTATE_COMPAT),\n+\t\tOPT_BIT('t', NULL, &output_option, \"Show raw timestamp (Default: off)\", OUTPUT_RAW_TIMESTAMP),\n+\t\tOPT_BIT('l', NULL, &output_option, \"Show long commit SHA1 (Default: off)\", OUTPUT_LONG_OBJECT_NAME),\n+\t\tOPT_BIT('s', NULL, &output_option, \"Suppress author name and timestamp (Default: off)\", OUTPUT_NO_AUTHOR),\n+\t\tOPT_BIT('w', NULL, &xdl_opts, \"Ignore whitespace differences\", XDF_IGNORE_WHITESPACE),\n+\t\tOPT_STRING('S', NULL, &revs_file, \"file\", \"Use revisions from instead of calling git-rev-list\"),\n+\t\tOPT_STRING(0, \"contents\", &contents_from, \"file\", \"Use 's contents as the final image\"),\n+\t\t{ OPTION_CALLBACK, 'C', NULL, &opt, \"score\", \"Find line copies within and across files\", PARSE_OPT_OPTARG, blame_copy_callback },\n+\t\t{ OPTION_CALLBACK, 'M', NULL, &opt, \"score\", \"Find line movements within and across files\", PARSE_OPT_OPTARG, blame_move_callback },\n+\t\tOPT_CALLBACK('L', NULL, &bottomtop, \"n,m\", \"Process only line range n,m, counting from 1\", blame_bottomtop_callback),\n+\t\tOPT_END()\n+\t};\n \n \tcmd_is_annotate = !strcmp(argv[0], \"annotate\");\n \n+\targc = parse_options(argc, argv, options, blame_opt_usage,\n+\t\tPARSE_OPT_STOP_AT_NON_OPTION | PARSE_OPT_KEEP_DASHDASH);\n+\n+\t/*\n+\t * parse_options() offsets things by one - undo for now to make\n+\t * old code happy.\n+\t */\n+\targc++;\n+\targv--;\n+\n \tgit_config(git_blame_config, NULL);\n \tsave_commit_buffer = 0;\n \n-\topt = 0;\n \tseen_dashdash = 0;\n \tfor (unk = i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tif (*arg != '-')\n \t\t\tbreak;\n-\t\telse if (!strcmp(\"-b\", arg))\n-\t\t\tblank_boundary = 1;\n-\t\telse if (!strcmp(\"--root\", arg))\n-\t\t\tshow_root = 1;\n-\t\telse if (!strcmp(arg, \"--show-stats\"))\n-\t\t\tshow_stats = 1;\n-\t\telse if (!strcmp(\"-c\", arg))\n-\t\t\toutput_option |= OUTPUT_ANNOTATE_COMPAT;\n-\t\telse if (!strcmp(\"-t\", arg))\n-\t\t\toutput_option |= OUTPUT_RAW_TIMESTAMP;\n-\t\telse if (!strcmp(\"-l\", arg))\n-\t\t\toutput_option |= OUTPUT_LONG_OBJECT_NAME;\n-\t\telse if (!strcmp(\"-s\", arg))\n-\t\t\toutput_option |= OUTPUT_NO_AUTHOR;\n-\t\telse if (!strcmp(\"-w\", arg))\n-\t\t\txdl_opts |= XDF_IGNORE_WHITESPACE;\n-\t\telse if (!strcmp(\"-S\", arg) && ++i < argc)\n-\t\t\trevs_file = argv[i];\n-\t\telse if (!prefixcmp(arg, \"-M\")) {\n-\t\t\topt |= PICKAXE_BLAME_MOVE;\n-\t\t\tblame_move_score = parse_score(arg+2);\n-\t\t}\n-\t\telse if (!prefixcmp(arg, \"-C\")) {\n-\t\t\t/*\n-\t\t\t * -C enables copy from removed files;\n-\t\t\t * -C -C enables copy from existing files, but only\n-\t\t\t * when blaming a new file;\n-\t\t\t * -C -C -C enables copy from existing files for\n-\t\t\t * everybody\n-\t\t\t */\n-\t\t\tif (opt & PICKAXE_BLAME_COPY_HARDER)\n-\t\t\t\topt |= PICKAXE_BLAME_COPY_HARDEST;\n-\t\t\tif (opt & PICKAXE_BLAME_COPY)\n-\t\t\t\topt |= PICKAXE_BLAME_COPY_HARDER;\n-\t\t\topt |= PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE;\n-\t\t\tblame_copy_score = parse_score(arg+2);\n-\t\t}\n-\t\telse if (!prefixcmp(arg, \"-L\")) {\n-\t\t\tif (!arg[2]) {\n-\t\t\t\tif (++i >= argc)\n-\t\t\t\t\tusage(blame_usage);\n-\t\t\t\targ = argv[i];\n-\t\t\t}\n-\t\t\telse\n-\t\t\t\targ += 2;\n-\t\t\tif (bottomtop)\n-\t\t\t\tdie(\"More than one '-L n,m' option given\");\n-\t\t\tbottomtop = arg;\n-\t\t}\n-\t\telse if (!strcmp(\"--contents\", arg)) {\n-\t\t\tif (++i >= argc)\n-\t\t\t\tusage(blame_usage);\n-\t\t\tcontents_from = argv[i];\n-\t\t}\n-\t\telse if (!strcmp(\"--incremental\", arg))\n-\t\t\tincremental = 1;\n-\t\telse if (!strcmp(\"--score-debug\", arg))\n-\t\t\toutput_option |= OUTPUT_SHOW_SCORE;\n-\t\telse if (!strcmp(\"-f\", arg) ||\n-\t\t\t !strcmp(\"--show-name\", arg))\n-\t\t\toutput_option |= OUTPUT_SHOW_NAME;\n-\t\telse if (!strcmp(\"-n\", arg) ||\n-\t\t\t !strcmp(\"--show-number\", arg))\n-\t\t\toutput_option |= OUTPUT_SHOW_NUMBER;\n-\t\telse if (!strcmp(\"-p\", arg) ||\n-\t\t\t !strcmp(\"--porcelain\", arg))\n-\t\t\toutput_option |= OUTPUT_PORCELAIN;\n \t\telse if (!strcmp(\"--\", arg)) {\n \t\t\tseen_dashdash = 1;\n \t\t\ti++;\n"},{"id":"80677","messageId":"7vr6aoirqh.fsf@gitster.siamese.dyndns.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806222207220.2926@woody.linux-foundation.org","subject":"Re: Convert 'git blame' to parse_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-23T06:35:02Z","receivedAt":"2008-06-23T06:35:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> That builtin-blame option parsing is really ugly,...\n\nYeah, but wasn't it because it needed to be compatible with both annotate\nsyntax and rev-list style \"range\" notation at the same time?\n\n> +static int blame_bottomtop_callback(const struct option *option, const char *arg, int unset)\n> +{\n> +\tconst char **bottomtop = option->value;\n> +\tif (!arg)\n> +\t\treturn -1;\n> +\tif (*bottomtop)\n> +\t\tdie(\"More than one '-L n,m' option given\");\n> +\t*bottomtop = arg;\n> +\treturn 0;\n> +}\n\nHmmmm. I actually wanted to eventually allow more than one -L so that we\ncan blame two functions inside a file, for example. Would this make it\neven harder, I have to wonder...\n"},{"id":"80685","messageId":"20080623082223.GA12130@artemis.madism.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806222207220.2926@woody.linux-foundation.org","subject":"[RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T08:22:23Z","receivedAt":"2008-06-23T08:22:23Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 05:15:41AM +0000, Linus Torvalds wrote:\n> I think I'll want to add a PARSE_OPT_IGNORE_UNRECOGNIZED flag, and also \n> make it not write the resulting array into argv[0..] but back into \n> argv[1..] (so that you can use parse_options() as a _filter_ for options \n> parsing and make it easier to do partial conversions), but in the meantime \n> this mostly works.\n\nYou can't, mainly because of option aggregation: if the parser1 knows\nabout -a and -b, parser2 about -c, then, this kind of things is\nproblematic: -acb because you need to go to the parser '2' to know about\n-c, and you can't filter the arguments and keep -c and give -b to\nparser1 again, *BECAUSE* 'b' could also be -c argument.\n\nThis \"PARSE_OPT_IGNORE_UNRECOGNIZED\" thing has been discussed many times\nin the past, but it just doesn't fly.\n\nThough to help migrations we can probably introduce a new parse option\nconstruct that would be a callback that is responsible for dealing with\n\"things\" the upper level parser doesn't know about, something where the\ncallback could be:\n\nenum {\n FLAG_ERROR = -1,\n FLAG_NOT_FOR_ME,\n FLAG_IS_FOR_ME,\n FLAG_AND_VALUE_ARE_FOR_ME,\n}\n\nint (*parse_opt_unknown_cb)(int shortopt, const char *longopt,\n const char *value, void *priv);\n\nWhere the callback is supposed to work that way:\nwe pass to it either a shortopt ('\\0' else) or a longopt (NULL else) but\nnever both, and what the parser could find as a possible value in value\n(NULL if no value found). Then the parser does what it has to, and\nreturns one of the previous enum values. ERROR would be a fatal error\n(-1 chosen so that one can return error(...)), NOT_FOR_ME if it didn't\nwant the flag after all, IS_FOR_ME if it took only the flag, without the\nvalue, the last one being if it consumed both the option flag _and_ the\nvalue.\n\nOf course for things like --long-opt=value if the callback doesn't eat\nthe value, then parse_options would have to barf loudly.\n\n\nThe major drawback of this method is that parse_options won't generate\nthe nice usage help for the things that recurse in such a function. But\nat least it eases migration to parseoptions, because I believe rev_parse\nand diff_opt_parse to be _way_ easier to migrate to such a callback\n_and_ with the old API together (I really believe that the old big\nconsuming loops could use such a callback directly e.g.) so that\ncommands using diff and revision options can be migrated to parse-opt\n*slowly* rather than all at a time (which is a no-go and is such a big\namount of work that I avoided it in despair).\n\nIf people believe it's a sane approach I can do it. I absolutely don't\nknow why no one thought of that before, but I don't see any major\ndrawback (except for the \"help\" bits, but like I said, it's a \"code\nupgrade path\" and not meant to be the final state).\n\n\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80695","messageId":"alpine.DEB.1.00.0806231312130.6440@racer","threadId":"14096","inReplyTo":"20080623082223.GA12130@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-23T12:26:41Z","receivedAt":"2008-06-23T12:26:41Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 23 Jun 2008, Pierre Habouzit wrote:\n\n> This \"PARSE_OPT_IGNORE_UNRECOGNIZED\" thing has been discussed many times \n> in the past, but it just doesn't fly.\n> \n> Though to help migrations we can probably introduce a new parse option\n> construct that would be a callback that is responsible for dealing with\n> \"things\" the upper level parser doesn't know about, something where the\n> callback could be:\n> \n> enum {\n> FLAG_ERROR = -1,\n> FLAG_NOT_FOR_ME,\n> FLAG_IS_FOR_ME,\n> FLAG_AND_VALUE_ARE_FOR_ME,\n> }\n> \n> int (*parse_opt_unknown_cb)(int shortopt, const char *longopt,\n> const char *value, void *priv);\n\nI believe that this is what Junio was talking about when he mentioned \ncallbacks.\n\nHowever, I think it buys us more trouble than it saves us.\n\nThinking about the recursive approach again, I came up with this POC:\n\n-- snip --\n[PATCH] parse-options: make it possible to have option \"subtables\"\n\nSome programs share an awful lot of options, such as for the diff\nmachinery. Allow an option table to recurse into another option\ntable with the new OPTION_OPTIONS() directive.\n\nThe subtable is expected to store the values into a struct whose address\nis passed as 2nd parameter to OPTION_OPTIONS().\n\nSubtables can recurse into subtables, too.\n\nExample:\n\n\t#define null ((struct diff_options *)NULL)\n\tstruct option diff__options[] = {\n\t\tOPT_INTEGER('l', NULL, &null->rename_limit,\n\t\t\t\"set rename limit\"),\n\t\t[...]\n\t};\n\n\tstruct option diff_files__options[] = {\n\t\t[...]\n\t\tOPTION_OPTIONS(diff__options, &revopt.diff_options),\n\t\t[...]\n\t};\n\nSigned-off-by: Johannes Schindelin \n---\n parse-options.c | 42 ++++++++++++++++++++++++++++++++++++++++++\n parse-options.h | 5 +++++\n test-parse-options.c | 16 ++++++++++++++++\n 3 files changed, 63 insertions(+), 0 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex b8bde2b..c13c503 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 \"cache.h\"\n \n #define OPT_SHORT 1\n #define OPT_UNSET 2\n@@ -247,6 +248,41 @@ void check_typos(const char *arg, const struct option *options)\n \t}\n }\n \n+struct growable_options {\n+\tint nr, alloc;\n+\tstruct option *options;\n+};\n+\n+static void *add_base_var(void *value, void *base_var)\n+{\n+\treturn (void *)(((char *)base_var) +\n+\t\t\t(((char *)value) - ((char *)NULL)));\n+}\n+\n+static void expand_option_tables(const struct option *options, void *base_var,\n+\t\tstruct growable_options *result, int add_option_end)\n+{\n+\tfor (; options->type != OPTION_END; options++) {\n+\t\tif (options->type == OPTION_OPTIONS)\n+\t\t\texpand_option_tables(options->option_table,\n+\t\t\t\t\tadd_base_var(options->value, base_var),\n+\t\t\t\t\tresult, 0);\n+\t\telse {\n+\t\t\tALLOC_GROW(result->options,\n+\t\t\t\t\tresult->nr + 1, result->alloc);\n+\t\t\tresult->options[result->nr] = *options;\n+\t\t\tif (base_var)\n+\t\t\t\tresult->options[result->nr].value =\n+\t\t\t\t\tadd_base_var(options->value, base_var);\n+\t\t\tresult->nr++;\n+\t\t}\n+\t}\n+\tif (add_option_end) {\n+\t\tALLOC_GROW(result->options, result->nr + 1, result->alloc);\n+\t\tresult->options[result->nr++].type = OPTION_END;\n+\t}\n+}\n+\n static NORETURN void usage_with_options_internal(const char * const *,\n const struct option *, int);\n \n@@ -254,6 +290,10 @@ 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, argv, argc - 1, 0, NULL };\n+\tstruct growable_options growable_options = { 0, 0, NULL };\n+\n+\texpand_option_tables(options, NULL, &growable_options, 1);\n+\toptions = growable_options.options;\n \n \tfor (; args.argc; args.argc--, args.argv++) {\n \t\tconst char *arg = args.argv[0];\n@@ -298,6 +338,8 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \t\t\tusage_with_options(usagestr, options);\n \t}\n \n+\tfree(growable_options.options);\n+\n \tmemmove(args.out + args.cpidx, args.argv, args.argc * sizeof(*args.out));\n \targs.out[args.cpidx + args.argc] = NULL;\n \treturn args.cpidx + args.argc;\ndiff --git a/parse-options.h b/parse-options.h\nindex 4ee443d..be6e858 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -15,6 +15,8 @@ enum parse_opt_type {\n \tOPTION_STRING,\n \tOPTION_INTEGER,\n \tOPTION_CALLBACK,\n+\t/* recursing into another options table */\n+\tOPTION_OPTIONS,\n };\n \n enum parse_opt_flags {\n@@ -83,6 +85,7 @@ struct option {\n \tint flags;\n \tparse_opt_cb *callback;\n \tintptr_t defval;\n+\tstruct option *option_table;\n };\n \n #define OPT_END() { OPTION_END }\n@@ -99,6 +102,8 @@ struct option {\n \t parse_opt_approxidate_cb }\n #define OPT_CALLBACK(s, l, v, a, h, f) \\\n \t{ OPTION_CALLBACK, (s), (l), (v), (a), (h), 0, (f) }\n+#define OPT_OPTIONS(table, base_var) \\\n+\t{ OPTION_OPTIONS, 0, NULL, base_var, NULL, NULL, 0, NULL, 0, table }\n \n /* parse_options() will filter out the processed options and leave the\n * non-option argments in argv[].\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 2a79e72..5032e71 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -18,8 +18,21 @@ int length_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+struct some_struct {\n+\tint an_int;\n+\tchar *a_string;\n+};\n+#define some_struct_null ((struct some_struct *)NULL)\n+struct option an_option_table[] = {\n+\tOPT_GROUP(\"An option table\"),\n+\tOPT_INTEGER('Y', NULL, &some_struct_null->an_int, \"an integer\"),\n+\tOPT_STRING(0, \"a-string\", &some_struct_null->a_string,\n+\t\t\"a-string\", \"get another string\"),\n+};\n+\n int main(int argc, const char **argv)\n {\n+\tstruct some_struct some = { 0, NULL };\n \tconst char *usage[] = {\n \t\t\"test-parse-options \",\n \t\tNULL\n@@ -42,6 +55,7 @@ int main(int argc, const char **argv)\n \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n \t\tOPT_SET_PTR(0, \"default-string\", &string,\n \t\t\t\"set string to default\", (unsigned long)\"default\"),\n+\t\tOPT_OPTIONS(an_option_table, (void *)&some),\n \t\tOPT_GROUP(\"Magic arguments\"),\n \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n \t\tOPT_GROUP(\"Standard options\"),\n@@ -62,6 +76,8 @@ int main(int argc, const char **argv)\n \tprintf(\"verbose: %d\\n\", verbose);\n \tprintf(\"quiet: %s\\n\", quiet ? \"yes\" : \"no\");\n \tprintf(\"dry run: %s\\n\", dry_run ? \"yes\" : \"no\");\n+\tprintf(\"an int: %d\\n\", some.an_int);\n+\tprintf(\"a string: %s\\n\", some.a_string);\n \n \tfor (i = 0; i < argc; i++)\n \t\tprintf(\"arg %02d: %s\\n\", i, argv[i]);\n-- \n1.5.6.127.g3fb9f\n-- snap --\n\n\nMaybe this is food for thought.\n\nCiao,\nDscho\n"},{"id":"80696","messageId":"alpine.DEB.1.00.0806231326580.6440@racer","threadId":"14096","inReplyTo":"7vr6aoirqh.fsf@gitster.siamese.dyndns.org","subject":"Re: Convert 'git blame' to parse_options()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-23T12:28:54Z","receivedAt":"2008-06-23T12:28:54Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 22 Jun 2008, Junio C Hamano wrote:\n\n> Linus Torvalds writes:\n> \n> > +static int blame_bottomtop_callback(const struct option *option, const char *arg, int unset)\n> > +{\n> > +\tconst char **bottomtop = option->value;\n> > +\tif (!arg)\n> > +\t\treturn -1;\n> > +\tif (*bottomtop)\n> > +\t\tdie(\"More than one '-L n,m' option given\");\n> > +\t*bottomtop = arg;\n> > +\treturn 0;\n> > +}\n> \n> Hmmmm. I actually wanted to eventually allow more than one -L so that we\n> can blame two functions inside a file, for example. Would this make it\n> even harder, I have to wonder...\n\nIMHO this would not change anything in the way of making it harder; it is \njust a matter of adding the pairs to a container. The more tricky thing \nis how to handle a bunch of intervals efficiently, without introducing \ntoo much ugliness.\n\nCiao,\nDscho\n"},{"id":"80715","messageId":"20080623155315.GA18593@artemis.madism.org","threadId":"14096","inReplyTo":"alpine.DEB.1.00.0806231312130.6440@racer","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T15:53:15Z","receivedAt":"2008-06-23T15:53:15Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 12:26:41PM +0000, Johannes Schindelin wrote:\n> Hi,\n> \n> On Mon, 23 Jun 2008, Pierre Habouzit wrote:\n> \n> > This \"PARSE_OPT_IGNORE_UNRECOGNIZED\" thing has been discussed many times \n> > in the past, but it just doesn't fly.\n> > \n> > Though to help migrations we can probably introduce a new parse option\n> > construct that would be a callback that is responsible for dealing with\n> > \"things\" the upper level parser doesn't know about, something where the\n> > callback could be:\n> > \n> > enum {\n> > FLAG_ERROR = -1,\n> > FLAG_NOT_FOR_ME,\n> > FLAG_IS_FOR_ME,\n> > FLAG_AND_VALUE_ARE_FOR_ME,\n> > }\n> > \n> > int (*parse_opt_unknown_cb)(int shortopt, const char *longopt,\n> > const char *value, void *priv);\n> \n> I believe that this is what Junio was talking about when he mentioned \n> callbacks.\n> \n> However, I think it buys us more trouble than it saves us.\n> \n> Thinking about the recursive approach again, I came up with this POC:\n\n Well I proposed something like that in the past, and we believed it to\nbe too cumbersome. I can live with it well fwiw, but it doesn't solve\nthe issue of migrating a very complex option parsing chain to\nparse-options well IMHO. THe big problem with diff and rev opt parsing\nis that one you want to migrate _any_ of the commands, you have to\nmigrate _all_ of them, which is huge.\n\n I believe the callback proposal as an _intermediate_ step allows a\nbetter \"upgrade\" path. We can then converge to this proposal, or a big\nfat macro, I don't care what we do in the end, but as one guy that\nmigrated quite a few commands, I care about the work being doable in an\nincremental way.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80718","messageId":"alpine.LFD.1.10.0806230908500.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"20080623082223.GA12130@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T16:11:50Z","receivedAt":"2008-06-23T16:11:50Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 23 Jun 2008, Pierre Habouzit wrote:\n> \n> You can't, mainly because of option aggregation: if the parser1 knows\n> about -a and -b, parser2 about -c, then, this kind of things is\n> problematic: -acb because you need to go to the parser '2' to know about\n> -c, and you can't filter the arguments and keep -c and give -b to\n> parser1 again, *BECAUSE* 'b' could also be -c argument.\n\nSure you can. You just rewrite the arguments themselves.\n\nThat said, anybody who doesn't use parse_options() right now won't be \naccepting something like \"-abc\" *anyway*, and people currently need to use \n\"-a -b -c\".\n\n> int (*parse_opt_unknown_cb)(int shortopt, const char *longopt,\n> const char *value, void *priv);\n\nThis doesn't really change anything. It just makes it harder to write a \nsimple partial parser. Just passing in \"IGNORE_UNKNOWN\" (and probably \n\"STOP_AT_UNKNOWN\") is the simplest way to have multiple passes.\n\n\t\tLinus\n"},{"id":"80721","messageId":"alpine.LFD.1.10.0806230912230.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"alpine.DEB.1.00.0806231312130.6440@racer","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T16:25:10Z","receivedAt":"2008-06-23T16:25:10Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 23 Jun 2008, Johannes Schindelin wrote:\n> \n> Thinking about the recursive approach again, I came up with this POC:\n\n\"recursive\" is pointless.\n\nThe problem with the current \"parse_options()\" is not that it's recursive \n(although that has been claimed multiple times!.\n\nThe problem with parse_options() is that it's currently impossible to \nwrite something that handles _partial_ cases.\n\nLet me explain.\n\nLook at cmd_apply() in builtin-apply.c. Notice how it currently absolutely \nCANNOT sanely be turned into using \"parse_options()\", not because it needs \nany \"recursive\" handling, but simply because it wants to do *incremental* \nhandling.\n\nIt should be perfectly possible to change that argument loop from\n\n\tfor (i = 1; i < argc; i++) {\n\t\tconst char *arg = argv[i];\n\t\tif (strcmp(arg, \"-\")) {\n\t\t\t.. handle ..\n\t\t\tcontinue;\n\t\t}\n\t\t...\n\nto doing something like this:\n\n\tfor (;;) {\n\t\tconst char *arg;\n\t\targc = parse_options(argc, argv,\n\t\t\toptions, usage, PARSE_OPT_STOP_AT_UNKNOWN);\n\t\tif (!argc)\n\t\t\tbreak;\n\t\targ = argv[1];\n\t\targv++;\n\t\targc--;\n\t\tif (strcmp(arg, \"-\")) {\n\t\t\t.. handle ..\n\t\t\tcontinue;\n\t\t}\n\t\t...\t\n\nor whatever. See?\n\nCould you handle that with callbacks? Of course. \"You can solve any \nproblem in computer science with an added level of indirection\". But would \nit be simpler to convert existing users? Hell no. \n\nCould you handle the \"recursive\" use of parse_options() in builtin-blame.c \nby teaching it about recursion? Yes. But again, it's just _simpler_ to \njust teach parse_options() to parse the things it knows about, and leave \nthe other things in place.\n\n\t\t\tLinus\n"},{"id":"80722","messageId":"alpine.DEB.1.00.0806231709540.6440@racer","threadId":"14096","inReplyTo":"20080623155315.GA18593@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-23T16:25:52Z","receivedAt":"2008-06-23T16:25:52Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 23 Jun 2008, Pierre Habouzit wrote:\n\n> On Mon, Jun 23, 2008 at 12:26:41PM +0000, Johannes Schindelin wrote:\n> \n> > On Mon, 23 Jun 2008, Pierre Habouzit wrote:\n> > \n> > > This \"PARSE_OPT_IGNORE_UNRECOGNIZED\" thing has been discussed many \n> > > times in the past, but it just doesn't fly.\n> > > \n> > > Though to help migrations we can probably introduce a new parse \n> > > option construct that would be a callback that is responsible for \n> > > dealing with \"things\" the upper level parser doesn't know about, \n> > > something where the callback could be:\n> > > \n> > > enum {\n> > > FLAG_ERROR = -1,\n> > > FLAG_NOT_FOR_ME,\n> > > FLAG_IS_FOR_ME,\n> > > FLAG_AND_VALUE_ARE_FOR_ME,\n> > > }\n> > > \n> > > int (*parse_opt_unknown_cb)(int shortopt, const char *longopt,\n> > > const char *value, void *priv);\n> > \n> > I believe that this is what Junio was talking about when he mentioned \n> > callbacks.\n> > \n> > However, I think it buys us more trouble than it saves us.\n> > \n> > Thinking about the recursive approach again, I came up with this POC:\n> \n> Well I proposed something like that in the past, and we believed it to\n> be too cumbersome.\n\nIIRC your solution was a bit involved, while I think that mine is at least \nsmall enough to be simple.\n\nNote: it is a bit wasteful, allocating space for a new option table \nin every case, even if there are no OPTION_OPTIONS, but I think that is \nokay.\n\n> I can live with it well fwiw, but it doesn't solve the issue of \n> migrating a very complex option parsing chain to parse-options well \n> IMHO. THe big problem with diff and rev opt parsing is that one you want \n> to migrate _any_ of the commands, you have to migrate _all_ of them, \n> which is huge.\n\nI don't think you have to migrate all of them in one go. To the contrary, \nif we have both \"diff_opt_parse()\" as well as a \"struct options \n*diff__options\", it can be done one by one. During that time, though, \npeople would have to add new diff options to both places.\n \nCiao,\nDscho\n"},{"id":"80724","messageId":"20080623164917.GA25474@sigill.intra.peff.net","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806230912230.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-23T16:49:17Z","receivedAt":"2008-06-23T16:49:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2008 at 09:25:10AM -0700, Linus Torvalds wrote:\n\n> Could you handle the \"recursive\" use of parse_options() in builtin-blame.c \n> by teaching it about recursion? Yes. But again, it's just _simpler_ to \n> just teach parse_options() to parse the things it knows about, and leave \n> the other things in place.\n\nIf I know that I have option \"-a\", what is the correct partial parsing\nof:\n\n git foo -b -a\n\n?\n\n-Peff\n"},{"id":"80725","messageId":"alpine.DEB.1.00.0806231756340.6440@racer","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806230912230.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-23T17:04:48Z","receivedAt":"2008-06-23T17:04:48Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 23 Jun 2008, Linus Torvalds wrote:\n\n> On Mon, 23 Jun 2008, Johannes Schindelin wrote:\n> > \n> > Thinking about the recursive approach again, I came up with this POC:\n> \n> \"recursive\" is pointless.\n\nNope, it is not.\n\nTo keep things _simple_ a callback is not good. Sure, you can work around \nthe limitations of callbacks for aggregation, but the code change looks \nhorrible. And the same holds true for the help message.\n\nJust compare that to the recursive approach.\n\nOkay, now for the \"granted, your approach has merits\" part.\n\n> Look at cmd_apply() in builtin-apply.c. Notice how it currently \n> absolutely CANNOT sanely be turned into using \"parse_options()\", not \n> because it needs any \"recursive\" handling, but simply because it wants \n> to do *incremental* handling.\n\nThat is a totally independent issue from the one I discussed, namely sane \nhandling of the diff (and rev) options.\n\n> \tfor (;;) {\n> \t\tconst char *arg;\n> \t\targc = parse_options(argc, argv,\n> \t\t\toptions, usage, PARSE_OPT_STOP_AT_UNKNOWN);\n\nWe do have PARSE_OPT_STOP_AT_NON_OPTION since a0ec9d25(parseopt: add flag \nto stop on first non option), which tries to solve a _similar_ problem, \nand it should be not hard at all to get PARSE_OPT_STOP_AT_UNKNOWN without \nchanging the loop as you suggested.\n\nHeck, we could just as easily introduce PARSE_OPT_IGNORE_UNKNOWN.\n\n> Could you handle the \"recursive\" use of parse_options() in builtin-blame.c \n> by teaching it about recursion? Yes. But again, it's just _simpler_ to \n> just teach parse_options() to parse the things it knows about, and leave \n> the other things in place.\n\nAnd here I disagree. You might not need a nice \"--help\" output, but most \nmortals do. And this is not easy with your approach.\n\nIn contrast, by using my approach of having an option_table for a bundle \nof common options, which just set variables in a certain struct, you can \nhave a relatively painless migration, and you get all the benefits of \nparse-options.\n\nBut I guess the approach of whoever has more time to work on it will \nwin... ;-)\n\nCiao,\nDscho\n"},{"id":"80726","messageId":"alpine.LFD.1.10.0806230953550.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"20080623164917.GA25474@sigill.intra.peff.net","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T17:06:52Z","receivedAt":"2008-06-23T17:06:52Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 23 Jun 2008, Jeff King wrote:\n> On Mon, Jun 23, 2008 at 09:25:10AM -0700, Linus Torvalds wrote:\n> \n> > Could you handle the \"recursive\" use of parse_options() in builtin-blame.c \n> > by teaching it about recursion? Yes. But again, it's just _simpler_ to \n> > just teach parse_options() to parse the things it knows about, and leave \n> > the other things in place.\n> \n> If I know that I have option \"-a\", what is the correct partial parsing\n> of:\n> \n> git foo -b -a\n> \n> ?\n\nYou'd start off with argv[] looking like [ \"foo\" \"-b\" \"-a\" ] and then \nafter calling parse_options with that, depending on whether it has \nPARSE_OPT_CONTINUE_ON_UNKNOWN or not, you'd either end up with the \"-a\" \nhandled (and argv[] now being just [ \"foo\" \"-b\" ]), or if you have \nPARSE_OPT_STOP_ON_UNKNOWN then parse_options() would return without having \ndone anything, and expecting you to handle the unknown option first and \nthen restarting the argument parsing.\n\nThe problem with parse_options() right now is:\n\n - it cannot do this at all (it can stop or ignore *non*arguments, but \n not things that start with \"-\" and will always error out)\n\n - it actually puts the result somewhere different than the source, which \n makes it very annoying to work with together with anything else that \n also wants to look at, or change, argv/argc.\n\n IOW, right now it takes it's arguments from argv[1...], but then puts \n back the remainder into argv[0...]. Similarly, it takes a \"count plus \n one\" value of argc, but then _returns_ a \"count plus zero\".\n\nThat second thing is an annoyance, and could be handled either with a new \nflag (PARSE_OPT_PUT_BACK_IDENTICALLY), or by just changing the calling \nconvention. The latter is the \"right thing\", but it needs trivial changes \nin all existing callers.\n\nI actually suspect that the best fix for the second issue is to yes, \nchange the calling convention so that it puts things back where it found \nthem, and returns a \"count plus one\" count, *but* have a special case \nwhere \"if all arguments are used, return zero\".\n\n[ IOW, it would never ever return \"1\". If there is one argument left in \n argv[1], it returns 2 because it counts \"argv[0]\" even if it doesn't \n _use_ it. That's exactly the same way argv/argc works normally. But \n then, if it actually used up everything, it wouldn't return 1, but \n return 0 to make it\n\n (a) easier and more obvious to test for \"done\" in general\n\n (b) easier to convert existing users that basically expect a zero \n return value to mean \"I ate all the arguments\". Many existing users \n of \"parse_options()\" don't care about anything else, and wouldn't \n need any changes at all.\n\n but I haven't actually looked too closely yet to be able to say whether \n this would avoid the bulk of the problems with changing the existing \n practice of 'parse_option()' callers. ]\n\nHmm?\n\n\t\tLinus\n"},{"id":"80727","messageId":"20080623171505.GB27265@sigill.intra.peff.net","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806230953550.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-23T17:15:05Z","receivedAt":"2008-06-23T17:15:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2008 at 10:06:52AM -0700, Linus Torvalds wrote:\n\n> You'd start off with argv[] looking like [ \"foo\" \"-b\" \"-a\" ] and then \n> after calling parse_options with that, depending on whether it has \n> PARSE_OPT_CONTINUE_ON_UNKNOWN or not, you'd either end up with the \"-a\" \n> handled (and argv[] now being just [ \"foo\" \"-b\" ]), or if you have \n\nHow can that be correct, if you don't know whether \"-b\" takes an\nargument?\n\n> PARSE_OPT_STOP_ON_UNKNOWN then parse_options() would return without having \n> done anything, and expecting you to handle the unknown option first and \n> then restarting the argument parsing.\n\nThat is the only thing that makes sense to me, since the command line\nhas to be parsed left-to-right (because the syntactic function of an\nelement relies on the semantics of the element to its left).\n\n-Peff\n"},{"id":"80729","messageId":"alpine.LFD.1.10.0806231019170.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"alpine.DEB.1.00.0806231756340.6440@racer","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T17:21:47Z","receivedAt":"2008-06-23T17:21:47Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 23 Jun 2008, Johannes Schindelin wrote:\n> \n> > Look at cmd_apply() in builtin-apply.c. Notice how it currently \n> > absolutely CANNOT sanely be turned into using \"parse_options()\", not \n> > because it needs any \"recursive\" handling, but simply because it wants \n> > to do *incremental* handling.\n> \n> That is a totally independent issue from the one I discussed, namely sane \n> handling of the diff (and rev) options.\n\nNo it is not.\n\nYOU THINK it is \"independent\" just because YOUR SOLUTION can do only one \ncase.\n\nAnd I say: my solution handles both cases, so it's not \"independent\" any \nmore.\n\nSee? \n\n(And yes, I can handle even the --help cases. It's actually not that hard \nto just remember all the different option structs you've seen, and handle \nall of that totally independently internally in parse_options(). You don't \nneed recursion, you just need a trivial list of options.)\n\n\t\tLinus\n"},{"id":"80730","messageId":"20080623172612.GD27265@sigill.intra.peff.net","threadId":"14096","inReplyTo":"alpine.DEB.1.00.0806231756340.6440@racer","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-23T17:26:12Z","receivedAt":"2008-06-23T17:26:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2008 at 06:04:48PM +0100, Johannes Schindelin wrote:\n\n> > > Thinking about the recursive approach again, I came up with this POC:\n> > \n> > \"recursive\" is pointless.\n> \n> Nope, it is not.\n\nAIUI, one difference between your approach and Pierre's is that he is\nsuggesting (and I have suggested this in the past, too) a big\nDIFF_OPTIONS macro that you stick in the options table for each command.\nWhereas you are allowing for subtables accessible via pointers.\n\nYour approach should yield a much leaner text size (which was what\nstarted this whole thing) since we don't end up with the same repeated\nsubsets of options tables, and in particular the one-liner help text\n(yes, compilers can sometimes point share the string literal components,\nbut most of our options tables are declared as static, so unless the\nlinker is very smart, I think we will end up with duplicates).\n\n> Heck, we could just as easily introduce PARSE_OPT_IGNORE_UNKNOWN.\n\nWe could even send it to the list with message-id\n\n http://mid.gmane.org/1213758236-979-2-git-send-email-shawn.bohrer@gmail.com\n\nand then Junio and I could complain that the concept is broken.\n\n-Peff\n"},{"id":"80732","messageId":"alpine.LFD.1.10.0806231027210.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"20080623171505.GB27265@sigill.intra.peff.net","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T17:32:21Z","receivedAt":"2008-06-23T17:32:21Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 23 Jun 2008, Jeff King wrote:\n> \n> How can that be correct, if you don't know whether \"-b\" takes an\n> argument?\n\nDid you read my post or not?\n\nIf you have that case, then use STOP_ON_UNKNOWN.\n\n> That is the only thing that makes sense to me, since the command line\n> has to be parsed left-to-right (because the syntactic function of an\n> element relies on the semantics of the element to its left).\n\nUmm. Helloo, reality.. There are actually very few options that take a \nflag for their arguments. In particular, the option parsing we really \n_care_ about (revision parsing - see builtin-blame.c which is exactly \nwhere I wanted to convert things) very much DOES NOT.\n\nAnd that CONTINUE_ON_UNKNOWN behaviour is not some kind of theoretical \nflag. It's the flag that builtin-blame.c *wants*. It's the flag that \ndescribes hat builtin-blame.c does right now in the current git master \nbranch.\n\nTry just looking at the code!\n\nSo I'm really not interested in arguing about \"theoretical issues\", when \nwe have a real-life *practical* issue to solve.\n\nSolve builtin-blame.c for me. I sent out a patch yesterday, but in the \ndescription of that patch I also described exactly why I want \nCONTINUE_ON_UNKNOWN.\n\nSo can we please stop the clusterfuck now?\n\n\t\tLinus\n"},{"id":"80735","messageId":"20080623181517.GA28527@sigill.intra.peff.net","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806231027210.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-23T18:15:17Z","receivedAt":"2008-06-23T18:15:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2008 at 10:32:21AM -0700, Linus Torvalds wrote:\n\n> > How can that be correct, if you don't know whether \"-b\" takes an\n> > argument?\n> \n> Did you read my post or not?\n> \n> If you have that case, then use STOP_ON_UNKNOWN.\n\nHow do you know that is the case, unless you know what the other option\nparsers are going to do? Or are you suggesting that I check every other\ndownstream option parser to make sure that it's OK in this particular\ninstance, use IGNORE_UNKNOWN, and then laugh maniacally when somebody\nadds such an option to the diff option parser later?\n\n> Umm. Helloo, reality.. There are actually very few options that take a \n> flag for their arguments. In particular, the option parsing we really \n> _care_ about (revision parsing - see builtin-blame.c which is exactly \n> where I wanted to convert things) very much DOES NOT.\n\nReality: revision.c, lines 1008-1012. \"-n\" takes an argument.\nReality: revision.c, lines 1075-1080. \"--default\" takes an argument.\n\n> Try just looking at the code!\n\nI did. Or maybe you missed the thread where this exact feature was\nmentioned, and I already looked at the code and mentioned those two\nspots. It's right here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/85354/focus=85355\n\n> So I'm really not interested in arguing about \"theoretical issues\", when \n> we have a real-life *practical* issue to solve.\n> \n> Solve builtin-blame.c for me. I sent out a patch yesterday, but in the \n> description of that patch I also described exactly why I want \n> CONTINUE_ON_UNKNOWN.\n\nThere is already a discussion underway about the proper solution. This\nisn't just a git-blame issue, but rather an issue with all commands that\nhave their own options and take revision parameters. So I think rather\nthan doing a halfway fix that happens to work with git-blame, it is more\nuseful to focus on a solution that works everywhere and fix _all_ of the\nproblems.\n\n-Peff\n"},{"id":"80737","messageId":"alpine.LFD.1.10.0806231114180.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806231027210.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T18:20:46Z","receivedAt":"2008-06-23T18:20:46Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 23 Jun 2008, Linus Torvalds wrote:\n> \n> Umm. Helloo, reality.. There are actually very few options that take a \n> flag for their arguments. In particular, the option parsing we really \n> _care_ about (revision parsing - see builtin-blame.c which is exactly \n> where I wanted to convert things) very much DOES NOT.\n\nActually, I guess \"--default\" does, but if you try to mix that up with (a) \na default head that starts with a dash and (b) git-blame, you're doing \nsomething pretty odd.\n\nAnd yes, \"-n\" does too, but if you pass it negative numbers you get what \nyou deserve.\n\nThe point being, we really _do_ have a real-life existing case for \nPARSE_OPT_CONTINUE_ON_UNKNOWN, which is hard to handle any other way. \nCurrently you can literally do\n\n\tgit blame --since=April -b Makefile\n\nand while it's a totally made-up example, it's one I've picked to show \nexactly what does *not* work with my patch that started this whole thread.\n\nAnd guess what you need to fix it?\n\nIf you guessed PARSE_OPT_CONTINUE_ON_UNKNOWN, you win a prize. \n\n\t\tLinus\n"},{"id":"80738","messageId":"20080623183358.GA28941@sigill.intra.peff.net","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806231114180.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-23T18:33:58Z","receivedAt":"2008-06-23T18:33:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2008 at 11:20:46AM -0700, Linus Torvalds wrote:\n\n> Actually, I guess \"--default\" does, but if you try to mix that up with (a) \n> a default head that starts with a dash and (b) git-blame, you're doing \n> something pretty odd.\n\nYes, and I think that is why \"in practice\" this works with git-blame.\n\n> And yes, \"-n\" does too, but if you pass it negative numbers you get what \n> you deserve.\n\nIt's worse than that. We assume by default that the option has no\nargument, so the argument becomes a non-option parameter to the original\ncommand. Try (with current git-blame, but I think your patch doesn't\nchange this):\n\n $ git blame -n 1 git.c\n fatal: bad revision '1'\n\n $ git blame --default HEAD git.c\n fatal: cannot stat path HEAD: No such file or directory\n\nOops. Now again, as it happens, git-blame seems to ignore \"-n\" entirely\n(though I would have expected it to limit the depth of the blame\ntraversal), so maybe people shouldn't be using it.\n\n> The point being, we really _do_ have a real-life existing case for \n> PARSE_OPT_CONTINUE_ON_UNKNOWN, which is hard to handle any other way. \n> Currently you can literally do\n> \n> \tgit blame --since=April -b Makefile\n> \n> and while it's a totally made-up example, it's one I've picked to show \n> exactly what does *not* work with my patch that started this whole thread.\n> \n> And guess what you need to fix it?\n> \n> If you guessed PARSE_OPT_CONTINUE_ON_UNKNOWN, you win a prize. \n\nI guessed \"to convert the revision and diff options to a format that\nparse_options understands, so we can add them as appropriate to the\noptions tables of the various commands.\" Do I win anything?\n\n-Peff\n"},{"id":"80739","messageId":"alpine.LFD.1.10.0806231133190.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"20080623181517.GA28527@sigill.intra.peff.net","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T18:36:50Z","receivedAt":"2008-06-23T18:36:50Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 23 Jun 2008, Jeff King wrote:\n> \n> There is already a discussion underway about the proper solution.\n\nNo there isn't, not any I have seen.\n\nThe recursion discussion has been going on for ages. Nothing happened, \nbecause people claimed it was too complex. And it doesn't solve all the \n_other_ issues where the current behaviour is just painful.\n\nThat said, I don't care how this gets fixed. If you want to fix it some \nother way, fine. Just _do_ it, dammit. And make the interface easier to \nuse for incremental changes of existing code.\n\nBecause no, I don't care about builtin-blame.c in particular either. But I \n*do* care about the fact that currently it's sometimes a total *bitch* to \nconvert the trivial ad-hoc argument parsing to use parse_options(). It \ntakes effort and thinking.\n\nAnd I seriously doubt that recursive or anything else will help that. But \nI _guarantee_ that an incremental mode will make it much much easier to do \npartial conversions of all the easy cases. And _that_ is what I care \nabout.\n\n\t\tLinus\n"},{"id":"80741","messageId":"alpine.DEB.1.00.0806231938020.6440@racer","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806231019170.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-23T18:39:25Z","receivedAt":"2008-06-23T18:39:25Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 23 Jun 2008, Linus Torvalds wrote:\n\n> (And yes, I can handle even the --help cases. It's actually not that \n> hard to just remember all the different option structs you've seen, and \n> handle all of that totally independently internally in parse_options(). \n> You don't need recursion, you just need a trivial list of options.)\n\nSo what you actually do is just linearizing the recursion. Well then, \nyes, your solution is good.\n\nHowever, I thought that recursion (as I implemented it) would make the \nrevlist option parsing (which includes diff parsing) quite nice.\n\nWhatever,\nDscho\n"},{"id":"80742","messageId":"alpine.DEB.1.00.0806231940100.6440@racer","threadId":"14096","inReplyTo":"20080623172612.GD27265@sigill.intra.peff.net","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-23T18:41:10Z","receivedAt":"2008-06-23T18:41:10Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 23 Jun 2008, Jeff King wrote:\n\n> On Mon, Jun 23, 2008 at 06:04:48PM +0100, Johannes Schindelin wrote:\n> \n> > > > Thinking about the recursive approach again, I came up with this \n> > > > POC:\n> > > \n> > > \"recursive\" is pointless.\n> > \n> > Nope, it is not.\n> \n> AIUI, one difference between your approach and Pierre's is that he is \n> suggesting (and I have suggested this in the past, too) a big \n> DIFF_OPTIONS macro that you stick in the options table for each command. \n> Whereas you are allowing for subtables accessible via pointers.\n> \n> Your approach should yield a much leaner text size\n\nYes, this is the only difference between Pierre's current approach and my \nPOC.\n\n> > Heck, we could just as easily introduce PARSE_OPT_IGNORE_UNKNOWN.\n> \n> We could even send it to the list with message-id\n> \n> http://mid.gmane.org/1213758236-979-2-git-send-email-shawn.bohrer@gmail.com\n> \n> and then Junio and I could complain that the concept is broken.\n\nHeh. I thought I saw it, but a quick search did not reveal it, and I \nreally don't have time to work on this, unfortunately.\n\nThanks,\nDscho\n"},{"id":"80743","messageId":"alpine.LFD.1.10.0806231137070.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"20080623183358.GA28941@sigill.intra.peff.net","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T18:47:49Z","receivedAt":"2008-06-23T18:47:49Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 23 Jun 2008, Jeff King wrote:\n> \n> It's worse than that. We assume by default that the option has no\n> argument, so the argument becomes a non-option parameter to the original\n> command. Try (with current git-blame, but I think your patch doesn't\n> change this):\n> \n> $ git blame -n 1 git.c\n> fatal: bad revision '1'\n> \n> $ git blame --default HEAD git.c\n> fatal: cannot stat path HEAD: No such file or directory\n> \n> Oops.\n\nOops. And then, how would you fix this most easily?\n\nBe honest now.\n\nHint: it _still_ involves PARSE_OPT_CONTINUE_ON_UNKNOWN. It would fix both \ncases.\n\nIOW, the whole builtin-blame.c option parsing *should* look like this:\n\n\targc = parse_options(argc, argv, options, usage, PARSE_OPT_CONTINUE_ON_UNKNOWN);\n\tinit_revisions(&revs, NULL);\n\tsetup_revisions(argc, argv, &revs, NULL);\n\nand it should just work.\n\nBut we should *also* have some way to do things like the code in \nbuiltin-ls-files.c which have a few options that don't work well with the \ncurrent parse_options(). Yes, you can make all of them work with \ncallbacks, but that often ends up requiring moving arguments around. \nThere's no way to make a trivial conversion for 90% of the cases, and then \nleaving the 10% that need other changes.\n\nExample: many arguments cause multiple option variables to change. \nparse_options() simply can't handle that well - you can do it with a \ncallback, but then you need to make the option variables global or make \nthem a structure or something. All of which just makes it nasty to do \npartial conversions for the simple cases.\n\nAnd I guarantee that just adding PARSE_OPT_{CONTINUE|STOP}_ON_UNKNOWN is \ngoing to be the smallest patch, and make for the easiest usage case. It \nmay not be \"pretty\", but I can whip up a patch in five minutes.\n\nOr are we going to sit around discussing this for another five months?\n\n\t\t\tLinus\n"},{"id":"80745","messageId":"alpine.LFD.1.10.0806231158340.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806231137070.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T19:16:17Z","receivedAt":"2008-06-23T19:16:17Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 23 Jun 2008, Linus Torvalds wrote:\n> \n> Or are we going to sit around discussing this for another five months?\n\nSo the gauntlet is thrown.\n\nThis is the trivial version that (on top of my original patch in this \nthread) makes\n\n\tgit blame --since=April -b Makefile\n\nwork.\n\nDoes it handle all cases? No. In particular, because it still makes \nbuiltin-blame.c use PARSE_OPT_STOP_AT_NON_OPTION, and because cmd_blame.c \nsimply doesn't do the sane thing (it took me more than five minutes just \nbecause I tried to make it do the same thing and not do any argument \nparsing at all, but I just gave up), you still cannot pass it things like\n\n\t--default HEAD\n\nbecause it will stop at HEAD, and then do the exact wrong thing.\n\nCould it be improved? I'm sure. And I didn't test it much at all. I also \ndidn't change any existing users that could possibly use the new \nPARSE_OPT_STOP_AT_UNKNOWN thing. So this patch is an example patch only, \nto show what I'm talking about when I say \"simple\".\n\n\t\tLinus\n\n---\n builtin-blame.c | 2 +-\n parse-options.c | 26 ++++++++++++++++++++++----\n parse-options.h | 2 ++\n 3 files changed, 25 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex f04bd93..059062c 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -2195,7 +2195,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tcmd_is_annotate = !strcmp(argv[0], \"annotate\");\n \n \targc = parse_options(argc, argv, options, blame_opt_usage,\n-\t\tPARSE_OPT_STOP_AT_NON_OPTION | PARSE_OPT_KEEP_DASHDASH);\n+\t\tPARSE_OPT_STOP_AT_NON_OPTION | PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN);\n \n \t/*\n \t * parse_options() offsets things by one - undo for now to make\ndiff --git a/parse-options.c b/parse-options.c\nindex acf3fe3..5b4653f 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -7,7 +7,7 @@\n struct optparse_t {\n \tconst char **argv;\n \tconst char **out;\n-\tint argc, cpidx;\n+\tint argc, cpidx, flags;\n \tconst char *opt;\n };\n \n@@ -139,6 +139,8 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)\n \t\t\treturn get_value(p, options, OPT_SHORT);\n \t\t}\n \t}\n+\tif (p->flags & (PARSE_OPT_STOP_AT_UNKNOWN | PARSE_OPT_KEEP_UNKNOWN))\n+\t\treturn -1;\n \treturn error(\"unknown switch `%c'\", *p->opt);\n }\n \n@@ -224,6 +226,8 @@ is_abbreviated:\n \t\t\tabbrev_option->long_name);\n \tif (abbrev_option)\n \t\treturn get_value(p, abbrev_option, abbrev_flags);\n+\tif (p->flags & (PARSE_OPT_STOP_AT_UNKNOWN | PARSE_OPT_KEEP_UNKNOWN))\n+\t\treturn -1;\n \treturn error(\"unknown option `%s'\", arg);\n }\n \n@@ -253,7 +257,7 @@ static NORETURN void usage_with_options_internal(const char * const *,\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, argv, argc - 1, 0, NULL };\n+\tstruct optparse_t args = { argv + 1, argv, argc - 1, 0, flags, NULL };\n \n \tfor (; args.argc; args.argc--, args.argv++) {\n \t\tconst char *arg = args.argv[0];\n@@ -269,8 +273,15 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \t\t\targs.opt = arg + 1;\n \t\t\tif (*args.opt == 'h')\n \t\t\t\tusage_with_options(usagestr, options);\n-\t\t\tif (parse_short_opt(&args, options) < 0)\n+\t\t\tif (parse_short_opt(&args, options) < 0) {\n+\t\t\t\tif (flags & PARSE_OPT_STOP_AT_UNKNOWN) \n+\t\t\t\t\tbreak;\n+\t\t\t\tif (flags & PARSE_OPT_KEEP_UNKNOWN) {\n+\t\t\t\t\targs.out[args.cpidx++] = args.argv[0];\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n \t\t\t\tusage_with_options(usagestr, options);\n+\t\t\t}\n \t\t\tif (args.opt)\n \t\t\t\tcheck_typos(arg + 1, options);\n \t\t\twhile (args.opt) {\n@@ -294,8 +305,15 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \t\t\tusage_with_options_internal(usagestr, options, 1);\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)) {\n+\t\t\tif (flags & PARSE_OPT_STOP_AT_UNKNOWN) \n+\t\t\t\tbreak;\n+\t\t\tif (flags & PARSE_OPT_KEEP_UNKNOWN) {\n+\t\t\t\targs.out[args.cpidx++] = args.argv[0];\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tusage_with_options(usagestr, options);\n+\t\t}\n \t}\n \n \tmemmove(args.out + args.cpidx, args.argv, args.argc * sizeof(*args.out));\ndiff --git a/parse-options.h b/parse-options.h\nindex 4ee443d..a98f0ec 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -20,6 +20,8 @@ enum parse_opt_type {\n enum parse_opt_flags {\n \tPARSE_OPT_KEEP_DASHDASH = 1,\n \tPARSE_OPT_STOP_AT_NON_OPTION = 2,\n+\tPARSE_OPT_KEEP_UNKNOWN = 4,\n+\tPARSE_OPT_STOP_AT_UNKNOWN = 8,\n };\n \n enum parse_opt_option_flags {\n"},{"id":"80746","messageId":"20080623192419.GA13395@artemis.madism.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806230912230.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T19:24:20Z","receivedAt":"2008-06-23T19:24:20Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 04:25:10PM +0000, Linus Torvalds wrote:\n> \n> \n> On Mon, 23 Jun 2008, Johannes Schindelin wrote:\n> > \n> > Thinking about the recursive approach again, I came up with this POC:\n> \n> \"recursive\" is pointless.\n> \n> The problem with the current \"parse_options()\" is not that it's recursive \n> (although that has been claimed multiple times!.\n> \n> The problem with parse_options() is that it's currently impossible to \n> write something that handles _partial_ cases.\n> \n> Let me explain.\n> \n> Look at cmd_apply() in builtin-apply.c. Notice how it currently absolutely \n> CANNOT sanely be turned into using \"parse_options()\", not because it needs \n> any \"recursive\" handling, but simply because it wants to do *incremental* \n> handling.\n> \n> It should be perfectly possible to change that argument loop from\n> \n> \tfor (i = 1; i < argc; i++) {\n> \t\tconst char *arg = argv[i];\n> \t\tif (strcmp(arg, \"-\")) {\n> \t\t\t.. handle ..\n> \t\t\tcontinue;\n> \t\t}\n> \t\t...\n> \n> to doing something like this:\n> \n> \tfor (;;) {\n> \t\tconst char *arg;\n> \t\targc = parse_options(argc, argv,\n> \t\t\toptions, usage, PARSE_OPT_STOP_AT_UNKNOWN);\n> \t\tif (!argc)\n> \t\t\tbreak;\n> \t\targ = argv[1];\n> \t\targv++;\n> \t\targc--;\n> \t\tif (strcmp(arg, \"-\")) {\n> \t\t\t.. handle ..\n> \t\t\tcontinue;\n> \t\t}\n> \t\t...\t\n> \n> or whatever. See?\n\n Indeed, I read the thread many times, and I like the incremental\napproach very much, it really makes sense, and we can easily migrate\ncode this way indeed. In fact that should have been the way\nparse_options was designed from the beginning.\n\n There is a bit of work to do from this \"handwaved\" solution, because\npeople care about the filtered argv, so one should rememeber some kind\nof \"writing\" position. SOmething like:\n\n\n parse_opt_ctx_t ctx;\n\n /* will basically copy argc/argv */\n parse_options_start(&ctx, argc, argv);\n\n for (;;) {\n const char *arg;\n int res = parse_options_step(&ctx, options, usage, 0));\n\n if (res == PARSE_OPT_HELP) {\n /* generate help and exit */\n }\n if (res == PARSE_OPT_DONE)\n break;\n\n arg = ctx->argv[ctx->pos++];\n if (strcmp(arg, \"-\")) {\n ... handle ....\n continue;\n }\n }\n\n argc = parse_options_end(&ctx);\n\n /* at this point (argc,argv) is almost what parse_options would have\n left us */\n\n\n THis way, parse_options can be written:\n\n parse_opt_ctx_t ctx;\n parse_options_start(&ctx, argc, argv);\n switch (parse_options_step(&ctx, options, usage, 0)) {\n\t case PARSE_OPT_HELP: exit....\n\t case PARSE_OPT_DONE: break;\n\t default: exit(error(\"unknown option ....\");\n }\n return parse_options_end(&ctx);\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80748","messageId":"20080623195314.GA29569@sigill.intra.peff.net","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806231137070.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-23T19:53:14Z","receivedAt":"2008-06-23T19:53:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2008 at 11:47:49AM -0700, Linus Torvalds wrote:\n\n> > $ git blame --default HEAD git.c\n> > fatal: cannot stat path HEAD: No such file or directory\n> > \n> > Oops.\n> \n> Oops. And then, how would you fix this most easily?\n> \n> Be honest now.\n\nYour statement is obviously loaded. I haven't seen _anything_ that fixes\nit except what I have already mentioned. But I'm sure you are going to\ncomplain that it isn't easy enough.\n\n> Example: many arguments cause multiple option variables to change. \n> parse_options() simply can't handle that well - you can do it with a \n> callback, but then you need to make the option variables global or make \n> them a structure or something. All of which just makes it nasty to do \n> partial conversions for the simple cases.\n\nI'm not so sure. I assumed that most of the callbacks would simply take\na \"struct rev_list\". So you would end up in builtin-blame.c with:\n\n ...\n OPT__REVISION(&my_rev_list),\n ...\n\nin your options table. And if setup_revisions takes options that affect\nthings that _aren't_ in that struct, then they probably ought to be.\n\n> And I guarantee that just adding PARSE_OPT_{CONTINUE|STOP}_ON_UNKNOWN is \n> going to be the smallest patch, and make for the easiest usage case. It \n> may not be \"pretty\", but I can whip up a patch in five minutes.\n\nI don't have a problem with STOP_ON_UNKNOWN, as I think it is a building\nblock upon which sane things can be done (like linearly going through\neach parser and saying \"did you want this?\"). I think IGNORE/CONTINUE\nhas a fundamental flaw.\n\n> Or are we going to sit around discussing this for another five months?\n\nPlease! :)\n\nPierre was working on the approach I mentioned, but I think he is short\non time. I will take a look at the conversion, but I have a few other\nfixes on my plate first.\n\nIn the meantime, I don't think your patch makes anything _worse_, since\nwe already have these sorts of bugs in the current parsing code.\n\n-Peff\n"},{"id":"80753","messageId":"20080623200414.GB13395@artemis.madism.org","threadId":"14096","inReplyTo":"20080623195314.GA29569@sigill.intra.peff.net","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T20:04:14Z","receivedAt":"2008-06-23T20:04:14Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 07:53:14PM +0000, Jeff King wrote:\n> On Mon, Jun 23, 2008 at 11:47:49AM -0700, Linus Torvalds wrote:\n> > Or are we going to sit around discussing this for another five months?\n> \n> Please! :)\n> \n> Pierre was working on the approach I mentioned, but I think he is short\n> on time. I will take a look at the conversion, but I have a few other\n> fixes on my plate first.\n> \n> In the meantime, I don't think your patch makes anything _worse_, since\n> we already have these sorts of bugs in the current parsing code.\n\n To be fair, I lack the time to do a complete parse option overhaul\nthat is like a few days of work doing that only (and my employer will\nprobably complain if I do so ;P). Though I'm trying to make\nparse_options incremental right now, and I believe that it can work\nquite well, and allow way more incremental conversions.\n\n One can overcome many limitations by exposing some stuff from the\nparse_options logic, and just parse things in one pass. The _really_\nnice thing with this approach is that you can trivially merge\nparse_options descriptors by chaining parse_option_step (see my other\nmail).\n\n I'll probably have a RFC series soon.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80755","messageId":"alpine.LFD.1.10.0806231307070.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"20080623195314.GA29569@sigill.intra.peff.net","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T20:12:28Z","receivedAt":"2008-06-23T20:12:28Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 23 Jun 2008, Jeff King wrote:\n> \n> I'm not so sure. I assumed that most of the callbacks would simply take\n> a \"struct rev_list\". So you would end up in builtin-blame.c with:\n> \n> ...\n> OPT__REVISION(&my_rev_list),\n> ...\n> \n> in your options table. And if setup_revisions takes options that affect\n> things that _aren't_ in that struct, then they probably ought to be.\n\nThe world isn't like \"it ought to be\". It is like it is.\n\nIf you keep on dreaming about how things \"ought to be\", you'll never \nconfront the things as they are. The fact is, turning the existing very \nsimple argument parsers into using \"parse_options()\" is just _hard_.\n\nThen you say that it all \"ought to be\" in those options already.\n\nSure. Do we have some invisible sky wizard to make it so? \n\nThe thign is, parse_options() was introduced 8 months ago or so. Have you \nlooked at the output of\n\n\tgit grep 'str[n]*cmp.*\"--'\n\nlately? Yes, they probably all \"ought to be\" something or other, but as it \nis, the fact is that they are a damn pain to convert.\n\nI tried to do just _one_ program. Trust me, I'm not going to even bother \ntrying to do another unless parse_options() is made more palatable to do \nin small pieces. \n\n\t\t\tLinus\n"},{"id":"80762","messageId":"20080623210935.GC13395@artemis.madism.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806231158340.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T21:09:35Z","receivedAt":"2008-06-23T21:09:35Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 07:16:17PM +0000, Linus Torvalds wrote:\n> \n> \n> On Mon, 23 Jun 2008, Linus Torvalds wrote:\n> > \n> > Or are we going to sit around discussing this for another five months?\n> \n> So the gauntlet is thrown.\n\n Let's see if I can catch it elegantly.\n\n Following is a series split into incremental patches to migrate the\nparse_options API to an incremental one. Only the last step provides a\nfunctional incremental API.\n\n With that, you write parsers this way:\n\n\n\n{\n struct parse_opt_ctx_t ctx;\n\n parse_options_start(&ctx, argc, argv, 0);\n\n for (;;) {\n const char *arg;\n\n switch (parse_options_step(&ctx, options, usagestr)) {\n case PARSE_OPT_HELP:\n /* dump your help here, the one for options/usagestr is already dumped */\n exit(129);\n case PARSE_OPT_DONE:\n goto done;\n }\n\n arg = *ctx->argv++;\n ctx->argc--;\n\n if (strcmp(arg, \"-\")) {\n /* you're on baby ! */\n } else if ....\n } else {\n error(\"unknown option %s\", arg);\n parse_options_usage(options, usagestr);\n /* dump your help here */\n exit(129);\n }\n }\n\ndone:\n argc = parse_options_end(&ctx);\n}\n\n\nIt's slightly more involved than what Linus handwaved, but I believe\nit's usable this way.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80766","messageId":"1214255482-2086-1-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"20080623210935.GC13395@artemis.madism.org","subject":"[PATCH] parse-opt: have parse_options_{start,end}.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T21:11:18Z","receivedAt":"2008-06-23T21:11:18Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Make the struct optparse_t public under the better name parse_opt_ctx_t.\n\nSigned-off-by: Pierre Habouzit \n---\n parse-options.c | 69 +++++++++++++++++++++++++++++++------------------------\n parse-options.h | 16 ++++++++++++\n 2 files changed, 55 insertions(+), 30 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex b8bde2b..774e647 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -4,14 +4,7 @@\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n-struct optparse_t {\n-\tconst char **argv;\n-\tconst char **out;\n-\tint argc, cpidx;\n-\tconst char *opt;\n-};\n-\n-static inline const char *get_arg(struct optparse_t *p)\n+static inline const char *get_arg(struct parse_opt_ctx_t *p)\n {\n \tif (p->opt) {\n \t\tconst char *res = p->opt;\n@@ -37,7 +30,7 @@ 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 int get_value(struct optparse_t *p,\n+static int get_value(struct parse_opt_ctx_t *p,\n const struct option *opt, int flags)\n {\n \tconst char *s, *arg;\n@@ -131,7 +124,7 @@ 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 parse_opt_ctx_t *p, const struct option *options)\n {\n \tfor (; options->type != OPTION_END; options++) {\n \t\tif (options->short_name == *p->opt) {\n@@ -142,7 +135,7 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)\n \treturn error(\"unknown switch `%c'\", *p->opt);\n }\n \n-static int parse_long_opt(struct optparse_t *p, const char *arg,\n+static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,\n const struct option *options)\n {\n \tconst char *arg_end = strchr(arg, '=');\n@@ -247,45 +240,63 @@ void check_typos(const char *arg, const struct option *options)\n \t}\n }\n \n+void parse_options_start(struct parse_opt_ctx_t *ctx,\n+ int argc, const char **argv, int flags)\n+{\n+\tmemset(ctx, 0, sizeof(*ctx));\n+\tctx->argc = argc - 1;\n+\tctx->argv = argv + 1;\n+\tctx->out = argv;\n+\tctx->flags = flags;\n+}\n+\n+int parse_options_end(struct parse_opt_ctx_t *ctx)\n+{\n+\tmemmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));\n+\tctx->out[ctx->cpidx + ctx->argc] = NULL;\n+\treturn ctx->cpidx + ctx->argc;\n+}\n+\n static NORETURN void usage_with_options_internal(const char * const *,\n const struct option *, int);\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, argv, argc - 1, 0, NULL };\n+\tstruct parse_opt_ctx_t ctx;\n \n-\tfor (; args.argc; args.argc--, args.argv++) {\n-\t\tconst char *arg = args.argv[0];\n+\tparse_options_start(&ctx, argc, argv, flags);\n+\tfor (; ctx.argc; ctx.argc--, ctx.argv++) {\n+\t\tconst char *arg = ctx.argv[0];\n \n \t\tif (*arg != '-' || !arg[1]) {\n-\t\t\tif (flags & PARSE_OPT_STOP_AT_NON_OPTION)\n+\t\t\tif (ctx.flags & PARSE_OPT_STOP_AT_NON_OPTION)\n \t\t\t\tbreak;\n-\t\t\targs.out[args.cpidx++] = args.argv[0];\n+\t\t\tctx.out[ctx.cpidx++] = ctx.argv[0];\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (arg[1] != '-') {\n-\t\t\targs.opt = arg + 1;\n-\t\t\tif (*args.opt == 'h')\n+\t\t\tctx.opt = arg + 1;\n+\t\t\tif (*ctx.opt == 'h')\n \t\t\t\tusage_with_options(usagestr, options);\n-\t\t\tif (parse_short_opt(&args, options) < 0)\n+\t\t\tif (parse_short_opt(&ctx, options) < 0)\n \t\t\t\tusage_with_options(usagestr, options);\n-\t\t\tif (args.opt)\n+\t\t\tif (ctx.opt)\n \t\t\t\tcheck_typos(arg + 1, options);\n-\t\t\twhile (args.opt) {\n-\t\t\t\tif (*args.opt == 'h')\n+\t\t\twhile (ctx.opt) {\n+\t\t\t\tif (*ctx.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(&ctx, options) < 0)\n \t\t\t\t\tusage_with_options(usagestr, options);\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (!arg[2]) { /* \"--\" */\n-\t\t\tif (!(flags & PARSE_OPT_KEEP_DASHDASH)) {\n-\t\t\t\targs.argc--;\n-\t\t\t\targs.argv++;\n+\t\t\tif (!(ctx.flags & PARSE_OPT_KEEP_DASHDASH)) {\n+\t\t\t\tctx.argc--;\n+\t\t\t\tctx.argv++;\n \t\t\t}\n \t\t\tbreak;\n \t\t}\n@@ -294,13 +305,11 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \t\t\tusage_with_options_internal(usagestr, options, 1);\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(&ctx, arg + 2, options))\n \t\t\tusage_with_options(usagestr, options);\n \t}\n \n-\tmemmove(args.out + args.cpidx, args.argv, args.argc * sizeof(*args.out));\n-\targs.out[args.cpidx + args.argc] = NULL;\n-\treturn args.cpidx + args.argc;\n+\treturn parse_options_end(&ctx);\n }\n \n #define USAGE_OPTS_WIDTH 24\ndiff --git a/parse-options.h b/parse-options.h\nindex 4ee443d..db6c986 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -111,6 +111,22 @@ extern int parse_options(int argc, const char **argv,\n extern NORETURN void usage_with_options(const char * const *usagestr,\n const struct option *options);\n \n+/*----- incremantal advanced APIs -----*/\n+\n+struct parse_opt_ctx_t {\n+\tconst char **argv;\n+\tconst char **out;\n+\tint argc, cpidx;\n+\tconst char *opt;\n+\tint flags;\n+};\n+\n+extern void parse_options_start(struct parse_opt_ctx_t *ctx,\n+ int argc, const char **argv, int flags);\n+\n+extern int parse_options_end(struct parse_opt_ctx_t *ctx);\n+\n+\n /*----- some often used options -----*/\n extern int parse_opt_abbrev_cb(const struct option *, const char *, int);\n extern int parse_opt_approxidate_cb(const struct option *, const char *, int);\n-- \n1.5.6.117.g5fe2\n"},{"id":"80765","messageId":"1214255482-2086-2-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"1214255482-2086-1-git-send-email-madcoder@debian.org","subject":"[PATCH] parse-opt: Export a non NORETURN usage dumper.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T21:11:19Z","receivedAt":"2008-06-23T21:11:19Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Signed-off-by: Pierre Habouzit \n---\n parse-options.c | 24 +++++++++++++++++-------\n parse-options.h | 9 +++++++++\n 2 files changed, 26 insertions(+), 7 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 774e647..4e9e675 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -257,8 +257,8 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n \treturn ctx->cpidx + ctx->argc;\n }\n \n-static NORETURN void usage_with_options_internal(const char * const *,\n- const struct option *, int);\n+static int usage_with_options_internal(const char * const *,\n+ const struct option *, int, int);\n \n int parse_options(int argc, const char **argv, const struct option *options,\n const char * const usagestr[], int flags)\n@@ -302,7 +302,7 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \t\t}\n \n \t\tif (!strcmp(arg + 2, \"help-all\"))\n-\t\t\tusage_with_options_internal(usagestr, options, 1);\n+\t\t\tusage_with_options_internal(usagestr, options, 1, 1);\n \t\tif (!strcmp(arg + 2, \"help\"))\n \t\t\tusage_with_options(usagestr, options);\n \t\tif (parse_long_opt(&ctx, arg + 2, options))\n@@ -315,8 +315,8 @@ 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_internal(const char * const *usagestr,\n- const struct option *opts, int full)\n+int usage_with_options_internal(const char * const *usagestr,\n+ const struct option *opts, int full, int do_exit)\n {\n \tfprintf(stderr, \"usage: %s\\n\", *usagestr++);\n \twhile (*usagestr && **usagestr)\n@@ -401,15 +401,25 @@ void usage_with_options_internal(const char * const *usagestr,\n \t}\n \tfputc('\\n', stderr);\n \n-\texit(129);\n+\tif (do_exit)\n+\t\texit(129);\n+\treturn PARSE_OPT_HELP;\n }\n \n void usage_with_options(const char * const *usagestr,\n const struct option *opts)\n {\n-\tusage_with_options_internal(usagestr, opts, 0);\n+\tusage_with_options_internal(usagestr, opts, 0, 1);\n+\texit(129); /* make gcc happy */\n+}\n+\n+int parse_options_usage(const char * const *usagestr,\n+ const struct option *opts)\n+{\n+\treturn usage_with_options_internal(usagestr, opts, 0, 0);\n }\n \n+\n /*----- some often used options -----*/\n #include \"cache.h\"\n \ndiff --git a/parse-options.h b/parse-options.h\nindex db6c986..424ea46 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -113,6 +113,12 @@ extern NORETURN void usage_with_options(const char * const *usagestr,\n \n /*----- incremantal advanced APIs -----*/\n \n+enum {\n+\tPARSE_OPT_HELP = -1,\n+\tPARSE_OPT_DONE,\n+\tPARSE_OPT_UNKNOWN,\n+};\n+\n struct parse_opt_ctx_t {\n \tconst char **argv;\n \tconst char **out;\n@@ -121,6 +127,9 @@ struct parse_opt_ctx_t {\n \tint flags;\n };\n \n+extern int parse_options_usage(const char * const *usagestr,\n+ const struct option *opts);\n+\n extern void parse_options_start(struct parse_opt_ctx_t *ctx,\n int argc, const char **argv, int flags);\n \n-- \n1.5.6.117.g5fe2\n"},{"id":"80764","messageId":"1214255482-2086-3-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"1214255482-2086-2-git-send-email-madcoder@debian.org","subject":"[PATCH] parse-opt: create parse_options_step.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T21:11:20Z","receivedAt":"2008-06-23T21:11:20Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"For now it's unable to stop at unknown options, this commit merely\nreorganize some code around.\n\nSigned-off-by: Pierre Habouzit \n---\n parse-options.c | 89 +++++++++++++++++++++++++++++++-----------------------\n parse-options.h | 4 ++\n 2 files changed, 55 insertions(+), 38 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 4e9e675..71b3476 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -250,64 +250,79 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tctx->flags = flags;\n }\n \n-int parse_options_end(struct parse_opt_ctx_t *ctx)\n-{\n-\tmemmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));\n-\tctx->out[ctx->cpidx + ctx->argc] = NULL;\n-\treturn ctx->cpidx + ctx->argc;\n-}\n-\n static int usage_with_options_internal(const char * const *,\n- const struct option *, int, int);\n+ const struct option *, int);\n \n-int parse_options(int argc, const char **argv, const struct option *options,\n- const char * const usagestr[], int flags)\n+int parse_options_step(struct parse_opt_ctx_t *ctx,\n+ const struct option *options,\n+ const char * const usagestr[])\n {\n-\tstruct parse_opt_ctx_t ctx;\n-\n-\tparse_options_start(&ctx, argc, argv, flags);\n-\tfor (; ctx.argc; ctx.argc--, ctx.argv++) {\n-\t\tconst char *arg = ctx.argv[0];\n+\tfor (; ctx->argc; ctx->argc--, ctx->argv++) {\n+\t\tconst char *arg = ctx->argv[0];\n \n \t\tif (*arg != '-' || !arg[1]) {\n-\t\t\tif (ctx.flags & PARSE_OPT_STOP_AT_NON_OPTION)\n+\t\t\tif (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)\n \t\t\t\tbreak;\n-\t\t\tctx.out[ctx.cpidx++] = ctx.argv[0];\n+\t\t\tctx->out[ctx->cpidx++] = ctx->argv[0];\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (arg[1] != '-') {\n-\t\t\tctx.opt = arg + 1;\n-\t\t\tif (*ctx.opt == 'h')\n-\t\t\t\tusage_with_options(usagestr, options);\n-\t\t\tif (parse_short_opt(&ctx, options) < 0)\n+\t\t\tctx->opt = arg + 1;\n+\t\t\tif (*ctx->opt == 'h')\n+\t\t\t\treturn parse_options_usage(usagestr, options);\n+\t\t\tif (parse_short_opt(ctx, options) < 0)\n \t\t\t\tusage_with_options(usagestr, options);\n-\t\t\tif (ctx.opt)\n+\t\t\tif (ctx->opt)\n \t\t\t\tcheck_typos(arg + 1, options);\n-\t\t\twhile (ctx.opt) {\n-\t\t\t\tif (*ctx.opt == 'h')\n-\t\t\t\t\tusage_with_options(usagestr, options);\n-\t\t\t\tif (parse_short_opt(&ctx, options) < 0)\n+\t\t\twhile (ctx->opt) {\n+\t\t\t\tif (*ctx->opt == 'h')\n+\t\t\t\t\treturn parse_options_usage(usagestr, options);\n+\t\t\t\tif (parse_short_opt(ctx, options) < 0)\n \t\t\t\t\tusage_with_options(usagestr, options);\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (!arg[2]) { /* \"--\" */\n-\t\t\tif (!(ctx.flags & PARSE_OPT_KEEP_DASHDASH)) {\n-\t\t\t\tctx.argc--;\n-\t\t\t\tctx.argv++;\n+\t\t\tif (!(ctx->flags & PARSE_OPT_KEEP_DASHDASH)) {\n+\t\t\t\tctx->argc--;\n+\t\t\t\tctx->argv++;\n \t\t\t}\n \t\t\tbreak;\n \t\t}\n \n \t\tif (!strcmp(arg + 2, \"help-all\"))\n-\t\t\tusage_with_options_internal(usagestr, options, 1, 1);\n+\t\t\treturn usage_with_options_internal(usagestr, options, 1);\n \t\tif (!strcmp(arg + 2, \"help\"))\n-\t\t\tusage_with_options(usagestr, options);\n-\t\tif (parse_long_opt(&ctx, arg + 2, options))\n+\t\t\treturn parse_options_usage(usagestr, options);\n+\t\tif (parse_long_opt(ctx, arg + 2, options))\n \t\t\tusage_with_options(usagestr, options);\n \t}\n+\treturn PARSE_OPT_DONE;\n+}\n+\n+int parse_options_end(struct parse_opt_ctx_t *ctx)\n+{\n+\tmemmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));\n+\tctx->out[ctx->cpidx + ctx->argc] = NULL;\n+\treturn ctx->cpidx + ctx->argc;\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 parse_opt_ctx_t ctx;\n+\n+\tparse_options_start(&ctx, argc, argv, flags);\n+\tswitch (parse_options_step(&ctx, options, usagestr)) {\n+\tcase PARSE_OPT_HELP:\n+\t\texit(129);\n+\tcase PARSE_OPT_DONE:\n+\t\tbreak;\n+\tdefault: /* PARSE_OPT_UNKNOWN */\n+\t\tabort(); /* unreached yet */\n+\t}\n \n \treturn parse_options_end(&ctx);\n }\n@@ -316,7 +331,7 @@ int parse_options(int argc, const char **argv, const struct option *options,\n #define USAGE_GAP 2\n \n int usage_with_options_internal(const char * const *usagestr,\n- const struct option *opts, int full, int do_exit)\n+ const struct option *opts, int full)\n {\n \tfprintf(stderr, \"usage: %s\\n\", *usagestr++);\n \twhile (*usagestr && **usagestr)\n@@ -401,22 +416,20 @@ int usage_with_options_internal(const char * const *usagestr,\n \t}\n \tfputc('\\n', stderr);\n \n-\tif (do_exit)\n-\t\texit(129);\n \treturn PARSE_OPT_HELP;\n }\n \n void usage_with_options(const char * const *usagestr,\n const struct option *opts)\n {\n-\tusage_with_options_internal(usagestr, opts, 0, 1);\n-\texit(129); /* make gcc happy */\n+\tusage_with_options_internal(usagestr, opts, 0);\n+\texit(129);\n }\n \n int parse_options_usage(const char * const *usagestr,\n const struct option *opts)\n {\n-\treturn usage_with_options_internal(usagestr, opts, 0, 0);\n+\treturn usage_with_options_internal(usagestr, opts, 0);\n }\n \n \ndiff --git a/parse-options.h b/parse-options.h\nindex 424ea46..9da5e8c 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -133,6 +133,10 @@ extern int parse_options_usage(const char * const *usagestr,\n extern void parse_options_start(struct parse_opt_ctx_t *ctx,\n int argc, const char **argv, int flags);\n \n+extern int parse_options_step(struct parse_opt_ctx_t *ctx,\n+ const struct option *options,\n+ const char * const usagestr[]);\n+\n extern int parse_options_end(struct parse_opt_ctx_t *ctx);\n \n \n-- \n1.5.6.117.g5fe2\n"},{"id":"80767","messageId":"1214255482-2086-4-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"1214255482-2086-3-git-send-email-madcoder@debian.org","subject":"[PATCH] parse-opt: do not pring errors on unknown options, return -2 intead.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T21:11:21Z","receivedAt":"2008-06-23T21:11:21Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"This way we can catch \"unknown\" options more easily.\n\nSigned-off-by: Pierre Habouzit \n---\n parse-options.c | 35 ++++++++++++++++++++++++++---------\n 1 files changed, 26 insertions(+), 9 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 71b3476..1a1d4dc 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -132,7 +132,7 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *optio\n \t\t\treturn get_value(p, options, OPT_SHORT);\n \t\t}\n \t}\n-\treturn error(\"unknown switch `%c'\", *p->opt);\n+\treturn -2;\n }\n \n static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,\n@@ -217,7 +217,7 @@ 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 -2;\n }\n \n void check_typos(const char *arg, const struct option *options)\n@@ -271,15 +271,23 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tctx->opt = arg + 1;\n \t\t\tif (*ctx->opt == 'h')\n \t\t\t\treturn parse_options_usage(usagestr, options);\n-\t\t\tif (parse_short_opt(ctx, options) < 0)\n-\t\t\t\tusage_with_options(usagestr, options);\n+\t\t\tswitch (parse_short_opt(ctx, options)) {\n+\t\t\t case -1:\n+\t\t\t\treturn parse_options_usage(usagestr, options);\n+\t\t\t case -2:\n+\t\t\t\treturn PARSE_OPT_UNKNOWN;\n+\t\t\t}\n \t\t\tif (ctx->opt)\n \t\t\t\tcheck_typos(arg + 1, options);\n \t\t\twhile (ctx->opt) {\n \t\t\t\tif (*ctx->opt == 'h')\n \t\t\t\t\treturn parse_options_usage(usagestr, options);\n-\t\t\t\tif (parse_short_opt(ctx, options) < 0)\n-\t\t\t\t\tusage_with_options(usagestr, options);\n+\t\t\t\tswitch (parse_short_opt(ctx, options)) {\n+\t\t\t\t case -1:\n+\t\t\t\t\treturn parse_options_usage(usagestr, options);\n+\t\t\t\t case -2:\n+\t\t\t\t\treturn PARSE_OPT_UNKNOWN;\n+\t\t\t\t}\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n@@ -296,8 +304,12 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\treturn usage_with_options_internal(usagestr, options, 1);\n \t\tif (!strcmp(arg + 2, \"help\"))\n \t\t\treturn parse_options_usage(usagestr, options);\n-\t\tif (parse_long_opt(ctx, arg + 2, options))\n-\t\t\tusage_with_options(usagestr, options);\n+\t\tswitch (parse_long_opt(ctx, arg + 2, options)) {\n+\t\t case -1:\n+\t\t\treturn parse_options_usage(usagestr, options);\n+\t\t case -2:\n+\t\t\treturn PARSE_OPT_UNKNOWN;\n+\t\t}\n \t}\n \treturn PARSE_OPT_DONE;\n }\n@@ -321,7 +333,12 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \tcase PARSE_OPT_DONE:\n \t\tbreak;\n \tdefault: /* PARSE_OPT_UNKNOWN */\n-\t\tabort(); /* unreached yet */\n+\t\tif (ctx.argv[0][1] == '-') {\n+\t\t\terror(\"unknown option `%s'\", ctx.argv[0] + 2);\n+\t\t} else {\n+\t\t\terror(\"unknown switch `%c'\", *ctx.opt);\n+\t\t}\n+\t\tusage_with_options(usagestr, options);\n \t}\n \n \treturn parse_options_end(&ctx);\n-- \n1.5.6.117.g5fe2\n"},{"id":"80763","messageId":"1214255482-2086-5-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"1214255482-2086-4-git-send-email-madcoder@debian.org","subject":"[PATCH] parse-opt: fake short strings for callers to believe in.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T21:11:22Z","receivedAt":"2008-06-23T21:11:22Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"If we begin to parse -abc and that the parser knew about -a and -b, it\nwill fake a -c switch for the caller to deal with.\n\nOf course in the case of -acb (supposing -c is not taking an argument) the\ncaller will have to be especially clever to do the same thing. We could\nthink about exposing an API to do so if it's really needed, but oh well...\n\nSigned-off-by: Pierre Habouzit \n---\n parse-options.c | 18 ++++++++++++++++--\n parse-options.h | 12 ++++++++++++\n 2 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 1a1d4dc..f3fd296 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -248,6 +248,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tctx->argv = argv + 1;\n \tctx->out = argv;\n \tctx->flags = flags;\n+\tstrbuf_init(&ctx->buf, 0);\n }\n \n static int usage_with_options_internal(const char * const *,\n@@ -257,6 +258,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n const struct option *options,\n const char * const usagestr[])\n {\n+\t/* we must reset ->opt, unknown short option leave it dangling */\n+\tctx->opt = NULL;\n+\n \tfor (; ctx->argc; ctx->argc--, ctx->argv++) {\n \t\tconst char *arg = ctx->argv[0];\n \n@@ -275,7 +279,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t case -1:\n \t\t\t\treturn parse_options_usage(usagestr, options);\n \t\t\t case -2:\n-\t\t\t\treturn PARSE_OPT_UNKNOWN;\n+\t\t\t\tgoto unknown_fixup;\n \t\t\t}\n \t\t\tif (ctx->opt)\n \t\t\t\tcheck_typos(arg + 1, options);\n@@ -286,10 +290,19 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\t case -1:\n \t\t\t\t\treturn parse_options_usage(usagestr, options);\n \t\t\t\t case -2:\n-\t\t\t\t\treturn PARSE_OPT_UNKNOWN;\n+\t\t\t\t\tgoto unknown_fixup;\n \t\t\t\t}\n \t\t\t}\n \t\t\tcontinue;\n+unknown_fixup:\n+\t\t\t/* fake a short option thing to hide the fact that we may have\n+\t\t\t * started to parse aggregated stuff\n+\t\t\t */\n+\t\t\tstrbuf_reset(&ctx->buf);\n+\t\t\tstrbuf_addch(&ctx->buf, '-');\n+\t\t\tstrbuf_addstr(&ctx->buf, ctx->opt);\n+\t\t\t*ctx->argv = ctx->buf.buf;\n+\t\t\treturn PARSE_OPT_UNKNOWN;\n \t\t}\n \n \t\tif (!arg[2]) { /* \"--\" */\n@@ -318,6 +331,7 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n {\n \tmemmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));\n \tctx->out[ctx->cpidx + ctx->argc] = NULL;\n+\tstrbuf_release(&ctx->buf);\n \treturn ctx->cpidx + ctx->argc;\n }\n \ndiff --git a/parse-options.h b/parse-options.h\nindex 9da5e8c..14447d5 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -1,6 +1,8 @@\n #ifndef PARSE_OPTIONS_H\n #define PARSE_OPTIONS_H\n \n+#include \"strbuf.h\"\n+\n enum parse_opt_type {\n \t/* special types */\n \tOPTION_END,\n@@ -119,12 +121,18 @@ enum {\n \tPARSE_OPT_UNKNOWN,\n };\n \n+/*\n+ * It's okay for the caller to consume argv/argc in the usual way.\n+ * Other fields of that structure are private to parse-options and should not\n+ * be modified in any way.\n+ */\n struct parse_opt_ctx_t {\n \tconst char **argv;\n \tconst char **out;\n \tint argc, cpidx;\n \tconst char *opt;\n \tint flags;\n+\tstruct strbuf buf;\n };\n \n extern int parse_options_usage(const char * const *usagestr,\n@@ -133,6 +141,10 @@ extern int parse_options_usage(const char * const *usagestr,\n extern void parse_options_start(struct parse_opt_ctx_t *ctx,\n int argc, const char **argv, int flags);\n \n+/* Warning: you cannot keep pointers to ctx->argv during the parse\n+ * because some \"option strings\" are faked. It's okay to use\n+ * ctx->argv after a parse_options_end obviously\n+ */\n extern int parse_options_step(struct parse_opt_ctx_t *ctx,\n const struct option *options,\n const char * const usagestr[]);\n-- \n1.5.6.117.g5fe2\n"},{"id":"80771","messageId":"20080623212322.GD13395@artemis.madism.org","threadId":"14096","inReplyTo":"20080623210935.GC13395@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T21:23:22Z","receivedAt":"2008-06-23T21:23:22Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 09:09:35PM +0000, Pierre Habouzit wrote:\n> {\n> struct parse_opt_ctx_t ctx;\n> \n> parse_options_start(&ctx, argc, argv, 0);\n> \n> for (;;) {\n> const char *arg;\n> \n> switch (parse_options_step(&ctx, options, usagestr)) {\n> case PARSE_OPT_HELP:\n> /* dump your help here, the one for options/usagestr is already dumped */\n\n At the expense of reworking how help is output, especially in the line\nabove, we also can have the nice feature that you can chain\nparse_options_step with different options structure basically this way\n(error handling removed):\n\n if (parse_options_step(&ctx, options1) == PARSE_OPT_UNKNOWN &&\n parse_options_step(&ctx, options2) == PARSE_OPT_UNKNOWN &&\n ...\n parse_options_step(&ctx, options_n) == PARSE_OPT_UNKNOWN)\n {\n /* you're on ! */\n }\n\n But with the proposed series, it's not possible (well in the sense\nthat for valid command lines it will work, but you'll end up with funny\nusage dumps for invalid ones).\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80772","messageId":"7vprq7hmkx.fsf@gitster.siamese.dyndns.org","threadId":"14096","inReplyTo":"20080623210935.GC13395@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-23T21:23:58Z","receivedAt":"2008-06-23T21:23:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit writes:\n\n> With that, you write parsers this way:\n>\n> {\n> struct parse_opt_ctx_t ctx;\n>\n> parse_options_start(&ctx, argc, argv, 0);\n>\n> for (;;) {\n> const char *arg;\n>\n> switch (parse_options_step(&ctx, options, usagestr)) {\n> case PARSE_OPT_HELP:\n> /* dump your help here, the one for options/usagestr is already dumped */\n> exit(129);\n> case PARSE_OPT_DONE:\n> goto done;\n> }\n>\n> arg = *ctx->argv++;\n> ctx->argc--;\n>\n> if (strcmp(arg, \"-\")) {\n> /* you're on baby ! */\n> } else if ....\n> } else {\n> error(\"unknown option %s\", arg);\n> parse_options_usage(options, usagestr);\n> /* dump your help here */\n> exit(129);\n> }\n> }\n>\n> done:\n> argc = parse_options_end(&ctx);\n> }\n\nNice. I have started doing the same (insignificant details are different;\ne.g. I used \"positive is unknown\" convention instead ) and then the\nsolution is sitting in my mbox ;-)\n"},{"id":"80773","messageId":"alpine.LFD.1.10.0806231425270.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"20080623210935.GC13395@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T21:26:39Z","receivedAt":"2008-06-23T21:26:39Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 23 Jun 2008, Pierre Habouzit wrote:\n> \n> Let's see if I can catch it elegantly.\n\nNo.\n\nLook at builtin-blame.c.\n\nTHEN get back to me.\n\nTrust me, you need what I wrote. Something that parses all the options in \none go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT \nfunction than the caller will call it!\n\nWhy is that so hard to accept? Especially since I already wrote the code \nand sent it out?\n\n\t\t\tLinus\n"},{"id":"80775","messageId":"20080623212855.GE13395@artemis.madism.org","threadId":"14096","inReplyTo":"7vprq7hmkx.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T21:28:56Z","receivedAt":"2008-06-23T21:28:56Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 09:23:58PM +0000, Junio C Hamano wrote:\n> Pierre Habouzit writes:\n> \n> > With that, you write parsers this way:\n> >\n> > {\n> > struct parse_opt_ctx_t ctx;\n> >\n> > parse_options_start(&ctx, argc, argv, 0);\n> >\n> > for (;;) {\n> > const char *arg;\n> >\n> > switch (parse_options_step(&ctx, options, usagestr)) {\n> > case PARSE_OPT_HELP:\n> > /* dump your help here, the one for options/usagestr is already dumped */\n> > exit(129);\n> > case PARSE_OPT_DONE:\n> > goto done;\n> > }\n> >\n> > arg = *ctx->argv++;\n> > ctx->argc--;\n> >\n> > if (strcmp(arg, \"-\")) {\n> > /* you're on baby ! */\n> > } else if ....\n> > } else {\n> > error(\"unknown option %s\", arg);\n> > parse_options_usage(options, usagestr);\n> > /* dump your help here */\n> > exit(129);\n> > }\n> > }\n> >\n> > done:\n> > argc = parse_options_end(&ctx);\n> > }\n> \n> Nice. I have started doing the same (insignificant details are different;\n> e.g. I used \"positive is unknown\" convention instead ) and then the\n> solution is sitting in my mbox ;-)\n\nWell it's still rough on the edges, totally untested (I didn't bother to\nrun the testsuite), but I wanted some feedback before I cook this to\nsomething nicer.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80778","messageId":"alpine.LFD.1.10.0806231428230.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806231425270.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T21:41:40Z","receivedAt":"2008-06-23T21:41:40Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 23 Jun 2008, Linus Torvalds wrote:\n> \n> Trust me, you need what I wrote. Something that parses all the options in \n> one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT \n> function than the caller will call it!\n\nSide note: of course you can always just queue them up yourself, and then \npass those off to the later stage. But where do you queue them? Can you \nre-use argv[]? If so, what's the difference with the simpler patch that I \nalready posted? (simpler both from a usage standpoint _and_ from a patch \nsize standpoint).\n\nThat said, I *really* don't care what the end result is, but I would \nstrongly urge people to actually look at what existing code does. That \nincludes at a minimum builtin-blame.c, but also all the existing users \nwhere _most_ of the arguments are just trivial. builtin-apply.c is a great \nexample of something where it's almost all trivial, but a lot of the \noptions set more than one value.\n\nThe guiding light here should not be how pretty or clean option-parse.c \nis, but how easy it is to convert the hundreds of existing options that \nhaven't been converted yet. And a lot of them haven't been converted \nbecause it's currently very painful to do multi-valued things, even if \nmost of those values really are pretty simple.\n\nIn that vein - maybe it would be a good idea to allow multiples of the \nsame option, so that things like builtin-apply.c can handle the \"-p\" \noption (to give an example of this behaviour) as a set of TWO options that \ndo\n\n\tOPT_INTEGER('p', NULL, &p_value, \"Patch depth\"),\n\tOPT_BOOLEAN('p', NULL, &p_value_known),\n\nwhere \"-p3\" would then trigger on both, once setting \"p_value\" to 3, and \nonce setting p_value_known to 1.\n\nI dunno. But _this_ is the real problem with parse-options.c. It's hard to \nconvert existing really simple parsers. Whether the problem is writing a \nsane set of rules for it without using callbacks (callbacks are neither \neasy nor clean), or whether the problem is that the parsing is done in \nmultiple different places..\n\n\t\t\tLinus\n"},{"id":"80779","messageId":"20080623214758.GF13395@artemis.madism.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806231425270.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T21:47:58Z","receivedAt":"2008-06-23T21:47:58Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 09:26:39PM +0000, Linus Torvalds wrote:\n> \n> \n> On Mon, 23 Jun 2008, Pierre Habouzit wrote:\n> > \n> > Let's see if I can catch it elegantly.\n> \n> No.\n> \n> Look at builtin-blame.c.\n> \n> THEN get back to me.\n> \n> Trust me, you need what I wrote. Something that parses all the options in \n> one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT \n> function than the caller will call it!\n\nWell I really believe that with an incremental parse_revisions (I have\nsent the patch to split {init,parse,setup}_revisions already) instead of\nthe 'do all at once' version, you can write the code in builtin-blame in\none pass exactly with the series I wrote.\n\n> Why is that so hard to accept? Especially since I already wrote the code \n> and sent it out?\n\nI don't find it \"hard to accept\", I just don't like the idea because\nit's broken wrt some parse-options mechanisms that I like a lot, and\nwould be sad to see broken, and that with a little work can be\novercomed. Note that builtin-blame is contains probably the most\ninvolved option parser in git.git, and isn't really what one could call\nrepresentative of what awaits us on the path of migrating git.git to\nparse-options fully.\n\nI don't expect it to be easy to migrate at all. I do believe your code\nworks too, it's obvious to see it does actually. But it could be made\nbetter, with real code that I will provide, just not tonight because I'm\nexhausted.\n\nNote that an incremental parse_revisions/parse_diff_opts/.. are a\nprecodition to many CLI parsers conversions anyways, so if the task\nseems a bit exagerated for the sake of git-blame only, it actually helps\nfor dozens of other commands, where the conversion will just be trivial.\n\nI fully understand you frustration wrt the poor handling of the command\nline arguments in many git commands. Believe me I really do. I know I\nhaven't been active on this front for something like 6 months, but I'm\nsadly not payed to work on git.git ;p Though things are moving right\nnow, so why don't we wait another week to see what other propose ?\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80780","messageId":"7vd4m7hkjf.fsf@gitster.siamese.dyndns.org","threadId":"14096","inReplyTo":"1214255482-2086-4-git-send-email-madcoder@debian.org","subject":"Re: [PATCH] parse-opt: do not pring errors on unknown options, return -2 intead.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-23T22:08:04Z","receivedAt":"2008-06-23T22:08:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"You'd need something like this on top.\n\nDocumentation/technical/api-parse-options says that non-zero return is\nused to signal an error.\n\n\n\n parse-options.c | 16 ++++++++--------\n 1 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 132d34c..71a8056 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -94,14 +94,14 @@ static int get_value(struct parse_opt_ctx_t *p,\n \n \tcase OPTION_CALLBACK:\n \t\tif (unset)\n-\t\t\treturn (*opt->callback)(opt, NULL, 1);\n+\t\t\treturn (*opt->callback)(opt, NULL, 1) ? (-1) : 0;\n \t\tif (opt->flags & PARSE_OPT_NOARG)\n-\t\t\treturn (*opt->callback)(opt, NULL, 0);\n+\t\t\treturn (*opt->callback)(opt, NULL, 0) ? (-1) : 0;\n \t\tif (opt->flags & PARSE_OPT_OPTARG && !p->opt)\n-\t\t\treturn (*opt->callback)(opt, NULL, 0);\n+\t\t\treturn (*opt->callback)(opt, NULL, 0) ? (-1) : 0;\n \t\tif (!arg)\n \t\t\treturn opterror(opt, \"requires a value\", flags);\n-\t\treturn (*opt->callback)(opt, get_arg(p), 0);\n+\t\treturn (*opt->callback)(opt, get_arg(p), 0) ? (-1) : 0;\n \n \tcase OPTION_INTEGER:\n \t\tif (unset) {\n@@ -276,9 +276,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tif (*ctx->opt == 'h')\n \t\t\t\treturn parse_options_usage(usagestr, options);\n \t\t\tswitch (parse_short_opt(ctx, options)) {\n-\t\t\t case -1:\n+\t\t\tcase -1:\n \t\t\t\treturn parse_options_usage(usagestr, options);\n-\t\t\t case -2:\n+\t\t\tcase -2:\n \t\t\t\tgoto unknown_fixup;\n \t\t\t}\n \t\t\tif (ctx->opt)\n@@ -318,9 +318,9 @@ unknown_fixup:\n \t\tif (!strcmp(arg + 2, \"help\"))\n \t\t\treturn parse_options_usage(usagestr, options);\n \t\tswitch (parse_long_opt(ctx, arg + 2, options)) {\n-\t\t case -1:\n+\t\tcase -1:\n \t\t\treturn parse_options_usage(usagestr, options);\n-\t\t case -2:\n+\t\tcase -2:\n \t\t\treturn PARSE_OPT_UNKNOWN;\n \t\t}\n \t}\n"},{"id":"80781","messageId":"7v3an3hke8.fsf@gitster.siamese.dyndns.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806231425270.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-23T22:11:11Z","receivedAt":"2008-06-23T22:11:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Mon, 23 Jun 2008, Pierre Habouzit wrote:\n>> \n>> Let's see if I can catch it elegantly.\n>\n> No.\n>\n> Look at builtin-blame.c.\n>\n> THEN get back to me.\n>\n> Trust me, you need what I wrote. Something that parses all the options in \n> one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT \n> function than the caller will call it!\n\nI do not think you two are saying vastly different things.\n\nThe series Pierre has just posted is prerequisite for teaching\nparse_options() to queue the unknown ones (besides allowing a finer\ngrained stepwise parsing), so that the caller like cmd_blame() can call\nrevision parser with the remainder.\n"},{"id":"80782","messageId":"20080623221326.GG13395@artemis.madism.org","threadId":"14096","inReplyTo":"7vd4m7hkjf.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] parse-opt: do not pring errors on unknown options, return -2 intead.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T22:13:26Z","receivedAt":"2008-06-23T22:13:26Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 10:08:04PM +0000, Junio C Hamano wrote:\n> You'd need something like this on top.\n> \n> Documentation/technical/api-parse-options says that non-zero return is\n> used to signal an error.\n\nOh right, like said, it's still rough on the edges :)\n\nI'm not 100% convinced by the current usage generation yet. But I'll\nsleep on it first.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80784","messageId":"20080623222404.GM13395@artemis.madism.org","threadId":"14096","inReplyTo":"7v3an3hke8.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T22:24:05Z","receivedAt":"2008-06-23T22:24:05Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 10:11:11PM +0000, Junio C Hamano wrote:\n> Linus Torvalds writes:\n> \n> > On Mon, 23 Jun 2008, Pierre Habouzit wrote:\n> >> \n> >> Let's see if I can catch it elegantly.\n> >\n> > No.\n> >\n> > Look at builtin-blame.c.\n> >\n> > THEN get back to me.\n> >\n> > Trust me, you need what I wrote. Something that parses all the options in \n> > one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT \n> > function than the caller will call it!\n> \n> I do not think you two are saying vastly different things.\n> \n> The series Pierre has just posted is prerequisite for teaching\n> parse_options() to queue the unknown ones (besides allowing a finer\n> grained stepwise parsing), so that the caller like cmd_blame() can call\n> revision parser with the remainder.\n\nYeah actually one can use it this way, what I wrote can be used to do\nthe very same thing as your _KEEP_UNKNOWN flag, but also more.\n \nThough I didn't fixed the fact that parse_options clobbers argv[0],\nwhich can be easily fixed. The issue with that is that _some_ callers\nuse the fact that the filtered argv is NULL terminated. I'm unsure if\nposix says that argv[argc] is readable and NULL at all, if it isn't,\nthen changing that would break git for some commands on some OSes other\nthan Linux and BSD where AFAICT argv[argc] == NULL holds.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80786","messageId":"20080623223608.GN13395@artemis.madism.org","threadId":"14096","inReplyTo":"20080623222404.GM13395@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T22:36:08Z","receivedAt":"2008-06-23T22:36:08Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 10:24:05PM +0000, Pierre Habouzit wrote:\n> Yeah actually one can use it this way, what I wrote can be used to do\n> the very same thing as your _KEEP_UNKNOWN flag, but also more.\n> \n> Though I didn't fixed the fact that parse_options clobbers argv[0],\n> which can be easily fixed. The issue with that is that _some_ callers\n> use the fact that the filtered argv is NULL terminated. I'm unsure if\n> posix says that argv[argc] is readable and NULL at all, if it isn't,\n> then changing that would break git for some commands on some OSes other\n> than Linux and BSD where AFAICT argv[argc] == NULL holds.\n\nActually one can do it this way without breaking \"backward\ncompatibility\" here:\n\n----8<---\nFrom 8955adf557fded857eacc6c03c885a3d6334580f Mon Sep 17 00:00:00 2001\nFrom: Pierre Habouzit \nDate: Tue, 24 Jun 2008 00:31:31 +0200\nSubject: [PATCH] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.\n\nThis way, argv[0] isn't clobbered, to the cost of maybe not having a\nresulting NULL terminated argv array.\n\nSigned-off-by: Pierre Habouzit \n---\n parse-options.c | 8 +++++---\n parse-options.h | 2 ++\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 1ade2ac..c1c5a94 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -246,7 +246,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tmemset(ctx, 0, sizeof(*ctx));\n \tctx->argc = argc - 1;\n \tctx->argv = argv + 1;\n-\tctx->out = argv;\n+\tctx->out = argv + ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n \tctx->flags = flags;\n \tstrbuf_init(&ctx->buf, 0);\n }\n@@ -329,10 +329,12 @@ unknown_fixup:\n \n int parse_options_end(struct parse_opt_ctx_t *ctx)\n {\n+\tint res = ctx->cpidx + ctx->argc;\n \tmemmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));\n-\tctx->out[ctx->cpidx + ctx->argc] = NULL;\n+\tif (ctx->out + res < ctx->argv + ctx->argc)\n+\t\tctx->out[res] = NULL;\n \tstrbuf_release(&ctx->buf);\n-\treturn ctx->cpidx + ctx->argc;\n+\treturn res;\n }\n \n int parse_options(int argc, const char **argv, const struct option *options,\ndiff --git a/parse-options.h b/parse-options.h\nindex 14447d5..6745c7d 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -22,6 +22,8 @@ enum parse_opt_type {\n enum parse_opt_flags {\n \tPARSE_OPT_KEEP_DASHDASH = 1,\n \tPARSE_OPT_STOP_AT_NON_OPTION = 2,\n+\t/* using that option, the filtered argv may not be NULL terminated */\n+\tPARSE_OPT_KEEP_ARGV0 = 4,\n };\n \n enum parse_opt_option_flags {\n-- \n1.5.6.117.g4959d.dirty\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80787","messageId":"7vmylbg4ks.fsf@gitster.siamese.dyndns.org","threadId":"14096","inReplyTo":"20080623222404.GM13395@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-23T22:38:11Z","receivedAt":"2008-06-23T22:38:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit writes:\n\n> Though I didn't fixed the fact that parse_options clobbers argv[0],\n> which can be easily fixed. The issue with that is that _some_ callers\n> use the fact that the filtered argv is NULL terminated.\n\nIsn't it just the matter of (1) leave argv[0] as is, (2) start copy dest\nat argv[1] not argv[0], and (3) terminate argv[nargc] = NULL where nargc\nis what you did not handle?\n\nYou have argc args in incoming argv[], you consume zero or more of them\nstarting from argv[1] up to potentially argv[argc-1] (and argv[argc] is\nNULL). So why is it an issue? Sorry, I do not understand.\n"},{"id":"80792","messageId":"20080623233146.GP13395@artemis.madism.org","threadId":"14096","inReplyTo":"7vmylbg4ks.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-23T23:31:46Z","receivedAt":"2008-06-23T23:31:46Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 10:38:11PM +0000, Junio C Hamano wrote:\n> Pierre Habouzit writes:\n> \n> > Though I didn't fixed the fact that parse_options clobbers argv[0],\n> > which can be easily fixed. The issue with that is that _some_ callers\n> > use the fact that the filtered argv is NULL terminated.\n> \n> Isn't it just the matter of (1) leave argv[0] as is, (2) start copy dest\n> at argv[1] not argv[0], and (3) terminate argv[nargc] = NULL where nargc\n> is what you did not handle?\n> \n> You have argc args in incoming argv[], you consume zero or more of them\n> starting from argv[1] up to potentially argv[argc-1] (and argv[argc] is\n> NULL). So why is it an issue? Sorry, I do not understand.\n\n Are we sure argv[argc] is NULL when those are main() arguments ? If\nyes there is no issue, and I should read posix more carefully, but I was\nunder the impression that POSIX wasn't enforcing that.\n\n Unrelated but worth to note: many parse_options users just don't care\nabout argv[0] and having it kept each time would rather be a pain for\nthem (they would need to call argv++, argc-- themselves).\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80793","messageId":"alpine.LFD.1.10.0806231638190.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"20080623233146.GP13395@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-23T23:40:37Z","receivedAt":"2008-06-23T23:40:37Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 24 Jun 2008, Pierre Habouzit wrote:\n> \n> Are we sure argv[argc] is NULL when those are main() arguments ?\n\nYes. There's tons of code that ends up not caring about argc at all, so \nany system that doesn't NULL-terminate would break.\n\nIn fact, git itself already depends on it, because it turns argv directly \ninto the pathspec name pattern array.\n\n\t\tLinus\n"},{"id":"80794","messageId":"7v8wwvg15z.fsf@gitster.siamese.dyndns.org","threadId":"14096","inReplyTo":"20080623233146.GP13395@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-23T23:51:52Z","receivedAt":"2008-06-23T23:51:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit writes:\n\n> Unrelated but worth to note: many parse_options users just don't care\n> about argv[0] and having it kept each time would rather be a pain for\n> them (they would need to call argv++, argc-- themselves).\n\nNot necessarily. If they were parsing by hand, they were written to deal\nwith the fact that argv[0] is not the program argument (iow, they start\ncounting from one). And before and after calling parse_options(), they\nneed to change that assumption anyway, because parse_options() makes\nargv[0] disappear.\n\nSee for example these:\n\n * 5eee6b2 (Make builtin-reset.c use parse_options., 2008-03-04)\n\tThe caller used to start counting from 1, after the conversion it\n\thas to count from 0.\n\n * 8320199 (Rewrite builtin-fetch option parsing to use parse_options()., 2007-12-04)\n * 3968658 (Make builtin-tag.c use parse_options., 2007-11-09)\n\tThe caller used to say (i==argc) is \"no param\" case, now it has to\n\tsay \"!argc\" is the equivalent condition.\n\nI am not saying that forcing these callers to change their loop invariants\nin order to use parse_options() was bad. I am saying that your \"would\nrather be a pain for them\" is a bogus argument --- they needed to pay that\nprice when converting to parse_options() that munges argv[0] anyway.\n\nBut this argv[0] munging exactly is why parse_options() hurts users who\nwant to cascade option parsing.\n\nDidn't one of the recent series have an option to tell parse_options() to\nstart parsing from argv[0] (persumably because it got an argument array\nfrom somebody who munges it to drop argv[0])? I think it was merge-in-C\nfrom Miklos, but the approach feels very much backwards. The caller\nshould be (at least) able to choose to say KEEP_ARGV0 like one of your\npatch does.\n"},{"id":"80796","messageId":"7vzlpbeksn.fsf@gitster.siamese.dyndns.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806231114180.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-24T00:30:48Z","receivedAt":"2008-06-24T00:30:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Mon, 23 Jun 2008, Linus Torvalds wrote:\n>> \n>> Umm. Helloo, reality.. There are actually very few options that take a \n>> flag for their arguments. In particular, the option parsing we really \n>> _care_ about (revision parsing - see builtin-blame.c which is exactly \n>> where I wanted to convert things) very much DOES NOT.\n>\n> Actually, I guess \"--default\" does, but if you try to mix that up with (a) \n> a default head that starts with a dash and (b) git-blame, you're doing \n> something pretty odd.\n>\n> And yes, \"-n\" does too, but if you pass it negative numbers you get what \n> you deserve.\n>\n> The point being, we really _do_ have a real-life existing case for \n> PARSE_OPT_CONTINUE_ON_UNKNOWN, which is hard to handle any other way. \n> Currently you can literally do\n>\n> \tgit blame --since=April -b Makefile\n>\n> and while it's a totally made-up example, it's one I've picked to show \n> exactly what does *not* work with my patch that started this whole thread.\n>\n> And guess what you need to fix it?\n>\n> If you guessed PARSE_OPT_CONTINUE_ON_UNKNOWN, you win a prize. \n\nWith this on top of Pierre's series, and adding PARSE_OPT_SKIP_UNKNOWN to\nthe obvious place in your patch, \"blame --since=April -b Makefile\" would\nwork.\n\n-- >8 --\nSubject: [PATCH] parse-options: PARSE_OPT_SKIP_UNKNOWN\n\n---\n parse-options.c | 14 ++++++++++----\n parse-options.h | 1 +\n 2 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 71a8056..9f1eb65 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -341,20 +341,26 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \tstruct parse_opt_ctx_t ctx;\n \n \tparse_options_start(&ctx, argc, argv, flags);\n+\n+ again:\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n \t\texit(129);\n \tcase PARSE_OPT_DONE:\n \t\tbreak;\n \tdefault: /* PARSE_OPT_UNKNOWN */\n-\t\tif (ctx.argv[0][1] == '-') {\n+\t\tif (flags & PARSE_OPT_KEEP_UNKNOWN) {\n+\t\t\tctx.out[ctx.cpidx++] = ctx.argv[0];\n+\t\t\tctx.argc--;\n+\t\t\tctx.argv++;\n+\t\t\tgoto again;\n+\t\t}\n+\t\tif (ctx.argv[0][1] == '-')\n \t\t\terror(\"unknown option `%s'\", ctx.argv[0] + 2);\n-\t\t} else {\n+\t\telse\n \t\t\terror(\"unknown switch `%c'\", *ctx.opt);\n-\t\t}\n \t\tusage_with_options(usagestr, options);\n \t}\n-\n \treturn parse_options_end(&ctx);\n }\n \ndiff --git a/parse-options.h b/parse-options.h\nindex 403794f..30fbf7e 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -22,6 +22,7 @@ enum parse_opt_type {\n enum parse_opt_flags {\n \tPARSE_OPT_KEEP_DASHDASH = 1,\n \tPARSE_OPT_STOP_AT_NON_OPTION = 2,\n+\tPARSE_OPT_KEEP_UNKNOWN = 4,\n };\n \n enum parse_opt_option_flags {\n-- \n1.5.6.49.g112db\n"},{"id":"80800","messageId":"20080624012711.GA3816@sigio.intra.peff.net","threadId":"14096","inReplyTo":"20080623233146.GP13395@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-24T01:27:11Z","receivedAt":"2008-06-24T01:27:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 24, 2008 at 01:31:46AM +0200, Pierre Habouzit wrote:\n\n> Are we sure argv[argc] is NULL when those are main() arguments ? If\n> yes there is no issue, and I should read posix more carefully, but I was\n> under the impression that POSIX wasn't enforcing that.\n\nIt's not POSIX; it's actually in the C standard. 5.1.2.2.1, paragraph 2:\n\n \"argv[argc] shall be a null pointer\"\n\n-Peff\n"},{"id":"80833","messageId":"20080624053504.GB19224@sigill.intra.peff.net","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806231307070.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-24T05:35:04Z","receivedAt":"2008-06-24T05:35:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2008 at 01:12:28PM -0700, Linus Torvalds wrote:\n\n> > in your options table. And if setup_revisions takes options that affect\n> > things that _aren't_ in that struct, then they probably ought to be.\n> \n> The world isn't like \"it ought to be\". It is like it is.\n> \n> If you keep on dreaming about how things \"ought to be\", you'll never \n> confront the things as they are. The fact is, turning the existing very \n> simple argument parsers into using \"parse_options()\" is just _hard_.\n> \n> Then you say that it all \"ought to be\" in those options already.\n\nDid you even read my mail? All I said \"ought to be\" is that revision\noptions \"ought to be\" affecting things in the struct. As in, when I\nconvert this to parse_options, if there are any cases which don't fall\ninto that pattern then I \"ought to be\" able to convert them as part of\nthe code cleanup. And looking over the revision parsing code, it looks\nlike everything falls into that case already.\n\nSo if you have something FACTUAL to say that contradicts that point, by\nall means do it. You might save me some work if you find a showstopper\nin my approach.\n\n> Sure. Do we have some invisible sky wizard to make it so?\n\nWell, given that I already said I would work on it, I guess that makes\nme the sky wizard. Bow down before my wizardly might!\n\nThat being said, it looks like Pierre is working on a slightly different\napproach, and I am happy to sit back and see how that pans out for now.\nSo perhaps he is the sky wizard.\n\n> I tried to do just _one_ program. Trust me, I'm not going to even bother \n> trying to do another unless parse_options() is made more palatable to do \n> in small pieces. \n\nMaybe we are fundamentally talking across each other. I'm not talking\nabout some specifics for converting a bunch of programs, or some\nproblems with parse_options. I'm suggesting that the main \"two stage\"\noption parsing generally has the revision parsing as the \"second stage\".\nConverting that one thing to parse_options will help with converting\nmany programs (e.g., a failed conversion of shortlog that others and I\nworked on).\n\nYou seem to have a bunch of _other_ problems with parse_options. And\nthat is fine, but they have nothing whatsoever to do with anything I've\nsaid. So don't \"sky wizard\" _me_ about those problems. ;P\n\n-Peff\n"},{"id":"80843","messageId":"20080624075048.GA24357@artemis.madism.org","threadId":"14096","inReplyTo":"7v8wwvg15z.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T07:50:48Z","receivedAt":"2008-06-24T07:50:48Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Jun 23, 2008 at 11:51:52PM +0000, Junio C Hamano wrote:\n> Pierre Habouzit writes:\n> \n> > Unrelated but worth to note: many parse_options users just don't care\n> > about argv[0] and having it kept each time would rather be a pain for\n> > them (they would need to call argv++, argc-- themselves).\n> \n> Not necessarily. If they were parsing by hand, they were written to deal\n> with the fact that argv[0] is not the program argument (iow, they start\n> counting from one). And before and after calling parse_options(), they\n> need to change that assumption anyway, because parse_options() makes\n> argv[0] disappear.\n\n Sure, but for _some_ commands it's easier not having to argv++,\nargc--. The patch I sent just allow the caller to ask parse_options to\nkeep argv[0] in place or not, so that one can cascade parsers if wanted.\nNote that with the series I sent, it's less useful to cascade option\nparser, you rather mix more of them in your step by step parser. But oh\nwell, it's a useful feature anyways. We can even make it default, my\nsole concern what that NULL-terminated thing, and now that I know we can\ncount on it, then, well, I absolutely don't care :)\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80854","messageId":"20080624082447.GB24357@artemis.madism.org","threadId":"14096","inReplyTo":"7vzlpbeksn.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T08:24:47Z","receivedAt":"2008-06-24T08:24:47Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Jun 24, 2008 at 12:30:48AM +0000, Junio C Hamano wrote:\n> Linus Torvalds writes:\n> \n> > On Mon, 23 Jun 2008, Linus Torvalds wrote:\n> >> \n> >> Umm. Helloo, reality.. There are actually very few options that take a \n> >> flag for their arguments. In particular, the option parsing we really \n> >> _care_ about (revision parsing - see builtin-blame.c which is exactly \n> >> where I wanted to convert things) very much DOES NOT.\n> >\n> > Actually, I guess \"--default\" does, but if you try to mix that up with (a) \n> > a default head that starts with a dash and (b) git-blame, you're doing \n> > something pretty odd.\n> >\n> > And yes, \"-n\" does too, but if you pass it negative numbers you get what \n> > you deserve.\n> >\n> > The point being, we really _do_ have a real-life existing case for \n> > PARSE_OPT_CONTINUE_ON_UNKNOWN, which is hard to handle any other way. \n> > Currently you can literally do\n> >\n> > \tgit blame --since=April -b Makefile\n> >\n> > and while it's a totally made-up example, it's one I've picked to show \n> > exactly what does *not* work with my patch that started this whole thread.\n> >\n> > And guess what you need to fix it?\n> >\n> > If you guessed PARSE_OPT_CONTINUE_ON_UNKNOWN, you win a prize. \n> \n> With this on top of Pierre's series, and adding PARSE_OPT_SKIP_UNKNOWN to\n> the obvious place in your patch, \"blame --since=April -b Makefile\" would\n> work.\n> \n> -- >8 --\n> Subject: [PATCH] parse-options: PARSE_OPT_SKIP_UNKNOWN\n> \n> ---\n> parse-options.c | 14 ++++++++++----\n> parse-options.h | 1 +\n> 2 files changed, 11 insertions(+), 4 deletions(-)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index 71a8056..9f1eb65 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -341,20 +341,26 @@ int parse_options(int argc, const char **argv, const struct option *options,\n> \tstruct parse_opt_ctx_t ctx;\n> \n> \tparse_options_start(&ctx, argc, argv, flags);\n> +\n> + again:\n> \tswitch (parse_options_step(&ctx, options, usagestr)) {\n> \tcase PARSE_OPT_HELP:\n> \t\texit(129);\n> \tcase PARSE_OPT_DONE:\n> \t\tbreak;\n> \tdefault: /* PARSE_OPT_UNKNOWN */\n> -\t\tif (ctx.argv[0][1] == '-') {\n> +\t\tif (flags & PARSE_OPT_KEEP_UNKNOWN) {\n> +\t\t\tctx.out[ctx.cpidx++] = ctx.argv[0];\n\n Actually this doesn't work because it may point into the strbuf that\nwill be invalidated later. Our sole option here is to leak some memory I\nfear.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80860","messageId":"1214298732-6247-1-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806222207220.2926@woody.linux-foundation.org","subject":"Making parse-opt incremental, reworked series","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T09:12:05Z","receivedAt":"2008-06-24T09:12:05Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Here is a cleaned up series, with Junio's fixes, and some more.\n\nThe patch 7 is only a proof of concept, not meant to be merged at all, I\nbelieve the rest is ready enough for now.\n\nI'll try to do what I comment in patch 7/7 (reworking revisions option\nparsing to be incremental as well) as soon as I have some time for it.\n"},{"id":"80864","messageId":"1214298732-6247-2-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"1214298732-6247-1-git-send-email-madcoder@debian.org","subject":"[PATCH 1/7] parse-opt: have parse_options_{start,end}.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T09:12:06Z","receivedAt":"2008-06-24T09:12:06Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Make the struct optparse_t public under the better name parse_opt_ctx_t.\n\nSigned-off-by: Pierre Habouzit \n---\n parse-options.c | 69 +++++++++++++++++++++++++++++++------------------------\n parse-options.h | 16 ++++++++++++\n 2 files changed, 55 insertions(+), 30 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex b8bde2b..774e647 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -4,14 +4,7 @@\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n-struct optparse_t {\n-\tconst char **argv;\n-\tconst char **out;\n-\tint argc, cpidx;\n-\tconst char *opt;\n-};\n-\n-static inline const char *get_arg(struct optparse_t *p)\n+static inline const char *get_arg(struct parse_opt_ctx_t *p)\n {\n \tif (p->opt) {\n \t\tconst char *res = p->opt;\n@@ -37,7 +30,7 @@ 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 int get_value(struct optparse_t *p,\n+static int get_value(struct parse_opt_ctx_t *p,\n const struct option *opt, int flags)\n {\n \tconst char *s, *arg;\n@@ -131,7 +124,7 @@ 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 parse_opt_ctx_t *p, const struct option *options)\n {\n \tfor (; options->type != OPTION_END; options++) {\n \t\tif (options->short_name == *p->opt) {\n@@ -142,7 +135,7 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)\n \treturn error(\"unknown switch `%c'\", *p->opt);\n }\n \n-static int parse_long_opt(struct optparse_t *p, const char *arg,\n+static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,\n const struct option *options)\n {\n \tconst char *arg_end = strchr(arg, '=');\n@@ -247,45 +240,63 @@ void check_typos(const char *arg, const struct option *options)\n \t}\n }\n \n+void parse_options_start(struct parse_opt_ctx_t *ctx,\n+ int argc, const char **argv, int flags)\n+{\n+\tmemset(ctx, 0, sizeof(*ctx));\n+\tctx->argc = argc - 1;\n+\tctx->argv = argv + 1;\n+\tctx->out = argv;\n+\tctx->flags = flags;\n+}\n+\n+int parse_options_end(struct parse_opt_ctx_t *ctx)\n+{\n+\tmemmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));\n+\tctx->out[ctx->cpidx + ctx->argc] = NULL;\n+\treturn ctx->cpidx + ctx->argc;\n+}\n+\n static NORETURN void usage_with_options_internal(const char * const *,\n const struct option *, int);\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, argv, argc - 1, 0, NULL };\n+\tstruct parse_opt_ctx_t ctx;\n \n-\tfor (; args.argc; args.argc--, args.argv++) {\n-\t\tconst char *arg = args.argv[0];\n+\tparse_options_start(&ctx, argc, argv, flags);\n+\tfor (; ctx.argc; ctx.argc--, ctx.argv++) {\n+\t\tconst char *arg = ctx.argv[0];\n \n \t\tif (*arg != '-' || !arg[1]) {\n-\t\t\tif (flags & PARSE_OPT_STOP_AT_NON_OPTION)\n+\t\t\tif (ctx.flags & PARSE_OPT_STOP_AT_NON_OPTION)\n \t\t\t\tbreak;\n-\t\t\targs.out[args.cpidx++] = args.argv[0];\n+\t\t\tctx.out[ctx.cpidx++] = ctx.argv[0];\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (arg[1] != '-') {\n-\t\t\targs.opt = arg + 1;\n-\t\t\tif (*args.opt == 'h')\n+\t\t\tctx.opt = arg + 1;\n+\t\t\tif (*ctx.opt == 'h')\n \t\t\t\tusage_with_options(usagestr, options);\n-\t\t\tif (parse_short_opt(&args, options) < 0)\n+\t\t\tif (parse_short_opt(&ctx, options) < 0)\n \t\t\t\tusage_with_options(usagestr, options);\n-\t\t\tif (args.opt)\n+\t\t\tif (ctx.opt)\n \t\t\t\tcheck_typos(arg + 1, options);\n-\t\t\twhile (args.opt) {\n-\t\t\t\tif (*args.opt == 'h')\n+\t\t\twhile (ctx.opt) {\n+\t\t\t\tif (*ctx.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(&ctx, options) < 0)\n \t\t\t\t\tusage_with_options(usagestr, options);\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (!arg[2]) { /* \"--\" */\n-\t\t\tif (!(flags & PARSE_OPT_KEEP_DASHDASH)) {\n-\t\t\t\targs.argc--;\n-\t\t\t\targs.argv++;\n+\t\t\tif (!(ctx.flags & PARSE_OPT_KEEP_DASHDASH)) {\n+\t\t\t\tctx.argc--;\n+\t\t\t\tctx.argv++;\n \t\t\t}\n \t\t\tbreak;\n \t\t}\n@@ -294,13 +305,11 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \t\t\tusage_with_options_internal(usagestr, options, 1);\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(&ctx, arg + 2, options))\n \t\t\tusage_with_options(usagestr, options);\n \t}\n \n-\tmemmove(args.out + args.cpidx, args.argv, args.argc * sizeof(*args.out));\n-\targs.out[args.cpidx + args.argc] = NULL;\n-\treturn args.cpidx + args.argc;\n+\treturn parse_options_end(&ctx);\n }\n \n #define USAGE_OPTS_WIDTH 24\ndiff --git a/parse-options.h b/parse-options.h\nindex 4ee443d..db6c986 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -111,6 +111,22 @@ extern int parse_options(int argc, const char **argv,\n extern NORETURN void usage_with_options(const char * const *usagestr,\n const struct option *options);\n \n+/*----- incremantal advanced APIs -----*/\n+\n+struct parse_opt_ctx_t {\n+\tconst char **argv;\n+\tconst char **out;\n+\tint argc, cpidx;\n+\tconst char *opt;\n+\tint flags;\n+};\n+\n+extern void parse_options_start(struct parse_opt_ctx_t *ctx,\n+ int argc, const char **argv, int flags);\n+\n+extern int parse_options_end(struct parse_opt_ctx_t *ctx);\n+\n+\n /*----- some often used options -----*/\n extern int parse_opt_abbrev_cb(const struct option *, const char *, int);\n extern int parse_opt_approxidate_cb(const struct option *, const char *, int);\n-- \n1.5.6.110.g736c7.dirty\n"},{"id":"80859","messageId":"1214298732-6247-3-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"1214298732-6247-2-git-send-email-madcoder@debian.org","subject":"[PATCH 2/7] parse-opt: Export a non NORETURN usage dumper.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T09:12:07Z","receivedAt":"2008-06-24T09:12:07Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Signed-off-by: Pierre Habouzit \n---\n parse-options.c | 24 +++++++++++++++++-------\n parse-options.h | 9 +++++++++\n 2 files changed, 26 insertions(+), 7 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 774e647..4e9e675 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -257,8 +257,8 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n \treturn ctx->cpidx + ctx->argc;\n }\n \n-static NORETURN void usage_with_options_internal(const char * const *,\n- const struct option *, int);\n+static int usage_with_options_internal(const char * const *,\n+ const struct option *, int, int);\n \n int parse_options(int argc, const char **argv, const struct option *options,\n const char * const usagestr[], int flags)\n@@ -302,7 +302,7 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \t\t}\n \n \t\tif (!strcmp(arg + 2, \"help-all\"))\n-\t\t\tusage_with_options_internal(usagestr, options, 1);\n+\t\t\tusage_with_options_internal(usagestr, options, 1, 1);\n \t\tif (!strcmp(arg + 2, \"help\"))\n \t\t\tusage_with_options(usagestr, options);\n \t\tif (parse_long_opt(&ctx, arg + 2, options))\n@@ -315,8 +315,8 @@ 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_internal(const char * const *usagestr,\n- const struct option *opts, int full)\n+int usage_with_options_internal(const char * const *usagestr,\n+ const struct option *opts, int full, int do_exit)\n {\n \tfprintf(stderr, \"usage: %s\\n\", *usagestr++);\n \twhile (*usagestr && **usagestr)\n@@ -401,15 +401,25 @@ void usage_with_options_internal(const char * const *usagestr,\n \t}\n \tfputc('\\n', stderr);\n \n-\texit(129);\n+\tif (do_exit)\n+\t\texit(129);\n+\treturn PARSE_OPT_HELP;\n }\n \n void usage_with_options(const char * const *usagestr,\n const struct option *opts)\n {\n-\tusage_with_options_internal(usagestr, opts, 0);\n+\tusage_with_options_internal(usagestr, opts, 0, 1);\n+\texit(129); /* make gcc happy */\n+}\n+\n+int parse_options_usage(const char * const *usagestr,\n+ const struct option *opts)\n+{\n+\treturn usage_with_options_internal(usagestr, opts, 0, 0);\n }\n \n+\n /*----- some often used options -----*/\n #include \"cache.h\"\n \ndiff --git a/parse-options.h b/parse-options.h\nindex db6c986..424ea46 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -113,6 +113,12 @@ extern NORETURN void usage_with_options(const char * const *usagestr,\n \n /*----- incremantal advanced APIs -----*/\n \n+enum {\n+\tPARSE_OPT_HELP = -1,\n+\tPARSE_OPT_DONE,\n+\tPARSE_OPT_UNKNOWN,\n+};\n+\n struct parse_opt_ctx_t {\n \tconst char **argv;\n \tconst char **out;\n@@ -121,6 +127,9 @@ struct parse_opt_ctx_t {\n \tint flags;\n };\n \n+extern int parse_options_usage(const char * const *usagestr,\n+ const struct option *opts);\n+\n extern void parse_options_start(struct parse_opt_ctx_t *ctx,\n int argc, const char **argv, int flags);\n \n-- \n1.5.6.110.g736c7.dirty\n"},{"id":"80858","messageId":"1214298732-6247-4-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"1214298732-6247-3-git-send-email-madcoder@debian.org","subject":"[PATCH 3/7] parse-opt: create parse_options_step.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T09:12:08Z","receivedAt":"2008-06-24T09:12:08Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"For now it's unable to stop at unknown options, this commit merely\nreorganize some code around.\n\nSigned-off-by: Pierre Habouzit \n---\n parse-options.c | 89 +++++++++++++++++++++++++++++++-----------------------\n parse-options.h | 4 ++\n 2 files changed, 55 insertions(+), 38 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 4e9e675..71b3476 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -250,64 +250,79 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tctx->flags = flags;\n }\n \n-int parse_options_end(struct parse_opt_ctx_t *ctx)\n-{\n-\tmemmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));\n-\tctx->out[ctx->cpidx + ctx->argc] = NULL;\n-\treturn ctx->cpidx + ctx->argc;\n-}\n-\n static int usage_with_options_internal(const char * const *,\n- const struct option *, int, int);\n+ const struct option *, int);\n \n-int parse_options(int argc, const char **argv, const struct option *options,\n- const char * const usagestr[], int flags)\n+int parse_options_step(struct parse_opt_ctx_t *ctx,\n+ const struct option *options,\n+ const char * const usagestr[])\n {\n-\tstruct parse_opt_ctx_t ctx;\n-\n-\tparse_options_start(&ctx, argc, argv, flags);\n-\tfor (; ctx.argc; ctx.argc--, ctx.argv++) {\n-\t\tconst char *arg = ctx.argv[0];\n+\tfor (; ctx->argc; ctx->argc--, ctx->argv++) {\n+\t\tconst char *arg = ctx->argv[0];\n \n \t\tif (*arg != '-' || !arg[1]) {\n-\t\t\tif (ctx.flags & PARSE_OPT_STOP_AT_NON_OPTION)\n+\t\t\tif (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)\n \t\t\t\tbreak;\n-\t\t\tctx.out[ctx.cpidx++] = ctx.argv[0];\n+\t\t\tctx->out[ctx->cpidx++] = ctx->argv[0];\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (arg[1] != '-') {\n-\t\t\tctx.opt = arg + 1;\n-\t\t\tif (*ctx.opt == 'h')\n-\t\t\t\tusage_with_options(usagestr, options);\n-\t\t\tif (parse_short_opt(&ctx, options) < 0)\n+\t\t\tctx->opt = arg + 1;\n+\t\t\tif (*ctx->opt == 'h')\n+\t\t\t\treturn parse_options_usage(usagestr, options);\n+\t\t\tif (parse_short_opt(ctx, options) < 0)\n \t\t\t\tusage_with_options(usagestr, options);\n-\t\t\tif (ctx.opt)\n+\t\t\tif (ctx->opt)\n \t\t\t\tcheck_typos(arg + 1, options);\n-\t\t\twhile (ctx.opt) {\n-\t\t\t\tif (*ctx.opt == 'h')\n-\t\t\t\t\tusage_with_options(usagestr, options);\n-\t\t\t\tif (parse_short_opt(&ctx, options) < 0)\n+\t\t\twhile (ctx->opt) {\n+\t\t\t\tif (*ctx->opt == 'h')\n+\t\t\t\t\treturn parse_options_usage(usagestr, options);\n+\t\t\t\tif (parse_short_opt(ctx, options) < 0)\n \t\t\t\t\tusage_with_options(usagestr, options);\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (!arg[2]) { /* \"--\" */\n-\t\t\tif (!(ctx.flags & PARSE_OPT_KEEP_DASHDASH)) {\n-\t\t\t\tctx.argc--;\n-\t\t\t\tctx.argv++;\n+\t\t\tif (!(ctx->flags & PARSE_OPT_KEEP_DASHDASH)) {\n+\t\t\t\tctx->argc--;\n+\t\t\t\tctx->argv++;\n \t\t\t}\n \t\t\tbreak;\n \t\t}\n \n \t\tif (!strcmp(arg + 2, \"help-all\"))\n-\t\t\tusage_with_options_internal(usagestr, options, 1, 1);\n+\t\t\treturn usage_with_options_internal(usagestr, options, 1);\n \t\tif (!strcmp(arg + 2, \"help\"))\n-\t\t\tusage_with_options(usagestr, options);\n-\t\tif (parse_long_opt(&ctx, arg + 2, options))\n+\t\t\treturn parse_options_usage(usagestr, options);\n+\t\tif (parse_long_opt(ctx, arg + 2, options))\n \t\t\tusage_with_options(usagestr, options);\n \t}\n+\treturn PARSE_OPT_DONE;\n+}\n+\n+int parse_options_end(struct parse_opt_ctx_t *ctx)\n+{\n+\tmemmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));\n+\tctx->out[ctx->cpidx + ctx->argc] = NULL;\n+\treturn ctx->cpidx + ctx->argc;\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 parse_opt_ctx_t ctx;\n+\n+\tparse_options_start(&ctx, argc, argv, flags);\n+\tswitch (parse_options_step(&ctx, options, usagestr)) {\n+\tcase PARSE_OPT_HELP:\n+\t\texit(129);\n+\tcase PARSE_OPT_DONE:\n+\t\tbreak;\n+\tdefault: /* PARSE_OPT_UNKNOWN */\n+\t\tabort(); /* unreached yet */\n+\t}\n \n \treturn parse_options_end(&ctx);\n }\n@@ -316,7 +331,7 @@ int parse_options(int argc, const char **argv, const struct option *options,\n #define USAGE_GAP 2\n \n int usage_with_options_internal(const char * const *usagestr,\n- const struct option *opts, int full, int do_exit)\n+ const struct option *opts, int full)\n {\n \tfprintf(stderr, \"usage: %s\\n\", *usagestr++);\n \twhile (*usagestr && **usagestr)\n@@ -401,22 +416,20 @@ int usage_with_options_internal(const char * const *usagestr,\n \t}\n \tfputc('\\n', stderr);\n \n-\tif (do_exit)\n-\t\texit(129);\n \treturn PARSE_OPT_HELP;\n }\n \n void usage_with_options(const char * const *usagestr,\n const struct option *opts)\n {\n-\tusage_with_options_internal(usagestr, opts, 0, 1);\n-\texit(129); /* make gcc happy */\n+\tusage_with_options_internal(usagestr, opts, 0);\n+\texit(129);\n }\n \n int parse_options_usage(const char * const *usagestr,\n const struct option *opts)\n {\n-\treturn usage_with_options_internal(usagestr, opts, 0, 0);\n+\treturn usage_with_options_internal(usagestr, opts, 0);\n }\n \n \ndiff --git a/parse-options.h b/parse-options.h\nindex 424ea46..9da5e8c 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -133,6 +133,10 @@ extern int parse_options_usage(const char * const *usagestr,\n extern void parse_options_start(struct parse_opt_ctx_t *ctx,\n int argc, const char **argv, int flags);\n \n+extern int parse_options_step(struct parse_opt_ctx_t *ctx,\n+ const struct option *options,\n+ const char * const usagestr[]);\n+\n extern int parse_options_end(struct parse_opt_ctx_t *ctx);\n \n \n-- \n1.5.6.110.g736c7.dirty\n"},{"id":"80861","messageId":"1214298732-6247-5-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"1214298732-6247-4-git-send-email-madcoder@debian.org","subject":"[PATCH 4/7] parse-opt: do not pring errors on unknown options, return -2 intead.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T09:12:09Z","receivedAt":"2008-06-24T09:12:09Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"This way we can catch \"unknown\" options more easily.\n\nSigned-off-by: Pierre Habouzit \n---\n parse-options.c | 43 ++++++++++++++++++++++++++++++-------------\n 1 files changed, 30 insertions(+), 13 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 71b3476..90935f3 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -94,14 +94,14 @@ static int get_value(struct parse_opt_ctx_t *p,\n \n \tcase OPTION_CALLBACK:\n \t\tif (unset)\n-\t\t\treturn (*opt->callback)(opt, NULL, 1);\n+\t\t\treturn (*opt->callback)(opt, NULL, 1) ? (-1) : 0;\n \t\tif (opt->flags & PARSE_OPT_NOARG)\n-\t\t\treturn (*opt->callback)(opt, NULL, 0);\n+\t\t\treturn (*opt->callback)(opt, NULL, 0) ? (-1) : 0;\n \t\tif (opt->flags & PARSE_OPT_OPTARG && !p->opt)\n-\t\t\treturn (*opt->callback)(opt, NULL, 0);\n+\t\t\treturn (*opt->callback)(opt, NULL, 0) ? (-1) : 0;\n \t\tif (!arg)\n \t\t\treturn opterror(opt, \"requires a value\", flags);\n-\t\treturn (*opt->callback)(opt, get_arg(p), 0);\n+\t\treturn (*opt->callback)(opt, get_arg(p), 0) ? (-1) : 0;\n \n \tcase OPTION_INTEGER:\n \t\tif (unset) {\n@@ -132,7 +132,7 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *optio\n \t\t\treturn get_value(p, options, OPT_SHORT);\n \t\t}\n \t}\n-\treturn error(\"unknown switch `%c'\", *p->opt);\n+\treturn -2;\n }\n \n static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,\n@@ -217,7 +217,7 @@ 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 -2;\n }\n \n void check_typos(const char *arg, const struct option *options)\n@@ -271,15 +271,23 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tctx->opt = arg + 1;\n \t\t\tif (*ctx->opt == 'h')\n \t\t\t\treturn parse_options_usage(usagestr, options);\n-\t\t\tif (parse_short_opt(ctx, options) < 0)\n-\t\t\t\tusage_with_options(usagestr, options);\n+\t\t\tswitch (parse_short_opt(ctx, options)) {\n+\t\t\tcase -1:\n+\t\t\t\treturn parse_options_usage(usagestr, options);\n+\t\t\tcase -2:\n+\t\t\t\treturn PARSE_OPT_UNKNOWN;\n+\t\t\t}\n \t\t\tif (ctx->opt)\n \t\t\t\tcheck_typos(arg + 1, options);\n \t\t\twhile (ctx->opt) {\n \t\t\t\tif (*ctx->opt == 'h')\n \t\t\t\t\treturn parse_options_usage(usagestr, options);\n-\t\t\t\tif (parse_short_opt(ctx, options) < 0)\n-\t\t\t\t\tusage_with_options(usagestr, options);\n+\t\t\t\tswitch (parse_short_opt(ctx, options)) {\n+\t\t\t\tcase -1:\n+\t\t\t\t\treturn parse_options_usage(usagestr, options);\n+\t\t\t\tcase -2:\n+\t\t\t\t\treturn PARSE_OPT_UNKNOWN;\n+\t\t\t\t}\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n@@ -296,8 +304,12 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\treturn usage_with_options_internal(usagestr, options, 1);\n \t\tif (!strcmp(arg + 2, \"help\"))\n \t\t\treturn parse_options_usage(usagestr, options);\n-\t\tif (parse_long_opt(ctx, arg + 2, options))\n-\t\t\tusage_with_options(usagestr, options);\n+\t\tswitch (parse_long_opt(ctx, arg + 2, options)) {\n+\t\tcase -1:\n+\t\t\treturn parse_options_usage(usagestr, options);\n+\t\tcase -2:\n+\t\t\treturn PARSE_OPT_UNKNOWN;\n+\t\t}\n \t}\n \treturn PARSE_OPT_DONE;\n }\n@@ -321,7 +333,12 @@ int parse_options(int argc, const char **argv, const struct option *options,\n \tcase PARSE_OPT_DONE:\n \t\tbreak;\n \tdefault: /* PARSE_OPT_UNKNOWN */\n-\t\tabort(); /* unreached yet */\n+\t\tif (ctx.argv[0][1] == '-') {\n+\t\t\terror(\"unknown option `%s'\", ctx.argv[0] + 2);\n+\t\t} else {\n+\t\t\terror(\"unknown switch `%c'\", *ctx.opt);\n+\t\t}\n+\t\tusage_with_options(usagestr, options);\n \t}\n \n \treturn parse_options_end(&ctx);\n-- \n1.5.6.110.g736c7.dirty\n"},{"id":"80865","messageId":"1214298732-6247-6-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"1214298732-6247-5-git-send-email-madcoder@debian.org","subject":"[PATCH 5/7] parse-opt: fake short strings for callers to believe in.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T09:12:10Z","receivedAt":"2008-06-24T09:12:10Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"If we begin to parse -abc and that the parser knew about -a and -b, it\nwill fake a -c switch for the caller to deal with.\n\nOf course in the case of -acb (supposing -c is not taking an argument) the\ncaller will have to be especially clever to do the same thing. We could\nthink about exposing an API to do so if it's really needed, but oh well...\n\nSigned-off-by: Pierre Habouzit \n---\n parse-options.c | 12 ++++++++++++\n parse-options.h | 12 ++++++++++++\n 2 files changed, 24 insertions(+), 0 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 90935f3..ee82cf3 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -248,6 +248,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tctx->argv = argv + 1;\n \tctx->out = argv;\n \tctx->flags = flags;\n+\tstrbuf_init(&ctx->buf, 0);\n }\n \n static int usage_with_options_internal(const char * const *,\n@@ -257,6 +258,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n const struct option *options,\n const char * const usagestr[])\n {\n+\t/* we must reset ->opt, unknown short option leave it dangling */\n+\tctx->opt = NULL;\n+\n \tfor (; ctx->argc; ctx->argc--, ctx->argv++) {\n \t\tconst char *arg = ctx->argv[0];\n \n@@ -286,6 +290,13 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\tcase -1:\n \t\t\t\t\treturn parse_options_usage(usagestr, options);\n \t\t\t\tcase -2:\n+\t\t\t\t\t/* fake a short option thing to hide the fact that we may have\n+\t\t\t\t\t * started to parse aggregated stuff\n+\t\t\t\t\t */\n+\t\t\t\t\tstrbuf_reset(&ctx->buf);\n+\t\t\t\t\tstrbuf_addch(&ctx->buf, '-');\n+\t\t\t\t\tstrbuf_addstr(&ctx->buf, ctx->opt);\n+\t\t\t\t\t*ctx->argv = ctx->buf.buf;\n \t\t\t\t\treturn PARSE_OPT_UNKNOWN;\n \t\t\t\t}\n \t\t\t}\n@@ -318,6 +329,7 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n {\n \tmemmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));\n \tctx->out[ctx->cpidx + ctx->argc] = NULL;\n+\tstrbuf_release(&ctx->buf);\n \treturn ctx->cpidx + ctx->argc;\n }\n \ndiff --git a/parse-options.h b/parse-options.h\nindex 9da5e8c..14447d5 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -1,6 +1,8 @@\n #ifndef PARSE_OPTIONS_H\n #define PARSE_OPTIONS_H\n \n+#include \"strbuf.h\"\n+\n enum parse_opt_type {\n \t/* special types */\n \tOPTION_END,\n@@ -119,12 +121,18 @@ enum {\n \tPARSE_OPT_UNKNOWN,\n };\n \n+/*\n+ * It's okay for the caller to consume argv/argc in the usual way.\n+ * Other fields of that structure are private to parse-options and should not\n+ * be modified in any way.\n+ */\n struct parse_opt_ctx_t {\n \tconst char **argv;\n \tconst char **out;\n \tint argc, cpidx;\n \tconst char *opt;\n \tint flags;\n+\tstruct strbuf buf;\n };\n \n extern int parse_options_usage(const char * const *usagestr,\n@@ -133,6 +141,10 @@ extern int parse_options_usage(const char * const *usagestr,\n extern void parse_options_start(struct parse_opt_ctx_t *ctx,\n int argc, const char **argv, int flags);\n \n+/* Warning: you cannot keep pointers to ctx->argv during the parse\n+ * because some \"option strings\" are faked. It's okay to use\n+ * ctx->argv after a parse_options_end obviously\n+ */\n extern int parse_options_step(struct parse_opt_ctx_t *ctx,\n const struct option *options,\n const char * const usagestr[]);\n-- \n1.5.6.110.g736c7.dirty\n"},{"id":"80862","messageId":"1214298732-6247-7-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"1214298732-6247-6-git-send-email-madcoder@debian.org","subject":"[PATCH 6/7] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T09:12:11Z","receivedAt":"2008-06-24T09:12:11Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"This way, argv[0] isn't clobbered, to the cost of maybe not having a\nresulting NULL terminated argv array.\n\nSigned-off-by: Pierre Habouzit \n---\n parse-options.c | 7 ++++---\n parse-options.h | 2 ++\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex ee82cf3..a6b5e04 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -246,7 +246,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tmemset(ctx, 0, sizeof(*ctx));\n \tctx->argc = argc - 1;\n \tctx->argv = argv + 1;\n-\tctx->out = argv;\n+\tctx->out = argv + ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n \tctx->flags = flags;\n \tstrbuf_init(&ctx->buf, 0);\n }\n@@ -327,10 +327,11 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \n int parse_options_end(struct parse_opt_ctx_t *ctx)\n {\n+\tint res = ctx->cpidx + ctx->argc;\n \tmemmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));\n-\tctx->out[ctx->cpidx + ctx->argc] = NULL;\n+\tctx->out[res] = NULL;\n \tstrbuf_release(&ctx->buf);\n-\treturn ctx->cpidx + ctx->argc;\n+\treturn res + ((ctx->flags & PARSE_OPT_KEEP_ARGV0) != 0);\n }\n \n int parse_options(int argc, const char **argv, const struct option *options,\ndiff --git a/parse-options.h b/parse-options.h\nindex 14447d5..6745c7d 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -22,6 +22,8 @@ enum parse_opt_type {\n enum parse_opt_flags {\n \tPARSE_OPT_KEEP_DASHDASH = 1,\n \tPARSE_OPT_STOP_AT_NON_OPTION = 2,\n+\t/* using that option, the filtered argv may not be NULL terminated */\n+\tPARSE_OPT_KEEP_ARGV0 = 4,\n };\n \n enum parse_opt_option_flags {\n-- \n1.5.6.110.g736c7.dirty\n"},{"id":"80863","messageId":"1214298732-6247-8-git-send-email-madcoder@debian.org","threadId":"14096","inReplyTo":"1214298732-6247-7-git-send-email-madcoder@debian.org","subject":"[PATCH 7/7] Migrate git-blame to parse-option partially.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T09:12:12Z","receivedAt":"2008-06-24T09:12:12Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Signed-off-by: Pierre Habouzit \n---\n\n This patch is not really nice yet, but shows what we can do on top of the\n rest of the series. I shamelessly \"stole\" most of the code from Linus'\n patch.\n\n The nasty strdup can be removed safely using my setup_revisions split into\n setup_revisions and parse_revisions, by initializing revisions _before_\n actualy option parsing, making parse_revisions incremental (IOW eating\n options one at a time only) and do the whole thing in one single pass.\n\n IOW, one would see a inner loop like:\n\n+ for (;;) {\n+ int res;\n+\n+ switch (parse_options_step(&ctx, options, blame_opt_usage)) {\n+ case PARSE_OPT_HELP:\n+ exit(129);\n+ case PARSE_OPT_DONE:\n+ goto parse_done;\n+ }\n+\n+ if (!strcmp(ctx.argv[0], \"--reverse\")) {\n+ reverse = 1;\n+ ctx.argv[0] = \"--children\";\n+ }\n+ res = parse_revisions(&revs, ctx.argc, ctx.argv);\n+ if (res <= 0) {\n+ if (res == 0)\n+ error(\"unknown option %s\", ctx.argv[0]);\n+ usage_with_options(options, blame_opt_usage);\n+ }\n+ ctx.argv += j;\n+ ctx.argc -= j;\n+ }\n\n But despite being quite ugly (because of the cpidx fiddling and the strdup),\n this patch works as intended.\n\n builtin-blame.c | 195 ++++++++++++++++++++++++++++---------------------------\n 1 files changed, 100 insertions(+), 95 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex cf41511..632a28d 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -18,24 +18,11 @@\n #include \"cache-tree.h\"\n #include \"path-list.h\"\n #include \"mailmap.h\"\n+#include \"parse-options.h\"\n \n-static char blame_usage[] =\n-\"git-blame [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-p] [-w] [-L n,m] [-S ] [-M] [-C] [-C] [--contents ] [--incremental] [commit] [--] file\\n\"\n-\" -c Use the same output mode as git-annotate (Default: off)\\n\"\n-\" -b Show blank SHA-1 for boundary commits (Default: off)\\n\"\n-\" -l Show long commit SHA1 (Default: off)\\n\"\n-\" --root Do not treat root commits as boundaries (Default: off)\\n\"\n-\" -t Show raw timestamp (Default: off)\\n\"\n-\" -f, --show-name Show original filename (Default: auto)\\n\"\n-\" -n, --show-number Show original linenumber (Default: off)\\n\"\n-\" -s Suppress author name and timestamp (Default: off)\\n\"\n-\" -p, --porcelain Show in a format designed for machine consumption\\n\"\n-\" -w Ignore whitespace differences\\n\"\n-\" -L n,m Process only line range n,m, counting from 1\\n\"\n-\" -M, -C Find line movements within and across files\\n\"\n-\" --incremental Show blame entries as we find them, incrementally\\n\"\n-\" --contents file Use 's contents as the final image\\n\"\n-\" -S revs-file Use revisions from revs-file instead of calling git-rev-list\\n\";\n+static char blame_usage[] = \"git-blame [options] [--] file\";\n+\n+static const char *blame_opt_usage[] = { blame_usage, NULL };\n \n static int longest_file;\n static int longest_author;\n@@ -2219,6 +2206,50 @@ static const char *prepare_initial(struct scoreboard *sb)\n return final_commit_name;\n }\n \n+static int blame_copy_callback(const struct option *option, const char *arg, int unset)\n+{\n+\tint *opt = option->value;\n+\n+\t/*\n+\t * -C enables copy from removed files;\n+\t * -C -C enables copy from existing files, but only\n+\t * when blaming a new file;\n+\t * -C -C -C enables copy from existing files for\n+\t * everybody\n+\t */\n+\tif (*opt & PICKAXE_BLAME_COPY_HARDER)\n+\t\t*opt |= PICKAXE_BLAME_COPY_HARDEST;\n+\tif (*opt & PICKAXE_BLAME_COPY)\n+\t\t*opt |= PICKAXE_BLAME_COPY_HARDER;\n+\t*opt |= PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE;\n+\n+\tif (arg)\n+\t\tblame_copy_score = parse_score(arg);\n+\treturn 0;\n+}\n+\n+static int blame_move_callback(const struct option *option, const char *arg, int unset)\n+{\n+\tint *opt = option->value;\n+\n+\t*opt |= PICKAXE_BLAME_MOVE;\n+\n+\tif (arg)\n+\t\tblame_move_score = parse_score(arg);\n+\treturn 0;\n+}\n+\n+static int blame_bottomtop_callback(const struct option *option, const char *arg, int unset)\n+{\n+\tconst char **bottomtop = option->value;\n+\tif (!arg)\n+\t\treturn -1;\n+\tif (*bottomtop)\n+\t\tdie(\"More than one '-L n,m' option given\");\n+\t*bottomtop = arg;\n+\treturn 0;\n+}\n+\n int cmd_blame(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n@@ -2226,98 +2257,72 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tstruct scoreboard sb;\n \tstruct origin *o;\n \tstruct blame_entry *ent;\n-\tint i, seen_dashdash, unk, opt;\n+\tint i, seen_dashdash, unk;\n \tlong bottom, top, lno;\n-\tint output_option = 0;\n-\tint show_stats = 0;\n-\tconst char *revs_file = NULL;\n \tconst char *final_commit_name = NULL;\n \tenum object_type type;\n-\tconst char *bottomtop = NULL;\n-\tconst char *contents_from = NULL;\n+\n+\tstatic const char *bottomtop = NULL;\n+\tstatic int output_option = 0, opt = 0;\n+\tstatic int show_stats = 0;\n+\tstatic const char *revs_file = NULL;\n+\tstatic const char *contents_from = NULL;\n+\tstatic const struct option options[] = {\n+\t\tOPT_BOOLEAN(0, \"incremental\", &incremental, \"Show blame entries as we find them, incrementally\"),\n+\t\tOPT_BOOLEAN('b', NULL, &blank_boundary, \"Show blank SHA-1 for boundary commits (Default: off)\"),\n+\t\tOPT_BOOLEAN(0, \"root\", &show_root, \"Do not treat root commits as boundaries (Default: off)\"),\n+\t\tOPT_BOOLEAN(0, \"show-stats\", &show_stats, \"Show work cost statistics\"),\n+\t\tOPT_BIT(0, \"score-debug\", &output_option, \"Show output score for blame entries\", OUTPUT_SHOW_SCORE),\n+\t\tOPT_BIT('f', \"show-name\", &output_option, \"Show original filename (Default: auto)\", OUTPUT_SHOW_NAME),\n+\t\tOPT_BIT('n', \"show-number\", &output_option, \"Show original linenumber (Default: off)\", OUTPUT_SHOW_NUMBER),\n+\t\tOPT_BIT('p', \"porcelain\", &output_option, \"Show in a format designed for machine consumption\", OUTPUT_PORCELAIN),\n+\t\tOPT_BIT('c', NULL, &output_option, \"Use the same output mode as git-annotate (Default: off)\", OUTPUT_ANNOTATE_COMPAT),\n+\t\tOPT_BIT('t', NULL, &output_option, \"Show raw timestamp (Default: off)\", OUTPUT_RAW_TIMESTAMP),\n+\t\tOPT_BIT('l', NULL, &output_option, \"Show long commit SHA1 (Default: off)\", OUTPUT_LONG_OBJECT_NAME),\n+\t\tOPT_BIT('s', NULL, &output_option, \"Suppress author name and timestamp (Default: off)\", OUTPUT_NO_AUTHOR),\n+\t\tOPT_BIT('w', NULL, &xdl_opts, \"Ignore whitespace differences\", XDF_IGNORE_WHITESPACE),\n+\t\tOPT_STRING('S', NULL, &revs_file, \"file\", \"Use revisions from instead of calling git-rev-list\"),\n+\t\tOPT_STRING(0, \"contents\", &contents_from, \"file\", \"Use 's contents as the final image\"),\n+\t\t{ OPTION_CALLBACK, 'C', NULL, &opt, \"score\", \"Find line copies within and across files\", PARSE_OPT_OPTARG, blame_copy_callback },\n+\t\t{ OPTION_CALLBACK, 'M', NULL, &opt, \"score\", \"Find line movements within and across files\", PARSE_OPT_OPTARG, blame_move_callback },\n+\t\tOPT_CALLBACK('L', NULL, &bottomtop, \"n,m\", \"Process only line range n,m, counting from 1\", blame_bottomtop_callback),\n+\t\tOPT_END()\n+\t};\n+\n+\tstruct parse_opt_ctx_t ctx;\n \n \tcmd_is_annotate = !strcmp(argv[0], \"annotate\");\n \n \tgit_config(git_blame_config, NULL);\n \tsave_commit_buffer = 0;\n \n-\topt = 0;\n+\tparse_options_start(&ctx, argc, argv, PARSE_OPT_KEEP_DASHDASH |\n+\t PARSE_OPT_KEEP_ARGV0);\n+\tfor (;;) {\n+\t\tswitch (parse_options_step(&ctx, options, blame_opt_usage)) {\n+\t\tcase PARSE_OPT_HELP:\n+\t\t\texit(129);\n+\t\tcase PARSE_OPT_DONE:\n+\t\t\tgoto parse_done;\n+\t\t}\n+\n+\t\tif (!strcmp(ctx.argv[0], \"--reverse\")) {\n+\t\t\tctx.out[ctx.cpidx++] = \"--children\";\n+\t\t\treverse = 1;\n+\t\t} else {\n+\t\t\tctx.out[ctx.cpidx++] = strdup(ctx.argv[0]);\n+\t\t}\n+\t\tctx.argv++;\n+\t\tctx.argc--;\n+\t}\n+parse_done:\n+\targc = parse_options_end(&ctx);\n+\n \tseen_dashdash = 0;\n \tfor (unk = i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tif (*arg != '-')\n \t\t\tbreak;\n-\t\telse if (!strcmp(\"-b\", arg))\n-\t\t\tblank_boundary = 1;\n-\t\telse if (!strcmp(\"--root\", arg))\n-\t\t\tshow_root = 1;\n-\t\telse if (!strcmp(\"--reverse\", arg)) {\n-\t\t\targv[unk++] = \"--children\";\n-\t\t\treverse = 1;\n-\t\t}\n-\t\telse if (!strcmp(arg, \"--show-stats\"))\n-\t\t\tshow_stats = 1;\n-\t\telse if (!strcmp(\"-c\", arg))\n-\t\t\toutput_option |= OUTPUT_ANNOTATE_COMPAT;\n-\t\telse if (!strcmp(\"-t\", arg))\n-\t\t\toutput_option |= OUTPUT_RAW_TIMESTAMP;\n-\t\telse if (!strcmp(\"-l\", arg))\n-\t\t\toutput_option |= OUTPUT_LONG_OBJECT_NAME;\n-\t\telse if (!strcmp(\"-s\", arg))\n-\t\t\toutput_option |= OUTPUT_NO_AUTHOR;\n-\t\telse if (!strcmp(\"-w\", arg))\n-\t\t\txdl_opts |= XDF_IGNORE_WHITESPACE;\n-\t\telse if (!strcmp(\"-S\", arg) && ++i < argc)\n-\t\t\trevs_file = argv[i];\n-\t\telse if (!prefixcmp(arg, \"-M\")) {\n-\t\t\topt |= PICKAXE_BLAME_MOVE;\n-\t\t\tblame_move_score = parse_score(arg+2);\n-\t\t}\n-\t\telse if (!prefixcmp(arg, \"-C\")) {\n-\t\t\t/*\n-\t\t\t * -C enables copy from removed files;\n-\t\t\t * -C -C enables copy from existing files, but only\n-\t\t\t * when blaming a new file;\n-\t\t\t * -C -C -C enables copy from existing files for\n-\t\t\t * everybody\n-\t\t\t */\n-\t\t\tif (opt & PICKAXE_BLAME_COPY_HARDER)\n-\t\t\t\topt |= PICKAXE_BLAME_COPY_HARDEST;\n-\t\t\tif (opt & PICKAXE_BLAME_COPY)\n-\t\t\t\topt |= PICKAXE_BLAME_COPY_HARDER;\n-\t\t\topt |= PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE;\n-\t\t\tblame_copy_score = parse_score(arg+2);\n-\t\t}\n-\t\telse if (!prefixcmp(arg, \"-L\")) {\n-\t\t\tif (!arg[2]) {\n-\t\t\t\tif (++i >= argc)\n-\t\t\t\t\tusage(blame_usage);\n-\t\t\t\targ = argv[i];\n-\t\t\t}\n-\t\t\telse\n-\t\t\t\targ += 2;\n-\t\t\tif (bottomtop)\n-\t\t\t\tdie(\"More than one '-L n,m' option given\");\n-\t\t\tbottomtop = arg;\n-\t\t}\n-\t\telse if (!strcmp(\"--contents\", arg)) {\n-\t\t\tif (++i >= argc)\n-\t\t\t\tusage(blame_usage);\n-\t\t\tcontents_from = argv[i];\n-\t\t}\n-\t\telse if (!strcmp(\"--incremental\", arg))\n-\t\t\tincremental = 1;\n-\t\telse if (!strcmp(\"--score-debug\", arg))\n-\t\t\toutput_option |= OUTPUT_SHOW_SCORE;\n-\t\telse if (!strcmp(\"-f\", arg) ||\n-\t\t\t !strcmp(\"--show-name\", arg))\n-\t\t\toutput_option |= OUTPUT_SHOW_NAME;\n-\t\telse if (!strcmp(\"-n\", arg) ||\n-\t\t\t !strcmp(\"--show-number\", arg))\n-\t\t\toutput_option |= OUTPUT_SHOW_NUMBER;\n-\t\telse if (!strcmp(\"-p\", arg) ||\n-\t\t\t !strcmp(\"--porcelain\", arg))\n-\t\t\toutput_option |= OUTPUT_PORCELAIN;\n \t\telse if (!strcmp(\"--\", arg)) {\n \t\t\tseen_dashdash = 1;\n \t\t\ti++;\n-- \n1.5.6.110.g736c7.dirty\n"},{"id":"80868","messageId":"20080624100313.GC24357@artemis.madism.org","threadId":"14096","inReplyTo":"1214298732-6247-7-git-send-email-madcoder@debian.org","subject":"Re: [PATCH 6/7] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T10:03:13Z","receivedAt":"2008-06-24T10:03:13Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Jun 24, 2008 at 09:12:11AM +0000, Pierre Habouzit wrote:\n> This way, argv[0] isn't clobbered, to the cost of maybe not having a\n> resulting NULL terminated argv array.\n> \n> Signed-off-by: Pierre Habouzit \n> ---\n> parse-options.c | 7 ++++---\n> parse-options.h | 2 ++\n> 2 files changed, 6 insertions(+), 3 deletions(-)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index ee82cf3..a6b5e04 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -246,7 +246,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n> \tmemset(ctx, 0, sizeof(*ctx));\n> \tctx->argc = argc - 1;\n> \tctx->argv = argv + 1;\n> -\tctx->out = argv;\n> +\tctx->out = argv + ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n> \tctx->flags = flags;\n> \tstrbuf_init(&ctx->buf, 0);\n> }\n> @@ -327,10 +327,11 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n> \n> int parse_options_end(struct parse_opt_ctx_t *ctx)\n> {\n> +\tint res = ctx->cpidx + ctx->argc;\n> \tmemmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));\n> -\tctx->out[ctx->cpidx + ctx->argc] = NULL;\n> +\tctx->out[res] = NULL;\n> \tstrbuf_release(&ctx->buf);\n> -\treturn ctx->cpidx + ctx->argc;\n> +\treturn res + ((ctx->flags & PARSE_OPT_KEEP_ARGV0) != 0);\n> }\n> \n> int parse_options(int argc, const char **argv, const struct option *options,\n> diff --git a/parse-options.h b/parse-options.h\n> index 14447d5..6745c7d 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -22,6 +22,8 @@ enum parse_opt_type {\n> enum parse_opt_flags {\n> \tPARSE_OPT_KEEP_DASHDASH = 1,\n> \tPARSE_OPT_STOP_AT_NON_OPTION = 2,\n> +\t/* using that option, the filtered argv may not be NULL terminated */\n\n This comment is bogus and shall be stripped, I forgot to…\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80925","messageId":"alpine.LFD.1.10.0806240954150.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"20080624053504.GB19224@sigill.intra.peff.net","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-24T16:59:47Z","receivedAt":"2008-06-24T16:59:47Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 24 Jun 2008, Jeff King wrote:\n> \n> You seem to have a bunch of _other_ problems with parse_options. And\n> that is fine, but they have nothing whatsoever to do with anything I've\n> said. So don't \"sky wizard\" _me_ about those problems. ;P\n\nI have a _single_ problem I have with parse_options(), namely that it was \npainful to convert in pieces. It may well be that builtin-blame.c was one \nof the more painful cases, but it really was a _single_ issue.\n\nI also had a _single_ fix for it.\n\nI never had \"other\" problems.\n\nWhat happened was that you and Dscho and others then tried to pick that \n_single_ issue apart, because the solutions _you_ wanted (tying all the \nparsing together in one place) couldn't handle it as one issue and one \nproblem. Your solutions always looked at just some small part of it.\n\nSo no, I never introduced any other problems in the discussion at all. I \nhad a single issue, and a single solution. You were the one who then \nargued against it and had *another* solution that fractured the problem \nup, and didn't actually solve _any_ of my original issues.\n\nDo you see now?\n\nSo yes, we're arguing at cross purposes, but that's because you're \nconstantly taking up a totally different and totally uninteresting \nposition that has nothing what-so-ever to do with the original problem.\n\nAnd then you talk about how things \"ought to be\" in your world, to make \nyour solution relevant at all.\n\nAnd I'm trying to tell you that \"ought to be\" has no relevance, because \nyou're not even looking at the problem!\n\n\t\t\tLinus\n"},{"id":"80926","messageId":"alpine.LFD.1.10.0806241001140.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"20080624082447.GB24357@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-24T17:05:50Z","receivedAt":"2008-06-24T17:05:50Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 24 Jun 2008, Pierre Habouzit wrote:\n> \n> Actually this doesn't work because it may point into the strbuf that\n> will be invalidated later. Our sole option here is to leak some memory I\n> fear.\n\nI think leaking memory is ok (it's obviously going to be bounded by the \nsize of the arguments you pass into a program), but I also think you can \njust change the option strings in place.\n\nYeah, I know - it's impolite, and we even marked things \"const char\", but \n\"const\" in C is just a politeness thing, we can just choose to have a \nfunction with a big comment that changes the string anyway. We'll have to \nmake sure that the few places that actually create argument strings by \nhand (ie not from the ones supplied by a real \"execve()\") not do them so \nthat they need splitting (but no current ones would need to, obviously, \nsince splitting the argumens isn't even supported yet).\n\nOr, if people hate that, just leak a few malloc'ed areas. That's arguably \nthe more straightforward way.\n\n\t\t\tLinus\n"},{"id":"80930","messageId":"alpine.DEB.1.00.0806241811330.9925@racer","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806240954150.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-24T17:13:10Z","receivedAt":"2008-06-24T17:13:10Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 24 Jun 2008, Linus Torvalds wrote:\n\n> On Tue, 24 Jun 2008, Jeff King wrote:\n> > \n> > You seem to have a bunch of _other_ problems with parse_options. And \n> > that is fine, but they have nothing whatsoever to do with anything \n> > I've said. So don't \"sky wizard\" _me_ about those problems. ;P\n> \n> I have a _single_ problem I have with parse_options(), namely that it was \n> painful to convert in pieces.\n\nOkay.\n\nI never wanted anything else than having a way to convert the rest of the \nGit programs to parse options.\n\nOf course, my approach was not meant incremental, yours was.\n\nBut I never wanted anything as complicated as to merit a 7-mail patch \nseries that I have no time to even look at.\n\n> Do you see now?\n\nI do see now.\n\nSorry,\nDscho\n"},{"id":"80932","messageId":"alpine.LFD.1.10.0806241015390.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"1214298732-6247-7-git-send-email-madcoder@debian.org","subject":"Re: [PATCH 6/7] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-24T17:18:56Z","receivedAt":"2008-06-24T17:18:56Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 24 Jun 2008, Pierre Habouzit wrote:\n>\n> This way, argv[0] isn't clobbered, to the cost of maybe not having a\n> resulting NULL terminated argv array.\n\nUmm. I think it's much easier to do by always having\n\n\tctx->out = argv;\n\nand then just initializing cpix to 0 or 1:\n\n\tctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n\nbecause now parse_options_end() doesn't need to play games any more. It \ndoesn't need to care about PARSE_OPT_KEEP_ARGV0, it can just do exactly \nwhat it always used to do, because \"ctx->cpidx + ctx->argc\" automatically \ndoes the right thing (it is both the return value _and_ the index that you \nshould fill with NULL.\n\t\nHmm?\n\n\t\tLinus\n"},{"id":"80933","messageId":"alpine.LFD.1.10.0806241019370.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"1214298732-6247-6-git-send-email-madcoder@debian.org","subject":"Re: [PATCH 5/7] parse-opt: fake short strings for callers to believe in.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-24T17:20:28Z","receivedAt":"2008-06-24T17:20:28Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 24 Jun 2008, Pierre Habouzit wrote:\n>\n> If we begin to parse -abc and that the parser knew about -a and -b, it\n> will fake a -c switch for the caller to deal with.\n> \n> Of course in the case of -acb (supposing -c is not taking an argument) the\n> caller will have to be especially clever to do the same thing. We could\n> think about exposing an API to do so if it's really needed, but oh well...\n\nWell, if the other parser is _also_ parse_options() (ie you just cascade \nthem incrementally in a loop), then the other parser should get it right \nautomatically. No?\n\n\t\tLinus\n"},{"id":"80934","messageId":"20080624173428.GA9500@sigill.intra.peff.net","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806240954150.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-24T17:34:28Z","receivedAt":"2008-06-24T17:34:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 24, 2008 at 09:59:47AM -0700, Linus Torvalds wrote:\n\n> I have a _single_ problem I have with parse_options(), namely that it was \n> painful to convert in pieces. It may well be that builtin-blame.c was one \n> of the more painful cases, but it really was a _single_ issue.\n> \n> I also had a _single_ fix for it.\n> \n> I never had \"other\" problems.\n>\n> What happened was that you and Dscho and others then tried to pick that \n> _single_ issue apart, because the solutions _you_ wanted (tying all the \n\nPerhaps I was confused about the definition of \"single\", because\nthroughout this thread you seem to be making multiple complaints about\nparse_options, including its lack of a \"stop on unknown\" flag, a\n\"continue on unknown flag\", and the movement of arguments within the\nargv array.\n\nBut whether you want to call that a \"single\" problem or not, my point\nwas that I am not talking about most of those things.\n\nSo I will say one last time, as clearly as I possibly can, what I was\ntrying to bring to the discussion:\n\n - You proposed a CONTINUE_ON_UNKNOWN type of flag.\n\n - It is impossible for that mechanism to be correct in all cases, due\n to the syntactic rules for command lines. IOW, you cannot parse an\n element until you know the function of the element to the left.\n\n - I wanted to mention it specifically, because that exact mechanism\n had already been proposed in a patch last week, and Junio said \"this\n conceptually is broken\".\n\n - There has been discussion underway about what is the best mechanism\n to solve the same situation.\n\nThat is the entirety of my point. I am glad you are trying to increase\nparse_options uptake. There is obviously a problem with multi-stage\nparsers. I talked about one way for them to be handled. I think there\nare multiple ways of going about it. It looks like STOP_ON_UNKNOWN\nis the way that Pierre is pursuing. I think this is good, because it\ndoesn't suffer from the corner cases that CONTINUE_ON_UNKNOWN does.\n\nAnd now I will stop making these points, because I don't think I am\ncapable of saying them any more clearly than I already have, and because\nPierre seems to be moving in a sane direction.\n\n> And then you talk about how things \"ought to be\" in your world, to make \n> your solution relevant at all.\n> \n> And I'm trying to tell you that \"ought to be\" has no relevance, because \n> you're not even looking at the problem!\n\nAgain, did you even read the mail you are responding to? The phrase\n\"ought to be\" was totally incidental to the point I was making. I could\njust as easily have said \"and this is the method that I think will be\nacceptable for dealing with this problem.\" But for some reason you\ninsist on harping on the phrase as if I have proposed magical fairies\nshould come work on the code, and totally ignoring the actual points\nthat were made.\n\n-Peff\n"},{"id":"80937","messageId":"alpine.LFD.1.10.0806241036560.2926@woody.linux-foundation.org","threadId":"14096","inReplyTo":"20080624173428.GA9500@sigill.intra.peff.net","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-24T17:44:24Z","receivedAt":"2008-06-24T17:44:24Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 24 Jun 2008, Jeff King wrote:\n> \n> Perhaps I was confused about the definition of \"single\", because\n> throughout this thread you seem to be making multiple complaints about\n> parse_options, including its lack of a \"stop on unknown\" flag, a\n> \"continue on unknown flag\", and the movement of arguments within the\n> argv array.\n\nYou think that is multiple problems, but it's not.\n\nIt was a single issue - the issue of being able to do incremental parsing, \nand mixing *different* parsers together (not just two different calls to \n\"parse_options()\", but also having hand-parsing still in the picture for \nthings where it was hard to convert).\n\nAnd to solve that _single_ problem, I wanted parse_options() to be able \nto:\n\n - stop at unknown options (so that I could hand-parse them)\n\n - ignore unknown options (so that I could parse all the ones I knew \n about, and then either hand-parse the rest, or just pass them on to \n _another_ function that used some arbitrary model to parse the parts it \n knew about)\n\nSee? Single issue.\n\nAnd I even sent out a single patch for it. That single patch, btw, was \neven rather small. \n\nDid you ever look at that patch? Did you ever look at the code I was \ntrying to have use parse_options()? No.\n\n> So I will say one last time, as clearly as I possibly can, what I was\n> trying to bring to the discussion:\n\nYou constantly try to change the discussion to be about SOMETHIGN ELSE.\n\nFor example, you keep on bringing up this TOTAL RED HERRING:\n\n> - It is impossible for that mechanism to be correct in all cases, due\n> to the syntactic rules for command lines. IOW, you cannot parse an\n> element until you know the function of the element to the left.\n\nNOBODY F*CKIGN CARES!\n\nBecause what builtin-blame.c *already* does is exactly that.\n\nThis is what I'm complaining about with your totally IDIOTIC mails. You're \nignoring reality, and talking about how things \"ought to work\", and never \never apparently looked at how things *do* work.\n\nThe fact is, the one program I wanted to convert already does exactly what \nyou claim is \"impossible to be correct in all cases\". \n\nSo either shut up, or send a patch to fix what you consider a bug. I'm \nwaiting.\n\nSo screw it. I'm simply not interested in discussing this with you. You \naren't apparently interested in looking at the problems we have today, \nbecause you're only interested in fixing some theoretical case.\n\n\t\tLinus\n"},{"id":"80958","messageId":"20080624192634.GA9189@artemis.madism.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806241019370.2926@woody.linux-foundation.org","subject":"Re: [PATCH 5/7] parse-opt: fake short strings for callers to believe in.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T19:26:34Z","receivedAt":"2008-06-24T19:26:34Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Jun 24, 2008 at 05:20:28PM +0000, Linus Torvalds wrote:\n> \n> \n> On Tue, 24 Jun 2008, Pierre Habouzit wrote:\n> >\n> > If we begin to parse -abc and that the parser knew about -a and -b, it\n> > will fake a -c switch for the caller to deal with.\n> > \n> > Of course in the case of -acb (supposing -c is not taking an argument) the\n> > caller will have to be especially clever to do the same thing. We could\n> > think about exposing an API to do so if it's really needed, but oh well...\n> \n> Well, if the other parser is _also_ parse_options() (ie you just cascade \n> them incrementally in a loop), then the other parser should get it right \n> automatically. No?\n\n Exactly. There are minor glitches wrt the help generation to deal\nwith, but for pure parsing issues yes, it will work.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80959","messageId":"20080624192720.GB9189@artemis.madism.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806241015390.2926@woody.linux-foundation.org","subject":"Re: [PATCH 6/7] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T19:27:20Z","receivedAt":"2008-06-24T19:27:20Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Jun 24, 2008 at 05:18:56PM +0000, Linus Torvalds wrote:\n> \n> \n> On Tue, 24 Jun 2008, Pierre Habouzit wrote:\n> >\n> > This way, argv[0] isn't clobbered, to the cost of maybe not having a\n> > resulting NULL terminated argv array.\n> \n> Umm. I think it's much easier to do by always having\n> \n> \tctx->out = argv;\n> \n> and then just initializing cpix to 0 or 1:\n> \n> \tctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n> \n> because now parse_options_end() doesn't need to play games any more. It \n> doesn't need to care about PARSE_OPT_KEEP_ARGV0, it can just do exactly \n> what it always used to do, because \"ctx->cpidx + ctx->argc\" automatically \n> does the right thing (it is both the return value _and_ the index that you \n> should fill with NULL.\n\n Oh right, ack it's more elegant.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80961","messageId":"20080624193028.GC9189@artemis.madism.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806241001140.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T19:30:28Z","receivedAt":"2008-06-24T19:30:28Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Jun 24, 2008 at 05:05:50PM +0000, Linus Torvalds wrote:\n> \n> \n> On Tue, 24 Jun 2008, Pierre Habouzit wrote:\n> > \n> > Actually this doesn't work because it may point into the strbuf that\n> > will be invalidated later. Our sole option here is to leak some memory I\n> > fear.\n> \n> I think leaking memory is ok (it's obviously going to be bounded by the \n> size of the arguments you pass into a program), but I also think you can \n> just change the option strings in place.\n> \n> Yeah, I know - it's impolite, and we even marked things \"const char\", but \n> \"const\" in C is just a politeness thing, we can just choose to have a \n> function with a big comment that changes the string anyway. We'll have to \n> make sure that the few places that actually create argument strings by \n> hand (ie not from the ones supplied by a real \"execve()\") not do them so \n> that they need splitting (but no current ones would need to, obviously, \n> since splitting the argumens isn't even supported yet).\n> \n> Or, if people hate that, just leak a few malloc'ed areas. That's arguably \n> the more straightforward way.\n\n The problem is that we sometimes feed the option parser with hand\ncrafted const char *[] where strings are indeed in rodata, and well,\nchanging things is not only impolite, it tends to SIG{BUS,SEGV} ;)\n\n But I think too that leaking is an option. git rev-parse --parseopt\nalready leaks in the same ways e.g.\n\n Though for the win32 port where fork is replaced with threads, well,\nit may cause some issues, so I was reluctant wrt them. Of course it's\nunlikely that it will cause problems, but one never knows ;)\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80966","messageId":"20080624194346.GD9189@artemis.madism.org","threadId":"14096","inReplyTo":"20080624193028.GC9189@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T19:43:46Z","receivedAt":"2008-06-24T19:43:46Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Jun 24, 2008 at 07:30:28PM +0000, Pierre Habouzit wrote:\n> Though for the win32 port where fork is replaced with threads, well,\n> it may cause some issues, so I was reluctant wrt them. Of course it's\n> unlikely that it will cause problems, but one never knows ;)\n\n OTOH if it's really a problem, we could easily use a custom allocator\nin parse-options that registers a pthread_cleanup_push (or whichever\natexit() like pthread use, I'm not really into threads) that would\ncleanup this[0]. So maybe just leaking is the simplest way.\n\n As a consequence it makes the restriction about not keeping pointers\nto argv during the parse go away, and I frankly don't like this\nrestriction, it's counterintuitive and very error prone, which arguably\nmeans it's a really bad design.\n\n\n [0] In fact we may even want to have some kind of\n xmalloc_that_is_freed_at_exit that could be used where we\n purposely leak things, and cleanse that on atexit() for POSIX\n platforms and whatever win32 people like to use in their threads.\n It has the nice property to avoid lots of false positives when you\n are using valgrind and other memory checkers.\n\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80967","messageId":"20080624194617.GA11862@sigill.intra.peff.net","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806241036560.2926@woody.linux-foundation.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-24T19:46:18Z","receivedAt":"2008-06-24T19:46:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 24, 2008 at 10:44:24AM -0700, Linus Torvalds wrote:\n\n> And to solve that _single_ problem, I wanted parse_options() to be able \n> to:\n> \n> - stop at unknown options (so that I could hand-parse them)\n> \n> - ignore unknown options (so that I could parse all the ones I knew \n> about, and then either hand-parse the rest, or just pass them on to \n> _another_ function that used some arbitrary model to parse the parts it \n> knew about)\n> \n> See? Single issue.\n\nOK, fair enough. You are working towards a single goal. But I think\nthere is a flaw with that one sub-part of your goal (specifically your\nsecond bullet point). I don't care about the other parts, and in fact I\neven said \"this is a fine way of doing that\".\n\n> And I even sent out a single patch for it. That single patch, btw, was \n> even rather small. \n> \n> Did you ever look at that patch? Did you ever look at the code I was \n> trying to have use parse_options()? No.\n\nHow can you even say something so ridiculous? Were you sitting at my\ncomputer all day yesterday, making sure I didn't read your patch? No?\nFunnily enough, I was there. And guess what I saw? Yes, it was me\nREADING YOUR PATCH (in a mirror, of course).\n\nBut I don't expect you to have a camera installed over my shoulder. So I\nguess you would have had to just content yourself with THE EMAILS WHERE I\nMENTION YOUR PATCH AND ITS EFFECTS.\n\n> You constantly try to change the discussion to be about SOMETHIGN ELSE.\n> \n> For example, you keep on bringing up this TOTAL RED HERRING:\n> \n> > - It is impossible for that mechanism to be correct in all cases, due\n> > to the syntactic rules for command lines. IOW, you cannot parse an\n> > element until you know the function of the element to the left.\n> \n> NOBODY F*CKIGN CARES!\n\nI care. Apparently Junio cared in the thread that I pointed out earlier.\nOther commands that parse the options will care, if we are to ever port\nthem to parse_options. I understand that you don't care about those\nthings, and you only want git-blame to work. I am merely trying to help\namortize work that goes into fixing git-blame into helping to fix other\ncommands. To do that, I pointed out a flaw that might prevent the same\nsolution being used again.\n\nI understand that you are interested in incremental change, and that\ndoing something now in git-blame is better than doing nothing while we\nwait for a longer, more complete solution to arrive. But did I say \"No,\nwe shouldn't apply this patch from Linus because it's not the optimal\nsolution?\" No. Instead, I said \"In the meantime, your patch does not\nmake this particular problem any worse.\"\n\n> Because what builtin-blame.c *already* does is exactly that.\n\nI KNOW AND I EVEN SAID THAT. But here it is in case you are hard of\nhearing:\n\n From 20080623195314.GA29569@sigill.intra.peff.net:\n\n \"In the meantime, I don't think your patch makes anything _worse_,\n since we already have these sorts of bugs in the current parsing\n code.\"\n\n From 20080623183358.GA28941@sigill.intra.peff.net:\n\n \"It's worse than that...Try (with current git-blame...\n\n $ git blame -n 1 git.gc\n fatal: bad revision '1'\n\n> This is what I'm complaining about with your totally IDIOTIC mails. You're \n> ignoring reality, and talking about how things \"ought to work\", and never \n> ever apparently looked at how things *do* work.\n\nAgain, how in the world can you say that I didn't look at how things do\nwork WHEN I CUT AND PASTED A SAMPLE TRANSCRIPT SHOWING HOW THEY DO WORK?\n\nBTW, this is the third time you have mentioned the phrase \"ought to\nwork\" in a way that is totally disingenuous about what I actually said.\nI can understand that perhaps I was not clear in the initial statement.\nBut I would have thought the other two times I explained it further\nwould have made it clear. So did you truly not understand my point, or\nare you just being intentionally rude?\n\n> The fact is, the one program I wanted to convert already does exactly what \n> you claim is \"impossible to be correct in all cases\".\n\nAnd it's not correct as it is now. And we have lived with it. Which is\nwhy I didn't say \"your proposal makes things worse.\" In fact, I said the\nopposite.\n\n> So either shut up, or send a patch to fix what you consider a bug. I'm \n> waiting.\n\nI already volunteered to work on it. I'm sorry that the patch didn't\nmaterialize immediately, but I was working on other parts of git, just\nas I mentioned when I said I would work on it.\n\nIn the meantime, I think Pierre has proposed an alternate approach that\nalso has promise, so I think it makes sense to see how he progresses\nwith that rather than potentially duplicate work.\n\nAnd I will shut up now, because this is obviously getting nowhere. I\njust find your responses so rude and misinformed not about technical\nmatters, but about my statements and my intent that I feel the need to\npublicly defend myself.\n\n-Peff\n"},{"id":"80983","messageId":"20080624205546.GF9189@artemis.madism.org","threadId":"14096","inReplyTo":"alpine.LFD.1.10.0806241015390.2926@woody.linux-foundation.org","subject":"Re: [PATCH 6/7] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T20:55:46Z","receivedAt":"2008-06-24T20:55:46Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Jun 24, 2008 at 05:18:56PM +0000, Linus Torvalds wrote:\n> \n> \n> On Tue, 24 Jun 2008, Pierre Habouzit wrote:\n> >\n> > This way, argv[0] isn't clobbered, to the cost of maybe not having a\n> > resulting NULL terminated argv array.\n> \n> Umm. I think it's much easier to do by always having\n> \n> \tctx->out = argv;\n> \n> and then just initializing cpix to 0 or 1:\n> \n> \tctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n> \n> because now parse_options_end() doesn't need to play games any more. It \n> doesn't need to care about PARSE_OPT_KEEP_ARGV0, it can just do exactly \n> what it always used to do, because \"ctx->cpidx + ctx->argc\" automatically \n> does the right thing (it is both the return value _and_ the index that you \n> should fill with NULL.\n\nIt indeed is now a trivial patch:\n\n-----8<-----\n\nSubject: [PATCH] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.\n\nThis way, argv[0] isn't clobbered.\n\nSigned-off-by: Pierre Habouzit \n---\n parse-options.c | 1 +\n parse-options.h | 1 +\n 2 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 60a11e8..51a44e3 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -248,6 +248,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tctx->argc = argc - 1;\n \tctx->argv = argv + 1;\n \tctx->out = argv;\n+\tctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n \tctx->flags = flags;\n }\n \ndiff --git a/parse-options.h b/parse-options.h\nindex b391bb6..6299632 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -20,6 +20,7 @@ enum parse_opt_type {\n enum parse_opt_flags {\n \tPARSE_OPT_KEEP_DASHDASH = 1,\n \tPARSE_OPT_STOP_AT_NON_OPTION = 2,\n+\tPARSE_OPT_KEEP_ARGV0 = 4,\n };\n \n enum parse_opt_option_flags {\n-- \n1.5.6.120.g3adb8.dirty\n\n"},{"id":"80985","messageId":"20080624205801.GG9189@artemis.madism.org","threadId":"14096","inReplyTo":"1214298732-6247-6-git-send-email-madcoder@debian.org","subject":"[REPLACEMENT PATCH] parse-opt: fake short strings for callers to believe in.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T20:58:01Z","receivedAt":"2008-06-24T20:58:01Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"If we begin to parse -abc and that the parser knew about -a and -b, it\nwill fake a -c switch for the caller to deal with.\n\nOf course in the case of -acb (supposing -c is not taking an argument) the\ncaller will have to be especially clever to do the same thing. We could\nthink about exposing an API to do so if it's really needed, but oh well...\n\nSigned-off-by: Pierre Habouzit \n---\n\n On top of my leaky() mini series, one can on purpose leak some memory\n here, and avoid the strbuf and limitations at once. It means that in my\n current git-blame proof of concept one can drop the strdup(), which\n makes it slightly less disgusting.\n\n parse-options.c | 9 +++++++++\n parse-options.h | 5 +++++\n 2 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 90935f3..60a11e8 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 \"cache.h\"\n \n #define OPT_SHORT 1\n #define OPT_UNSET 2\n@@ -257,6 +258,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n const struct option *options,\n const char * const usagestr[])\n {\n+\t/* we must reset ->opt, unknown short option leave it dangling */\n+\tctx->opt = NULL;\n+\n \tfor (; ctx->argc; ctx->argc--, ctx->argv++) {\n \t\tconst char *arg = ctx->argv[0];\n \n@@ -286,6 +290,11 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\tcase -1:\n \t\t\t\t\treturn parse_options_usage(usagestr, options);\n \t\t\t\tcase -2:\n+\t\t\t\t\t/* fake a short option thing to hide the fact that we may have\n+\t\t\t\t\t * started to parse aggregated stuff\n+\t\t\t\t\t */\n+\t\t\t\t\tctx->argv[0] = leaky(xstrdup(ctx->opt - 1));\n+\t\t\t\t\t*(char *)ctx->argv[0] = '-';\n \t\t\t\t\treturn PARSE_OPT_UNKNOWN;\n \t\t\t\t}\n \t\t\t}\ndiff --git a/parse-options.h b/parse-options.h\nindex 9da5e8c..b391bb6 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -119,6 +119,11 @@ enum {\n \tPARSE_OPT_UNKNOWN,\n };\n \n+/*\n+ * It's okay for the caller to consume argv/argc in the usual way.\n+ * Other fields of that structure are private to parse-options and should not\n+ * be modified in any way.\n+ */\n struct parse_opt_ctx_t {\n \tconst char **argv;\n \tconst char **out;\n-- \n1.5.6.120.g3adb8.dirty\n\n"},{"id":"81079","messageId":"4861E10B.5080103@viscovery.net","threadId":"14096","inReplyTo":"20080624193028.GC9189@artemis.madism.org","subject":"Re: [RFC] Re: Convert 'git blame' to parse_options()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-06-25T06:09:15Z","receivedAt":"2008-06-25T06:09:15Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Pierre Habouzit schrieb:\n> Though for the win32 port where fork is replaced with threads, well,\n> it may cause some issues, so I was reluctant wrt them. Of course it's\n> unlikely that it will cause problems, but one never knows ;)\n\nWe use threads only for those fork()s that are not followed by an exec().\nSo this is not a reason to worry about memory leaks in the option parser.\n\n-- Hannes\n"},{"id":"81126","messageId":"48625F37.8030808@op5.se","threadId":"14096","inReplyTo":"20080624192634.GA9189@artemis.madism.org","subject":"Re: [PATCH 5/7] parse-opt: fake short strings for callers to believe in.","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2008-06-25T15:07:35Z","receivedAt":"2008-06-25T15:07:35Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Pierre Habouzit wrote:\n> On Tue, Jun 24, 2008 at 05:20:28PM +0000, Linus Torvalds wrote:\n>>\n>> On Tue, 24 Jun 2008, Pierre Habouzit wrote:\n>>> If we begin to parse -abc and that the parser knew about -a and -b, it\n>>> will fake a -c switch for the caller to deal with.\n>>>\n>>> Of course in the case of -acb (supposing -c is not taking an argument) the\n>>> caller will have to be especially clever to do the same thing. We could\n>>> think about exposing an API to do so if it's really needed, but oh well...\n>> Well, if the other parser is _also_ parse_options() (ie you just cascade \n>> them incrementally in a loop), then the other parser should get it right \n>> automatically. No?\n> \n> Exactly. There are minor glitches wrt the help generation to deal\n> with, but for pure parsing issues yes, it will work.\n> \n\nWhy not just provide some api-functions to return a strbuf of the short\nand long options each?\n\nparse_opt_short_help(strbuf *sb, options...);\nparse_opt_long_help(strbuf *sb_long, options...);\n\nThat way multi-parseopt programs can get their help-texts done right\nwith very little extra work.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n"},{"id":"81253","messageId":"20080626083506.GA7237@artemis.madism.org","threadId":"14096","inReplyTo":"20080624205801.GG9189@artemis.madism.org","subject":"Re: [REPLACEMENT PATCH] parse-opt: fake short strings for callers to believe in.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-26T08:35:06Z","receivedAt":"2008-06-26T08:35:06Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"\n Hi Junio,\n\n I saw that you merged this series into pu, for which I'm really glad,\nthough I believe that the version that works on top of the \"leaky()\"\nseries is better, since everyone agrees that the small leaks in the\noption parser are better than the limitations the one you merged has.\n\n I won't flood the list again with the patches, but you can find them\nin git://git.madism.org/~madcoder/git.git#ph/parseopt (based on current\nnext). This includes the two leaky() patches (with the remarks that have\nbeen made to me incorporated), the current state of the parse-options\nimprovements, and the current builtin-blame.c proof of concept (without\nthe ugly strdup).\n\n I really don't like the strbuf thingy, and I'd be glad to never see it\nmerged in next, it will cause more harm than it helps.\n\nTIA.\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"81254","messageId":"7v7iccsi6n.fsf@gitster.siamese.dyndns.org","threadId":"14096","inReplyTo":"20080626083506.GA7237@artemis.madism.org","subject":"Re: [REPLACEMENT PATCH] parse-opt: fake short strings for callers to believe in.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-26T08:40:16Z","receivedAt":"2008-06-26T08:40:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit writes:\n\n> I saw that you merged this series into pu, for which I'm really glad,\n\nHeh, you shouldn't be glad to have \"landed\" on 'pu', as being in 'pu' is\njust a sign of \"undecided\" ;-)\n\nAnd thanks for a reminder. Every bit of help to make integration smoother\nreally helps. I've been swamped and I still have your \"revisions:\nrefactor init_revisions and setup_revisions\" sitting in my inbox.\n"},{"id":"81259","messageId":"20080626093732.GC7237@artemis.madism.org","threadId":"14096","inReplyTo":"7v7iccsi6n.fsf@gitster.siamese.dyndns.org","subject":"Re: [REPLACEMENT PATCH] parse-opt: fake short strings for callers to believe in.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-26T09:37:32Z","receivedAt":"2008-06-26T09:37:32Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Jun 26, 2008 at 08:40:16AM +0000, Junio C Hamano wrote:\n> Pierre Habouzit writes:\n> \n> > I saw that you merged this series into pu, for which I'm really glad,\n> \n> Heh, you shouldn't be glad to have \"landed\" on 'pu', as being in 'pu' is\n> just a sign of \"undecided\" ;-)\n> \n> And thanks for a reminder. Every bit of help to make integration smoother\n> really helps.\n\n You're welcome.\n\n> I've been swamped and I still have your \"revisions: refactor\n> init_revisions and setup_revisions\" sitting in my inbox.\n\n Well I'll resubmit it probably in some kind of improved way that mixes\nbetter within the parse-options stuff. Trust me for not forgetting about\nit. IOW the refactor will remain, but it will probably become some kind\nof series with a second refactor where parse_revisions has a\n'eat-one-option-at-a-time' behavior as well.\n\n Though I believe the current stuff I point you to, to be quite ready\n(except for the builtin-blame proof of concept that is .. a PoC, not\nreally meant for next), and once it'll be ready for next, I'll try to\nwork on the options in git again, because it can be done in an\nincremental way now :)\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"}]}