{"thread":{"id":"14008","subject":"[RFC] convert shortlog to use parse_options","startedAt":"2008-06-18T03:03:54Z","lastAt":"2008-06-23T20:24:38Z","messageCount":17,"participants":["Shawn Bohrer","Junio C Hamano","Jeff King","Johannes Schindelin","Pierre Habouzit"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"80194","messageId":"1213758236-979-1-git-send-email-shawn.bohrer@gmail.com","threadId":"14008","inReplyTo":null,"subject":"[RFC] convert shortlog to use parse_options","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-06-18T03:03:54Z","receivedAt":"2008-06-18T03:03:54Z","isPatch":false,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"\nI guess I should have searched the list _before_ creating these patches\nsince I just now stumbled upon some of the questions about how this\nshould be done for example:\n\nhttp://kerneltrap.org/mailarchive/git/2008/3/1/1035344\n\n>From my testing this seems to work fine, but I may have missed a use\ncase. I actually created these patches because I was annoyed that:\n\ngit shortlog --author=bohrer -s HEAD\n\ndidn't work, and this also fixes that issue.\n\n--\nShawn\n"},{"id":"80195","messageId":"1213758236-979-2-git-send-email-shawn.bohrer@gmail.com","threadId":"14008","inReplyTo":"1213758236-979-1-git-send-email-shawn.bohrer@gmail.com","subject":"[PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-06-18T03:03:55Z","receivedAt":"2008-06-18T03:03:55Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"This adds the PARSE_OPT_NO_ERROR_ON_UNKNOWN flag which prevents\nparse_options() from erroring out when it finds an unknown option,\nand leaves the original command and unknown options in argv.\n\nThis option is useful if the option parsing needs to be done in\nmultiple stages for example if the remaining options will be passed\nto additional git commands.\n\nSigned-off-by: Shawn Bohrer \n---\n parse-options.c | 25 ++++++++++++++++++++-----\n parse-options.h | 5 +++--\n 2 files changed, 23 insertions(+), 7 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 8071711..2635e18 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -131,7 +131,8 @@ static int get_value(struct optparse_t *p,\n \t}\n }\n \n-static int parse_short_opt(struct optparse_t *p, const struct option *options)\n+static int parse_short_opt(struct optparse_t *p, const struct option *options,\n+\t\t\t int flags)\n {\n \tfor (; options->type != OPTION_END; options++) {\n \t\tif (options->short_name == *p->opt) {\n@@ -139,11 +140,16 @@ 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+\n+\tif (flags & PARSE_OPT_NO_ERROR_ON_UNKNOWN) {\n+\t\tp->out[p->cpidx++] = p->argv[0];\n+\t\treturn 0;\n+\t}\n \treturn error(\"unknown switch `%c'\", *p->opt);\n }\n \n static int parse_long_opt(struct optparse_t *p, const char *arg,\n- const struct option *options)\n+ const struct option *options, int flags)\n {\n \tconst char *arg_end = strchr(arg, '=');\n \tconst struct option *abbrev_option = NULL, *ambiguous_option = NULL;\n@@ -224,6 +230,11 @@ is_abbreviated:\n \t\t\tabbrev_option->long_name);\n \tif (abbrev_option)\n \t\treturn get_value(p, abbrev_option, abbrev_flags);\n+\n+\tif (flags & PARSE_OPT_NO_ERROR_ON_UNKNOWN) {\n+\t\tp->out[p->cpidx++] = p->argv[0];\n+\t\treturn 0;\n+\t}\n \treturn error(\"unknown option `%s'\", arg);\n }\n \n@@ -254,6 +265,8 @@ 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+\tif (flags & PARSE_OPT_NO_ERROR_ON_UNKNOWN)\n+\t\targs.out = argv + 1;\n \n \tfor (; args.argc; args.argc--, args.argv++) {\n \t\tconst char *arg = args.argv[0];\n@@ -269,14 +282,14 @@ 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, flags) < 0)\n \t\t\t\tusage_with_options(usagestr, options);\n \t\t\tif (args.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\t\t\tusage_with_options(usagestr, options);\n-\t\t\t\tif (parse_short_opt(&args, options) < 0)\n+\t\t\t\tif (parse_short_opt(&args, options, flags) < 0)\n \t\t\t\t\tusage_with_options(usagestr, options);\n \t\t\t}\n \t\t\tcontinue;\n@@ -294,11 +307,13 @@ 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, flags))\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+\tif (flags & PARSE_OPT_NO_ERROR_ON_UNKNOWN)\n+\t\t++args.cpidx;\n \targs.out[args.cpidx + args.argc] = NULL;\n \treturn args.cpidx + args.argc;\n }\ndiff --git a/parse-options.h b/parse-options.h\nindex 4ee443d..416ccdd 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -18,8 +18,9 @@ enum parse_opt_type {\n };\n \n enum parse_opt_flags {\n-\tPARSE_OPT_KEEP_DASHDASH = 1,\n-\tPARSE_OPT_STOP_AT_NON_OPTION = 2,\n+\tPARSE_OPT_KEEP_DASHDASH = 1,\n+\tPARSE_OPT_STOP_AT_NON_OPTION = 2,\n+\tPARSE_OPT_NO_ERROR_ON_UNKNOWN = 4\n };\n \n enum parse_opt_option_flags {\n-- \n1.5.4.3\n"},{"id":"80196","messageId":"1213758236-979-3-git-send-email-shawn.bohrer@gmail.com","threadId":"14008","inReplyTo":"1213758236-979-2-git-send-email-shawn.bohrer@gmail.com","subject":"[PATCH 2/2] git shortlog: Modify to use parse_options","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-06-18T03:03:56Z","receivedAt":"2008-06-18T03:03:56Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"Signed-off-by: Shawn Bohrer \n---\n builtin-shortlog.c | 54 +++++++++++++++++++++++++++------------------------\n 1 files changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin-shortlog.c b/builtin-shortlog.c\nindex e6a2865..b1087b5 100644\n--- a/builtin-shortlog.c\n+++ b/builtin-shortlog.c\n@@ -7,9 +7,12 @@\n #include \"utf8.h\"\n #include \"mailmap.h\"\n #include \"shortlog.h\"\n+#include \"parse-options.h\"\n \n-static const char shortlog_usage[] =\n-\"git-shortlog [-n] [-s] [-e] [-w] [... ]\";\n+static const char *const shortlog_usage[] = {\n+\t\"git-shortlog [-n] [-s] [-e] [-w] [... ]\",\n+\tNULL\n+};\n \n static int compare_by_number(const void *a1, const void *a2)\n {\n@@ -189,8 +192,6 @@ static const char wrap_arg_usage[] = \"-w[[,[,]]]\";\n \n static void parse_wrap_args(const char *arg, int *in1, int *in2, int *wrap)\n {\n-\targ += 2; /* skip -w */\n-\n \t*wrap = parse_uint(&arg, ',');\n \tif (*wrap < 0)\n \t\tdie(wrap_arg_usage);\n@@ -230,35 +231,38 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \tstruct shortlog log;\n \tstruct rev_info rev;\n \tint nongit;\n+\tconst char * wrap_options = NULL;\n+\tstruct option options[] = {\n+\t\tOPT_BOOLEAN('n', \"numbered\", &log.sort_by_number,\n+\t\t\t \"sort by number\"),\n+\t\tOPT_BOOLEAN('s', \"summary\", &log.summary,\n+\t\t\t \"only provide commit count summary\"),\n+\t\tOPT_BOOLEAN('e', \"email\", &log.email,\n+\t\t\t \"show email address of author\"),\n+\t\t{ OPTION_STRING, 'w', NULL, &wrap_options,\n+\t\t \"[[,[,]]]\", \"linewrap the output\",\n+\t\t PARSE_OPT_OPTARG, NULL, (intptr_t)\"-w\" },\n+\t\tOPT_END()\n+\t};\n \n \tprefix = setup_git_directory_gently(&nongit);\n \tshortlog_init(&log);\n \n-\t/* since -n is a shadowed rev argument, parse our args first */\n-\twhile (argc > 1) {\n-\t\tif (!strcmp(argv[1], \"-n\") || !strcmp(argv[1], \"--numbered\"))\n-\t\t\tlog.sort_by_number = 1;\n-\t\telse if (!strcmp(argv[1], \"-s\") ||\n-\t\t\t\t!strcmp(argv[1], \"--summary\"))\n-\t\t\tlog.summary = 1;\n-\t\telse if (!strcmp(argv[1], \"-e\") ||\n-\t\t\t !strcmp(argv[1], \"--email\"))\n-\t\t\tlog.email = 1;\n-\t\telse if (!prefixcmp(argv[1], \"-w\")) {\n-\t\t\tlog.wrap_lines = 1;\n-\t\t\tparse_wrap_args(argv[1], &log.in1, &log.in2, &log.wrap);\n-\t\t}\n-\t\telse if (!strcmp(argv[1], \"-h\") || !strcmp(argv[1], \"--help\"))\n-\t\t\tusage(shortlog_usage);\n-\t\telse\n-\t\t\tbreak;\n-\t\targv++;\n-\t\targc--;\n+\targc = parse_options(argc, argv, options, shortlog_usage,\n+\t\t\t PARSE_OPT_NO_ERROR_ON_UNKNOWN);\n+\n+\tif (wrap_options)\n+\t{\n+\t\tif (!prefixcmp(wrap_options, \"-w\"))\n+\t\t\twrap_options += 2; /* skip -w */\n+\t\tlog.wrap_lines = 1;\n+\t\tparse_wrap_args(wrap_options, &log.in1, &log.in2, &log.wrap);\n \t}\n+\n \tinit_revisions(&rev, prefix);\n \targc = setup_revisions(argc, argv, &rev, NULL);\n \tif (argc > 1)\n-\t\tdie (\"unrecognized argument: %s\", argv[1]);\n+\t\tusage_with_options(shortlog_usage, options);\n \n \t/* assume HEAD if from a tty */\n \tif (!nongit && !rev.pending.nr && isatty(0))\n-- \n1.5.4.3\n"},{"id":"80198","messageId":"7v1w2v2zsh.fsf@gitster.siamese.dyndns.org","threadId":"14008","inReplyTo":"1213758236-979-2-git-send-email-shawn.bohrer@gmail.com","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-18T03:21:50Z","receivedAt":"2008-06-18T03:21:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Bohrer writes:\n\n> This adds the PARSE_OPT_NO_ERROR_ON_UNKNOWN flag which prevents\n> parse_options() from erroring out when it finds an unknown option,\n> and leaves the original command and unknown options in argv.\n\nI have to say that this conceptually is broken. How would you tell\nwithout knowing what \"--flag\" is if the thing in argv[] after that is a\nparameter to that option or the end of the options?\n"},{"id":"80199","messageId":"20080618033010.GA19657@sigill.intra.peff.net","threadId":"14008","inReplyTo":"7v1w2v2zsh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-18T03:30:10Z","receivedAt":"2008-06-18T03:30:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 17, 2008 at 08:21:50PM -0700, Junio C Hamano wrote:\n\n> Shawn Bohrer writes:\n> \n> > This adds the PARSE_OPT_NO_ERROR_ON_UNKNOWN flag which prevents\n> > parse_options() from erroring out when it finds an unknown option,\n> > and leaves the original command and unknown options in argv.\n> \n> I have to say that this conceptually is broken. How would you tell\n> without knowing what \"--flag\" is if the thing in argv[] after that is a\n> parameter to that option or the end of the options?\n\nAgreed. I was just about to write the same thing. As it happens, I think\nin the case of git-shortlog that there is not likely to be such a\nparameter. The only three I see looking over setup_revisions are \"-n\"\n(which is masked by shortlog anyway), \"--default\", and \"-U\" (which one\nwould never need with shortlog).\n\nHowever I am still opposed to the concept, since its presence as a\nparseopt flag implies that it isn't fundamentally broken.\n\nI think the only right way to accomplish this is to convert the revision\nand diff parameters into a parseopt-understandable format.\n\n-Peff\n"},{"id":"80200","messageId":"20080618033423.GC19657@sigill.intra.peff.net","threadId":"14008","inReplyTo":"20080618033010.GA19657@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-18T03:34:23Z","receivedAt":"2008-06-18T03:34:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 17, 2008 at 11:30:10PM -0400, Jeff King wrote:\n\n> Agreed. I was just about to write the same thing. As it happens, I think\n> in the case of git-shortlog that there is not likely to be such a\n> parameter. The only three I see looking over setup_revisions are \"-n\"\n> (which is masked by shortlog anyway), \"--default\", and \"-U\" (which one\n> would never need with shortlog).\n\nBTW, looking in my personal repo, I have the start of the exact same\npatch (except I called it PARSE_OPT_STOP_AT_UNKNOWN). I think I\nabandoned it when I realized the fundamental flaw with the approach, but\nI guess I never got it to the point of sharing with the list.\n\n-Peff\n"},{"id":"80202","messageId":"7vwskn1g2p.fsf@gitster.siamese.dyndns.org","threadId":"14008","inReplyTo":"20080618033010.GA19657@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-18T05:13:02Z","receivedAt":"2008-06-18T05:13:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think the only right way to accomplish this is to convert the revision\n> and diff parameters into a parseopt-understandable format.\n\nNot necessarily. You could structure individual option parsers like how\ndiff option parsers are done. You iterate over argv[], feed diff option\nparser the current index into argv[] and ask if it is an option diff\nunderstands, have diff eat the option (and possibly its parameter) to\nadvance the index, or allow diff option to say \"I do not understand this\",\nand then handle it yourself or hand it to other parsers.\n"},{"id":"80231","messageId":"alpine.DEB.1.00.0806181709300.6439@racer","threadId":"14008","inReplyTo":"7vwskn1g2p.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-18T16:50:31Z","receivedAt":"2008-06-18T16:50:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 17 Jun 2008, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I think the only right way to accomplish this is to convert the revision\n> > and diff parameters into a parseopt-understandable format.\n> \n> Not necessarily. You could structure individual option parsers like how \n> diff option parsers are done. You iterate over argv[], feed diff option \n> parser the current index into argv[] and ask if it is an option diff \n> understands, have diff eat the option (and possibly its parameter) to \n> advance the index, or allow diff option to say \"I do not understand \n> this\", and then handle it yourself or hand it to other parsers.\n\nAFAIR Pierre tried a few ways, and settled with a macro to introduce the \ndiff options into a caller's options.\n\nIOW it would look something like this:\n\nstatic struct option builtin_what_options[] = {\n\t[... options specific to this command ...]\n\tDIFF__OPT(&diff_options)\n};\n\nCiao,\nDscho\n"},{"id":"80238","messageId":"7v8wx2zibp.fsf@gitster.siamese.dyndns.org","threadId":"14008","inReplyTo":"alpine.DEB.1.00.0806181709300.6439@racer","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-18T18:52:42Z","receivedAt":"2008-06-18T18:52:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Tue, 17 Jun 2008, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > I think the only right way to accomplish this is to convert the revision\n>> > and diff parameters into a parseopt-understandable format.\n>> \n>> Not necessarily. You could structure individual option parsers like how \n>> diff option parsers are done. You iterate over argv[], feed diff option \n>> parser the current index into argv[] and ask if it is an option diff \n>> understands, have diff eat the option (and possibly its parameter) to \n>> advance the index, or allow diff option to say \"I do not understand \n>> this\", and then handle it yourself or hand it to other parsers.\n>\n> AFAIR Pierre tried a few ways, and settled with a macro to introduce the \n> diff options into a caller's options.\n>\n> IOW it would look something like this:\n>\n> static struct option builtin_what_options[] = {\n> \t[... options specific to this command ...]\n> \tDIFF__OPT(&diff_options)\n> };\n\nI think that is the more painful approach Jeff mentioned, and my comment\nwas to show that it is not the only way.\n"},{"id":"80319","messageId":"20080619142527.GA8429@mediacenter","threadId":"14008","inReplyTo":"7v8wx2zibp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-06-19T14:25:27Z","receivedAt":"2008-06-19T14:25:27Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"On Wed, Jun 18, 2008 at 11:52:42AM -0700, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n> > On Tue, 17 Jun 2008, Junio C Hamano wrote:\n> >\n> >> Jeff King writes:\n> >> \n> >> > I think the only right way to accomplish this is to convert the revision\n> >> > and diff parameters into a parseopt-understandable format.\n> >> \n> >> Not necessarily. You could structure individual option parsers like how \n> >> diff option parsers are done. You iterate over argv[], feed diff option \n> >> parser the current index into argv[] and ask if it is an option diff \n> >> understands, have diff eat the option (and possibly its parameter) to \n> >> advance the index, or allow diff option to say \"I do not understand \n> >> this\", and then handle it yourself or hand it to other parsers.\n> >\n> > AFAIR Pierre tried a few ways, and settled with a macro to introduce the \n> > diff options into a caller's options.\n> >\n> > IOW it would look something like this:\n> >\n> > static struct option builtin_what_options[] = {\n> > \t[... options specific to this command ...]\n> > \tDIFF__OPT(&diff_options)\n> > };\n> \n> I think that is the more painful approach Jeff mentioned, and my comment\n> was to show that it is not the only way.\n> \n\nIt seems to me that you could implement Jeff's\nPARSE_OPT_STOP_AT_UNKNOWN, and then if multiple option parsers are\nneeded you would simply loop over parse_options for each of the\ncommands, waiting for argc to stop changing. Of course Jeff's flag\nwould also need to stop parse_options from eating the first argument.\nIs this sort of what you are suggesting Junio?\n\n--\nShawn\n"},{"id":"80628","messageId":"20080622170733.GA16252@artemis.madism.org","threadId":"14008","inReplyTo":"7vwskn1g2p.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-22T17:07:33Z","receivedAt":"2008-06-22T17:07:33Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Wed, Jun 18, 2008 at 05:13:02AM +0000, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > I think the only right way to accomplish this is to convert the revision\n> > and diff parameters into a parseopt-understandable format.\n> \n> Not necessarily. You could structure individual option parsers like how\n> diff option parsers are done. You iterate over argv[], feed diff option\n> parser the current index into argv[] and ask if it is an option diff\n> understands, have diff eat the option (and possibly its parameter) to\n> advance the index, or allow diff option to say \"I do not understand this\",\n> and then handle it yourself or hand it to other parsers.\n\n If you do that, you need to relocate pars option structures, and we\ndecided some time ago that it wasn't a good idea. Note that \"recursing\"\nis not really trivial, because with flags aggregation and stuff like\nthat, things that look like an option can also be a value in the context\nof an other option parser.\n\n That's why we settled for the other way Dscho pointed. But for that, I\nneed to work on it more than what I really have time to nowadays, and\nmoreover, it needs some things (the setup_revisions split and the log\ntraversal bits change) to be merged.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80635","messageId":"alpine.DEB.1.00.0806221953470.6439@racer","threadId":"14008","inReplyTo":"20080619142527.GA8429@mediacenter","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-22T19:07:18Z","receivedAt":"2008-06-22T19:07:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 19 Jun 2008, Shawn Bohrer wrote:\n\n> On Wed, Jun 18, 2008 at 11:52:42AM -0700, Junio C Hamano wrote:\n> > Johannes Schindelin writes:\n> > \n> > > On Tue, 17 Jun 2008, Junio C Hamano wrote:\n> > >\n> > >> Jeff King writes:\n> > >> \n> > >> > I think the only right way to accomplish this is to convert the \n> > >> > revision and diff parameters into a parseopt-understandable \n> > >> > format.\n> > >> \n> > >> Not necessarily. You could structure individual option parsers \n> > >> like how diff option parsers are done. You iterate over argv[], \n> > >> feed diff option parser the current index into argv[] and ask if it \n> > >> is an option diff understands, have diff eat the option (and \n> > >> possibly its parameter) to advance the index, or allow diff option \n> > >> to say \"I do not understand this\", and then handle it yourself or \n> > >> hand it to other parsers.\n> > >\n> > > AFAIR Pierre tried a few ways, and settled with a macro to introduce \n> > > the diff options into a caller's options.\n> > >\n> > > IOW it would look something like this:\n> > >\n> > > static struct option builtin_what_options[] = {\n> > > \t[... options specific to this command ...]\n> > > \tDIFF__OPT(&diff_options)\n> > > };\n> > \n> > I think that is the more painful approach Jeff mentioned, and my \n> > comment was to show that it is not the only way.\n> > \n> \n> It seems to me that you could implement Jeff's \n> PARSE_OPT_STOP_AT_UNKNOWN, and then if multiple option parsers are \n> needed you would simply loop over parse_options for each of the \n> commands, waiting for argc to stop changing. Of course Jeff's flag \n> would also need to stop parse_options from eating the first argument. Is \n> this sort of what you are suggesting Junio?\n\nI believe not. I think that Junio prefers some callback that can handle a \nwhole bunch of options (as opposed to the callback we can have now, to \nhandle arguments for a specific option).\n\nHowever, I am not sure what that would buy us over the approach Pierre \nsettled. Junio, maybe you thought that the option parsing macro would \nlive in parse-options.h? It was supposed to live in diff.h and \nrevision.h, respectively.\n\nCiao,\nDscho\n"},{"id":"80663","messageId":"7vod5skjq0.fsf@gitster.siamese.dyndns.org","threadId":"14008","inReplyTo":"20080622170733.GA16252@artemis.madism.org","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-23T01:45:11Z","receivedAt":"2008-06-23T01:45:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit writes:\n\n> On Wed, Jun 18, 2008 at 05:13:02AM +0000, Junio C Hamano wrote:\n>> Jeff King writes:\n>> \n>> > I think the only right way to accomplish this is to convert the revision\n>> > and diff parameters into a parseopt-understandable format.\n>> \n>> Not necessarily. You could structure individual option parsers like how\n>> diff option parsers are done. You iterate over argv[], feed diff option\n>> parser the current index into argv[] and ask if it is an option diff\n>> understands, have diff eat the option (and possibly its parameter) to\n>> advance the index, or allow diff option to say \"I do not understand this\",\n>> and then handle it yourself or hand it to other parsers.\n>\n> If you do that, you need to relocate pars option structures,...\n> ... Note that \"recursing\"\n> is not really trivial, because with flags aggregation and stuff like\n> that, things that look like an option can also be a value in the context\n> of an other option parser.\n\nNote that I was just saying \"not necessarily\" in response to \"the only\nright way\" to point out it is not the _only_ way.\n\nParse-options has been done in a tablish way and it would involve cost to\nmodify it in a way I outlined (even if such a rewrite would make chaining\ndifferent set of option parsers easier, as each parser needs to handle\nonly what it knows about and handling aggregation and stuff would become\ntrivial). I do not know if it is worth the cost, and I am not married to\nthe option parser structure that diff and revision part uses.\n"},{"id":"80749","messageId":"7v4p7khqp7.fsf@gitster.siamese.dyndns.org","threadId":"14008","inReplyTo":"alpine.DEB.1.00.0806221953470.6439@racer","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-23T19:55:00Z","receivedAt":"2008-06-23T19:55:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Thu, 19 Jun 2008, Shawn Bohrer wrote:\n>\n>> On Wed, Jun 18, 2008 at 11:52:42AM -0700, Junio C Hamano wrote:\n> I believe not. I think that Junio prefers some callback that can handle a \n> whole bunch of options (as opposed to the callback we can have now, to \n> handle arguments for a specific option).\n\nSorry, no. I do not want callbacks. I've been saying that parser\ncascading is easier if you use an incremental interface like diff option\nparser does.\n"},{"id":"80752","messageId":"20080623195906.GC29569@sigill.intra.peff.net","threadId":"14008","inReplyTo":"7v4p7khqp7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-23T19:59:07Z","receivedAt":"2008-06-23T19:59:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2008 at 12:55:00PM -0700, Junio C Hamano wrote:\n\n> >> On Wed, Jun 18, 2008 at 11:52:42AM -0700, Junio C Hamano wrote:\n> > I believe not. I think that Junio prefers some callback that can handle a \n> > whole bunch of options (as opposed to the callback we can have now, to \n> > handle arguments for a specific option).\n> \n> Sorry, no. I do not want callbacks. I've been saying that parser\n> cascading is easier if you use an incremental interface like diff option\n> parser does.\n\nNow I'm confused: my understanding is that the diff option parser just\nleaves unrecognized stuff in argv. But isn't that what a\nPARSE_OPTIONS_IGNORE_UNKNOWN flag would do, and isn't that wrong?\n\n-Peff\n"},{"id":"80756","messageId":"7vwskfhpo3.fsf@gitster.siamese.dyndns.org","threadId":"14008","inReplyTo":"20080623195906.GC29569@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-23T20:17:16Z","receivedAt":"2008-06-23T20:17:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Now I'm confused: my understanding is that the diff option parser just\n> leaves unrecognized stuff in argv. But isn't that what a\n> PARSE_OPTIONS_IGNORE_UNKNOWN flag would do, and isn't that wrong?\n\nI was thinking more about the way how the lower level diff_opt_parse()\nworks by letting the caller to handle things that it itself does not know\nhow.\n\nBut I say this because I am not interested in \"-a -b -c <=> -abc\" and\nhaven't thought about how you would go about parsing something like that\nsanely with partial knowledge.\n"},{"id":"80758","messageId":"alpine.DEB.1.00.0806232124130.6440@racer","threadId":"14008","inReplyTo":"7v4p7khqp7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-23T20:24:38Z","receivedAt":"2008-06-23T20:24:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 23 Jun 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Thu, 19 Jun 2008, Shawn Bohrer wrote:\n> >\n> >> On Wed, Jun 18, 2008 at 11:52:42AM -0700, Junio C Hamano wrote:\n> >\n> > I believe not. I think that Junio prefers some callback that can \n> > handle a whole bunch of options (as opposed to the callback we can \n> > have now, to handle arguments for a specific option).\n> \n> Sorry, no. I do not want callbacks. I've been saying that parser \n> cascading is easier if you use an incremental interface like diff option \n> parser does.\n\nSorry, I misunderstood. At least you clarified it for me now.\n\nThanks,\nDscho\n"}]}