{"thread":{"id":"12402","subject":"[PATCH] convert shortlog to use parse_options","startedAt":"2008-03-01T09:02:32Z","lastAt":"2008-03-02T08:29:51Z","messageCount":5,"participants":["Jeff King","Junio C Hamano","Pierre Habouzit"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"70500","messageId":"20080301090231.GA16937@coredump.intra.peff.net","threadId":"12402","inReplyTo":null,"subject":"[PATCH] convert shortlog to use parse_options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-01T09:02:32Z","receivedAt":"2008-03-01T09:02:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The behavior should be identical, except that we now support\noption bundling (e.g., \"git shortlog -nse\").\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAnother of my pre-1.5.4 cleanups (I think we may enter 1.5.5 freeze\nbefore I get through my backlog!). This was originally rejected because\nit changed the semantics of \"-w\" to allow \"-w X,Y,Z\". However, Pierre's\nc43a24834 forces short options to stick.\n\nThis is also, of course, respun on top of Daniel's shortlog changes.\n\n builtin-shortlog.c |   62 ++++++++++++++++++++++++++++++---------------------\n 1 files changed, 36 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin-shortlog.c b/builtin-shortlog.c\nindex af31aba..bf6802e 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] [<commit-id>... ]\";\n+static const char * const shortlog_usage[] = {\n+\t\"git-shortlog [-n] [-s] [-e] [<commit-id>... ]\",\n+\tNULL\n+};\n \n static int compare_by_number(const void *a1, const void *a2)\n {\n@@ -188,8 +191,6 @@ static const char wrap_arg_usage[] = \"-w[<width>[,<indent1>[,<indent2>]]]\";\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@@ -212,6 +213,16 @@ static void parse_wrap_args(const char *arg, int *in1, int *in2, int *wrap)\n \t\tdie(wrap_arg_usage);\n }\n \n+static int parse_wrap_callback(const struct option *opt, const char *arg,\n+\t\tint unset)\n+{\n+\tstruct shortlog *log = opt->value;\n+\tlog->wrap_lines = 1;\n+\tif (arg)\n+\t\tparse_wrap_args(arg, &log->wrap, &log->in1, &log->in2);\n+\treturn 0;\n+}\n+\n void shortlog_init(struct shortlog *log)\n {\n \tmemset(log, 0, sizeof(*log));\n@@ -231,29 +242,28 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \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-\t}\n+\tstruct option options[] = {\n+\t\tOPT_BOOLEAN('n', \"numbered\", &log.sort_by_number,\n+\t\t\t\t\"sort by number\"),\n+\t\tOPT_BOOLEAN('s', \"summary\", &log.summary,\n+\t\t\t\t\"show commit count summary\"),\n+\t\tOPT_BOOLEAN('e', \"email\", &log.email, \"show email addresses\"),\n+\t\t{\n+\t\t\tOPTION_CALLBACK, 'w', NULL, &log, \"wrap\",\n+\t\t\t\"line-wrap commit message\", PARSE_OPT_OPTARG,\n+\t\t\tparse_wrap_callback\n+\t\t},\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, options, shortlog_usage, 0);\n+\n \tinit_revisions(&rev, prefix);\n-\targc = setup_revisions(argc, argv, &rev, NULL);\n+\t/* setup_revisions expects to have the program name in argv[0],\n+\t * but parse_options has removed it. We have to do parse_options\n+\t * before setup_revisions, though, to claim \"-n\". */\n+\targc = setup_revisions(argc+1, argv-1, &rev, NULL);\n+\n \tif (argc > 1)\n \t\tdie (\"unrecognized argument: %s\", argv[1]);\n \n-- \n1.5.4.3.341.g4e44\n"},{"id":"70503","messageId":"7v8x124wfd.fsf@gitster.siamese.dyndns.org","threadId":"12402","inReplyTo":"20080301090231.GA16937@coredump.intra.peff.net","subject":"Re: [PATCH] convert shortlog to use parse_options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-01T09:37:26Z","receivedAt":"2008-03-01T09:37:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The behavior should be identical, except that we now support\n> option bundling (e.g., \"git shortlog -nse\").\n\nSorry, but this breaks\n\n\tgit shortlog -n -s -e --no-merges v1.5.4..\n\n"},{"id":"70584","messageId":"20080302061541.GA3935@coredump.intra.peff.net","threadId":"12402","inReplyTo":"7v8x124wfd.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] convert shortlog to use parse_options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-02T06:15:42Z","receivedAt":"2008-03-02T06:15:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 01, 2008 at 01:37:26AM -0800, Junio C Hamano wrote:\n\n> > The behavior should be identical, except that we now support\n> > option bundling (e.g., \"git shortlog -nse\").\n> \n> Sorry, but this breaks\n> \n> \tgit shortlog -n -s -e --no-merges v1.5.4..\n\nI didn't test this very extensively (obviously!) since it was just a\nrespin of an old patch. Sorry to waste your time with something that is\nso obviously broken.\n\nI started to write a patch to give parse-options a \"stop processing at\nthe first unknown option\" flag, but it was very unsatisfactory.\nSpecifically:\n\n  - the final parser has to know that it's the last, and complain about\n    unrecognized options. In this case, setup_revisions would be the\n    second and final parser, and it of course doesn't do this (though\n    perhaps one could check the residual options from setup_revisions\n    manually and barf on that)\n\n  - the fact that we have two sets of parsed options can't be\n    transparent to the user. We'd stop at \"--no-merges\", which means you\n    can't say \"git shortlog --no-merges -e\", which is silly. But we\n    can't possibly continue, since we don't know if \"-e\" is another\n    option or an argument there.\n\nAnd it doesn't work to do the setup_revisions first, since there is a\nconflict over the \"-n\" option (and we can't even munge the result\nafterwards, since one version takes an argument and one doesn't).\n\nSo I think using parse-options here should be put on hold until we have\nthe revision and diff parameters in a parseopt-understandable form. I\nwould think we could do something like:\n\n#define OPT__REVISION(x) \\\n        OPT_BOOLEAN(0, \"no-merges\", &(x)->no_merges, \"don't show merges\"),\n        OPT_BOOLEAN(0, \"boundary\", &(x)->boundary, \"show boundary commits\"),\n        ...\n\nand we could have unified options tables. I seem to recall some work\nbeing done in this area early on in the parse-options history, but I\ncan't seem to find any mention of it in the list archive. Pierre, does\nthis ring a bell?\n\n-Peff\n"},{"id":"70603","messageId":"20080302081139.GA5407@artemis.madism.org","threadId":"12402","inReplyTo":"20080302061541.GA3935@coredump.intra.peff.net","subject":"Re: [PATCH] convert shortlog to use parse_options","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-03-02T08:11:40Z","receivedAt":"2008-03-02T08:11:40Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sun, Mar 02, 2008 at 06:15:42AM +0000, Jeff King wrote:\n> #define OPT__REVISION(x) \\\n>         OPT_BOOLEAN(0, \"no-merges\", &(x)->no_merges, \"don't show merges\"),\n>         OPT_BOOLEAN(0, \"boundary\", &(x)->boundary, \"show boundary commits\"),\n>         ...\n> \n> and we could have unified options tables. I seem to recall some work\n> being done in this area early on in the parse-options history, but I\n> can't seem to find any mention of it in the list archive. Pierre, does\n> this ring a bell?\n\n  Yes, I didn't had the time to finish that, I just started some ground\nworks in the diff options area, I hope I didn't lost that work, it's\nprobably somewhere on my public repository. Though revision parsing are\nspecial because of --not, but I think the proper solution wrt --not and\n--all in revision parsing is to ask parse-opt to \"let\" some options stay\nas arguments, and do the final revision parsing with them kept.\n\n  Like you may have noticed, I didn't have a lot of time for git\nrecently, and that's a shame :| But maybe your mail will beat me into\nworking on this again now that 1.5.4 is released, because I also feared\nthat reworking diff and revision parsing options will probably introduce\nquite a few regressions, and it's rather better doing so at the\nbeginning of a release cycle :)\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"70605","messageId":"20080302082951.GC5407@artemis.madism.org","threadId":"12402","inReplyTo":"20080302081139.GA5407@artemis.madism.org","subject":"Re: [PATCH] convert shortlog to use parse_options","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-03-02T08:29:51Z","receivedAt":"2008-03-02T08:29:51Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sun, Mar 02, 2008 at 08:11:40AM +0000, Pierre Habouzit wrote:\n> On Sun, Mar 02, 2008 at 06:15:42AM +0000, Jeff King wrote:\n> > #define OPT__REVISION(x) \\\n> >         OPT_BOOLEAN(0, \"no-merges\", &(x)->no_merges, \"don't show merges\"),\n> >         OPT_BOOLEAN(0, \"boundary\", &(x)->boundary, \"show boundary commits\"),\n> >         ...\n> > \n> > and we could have unified options tables. I seem to recall some work\n> > being done in this area early on in the parse-options history, but I\n> > can't seem to find any mention of it in the list archive. Pierre, does\n> > this ring a bell?\n\n  In fact everything is here:\nhttp://git.madism.org/?p=~madcoder/git.git;a=shortlog;h=refs/heads/ph/parseopt\n\n  I just resent the first patch that I sent (with a bug) not so long\ntime ago. The last one is the current state of work for diff opts.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"}]}