{"thread":{"id":"14385","subject":"[PATCH] revisions: refactor handle_revision_opt into parse_revision_opt.","startedAt":"2008-07-09T21:38:33Z","lastAt":"2008-07-10T07:40:12Z","messageCount":6,"participants":["Pierre Habouzit","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"82759","messageId":"1215639514-1612-1-git-send-email-madcoder@debian.org","threadId":"14385","inReplyTo":null,"subject":"[PATCH] git-shortlog: migrate to parse-options partially.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-07-09T21:38:33Z","receivedAt":"2008-07-09T21:38:33Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Signed-off-by: Pierre Habouzit \n---\n builtin-shortlog.c | 135 +++++++++++++++++++++++++++++----------------------\n 1 files changed, 77 insertions(+), 58 deletions(-)\n\ndiff --git a/builtin-shortlog.c b/builtin-shortlog.c\nindex e6a2865..9107bff 100644\n--- a/builtin-shortlog.c\n+++ b/builtin-shortlog.c\n@@ -7,9 +7,14 @@\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 char const * const shortlog_usage[] = {\n+\t\"git-shortlog [-n] [-s] [-e] [-w] [rev-opts] [--] [... ]\",\n+\t\"\",\n+\t\"[rev-opts] are documented in git-rev-list(1)\",\n+\tNULL\n+};\n \n static int compare_by_number(const void *a1, const void *a2)\n {\n@@ -164,21 +169,19 @@ static void get_from_rev(struct rev_info *rev, struct shortlog *log)\n \t\tshortlog_add_commit(log, commit);\n }\n \n-static int parse_uint(char const **arg, int comma)\n+static int parse_uint(char const **arg, int comma, int defval)\n {\n \tunsigned long ul;\n \tint ret;\n \tchar *endp;\n \n \tul = strtoul(*arg, &endp, 10);\n-\tif (endp != *arg && *endp && *endp != comma)\n+\tif (*endp && *endp != comma)\n \t\treturn -1;\n-\tret = (int) ul;\n-\tif (ret != ul)\n+\tif (ul > INT_MAX)\n \t\treturn -1;\n-\t*arg = endp;\n-\tif (**arg)\n-\t\t(*arg)++;\n+\tret = *arg == endp ? defval : (int)ul;\n+\t*arg = *endp ? endp + 1 : endp;\n \treturn ret;\n }\n \n@@ -187,30 +190,30 @@ static const char wrap_arg_usage[] = \"-w[[,[,]]]\";\n #define DEFAULT_INDENT1 6\n #define DEFAULT_INDENT2 9\n \n-static void parse_wrap_args(const char *arg, int *in1, int *in2, int *wrap)\n+static int parse_wrap_args(const struct option *opt, const char *arg, int unset)\n {\n-\targ += 2; /* skip -w */\n-\n-\t*wrap = parse_uint(&arg, ',');\n-\tif (*wrap < 0)\n-\t\tdie(wrap_arg_usage);\n-\t*in1 = parse_uint(&arg, ',');\n-\tif (*in1 < 0)\n-\t\tdie(wrap_arg_usage);\n-\t*in2 = parse_uint(&arg, '\\0');\n-\tif (*in2 < 0)\n-\t\tdie(wrap_arg_usage);\n-\n-\tif (!*wrap)\n-\t\t*wrap = DEFAULT_WRAPLEN;\n-\tif (!*in1)\n-\t\t*in1 = DEFAULT_INDENT1;\n-\tif (!*in2)\n-\t\t*in2 = DEFAULT_INDENT2;\n-\tif (*wrap &&\n-\t ((*in1 && *wrap <= *in1) ||\n-\t (*in2 && *wrap <= *in2)))\n-\t\tdie(wrap_arg_usage);\n+\tstruct shortlog *log = opt->value;\n+\n+\tlog->wrap_lines = !unset;\n+\tif (unset)\n+\t\treturn 0;\n+\tif (!arg) {\n+\t\tlog->wrap = DEFAULT_WRAPLEN;\n+\t\tlog->in1 = DEFAULT_INDENT1;\n+\t\tlog->in2 = DEFAULT_INDENT2;\n+\t\treturn 0;\n+\t}\n+\n+\tlog->wrap = parse_uint(&arg, ',', DEFAULT_WRAPLEN);\n+\tlog->in1 = parse_uint(&arg, ',', DEFAULT_INDENT1);\n+\tlog->in2 = parse_uint(&arg, '\\0', DEFAULT_INDENT2);\n+\tif (log->wrap < 0 || log->in1 < 0 || log->in2 < 0)\n+\t\treturn error(wrap_arg_usage);\n+\tif (log->wrap &&\n+\t ((log->in1 && log->wrap <= log->in1) ||\n+\t (log->in2 && log->wrap <= log->in2)))\n+\t\treturn error(wrap_arg_usage);\n+\treturn 0;\n }\n \n void shortlog_init(struct shortlog *log)\n@@ -227,38 +230,54 @@ void shortlog_init(struct shortlog *log)\n \n int cmd_shortlog(int argc, const char **argv, const char *prefix)\n {\n-\tstruct shortlog log;\n-\tstruct rev_info rev;\n+\tstatic struct shortlog log;\n+\tstatic struct rev_info rev;\n \tint nongit;\n \n+\tstatic const struct option options[] = {\n+\t\tOPT_BOOLEAN('n', \"numbered\", &log.sort_by_number,\n+\t\t\t \"sort output according to the number of commits per author\"),\n+\t\tOPT_BOOLEAN('s', \"summary\", &log.summary,\n+\t\t\t \"Suppress commit descriptions, only provides commit count\"),\n+\t\tOPT_BOOLEAN('e', \"email\", &log.email,\n+\t\t\t \"Show the email address of each author\"),\n+\t\t{ OPTION_CALLBACK, 'w', NULL, &log, \"w[,i1[,i2]]\",\n+\t\t\t\"Linewrap output\", PARSE_OPT_OPTARG, &parse_wrap_args },\n+\t\tOPT_END(),\n+\t};\n+\n+\tstruct parse_opt_ctx_t ctx;\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+\tinit_revisions(&rev, prefix);\n+\tparse_options_start(&ctx, argc, argv, PARSE_OPT_KEEP_DASHDASH |\n+\t\t\t PARSE_OPT_KEEP_ARGV0);\n+\n+\tfor (;;) {\n+\t\tint n;\n+\t\tswitch (parse_options_step(&ctx, options, shortlog_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-\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\tn = handle_revision_opt(&rev, ctx.argc, ctx.argv,\n+\t\t\t\t\t&ctx.cpidx, ctx.out);\n+\t\tif (n <= 0) {\n+\t\t\terror(\"unknown option `%s'\", ctx.argv[0]);\n+\t\t\tusage_with_options(shortlog_usage, options);\n+\t\t}\n+\t\tctx.argv += n;\n+\t\tctx.argc -= n;\n+\t}\n+parse_done:\n+\targc = parse_options_end(&ctx);\n+\n+\tif (setup_revisions(argc, argv, &rev, NULL) != 1) {\n+\t\terror(\"unrecognized argument: %s\", argv[1]);\n+\t\tusage_with_options(shortlog_usage, options);\n \t}\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 \n \t/* assume HEAD if from a tty */\n \tif (!nongit && !rev.pending.nr && isatty(0))\n-- \n1.5.6.2.428.gdce6.dirty\n"},{"id":"82758","messageId":"1215639514-1612-2-git-send-email-madcoder@debian.org","threadId":"14385","inReplyTo":"1215639514-1612-1-git-send-email-madcoder@debian.org","subject":"[PATCH] revisions: refactor handle_revision_opt into parse_revision_opt.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-07-09T21:38:34Z","receivedAt":"2008-07-09T21:38:34Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"It seems we're using handle_revision_opt the same way each time, have a\nwrapper around it that does the 9-liner we copy each time instead.\n\nhandle_revision_opt can be static in the module for now, it's always\npossible to make it public again if needed.\n\nSigned-off-by: Pierre Habouzit \n---\n builtin-blame.c | 11 +----------\n builtin-shortlog.c | 10 +---------\n revision.c | 18 ++++++++++++++++--\n revision.h | 7 +++++--\n 4 files changed, 23 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 73d26c6..06c7de4 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -2305,8 +2305,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tparse_options_start(&ctx, argc, argv, PARSE_OPT_KEEP_DASHDASH |\n \t\t\t PARSE_OPT_KEEP_ARGV0);\n \tfor (;;) {\n-\t\tint n;\n-\n \t\tswitch (parse_options_step(&ctx, options, blame_opt_usage)) {\n \t\tcase PARSE_OPT_HELP:\n \t\t\texit(129);\n@@ -2320,14 +2318,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\t\tctx.argv[0] = \"--children\";\n \t\t\treverse = 1;\n \t\t}\n-\t\tn = handle_revision_opt(&revs, ctx.argc, ctx.argv,\n-\t\t\t\t\t&ctx.cpidx, ctx.out);\n-\t\tif (n <= 0) {\n-\t\t\terror(\"unknown option `%s'\", ctx.argv[0]);\n-\t\t\tusage_with_options(blame_opt_usage, options);\n-\t\t}\n-\t\tctx.argv += n;\n-\t\tctx.argc -= n;\n+\t\tparse_revision_opt(&revs, &ctx, options, blame_opt_usage);\n \t}\n parse_done:\n \targc = parse_options_end(&ctx);\ndiff --git a/builtin-shortlog.c b/builtin-shortlog.c\nindex 9107bff..0136202 100644\n--- a/builtin-shortlog.c\n+++ b/builtin-shortlog.c\n@@ -255,21 +255,13 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_KEEP_ARGV0);\n \n \tfor (;;) {\n-\t\tint n;\n \t\tswitch (parse_options_step(&ctx, options, shortlog_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-\t\tn = handle_revision_opt(&rev, ctx.argc, ctx.argv,\n-\t\t\t\t\t&ctx.cpidx, ctx.out);\n-\t\tif (n <= 0) {\n-\t\t\terror(\"unknown option `%s'\", ctx.argv[0]);\n-\t\t\tusage_with_options(shortlog_usage, options);\n-\t\t}\n-\t\tctx.argv += n;\n-\t\tctx.argc -= n;\n+\t\tparse_revision_opt(&rev, &ctx, options, shortlog_usage);\n \t}\n parse_done:\n \targc = parse_options_end(&ctx);\ndiff --git a/revision.c b/revision.c\nindex 9d5d933..bbd563e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -974,8 +974,8 @@ static void add_ignore_packed(struct rev_info *revs, const char *name)\n \trevs->ignore_packed[num] = NULL;\n }\n \n-int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,\n-\t\t\tint *unkc, const char **unkv)\n+static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,\n+\t\t\t int *unkc, const char **unkv)\n {\n \tconst char *arg = argv[0];\n \n@@ -1180,6 +1180,20 @@ int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,\n \treturn 1;\n }\n \n+void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n+\t\t\tconst struct option *options,\n+\t\t\tconst char * const usagestr[])\n+{\n+\tint n = handle_revision_opt(revs, ctx->argc, ctx->argv,\n+\t\t\t\t &ctx->cpidx, ctx->out);\n+\tif (n <= 0) {\n+\t\terror(\"unknown option `%s'\", ctx->argv[0]);\n+\t\tusage_with_options(usagestr, options);\n+\t}\n+\tctx->argv += n;\n+\tctx->argc -= n;\n+}\n+\n /*\n * Parse revision information, filling in the \"rev_info\" structure,\n * and removing the used arguments from the argument list.\ndiff --git a/revision.h b/revision.h\nindex cc80fcd..fa68c65 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -1,6 +1,8 @@\n #ifndef REVISION_H\n #define REVISION_H\n \n+#include \"parse-options.h\"\n+\n #define SEEN\t\t(1u<<0)\n #define UNINTERESTING (1u<<1)\n #define TREESAME\t(1u<<2)\n@@ -122,8 +124,9 @@ volatile show_early_output_fn_t show_early_output;\n \n extern void init_revisions(struct rev_info *revs, const char *prefix);\n extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def);\n-extern int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,\n-\t\t\t int *unkc, const char **unkv);\n+extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n+\t\t\t\t const struct option *options,\n+\t\t\t\t const char * const usagestr[]);\n extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,int cant_be_filename);\n \n extern int prepare_revision_walk(struct rev_info *revs);\n-- \n1.5.6.2.428.gdce6.dirty\n"},{"id":"82786","messageId":"20080710071418.GD3195@sigill.intra.peff.net","threadId":"14385","inReplyTo":"1215639514-1612-2-git-send-email-madcoder@debian.org","subject":"Re: [PATCH] revisions: refactor handle_revision_opt into parse_revision_opt.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-10T07:14:18Z","receivedAt":"2008-07-10T07:14:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 09, 2008 at 11:38:34PM +0200, Pierre Habouzit wrote:\n\n> It seems we're using handle_revision_opt the same way each time, have a\n> wrapper around it that does the 9-liner we copy each time instead.\n> \n> handle_revision_opt can be static in the module for now, it's always\n> possible to make it public again if needed.\n\nI have been on the road, and I finally got the chance to read through\nyour whole parseopt/blame refactoring. I think it looks good overall, as\ndo these patches.\n\nI have one comment, though I am not sure it is worth implementing.\n\nI was happy to see this refactoring, which I think improves readability:\n\n> -\t\tn = handle_revision_opt(&revs, ctx.argc, ctx.argv,\n> -\t\t\t\t\t&ctx.cpidx, ctx.out);\n> -\t\tif (n <= 0) {\n> -\t\t\terror(\"unknown option `%s'\", ctx.argv[0]);\n> -\t\t\tusage_with_options(blame_opt_usage, options);\n> -\t\t}\n> -\t\tctx.argv += n;\n> -\t\tctx.argc -= n;\n> +\t\tparse_revision_opt(&revs, &ctx, options, blame_opt_usage);\n\nbut it also seems like the top bit of that for loop is boilerplate, too:\n\n> \tfor (;;) {\n> \t\tswitch (parse_options_step(&ctx, options, shortlog_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\nAFAICT, the main reason for not folding this into your refactored\nfunction is that after the parse_options_step, but before we handle the\nrevision arg to parse_revision_opt, there needs to be an opportunity for\nthe caller to intercept and do something based on revision opts (like\nblame does with --reverse):\n\n\tif (!strcmp(ctx.argv[0], \"--reverse\")) {\n\t\tctx.argv[0] = \"--children\";\n\t\treverse = 1;\n\t}\n\nBut I wonder if it would be a suitable alternative to just add\n\"--reverse\" in this case to the blame options, but with an option flag\nfor \"parse me, but also pass me along to the next parser\" (which would\nbe added). Then we could do our thing in a callback.\n\nOf course, in this case, we do something a bit tricky by actually\n_rewriting_ the argument to \"--children\". So we would have to have\nsupport for callbacks rewriting arguments, or it would have to manually\ndo what \"--children\" should do. So perhaps it isn't worth the trouble.\nThis particular boilerplate is at least not very error-prone.\n\nSo food for thought, mainly, I suppose. Apologies if you already thought\nof this and I missed the discussion. I think I am up to date on my\nback-reading of the git list, but it is easy to lose some threads. :)\n\n-Peff\n"},{"id":"82790","messageId":"20080710073450.GA24819@artemis.madism.org","threadId":"14385","inReplyTo":"20080710071418.GD3195@sigill.intra.peff.net","subject":"Re: [PATCH] revisions: refactor handle_revision_opt into parse_revision_opt.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-07-10T07:34:50Z","receivedAt":"2008-07-10T07:34:50Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Jul 10, 2008 at 07:14:18AM +0000, Jeff King wrote:\n> On Wed, Jul 09, 2008 at 11:38:34PM +0200, Pierre Habouzit wrote:\n> \n> > It seems we're using handle_revision_opt the same way each time, have a\n> > wrapper around it that does the 9-liner we copy each time instead.\n> > \n> > handle_revision_opt can be static in the module for now, it's always\n> > possible to make it public again if needed.\n> \n> I have been on the road, and I finally got the chance to read through\n> your whole parseopt/blame refactoring. I think it looks good overall, as\n> do these patches.\n> \n> I have one comment, though I am not sure it is worth implementing.\n> \n> I was happy to see this refactoring, which I think improves readability:\n> \n> > -\t\tn = handle_revision_opt(&revs, ctx.argc, ctx.argv,\n> > -\t\t\t\t\t&ctx.cpidx, ctx.out);\n> > -\t\tif (n <= 0) {\n> > -\t\t\terror(\"unknown option `%s'\", ctx.argv[0]);\n> > -\t\t\tusage_with_options(blame_opt_usage, options);\n> > -\t\t}\n> > -\t\tctx.argv += n;\n> > -\t\tctx.argc -= n;\n> > +\t\tparse_revision_opt(&revs, &ctx, options, blame_opt_usage);\n> \n> but it also seems like the top bit of that for loop is boilerplate, too:\n> \n> > \tfor (;;) {\n> > \t\tswitch (parse_options_step(&ctx, options, shortlog_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> AFAICT, the main reason for not folding this into your refactored\n> function is that after the parse_options_step, but before we handle the\n> revision arg to parse_revision_opt, there needs to be an opportunity for\n> the caller to intercept and do something based on revision opts (like\n> blame does with --reverse):\n> \n> \tif (!strcmp(ctx.argv[0], \"--reverse\")) {\n> \t\tctx.argv[0] = \"--children\";\n> \t\treverse = 1;\n> \t}\n\n The other thing I would like to do is remove the exit(129) and replace\nit with proper documentation for the rev-list options, and this will\ndepend upon the fact that we parse revisions or something else inside\nthe loop. Of course this is boilerplate, but well, I wouldn't like to\nhide it and prevent people from thinking they can hook other things in\nthere.\n\n And I'm not very keen on adding more options to parse-options like you\npropose, our endgame is to get rid of parse_revision_opt in this form\nand have a full parse-opt thing from top to bottom. the \"--reverse\" hack\ncould be done really differently, because we really know what\n\"--children\" does and we could directly do what the revision option\nparser does. But oh well... For now I'm more interested to see more\ncommands migrated thanks to this facility, and see what we can refactor\nto get rid of the old parsers at once.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"82791","messageId":"7vfxqiqjgd.fsf@gitster.siamese.dyndns.org","threadId":"14385","inReplyTo":"20080710071418.GD3195@sigill.intra.peff.net","subject":"Re: [PATCH] revisions: refactor handle_revision_opt into parse_revision_opt.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-10T07:36:34Z","receivedAt":"2008-07-10T07:36:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> but it also seems like the top bit of that for loop is boilerplate, too:\n>\n>> \tfor (;;) {\n>> \t\tswitch (parse_options_step(&ctx, options, shortlog_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\nAnother thing we should be able to refactor is the option help formatter.\nCf. 071438a (Teach git-merge -X