{"thread":{"id":"46102","subject":"[PATCH 0/3] Use skip_prefix() in handle_revision_{,pseudo_}opt()","startedAt":"2017-06-02T19:10:22Z","lastAt":"2017-06-13T00:50:43Z","messageCount":22,"participants":["SZEDER Gábor","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"321418","messageId":"20170602191010.16834-1-szeder.dev@gmail.com","threadId":"46102","inReplyTo":null,"subject":"[PATCH 0/3] Use skip_prefix() in handle_revision_{,pseudo_}opt()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-02T19:10:07Z","receivedAt":"2017-06-02T19:10:22Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"While at it, the first one fixes a minor bug, which allowed e.g. 'git\nlog --no-min-parents-foobarbaz' to succeed.\n\nThe other two are fairly straightforward starts_with() ->\nskip_prefix() conversions.\n\nSZEDER Gábor (3):\n  revision.c: stricter parsing of '--no-{min,max}-parents'\n  revision.c: use skip_prefix() in handle_revision_opt()\n  revision.c: use skip_prefix() in handle_revision_pseudo_opt()\n\n revision.c | 76 ++++++++++++++++++++++++++++++++------------------------------\n 1 file changed, 39 insertions(+), 37 deletions(-)\n\n-- \n2.13.0.420.g54001f015\n\n"},{"id":"321419","messageId":"20170602191010.16834-2-szeder.dev@gmail.com","threadId":"46102","inReplyTo":"20170602191010.16834-1-szeder.dev@gmail.com","subject":"[PATCH 1/3] revision.c: stricter parsing of '--no-{min,max}-parents'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-02T19:10:08Z","receivedAt":"2017-06-02T19:10:23Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"These two options are parsed using starts_with(), allowing things like\n'git log --no-min-parents-foobarbaz' to succeed.\n\nUse strcmp() instead.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n revision.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex f88c14bab..2f37e1e3a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1812,11 +1812,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->max_parents = 1;\n \t} else if (starts_with(arg, \"--min-parents=\")) {\n \t\trevs->min_parents = atoi(arg+14);\n-\t} else if (starts_with(arg, \"--no-min-parents\")) {\n+\t} else if (!strcmp(arg, \"--no-min-parents\")) {\n \t\trevs->min_parents = 0;\n \t} else if (starts_with(arg, \"--max-parents=\")) {\n \t\trevs->max_parents = atoi(arg+14);\n-\t} else if (starts_with(arg, \"--no-max-parents\")) {\n+\t} else if (!strcmp(arg, \"--no-max-parents\")) {\n \t\trevs->max_parents = -1;\n \t} else if (!strcmp(arg, \"--boundary\")) {\n \t\trevs->boundary = 1;\n-- \n2.13.0.420.g54001f015\n\n"},{"id":"321420","messageId":"20170602191010.16834-3-szeder.dev@gmail.com","threadId":"46102","inReplyTo":"20170602191010.16834-1-szeder.dev@gmail.com","subject":"[PATCH 2/3] revision.c: use skip_prefix() in handle_revision_opt()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-02T19:10:09Z","receivedAt":"2017-06-02T19:10:25Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Instead of starts_with() and a bunch of magic numbers.\n\nWhile at it, there is an indentation fix where processing\n'--early-output', and a coding style fix where processing\n'--show-notes'.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n revision.c | 54 ++++++++++++++++++++++++++++--------------------------\n 1 file changed, 28 insertions(+), 26 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 2f37e1e3a..2b64b7e0e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1725,8 +1725,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->max_count = atoi(argv[1]);\n \t\trevs->no_walk = 0;\n \t\treturn 2;\n-\t} else if (starts_with(arg, \"-n\")) {\n-\t\trevs->max_count = atoi(arg + 2);\n+\t} else if (skip_prefix(arg, \"-n\", &optarg)) {\n+\t\trevs->max_count = atoi(optarg);\n \t\trevs->no_walk = 0;\n \t} else if ((argcount = parse_long_opt(\"max-age\", argv, &optarg))) {\n \t\trevs->max_age = atoi(optarg);\n@@ -1785,15 +1785,15 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--author-date-order\")) {\n \t\trevs->sort_order = REV_SORT_BY_AUTHOR_DATE;\n \t\trevs->topo_order = 1;\n-\t} else if (starts_with(arg, \"--early-output\")) {\n+\t} else if (skip_prefix(arg, \"--early-output\", &optarg)) {\n \t\tint count = 100;\n-\t\tswitch (arg[14]) {\n+\t\tswitch (*optarg) {\n \t\tcase '=':\n-\t\t\tcount = atoi(arg+15);\n+\t\t\tcount = atoi(optarg + 1);\n \t\t\t/* Fallthrough */\n \t\tcase 0:\n \t\t\trevs->topo_order = 1;\n-\t\t       revs->early_output = count;\n+\t\t\trevs->early_output = count;\n \t\t}\n \t} else if (!strcmp(arg, \"--parents\")) {\n \t\trevs->rewrite_parents = 1;\n@@ -1810,12 +1810,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->min_parents = 2;\n \t} else if (!strcmp(arg, \"--no-merges\")) {\n \t\trevs->max_parents = 1;\n-\t} else if (starts_with(arg, \"--min-parents=\")) {\n-\t\trevs->min_parents = atoi(arg+14);\n+\t} else if (skip_prefix(arg, \"--min-parents=\", &optarg)) {\n+\t\trevs->min_parents = atoi(optarg);\n \t} else if (!strcmp(arg, \"--no-min-parents\")) {\n \t\trevs->min_parents = 0;\n-\t} else if (starts_with(arg, \"--max-parents=\")) {\n-\t\trevs->max_parents = atoi(arg+14);\n+\t} else if (skip_prefix(arg, \"--max-parents=\", &optarg)) {\n+\t\trevs->max_parents = atoi(optarg);\n \t} else if (!strcmp(arg, \"--no-max-parents\")) {\n \t\trevs->max_parents = -1;\n \t} else if (!strcmp(arg, \"--boundary\")) {\n@@ -1897,14 +1897,15 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->verbose_header = 1;\n \t\trevs->pretty_given = 1;\n \t\tget_commit_format(NULL, revs);\n-\t} else if (starts_with(arg, \"--pretty=\") || starts_with(arg, \"--format=\")) {\n+\t} else if (skip_prefix(arg, \"--pretty=\", &optarg) ||\n+\t\t   skip_prefix(arg, \"--format=\", &optarg)) {\n \t\t/*\n \t\t * Detached form (\"--pretty X\" as opposed to \"--pretty=X\")\n \t\t * not allowed, since the argument is optional.\n \t\t */\n \t\trevs->verbose_header = 1;\n \t\trevs->pretty_given = 1;\n-\t\tget_commit_format(arg+9, revs);\n+\t\tget_commit_format(optarg, revs);\n \t} else if (!strcmp(arg, \"--expand-tabs\")) {\n \t\trevs->expand_tabs_in_log = 8;\n \t} else if (!strcmp(arg, \"--no-expand-tabs\")) {\n@@ -1922,26 +1923,27 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->show_signature = 1;\n \t} else if (!strcmp(arg, \"--no-show-signature\")) {\n \t\trevs->show_signature = 0;\n-\t} else if (!strcmp(arg, \"--show-linear-break\") ||\n-\t\t   starts_with(arg, \"--show-linear-break=\")) {\n-\t\tif (starts_with(arg, \"--show-linear-break=\"))\n-\t\t\trevs->break_bar = xstrdup(arg + 20);\n-\t\telse\n+\t} else if (skip_prefix(arg, \"--show-linear-break\", &optarg)) {\n+\t\tswitch (*optarg) {\n+\t\tcase '=':\n+\t\t\trevs->break_bar = xstrdup(optarg + 1);\n+\t\t\tbreak;\n+\t\tcase 0:\n \t\t\trevs->break_bar = \"                    ..........\";\n+\t\t}\n \t\trevs->track_linear = 1;\n \t\trevs->track_first_time = 1;\n-\t} else if (starts_with(arg, \"--show-notes=\") ||\n-\t\t   starts_with(arg, \"--notes=\")) {\n+\t} else if (skip_prefix(arg, \"--show-notes=\", &optarg) ||\n+\t\t   skip_prefix(arg, \"--notes=\", &optarg)) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\trevs->show_notes = 1;\n \t\trevs->show_notes_given = 1;\n-\t\tif (starts_with(arg, \"--show-notes\")) {\n+\t\tif (starts_with(arg, \"--show-notes=\")) {\n \t\t\tif (revs->notes_opt.use_default_notes < 0)\n \t\t\t\trevs->notes_opt.use_default_notes = 1;\n-\t\t\tstrbuf_addstr(&buf, arg+13);\n-\t\t}\n-\t\telse\n-\t\t\tstrbuf_addstr(&buf, arg+8);\n+\t\t\tstrbuf_addstr(&buf, optarg);\n+\t\t} else\n+\t\t\tstrbuf_addstr(&buf, optarg);\n \t\texpand_notes_ref(&buf);\n \t\tstring_list_append(&revs->notes_opt.extra_notes_refs,\n \t\t\t\t   strbuf_detach(&buf, NULL));\n@@ -1978,8 +1980,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->abbrev = 0;\n \t} else if (!strcmp(arg, \"--abbrev\")) {\n \t\trevs->abbrev = DEFAULT_ABBREV;\n-\t} else if (starts_with(arg, \"--abbrev=\")) {\n-\t\trevs->abbrev = strtoul(arg + 9, NULL, 10);\n+\t} else if (skip_prefix(arg, \"--abbrev=\", &optarg)) {\n+\t\trevs->abbrev = strtoul(optarg, NULL, 10);\n \t\tif (revs->abbrev < MINIMUM_ABBREV)\n \t\t\trevs->abbrev = MINIMUM_ABBREV;\n \t\telse if (revs->abbrev > 40)\n-- \n2.13.0.420.g54001f015\n\n"},{"id":"321421","messageId":"20170602191010.16834-4-szeder.dev@gmail.com","threadId":"46102","inReplyTo":"20170602191010.16834-1-szeder.dev@gmail.com","subject":"[PATCH 3/3] revision.c: use skip_prefix() in handle_revision_pseudo_opt()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-02T19:10:10Z","receivedAt":"2017-06-02T19:10:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Instead of starts_with() and a bunch of magic numbers.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n revision.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 2b64b7e0e..ab0279572 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2142,20 +2142,20 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t} else if ((argcount = parse_long_opt(\"exclude\", argv, &optarg))) {\n \t\tadd_ref_exclusion(&revs->ref_excludes, optarg);\n \t\treturn argcount;\n-\t} else if (starts_with(arg, \"--branches=\")) {\n+\t} else if (skip_prefix(arg, \"--branches=\", &optarg)) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref_in(handle_one_ref, arg + 11, \"refs/heads/\", &cb);\n+\t\tfor_each_glob_ref_in(handle_one_ref, optarg, \"refs/heads/\", &cb);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n-\t} else if (starts_with(arg, \"--tags=\")) {\n+\t} else if (skip_prefix(arg, \"--tags=\", &optarg)) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref_in(handle_one_ref, arg + 7, \"refs/tags/\", &cb);\n+\t\tfor_each_glob_ref_in(handle_one_ref, optarg, \"refs/tags/\", &cb);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n-\t} else if (starts_with(arg, \"--remotes=\")) {\n+\t} else if (skip_prefix(arg, \"--remotes=\", &optarg)) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref_in(handle_one_ref, arg + 10, \"refs/remotes/\", &cb);\n+\t\tfor_each_glob_ref_in(handle_one_ref, optarg, \"refs/remotes/\", &cb);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--reflog\")) {\n \t\tadd_reflogs_to_pending(revs, *flags);\n@@ -2165,14 +2165,14 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\t*flags ^= UNINTERESTING | BOTTOM;\n \t} else if (!strcmp(arg, \"--no-walk\")) {\n \t\trevs->no_walk = REVISION_WALK_NO_WALK_SORTED;\n-\t} else if (starts_with(arg, \"--no-walk=\")) {\n+\t} else if (skip_prefix(arg, \"--no-walk=\", &optarg)) {\n \t\t/*\n \t\t * Detached form (\"--no-walk X\" as opposed to \"--no-walk=X\")\n \t\t * not allowed, since the argument is optional.\n \t\t */\n-\t\tif (!strcmp(arg + 10, \"sorted\"))\n+\t\tif (!strcmp(optarg, \"sorted\"))\n \t\t\trevs->no_walk = REVISION_WALK_NO_WALK_SORTED;\n-\t\telse if (!strcmp(arg + 10, \"unsorted\"))\n+\t\telse if (!strcmp(optarg, \"unsorted\"))\n \t\t\trevs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;\n \t\telse\n \t\t\treturn error(\"invalid argument to --no-walk\");\n-- \n2.13.0.420.g54001f015\n\n"},{"id":"321432","messageId":"20170602201143.6avkhp224no3dukb@sigill.intra.peff.net","threadId":"46102","inReplyTo":"20170602191010.16834-3-szeder.dev@gmail.com","subject":"Re: [PATCH 2/3] revision.c: use skip_prefix() in handle_revision_opt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-02T20:11:43Z","receivedAt":"2017-06-02T20:11:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 02, 2017 at 09:10:09PM +0200, SZEDER Gábor wrote:\n\n> @@ -1785,15 +1785,15 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>  \t} else if (!strcmp(arg, \"--author-date-order\")) {\n>  \t\trevs->sort_order = REV_SORT_BY_AUTHOR_DATE;\n>  \t\trevs->topo_order = 1;\n> -\t} else if (starts_with(arg, \"--early-output\")) {\n> +\t} else if (skip_prefix(arg, \"--early-output\", &optarg)) {\n>  \t\tint count = 100;\n> -\t\tswitch (arg[14]) {\n> +\t\tswitch (*optarg) {\n>  \t\tcase '=':\n> -\t\t\tcount = atoi(arg+15);\n> +\t\t\tcount = atoi(optarg + 1);\n>  \t\t\t/* Fallthrough */\n>  \t\tcase 0:\n>  \t\t\trevs->topo_order = 1;\n> -\t\t       revs->early_output = count;\n> +\t\t\trevs->early_output = count;\n>  \t\t}\n\nWhat happens if I say \"--early-output-foobar\"? There should probably be\na \"default\" here that rejects it. Though we'd probably to goto to get to\nthe unknown block, yuck.\n\nPerhaps we could do:\n\n  if (skip_prefix(arg, \"--early-output\", &optarg) &&\n      (*optarg == '=' || !*optarg)) {\n          int count = *optarg ? atoi(optarg + 1) : 100;\n\t  revs->topo_order = 1;\n\t  revs->early_output = count;\n  }\n\nAlternatively, a helper like:\n\n  int match_opt(const char *have, const char *want, const char **argout)\n  {\n\tconst char *arg;\n\tif (!skip_prefix(have, want, &arg))\n\t\treturn 0;\n\tif (!*arg)\n\t\t*argout = NULL;\n\telse if (*arg == '=')\n\t\t*argout = arg + 1;\n\telse\n\t\treturn 0;\n\treturn 1;\n  }\n\nwould let us do:\n\n  if (match_opt(arg, \"--early-output\"), &optarg)) {\n\tint count = optarg ? atoi(optarg) : 100;\n\t...\n  }\n\nwhich is a little nicer and could maybe help other options (I didn't see\nany, though). If we're going to go that route, though, I suspect there\nmay be some helpers we already have. Looks like parse_long_opt() is\nalmost there, but doesn't handle options. I wonder if we could reuse\nbits of parse-options here (or even better, just parse-optify many of\nthese).\n\nAnyway, none of that is caused by your patch, but at least doing the\nminimal fix (my first hunk) seems like it fits into your series.\n\n-Peff\n"},{"id":"321435","messageId":"20170602201533.bw4cmsqsygfdif2o@sigill.intra.peff.net","threadId":"46102","inReplyTo":"20170602201143.6avkhp224no3dukb@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] revision.c: use skip_prefix() in handle_revision_opt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-02T20:15:33Z","receivedAt":"2017-06-02T20:15:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 02, 2017 at 04:11:43PM -0400, Jeff King wrote:\n\n>   if (match_opt(arg, \"--early-output\"), &optarg)) {\n> \tint count = optarg ? atoi(optarg) : 100;\n> \t...\n>   }\n> \n> which is a little nicer and could maybe help other options (I didn't see\n> any, though).\n\nI take it back. This would help --show-linear-break that your patch also\ntouches. And maybe --pretty, too.\n\n-Peff\n"},{"id":"321436","messageId":"20170602201733.r277dubonpyuavqv@sigill.intra.peff.net","threadId":"46102","inReplyTo":"20170602191010.16834-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/3] Use skip_prefix() in handle_revision_{,pseudo_}opt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-02T20:17:33Z","receivedAt":"2017-06-02T20:17:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 02, 2017 at 09:10:07PM +0200, SZEDER Gábor wrote:\n\n> While at it, the first one fixes a minor bug, which allowed e.g. 'git\n> log --no-min-parents-foobarbaz' to succeed.\n> \n> The other two are fairly straightforward starts_with() ->\n> skip_prefix() conversions.\n\nThese all look fine to me. It would be nice to address the extra cases I\nmentioned on top, but even without that this is a strict improvement.\n\n-Peff\n"},{"id":"321845","messageId":"20170609181733.6793-2-szeder.dev@gmail.com","threadId":"46102","inReplyTo":"20170609181733.6793-1-szeder.dev@gmail.com","subject":"[PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-09T18:17:29Z","receivedAt":"2017-06-09T18:18:07Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"rev_info.early_output started out as an unsigned int in cdcefbc97 (Add\n\"--early-output\" log flag for interactive GUI use, 2007-11-03), but\nlater it was turned into a single bit in a bit field in cc243c3ce\n(show: --ignore-missing, 2011-05-18) without explanation, though its\nusers still expect it to be a regular integer type.  Consequently, any\neven number given via '--early-output=<N>' effectively disabled the\nfeature.\n\nTurn it back into an unsigned int, restoring its original data type.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n revision.h | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.h b/revision.h\nindex a91dd3d5d..f96e7f7f4 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -74,8 +74,9 @@ struct rev_info {\n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n \n-\tunsigned int\tearly_output:1,\n-\t\t\tignore_missing:1,\n+\tunsigned int early_output;\n+\n+\tunsigned int\tignore_missing:1,\n \t\t\tignore_missing_links:1;\n \n \t/* Traversal flags */\n-- \n2.13.0.420.g54001f015\n\n"},{"id":"321846","messageId":"20170609181733.6793-1-szeder.dev@gmail.com","threadId":"46102","inReplyTo":"20170602201143.6avkhp224no3dukb@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] revision.c: use skip_prefix() in handle_revision_opt()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-09T18:17:28Z","receivedAt":"2017-06-09T18:18:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nOn Fri, Jun 2, 2017 at 10:11 PM, Jeff King <peff@peff.net> wrote:\n> On Fri, Jun 02, 2017 at 09:10:09PM +0200, SZEDER Gábor wrote:\n>\n>> @@ -1785,15 +1785,15 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>>       } else if (!strcmp(arg, \"--author-date-order\")) {\n>>               revs->sort_order = REV_SORT_BY_AUTHOR_DATE;\n>>               revs->topo_order = 1;\n>> -     } else if (starts_with(arg, \"--early-output\")) {\n>> +     } else if (skip_prefix(arg, \"--early-output\", &optarg)) {\n>>               int count = 100;\n>> -             switch (arg[14]) {\n>> +             switch (*optarg) {\n>>               case '=':\n>> -                     count = atoi(arg+15);\n>> +                     count = atoi(optarg + 1);\n>>                       /* Fallthrough */\n>>               case 0:\n>>                       revs->topo_order = 1;\n>> -                    revs->early_output = count;\n>> +                     revs->early_output = count;\n>>               }\n>\n> What happens if I say \"--early-output-foobar\"? There should probably be\n> a \"default\" here that rejects it. Though we'd probably to goto to get to\n> the unknown block, yuck.\n\nI don't even know what should happen when I say '--early--output', as\nit's not mentioned anywhere in 'git help log' :)\nAnd it's broken anyway...  see the first patch in v2.\n\nAs it is, 'git log' would act as if '--early-output-foobar' wasn't\nspecified at all: the switch statement only looks for '=' and '\\0',\nthat '-' it gets is neither and there is no default:, so it takes no\naction and the control flow resumes after the switch statement.\n\nAnyway, this option should be rejected, of course.\n\nEmbarrassingly, the code handling '--show-linear-break' did the right\nthing and refused '--show-linear-break-foo' until I came along and\nwith the switch to skip_prefix() I modelled its control flow after\nthat of '--early-output'.  So with this patch it too would ignore such\na bogus option.\n\n> Alternatively, a helper like:\n>\n>   int match_opt(const char *have, const char *want, const char **argout)\n>   {\n>         const char *arg;\n>         if (!skip_prefix(have, want, &arg))\n>                 return 0;\n>         if (!*arg)\n>                 *argout = NULL;\n>         else if (*arg == '=')\n>                 *argout = arg + 1;\n>         else\n>                 return 0;\n>         return 1;\n>   }\n>\n> would let us do:\n>\n>   if (match_opt(arg, \"--early-output\"), &optarg)) {\n>         int count = optarg ? atoi(optarg) : 100;\n>         ...\n>   }\n>\n> which is a little nicer and could maybe help other options (I didn't see\n> any, though).\n\nBesides '--show-linear-break' and '--pretty', other options that could\nbenefit from this, i.e. long options with an optional argument, are\n'--expand-tabs', '--abbrev' and '--no-walk'.  These are handled\ndifferently than '--early--output' and '--show-linear-break': each is\ncovered by two if branches, one with and one without the optional \nargument, i.e.:\n\n  } else if (!strcmp(arg, \"--option\")) {\n    ...\n  } else if (starts_with(arg, \"--option=\")) {\n    ...\n  } else ...\n\n'--pretty=' couldn't benefit, though, because it is special in that\nit's equivalent with '--format=', and the two are handled in the same\nbranch.\n\nSo inherently there are a few repeated option names and variable\nassignments, and that's not so good.  However, refactoring these to\nuse match_opt() adds 40% more lines than it removes and, more\nimportantly, increases the number of nested conditions.  Subjectively\nI don't think it's better, so I went with the \"follow the conventions\nof the surrounding code\" rule for the update.\n\n> If we're going to go that route, though, I suspect there\n> may be some helpers we already have. Looks like parse_long_opt() is\n> almost there, but doesn't handle options. I wonder if we could reuse\n> bits of parse-options here (or even better, just parse-optify many of\n> these).\n\nWell, parse-optifying this many options would be a tad too much for\nsomething that started out as a little while-at-it when I tried to\nmake sense of --format='%p', --parents/--children, -L:func:file and\ntheir various combinations :)\n\nAs far as I can tell, parse-options doesn't handle options with an\noptional argument by itself, but only with callback functions, so it\nis no help here as it is.\n\n\nSo, here comes v2.  The interdiff is below, the changes since v1 are:\n\n - Patch 1/5 is new to fix a more fundamental problem with\n   '--early-output'.\n - Patch 3/5 is new to fix this '--early-output-foo' issue and also\n   to tighten up the parsing of its integer argument, while at it.\n - A fix for '--show-linear-break-foo' in v1.\n - A little cleanup in the handling of '--show-notes/--notes'.\n\n\nSZEDER Gábor (5):\n  revision.h: turn rev_info.early_output back into an unsigned int\n  revision.c: stricter parsing of '--no-{min,max}-parents'\n  revision.c: stricter parsing of '--early-output'\n  revision.c: use skip_prefix() in handle_revision_opt()\n  revision.c: use skip_prefix() in handle_revision_pseudo_opt()\n\n revision.c | 87 +++++++++++++++++++++++++++++---------------------------------\n revision.h |  5 ++--\n 2 files changed, 44 insertions(+), 48 deletions(-)\n\n-- \n2.13.0.420.g54001f015\n\ndiff --git a/revision.c b/revision.c\nindex ab0279572..12a44189e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1785,16 +1785,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--author-date-order\")) {\n \t\trevs->sort_order = REV_SORT_BY_AUTHOR_DATE;\n \t\trevs->topo_order = 1;\n-\t} else if (skip_prefix(arg, \"--early-output\", &optarg)) {\n-\t\tint count = 100;\n-\t\tswitch (*optarg) {\n-\t\tcase '=':\n-\t\t\tcount = atoi(optarg + 1);\n-\t\t\t/* Fallthrough */\n-\t\tcase 0:\n-\t\t\trevs->topo_order = 1;\n-\t\t\trevs->early_output = count;\n-\t\t}\n+\t} else if (!strcmp(arg, \"--early-output\")) {\n+\t\trevs->early_output = 100;\n+\t\trevs->topo_order = 1;\n+\t} else if (skip_prefix(arg, \"--early-output=\", &optarg)) {\n+\t\tif (strtoul_ui(optarg, 10, &revs->early_output) < 0)\n+\t\t\tdie(\"'%s': not a non-negative integer\", optarg);\n+\t\trevs->topo_order = 1;\n \t} else if (!strcmp(arg, \"--parents\")) {\n \t\trevs->rewrite_parents = 1;\n \t\trevs->print_parents = 1;\n@@ -1923,14 +1920,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->show_signature = 1;\n \t} else if (!strcmp(arg, \"--no-show-signature\")) {\n \t\trevs->show_signature = 0;\n-\t} else if (skip_prefix(arg, \"--show-linear-break\", &optarg)) {\n-\t\tswitch (*optarg) {\n-\t\tcase '=':\n-\t\t\trevs->break_bar = xstrdup(optarg + 1);\n-\t\t\tbreak;\n-\t\tcase 0:\n-\t\t\trevs->break_bar = \"                    ..........\";\n-\t\t}\n+\t} else if (!strcmp(arg, \"--show-linear-break\")) {\n+\t\trevs->break_bar = \"                    ..........\";\n+\t\trevs->track_linear = 1;\n+\t\trevs->track_first_time = 1;\n+\t} else if (skip_prefix(arg, \"--show-linear-break=\", &optarg)) {\n+\t\trevs->break_bar = xstrdup(optarg);\n \t\trevs->track_linear = 1;\n \t\trevs->track_first_time = 1;\n \t} else if (skip_prefix(arg, \"--show-notes=\", &optarg) ||\n@@ -1938,12 +1933,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\trevs->show_notes = 1;\n \t\trevs->show_notes_given = 1;\n-\t\tif (starts_with(arg, \"--show-notes=\")) {\n-\t\t\tif (revs->notes_opt.use_default_notes < 0)\n-\t\t\t\trevs->notes_opt.use_default_notes = 1;\n-\t\t\tstrbuf_addstr(&buf, optarg);\n-\t\t} else\n-\t\t\tstrbuf_addstr(&buf, optarg);\n+\t\tif (starts_with(arg, \"--show-notes=\") &&\n+\t\t    revs->notes_opt.use_default_notes < 0)\n+\t\t\trevs->notes_opt.use_default_notes = 1;\n+\t\tstrbuf_addstr(&buf, optarg);\n \t\texpand_notes_ref(&buf);\n \t\tstring_list_append(&revs->notes_opt.extra_notes_refs,\n \t\t\t\t   strbuf_detach(&buf, NULL));\ndiff --git a/revision.h b/revision.h\nindex a91dd3d5d..f96e7f7f4 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -74,8 +74,9 @@ struct rev_info {\n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n \n-\tunsigned int\tearly_output:1,\n-\t\t\tignore_missing:1,\n+\tunsigned int early_output;\n+\n+\tunsigned int\tignore_missing:1,\n \t\t\tignore_missing_links:1;\n \n \t/* Traversal flags */\n"},{"id":"321847","messageId":"20170609181733.6793-3-szeder.dev@gmail.com","threadId":"46102","inReplyTo":"20170609181733.6793-1-szeder.dev@gmail.com","subject":"[PATCHv2 2/5] revision.c: stricter parsing of '--no-{min,max}-parents'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-09T18:17:30Z","receivedAt":"2017-06-09T18:18:13Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"These two options are parsed using starts_with(), allowing things like\n'git log --no-min-parents-foobarbaz' to succeed.\n\nUse strcmp() instead.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n revision.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex f88c14bab..2f37e1e3a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1812,11 +1812,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->max_parents = 1;\n \t} else if (starts_with(arg, \"--min-parents=\")) {\n \t\trevs->min_parents = atoi(arg+14);\n-\t} else if (starts_with(arg, \"--no-min-parents\")) {\n+\t} else if (!strcmp(arg, \"--no-min-parents\")) {\n \t\trevs->min_parents = 0;\n \t} else if (starts_with(arg, \"--max-parents=\")) {\n \t\trevs->max_parents = atoi(arg+14);\n-\t} else if (starts_with(arg, \"--no-max-parents\")) {\n+\t} else if (!strcmp(arg, \"--no-max-parents\")) {\n \t\trevs->max_parents = -1;\n \t} else if (!strcmp(arg, \"--boundary\")) {\n \t\trevs->boundary = 1;\n-- \n2.13.0.420.g54001f015\n\n"},{"id":"321848","messageId":"20170609181733.6793-4-szeder.dev@gmail.com","threadId":"46102","inReplyTo":"20170609181733.6793-1-szeder.dev@gmail.com","subject":"[PATCHv2 3/5] revision.c: stricter parsing of '--early-output'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-09T18:17:31Z","receivedAt":"2017-06-09T18:18:14Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The parsing of '--early-output' with or without its optional integer\nargument allowed bogus options like '--early-output-foobarbaz' to slip\nthrough and be ignored.\n\nFix it by parsing '--early-output' in the same way as other options\nwith an optional argument are parsed.  Furthermore, use strtoul_ui()\nto parse the optional integer argument and to refuse negative numbers.\n\nWhile at it, use skip_prefix() instead of starts_with() and magic\nnumbers.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n revision.c | 17 +++++++----------\n 1 file changed, 7 insertions(+), 10 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 2f37e1e3a..68531ff5d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1785,16 +1785,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--author-date-order\")) {\n \t\trevs->sort_order = REV_SORT_BY_AUTHOR_DATE;\n \t\trevs->topo_order = 1;\n-\t} else if (starts_with(arg, \"--early-output\")) {\n-\t\tint count = 100;\n-\t\tswitch (arg[14]) {\n-\t\tcase '=':\n-\t\t\tcount = atoi(arg+15);\n-\t\t\t/* Fallthrough */\n-\t\tcase 0:\n-\t\t\trevs->topo_order = 1;\n-\t\t       revs->early_output = count;\n-\t\t}\n+\t} else if (!strcmp(arg, \"--early-output\")) {\n+\t\trevs->early_output = 100;\n+\t\trevs->topo_order = 1;\n+\t} else if (skip_prefix(arg, \"--early-output=\", &optarg)) {\n+\t\tif (strtoul_ui(optarg, 10, &revs->early_output) < 0)\n+\t\t\tdie(\"'%s': not a non-negative integer\", optarg);\n+\t\trevs->topo_order = 1;\n \t} else if (!strcmp(arg, \"--parents\")) {\n \t\trevs->rewrite_parents = 1;\n \t\trevs->print_parents = 1;\n-- \n2.13.0.420.g54001f015\n\n"},{"id":"321849","messageId":"20170609181733.6793-6-szeder.dev@gmail.com","threadId":"46102","inReplyTo":"20170609181733.6793-1-szeder.dev@gmail.com","subject":"[PATCHv2 5/5] revision.c: use skip_prefix() in handle_revision_pseudo_opt()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-09T18:17:33Z","receivedAt":"2017-06-09T18:18:15Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Instead of starts_with() and a bunch of magic numbers.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n revision.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex c99c47c50..12a44189e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2135,20 +2135,20 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t} else if ((argcount = parse_long_opt(\"exclude\", argv, &optarg))) {\n \t\tadd_ref_exclusion(&revs->ref_excludes, optarg);\n \t\treturn argcount;\n-\t} else if (starts_with(arg, \"--branches=\")) {\n+\t} else if (skip_prefix(arg, \"--branches=\", &optarg)) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref_in(handle_one_ref, arg + 11, \"refs/heads/\", &cb);\n+\t\tfor_each_glob_ref_in(handle_one_ref, optarg, \"refs/heads/\", &cb);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n-\t} else if (starts_with(arg, \"--tags=\")) {\n+\t} else if (skip_prefix(arg, \"--tags=\", &optarg)) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref_in(handle_one_ref, arg + 7, \"refs/tags/\", &cb);\n+\t\tfor_each_glob_ref_in(handle_one_ref, optarg, \"refs/tags/\", &cb);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n-\t} else if (starts_with(arg, \"--remotes=\")) {\n+\t} else if (skip_prefix(arg, \"--remotes=\", &optarg)) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref_in(handle_one_ref, arg + 10, \"refs/remotes/\", &cb);\n+\t\tfor_each_glob_ref_in(handle_one_ref, optarg, \"refs/remotes/\", &cb);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--reflog\")) {\n \t\tadd_reflogs_to_pending(revs, *flags);\n@@ -2158,14 +2158,14 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\t*flags ^= UNINTERESTING | BOTTOM;\n \t} else if (!strcmp(arg, \"--no-walk\")) {\n \t\trevs->no_walk = REVISION_WALK_NO_WALK_SORTED;\n-\t} else if (starts_with(arg, \"--no-walk=\")) {\n+\t} else if (skip_prefix(arg, \"--no-walk=\", &optarg)) {\n \t\t/*\n \t\t * Detached form (\"--no-walk X\" as opposed to \"--no-walk=X\")\n \t\t * not allowed, since the argument is optional.\n \t\t */\n-\t\tif (!strcmp(arg + 10, \"sorted\"))\n+\t\tif (!strcmp(optarg, \"sorted\"))\n \t\t\trevs->no_walk = REVISION_WALK_NO_WALK_SORTED;\n-\t\telse if (!strcmp(arg + 10, \"unsorted\"))\n+\t\telse if (!strcmp(optarg, \"unsorted\"))\n \t\t\trevs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;\n \t\telse\n \t\t\treturn error(\"invalid argument to --no-walk\");\n-- \n2.13.0.420.g54001f015\n\n"},{"id":"321850","messageId":"20170609181733.6793-5-szeder.dev@gmail.com","threadId":"46102","inReplyTo":"20170609181733.6793-1-szeder.dev@gmail.com","subject":"[PATCHv2 4/5] revision.c: use skip_prefix() in handle_revision_opt()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-09T18:17:32Z","receivedAt":"2017-06-09T18:18:19Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Instead of starts_with() and a bunch of magic numbers.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n revision.c | 48 +++++++++++++++++++++++-------------------------\n 1 file changed, 23 insertions(+), 25 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 68531ff5d..c99c47c50 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1725,8 +1725,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->max_count = atoi(argv[1]);\n \t\trevs->no_walk = 0;\n \t\treturn 2;\n-\t} else if (starts_with(arg, \"-n\")) {\n-\t\trevs->max_count = atoi(arg + 2);\n+\t} else if (skip_prefix(arg, \"-n\", &optarg)) {\n+\t\trevs->max_count = atoi(optarg);\n \t\trevs->no_walk = 0;\n \t} else if ((argcount = parse_long_opt(\"max-age\", argv, &optarg))) {\n \t\trevs->max_age = atoi(optarg);\n@@ -1807,12 +1807,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->min_parents = 2;\n \t} else if (!strcmp(arg, \"--no-merges\")) {\n \t\trevs->max_parents = 1;\n-\t} else if (starts_with(arg, \"--min-parents=\")) {\n-\t\trevs->min_parents = atoi(arg+14);\n+\t} else if (skip_prefix(arg, \"--min-parents=\", &optarg)) {\n+\t\trevs->min_parents = atoi(optarg);\n \t} else if (!strcmp(arg, \"--no-min-parents\")) {\n \t\trevs->min_parents = 0;\n-\t} else if (starts_with(arg, \"--max-parents=\")) {\n-\t\trevs->max_parents = atoi(arg+14);\n+\t} else if (skip_prefix(arg, \"--max-parents=\", &optarg)) {\n+\t\trevs->max_parents = atoi(optarg);\n \t} else if (!strcmp(arg, \"--no-max-parents\")) {\n \t\trevs->max_parents = -1;\n \t} else if (!strcmp(arg, \"--boundary\")) {\n@@ -1894,14 +1894,15 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->verbose_header = 1;\n \t\trevs->pretty_given = 1;\n \t\tget_commit_format(NULL, revs);\n-\t} else if (starts_with(arg, \"--pretty=\") || starts_with(arg, \"--format=\")) {\n+\t} else if (skip_prefix(arg, \"--pretty=\", &optarg) ||\n+\t\t   skip_prefix(arg, \"--format=\", &optarg)) {\n \t\t/*\n \t\t * Detached form (\"--pretty X\" as opposed to \"--pretty=X\")\n \t\t * not allowed, since the argument is optional.\n \t\t */\n \t\trevs->verbose_header = 1;\n \t\trevs->pretty_given = 1;\n-\t\tget_commit_format(arg+9, revs);\n+\t\tget_commit_format(optarg, revs);\n \t} else if (!strcmp(arg, \"--expand-tabs\")) {\n \t\trevs->expand_tabs_in_log = 8;\n \t} else if (!strcmp(arg, \"--no-expand-tabs\")) {\n@@ -1919,26 +1920,23 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->show_signature = 1;\n \t} else if (!strcmp(arg, \"--no-show-signature\")) {\n \t\trevs->show_signature = 0;\n-\t} else if (!strcmp(arg, \"--show-linear-break\") ||\n-\t\t   starts_with(arg, \"--show-linear-break=\")) {\n-\t\tif (starts_with(arg, \"--show-linear-break=\"))\n-\t\t\trevs->break_bar = xstrdup(arg + 20);\n-\t\telse\n-\t\t\trevs->break_bar = \"                    ..........\";\n+\t} else if (!strcmp(arg, \"--show-linear-break\")) {\n+\t\trevs->break_bar = \"                    ..........\";\n+\t\trevs->track_linear = 1;\n+\t\trevs->track_first_time = 1;\n+\t} else if (skip_prefix(arg, \"--show-linear-break=\", &optarg)) {\n+\t\trevs->break_bar = xstrdup(optarg);\n \t\trevs->track_linear = 1;\n \t\trevs->track_first_time = 1;\n-\t} else if (starts_with(arg, \"--show-notes=\") ||\n-\t\t   starts_with(arg, \"--notes=\")) {\n+\t} else if (skip_prefix(arg, \"--show-notes=\", &optarg) ||\n+\t\t   skip_prefix(arg, \"--notes=\", &optarg)) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\trevs->show_notes = 1;\n \t\trevs->show_notes_given = 1;\n-\t\tif (starts_with(arg, \"--show-notes\")) {\n-\t\t\tif (revs->notes_opt.use_default_notes < 0)\n-\t\t\t\trevs->notes_opt.use_default_notes = 1;\n-\t\t\tstrbuf_addstr(&buf, arg+13);\n-\t\t}\n-\t\telse\n-\t\t\tstrbuf_addstr(&buf, arg+8);\n+\t\tif (starts_with(arg, \"--show-notes=\") &&\n+\t\t    revs->notes_opt.use_default_notes < 0)\n+\t\t\trevs->notes_opt.use_default_notes = 1;\n+\t\tstrbuf_addstr(&buf, optarg);\n \t\texpand_notes_ref(&buf);\n \t\tstring_list_append(&revs->notes_opt.extra_notes_refs,\n \t\t\t\t   strbuf_detach(&buf, NULL));\n@@ -1975,8 +1973,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->abbrev = 0;\n \t} else if (!strcmp(arg, \"--abbrev\")) {\n \t\trevs->abbrev = DEFAULT_ABBREV;\n-\t} else if (starts_with(arg, \"--abbrev=\")) {\n-\t\trevs->abbrev = strtoul(arg + 9, NULL, 10);\n+\t} else if (skip_prefix(arg, \"--abbrev=\", &optarg)) {\n+\t\trevs->abbrev = strtoul(optarg, NULL, 10);\n \t\tif (revs->abbrev < MINIMUM_ABBREV)\n \t\t\trevs->abbrev = MINIMUM_ABBREV;\n \t\telse if (revs->abbrev > 40)\n-- \n2.13.0.420.g54001f015\n\n"},{"id":"321877","messageId":"xmqqy3t030mf.fsf@gitster.mtv.corp.google.com","threadId":"46102","inReplyTo":"20170609181733.6793-4-szeder.dev@gmail.com","subject":"Re: [PATCHv2 3/5] revision.c: stricter parsing of '--early-output'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-10T04:52:40Z","receivedAt":"2017-06-10T04:52:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> The parsing of '--early-output' with or without its optional integer\n> argument allowed bogus options like '--early-output-foobarbaz' to slip\n> through and be ignored.\n>\n> Fix it by parsing '--early-output' in the same way as other options\n> with an optional argument are parsed.  Furthermore, use strtoul_ui()\n> to parse the optional integer argument and to refuse negative numbers.\n>\n> While at it, use skip_prefix() instead of starts_with() and magic\n> numbers.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>  revision.c | 17 +++++++----------\n>  1 file changed, 7 insertions(+), 10 deletions(-)\n>\n> diff --git a/revision.c b/revision.c\n> index 2f37e1e3a..68531ff5d 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1785,16 +1785,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>  \t} else if (!strcmp(arg, \"--author-date-order\")) {\n>  \t\trevs->sort_order = REV_SORT_BY_AUTHOR_DATE;\n>  \t\trevs->topo_order = 1;\n> -\t} else if (starts_with(arg, \"--early-output\")) {\n> -\t\tint count = 100;\n> -\t\tswitch (arg[14]) {\n> -\t\tcase '=':\n> -\t\t\tcount = atoi(arg+15);\n> -\t\t\t/* Fallthrough */\n> -\t\tcase 0:\n> -\t\t\trevs->topo_order = 1;\n> -\t\t       revs->early_output = count;\n> -\t\t}\n\nThis shows how correct patch 1/5 is, huh?  It's kind of surprising\nthat nobody complained about the breakage since May 2011 which in\nturn makes me suspect that early-output might not be missed if we\ndropped it someday, but that is a separate issue.  This series makes\nit work as the feature was envisioned to.\n\nThanks.\n\n\n> +\t} else if (!strcmp(arg, \"--early-output\")) {\n> +\t\trevs->early_output = 100;\n> +\t\trevs->topo_order = 1;\n> +\t} else if (skip_prefix(arg, \"--early-output=\", &optarg)) {\n> +\t\tif (strtoul_ui(optarg, 10, &revs->early_output) < 0)\n> +\t\t\tdie(\"'%s': not a non-negative integer\", optarg);\n> +\t\trevs->topo_order = 1;\n>  \t} else if (!strcmp(arg, \"--parents\")) {\n>  \t\trevs->rewrite_parents = 1;\n>  \t\trevs->print_parents = 1;\n"},{"id":"321885","messageId":"20170610063518.6ihim2za6vil24pt@sigill.intra.peff.net","threadId":"46102","inReplyTo":"20170609181733.6793-1-szeder.dev@gmail.com","subject":"Re: [PATCH 2/3] revision.c: use skip_prefix() in handle_revision_opt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-10T06:35:19Z","receivedAt":"2017-06-10T06:35:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 09, 2017 at 08:17:28PM +0200, SZEDER Gábor wrote:\n\n> > would let us do:\n> >\n> >   if (match_opt(arg, \"--early-output\"), &optarg)) {\n> >         int count = optarg ? atoi(optarg) : 100;\n> >         ...\n> >   }\n> >\n> > which is a little nicer and could maybe help other options (I didn't see\n> > any, though).\n> \n> Besides '--show-linear-break' and '--pretty', other options that could\n> benefit from this, i.e. long options with an optional argument, are\n> '--expand-tabs', '--abbrev' and '--no-walk'.  These are handled\n> differently than '--early--output' and '--show-linear-break': each is\n> covered by two if branches, one with and one without the optional \n> argument, i.e.:\n> \n>   } else if (!strcmp(arg, \"--option\")) {\n>     ...\n>   } else if (starts_with(arg, \"--option=\")) {\n>     ...\n>   } else ...\n\nI think those multi-branch cases end up as an improvement with a helper:\n\n  if (match_opt(arg, \"--no-walk\", &optarg)) {\n\tif (!optarg || !strcmp(optarg, \"sorted\"))\n\t\trevs->no_walk = REVISION_WALK_NO_WALK_SORTED;\n\telse if (!strcmp(optarg, \"unsorted\"))\n\t\trevs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;\n\telse\n\t\treturn error(...);\n  }\n\n> '--pretty=' couldn't benefit, though, because it is special in that\n> it's equivalent with '--format=', and the two are handled in the same\n> branch.\n\nI think you could still handle them both in the same branch, like:\n\n  if (match_opt(arg, \"--pretty\", &optarg) ||\n      skip_prefix(arg, \"--format=\", &optarg)) {\n       revs->verbose_header = 1;\n       revs->pretty-given = 1;\n       /* OK to pass NULL for --pretty case */\n       get_commit_format(optarg, revs);\n  }\n\n> So inherently there are a few repeated option names and variable\n> assignments, and that's not so good.  However, refactoring these to\n> use match_opt() adds 40% more lines than it removes and, more\n> importantly, increases the number of nested conditions.  Subjectively\n> I don't think it's better, so I went with the \"follow the conventions\n> of the surrounding code\" rule for the update.\n\nI care less about lines of boilerplate code and more about repeated\nlogic. In the --pretty example above, the first two lines of the block\nare common to both --pretty and --pretty=. If they ever need to change,\nsomebody has to update two spots.\n\nAnyway. I certainly don't insist on you working on this, especially if\nyou don't agree with the aesthetics. Just fixing the actual bugs would\nbe sufficient for my review. ;)\n\n> As far as I can tell, parse-options doesn't handle options with an\n> optional argument by itself, but only with callback functions, so it\n> is no help here as it is.\n\nThere's a flag, PARSE_OPT_OPTARG, which would do what you want. But I\nagree that converting the whole thing to parse-options would be a lot of\nwork (quite a few of these really aren't just \"this is a string\", but\nwould need independent callback functions.\n\n-Peff\n"},{"id":"321886","messageId":"20170610064102.w4kp6omxdznfe7fa@sigill.intra.peff.net","threadId":"46102","inReplyTo":"20170609181733.6793-2-szeder.dev@gmail.com","subject":"Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-10T06:41:02Z","receivedAt":"2017-06-10T06:41:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 09, 2017 at 08:17:29PM +0200, SZEDER Gábor wrote:\n\n> rev_info.early_output started out as an unsigned int in cdcefbc97 (Add\n> \"--early-output\" log flag for interactive GUI use, 2007-11-03), but\n> later it was turned into a single bit in a bit field in cc243c3ce\n> (show: --ignore-missing, 2011-05-18) without explanation, though its\n> users still expect it to be a regular integer type.  Consequently, any\n> even number given via '--early-output=<N>' effectively disabled the\n> feature.\n> \n> Turn it back into an unsigned int, restoring its original data type.\n\nThis confused me for a moment, as on my first read it seems like the\nobvious solution is to normalize the input to a bit-field, like:\n\n  revs->early_output = !!atoi(optarg);\n\nBut the \"users still expect\" bit was a bit subtle to me, as I thought\nyou meant users of Git. But you mean that the feature itself is not a\nboolean, but rather an integer count of how much early output to show.\n\nI'm not sure if I was just being thick or if that point (and the fact\nthat --early-output has basically been a noop since 2011!) should be\nmade more explicit.\n\nGiven that nobody noticed, I kind of wonder if we should consider\nripping the feature out entirely.\n\n-Peff\n"},{"id":"321887","messageId":"20170610064426.ylzdx2v4ffdz2jkh@sigill.intra.peff.net","threadId":"46102","inReplyTo":"20170609181733.6793-1-szeder.dev@gmail.com","subject":"Re: [PATCH 2/3] revision.c: use skip_prefix() in handle_revision_opt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-10T06:44:26Z","receivedAt":"2017-06-10T06:44:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 09, 2017 at 08:17:28PM +0200, SZEDER Gábor wrote:\n\n> So, here comes v2.  The interdiff is below, the changes since v1 are:\n> \n>  - Patch 1/5 is new to fix a more fundamental problem with\n>    '--early-output'.\n>  - Patch 3/5 is new to fix this '--early-output-foo' issue and also\n>    to tighten up the parsing of its integer argument, while at it.\n>  - A fix for '--show-linear-break-foo' in v1.\n>  - A little cleanup in the handling of '--show-notes/--notes'.\n> \n> \n> SZEDER Gábor (5):\n>   revision.h: turn rev_info.early_output back into an unsigned int\n>   revision.c: stricter parsing of '--no-{min,max}-parents'\n>   revision.c: stricter parsing of '--early-output'\n>   revision.c: use skip_prefix() in handle_revision_opt()\n>   revision.c: use skip_prefix() in handle_revision_pseudo_opt()\n> \n>  revision.c | 87 +++++++++++++++++++++++++++++---------------------------------\n>  revision.h |  5 ++--\n>  2 files changed, 44 insertions(+), 48 deletions(-)\n\nI noted a minor nit in the first commit message, but otherwise these all\nlook good to me.\n\n-Peff\n"},{"id":"321915","messageId":"20170610114101.21026-1-szeder.dev@gmail.com","threadId":"46102","inReplyTo":"20170610064102.w4kp6omxdznfe7fa@sigill.intra.peff.net","subject":"[PATCHv2.1] revision.h: turn rev_info.early_output back into an unsigned int","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-10T11:41:01Z","receivedAt":"2017-06-10T11:41:20Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"rev_info.early_output started out as an unsigned int in cdcefbc97 (Add\n\"--early-output\" log flag for interactive GUI use, 2007-11-03), but\nlater it was turned into a single bit in a bit field in cc243c3ce\n(show: --ignore-missing, 2011-05-18) without explanation, though the\ncode using it still expects it to be a regular integer type and uses\nit as a counter.  Consequently, any even number given via\n'--early-output=<N>', or indeed a plain '--early-output' defaulting to\n100 effectively disabled the feature.\n\nTurn rev_info.early_output back into its origin unsigned int data\ntype, making '--early-output' work again.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n\n> But the \"users still expect\" bit was a bit subtle to me, as I thought\n> you meant users of Git. But you mean that the feature itself is not a\n> boolean, but rather an integer count of how much early output to show.\n\nYeah, I wrote \"callsites\" first, but then realized it's not a\nfunction...\n\nHere is the same patch with an updated commit message now saying \"code\nusing it\" and \"used as a counter\" to make it clearer.  It also\nmentions that an argumentless '--early-output' turns off the feature,\ntoo.\n\nI won't resend the rest of the series.\n\n revision.h | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.h b/revision.h\nindex a91dd3d5d..f96e7f7f4 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -74,8 +74,9 @@ struct rev_info {\n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n \n-\tunsigned int\tearly_output:1,\n-\t\t\tignore_missing:1,\n+\tunsigned int early_output;\n+\n+\tunsigned int\tignore_missing:1,\n \t\t\tignore_missing_links:1;\n \n \t/* Traversal flags */\n-- \n2.13.0.420.g54001f015\n\n"},{"id":"321988","messageId":"xmqq60g1ndtz.fsf@gitster.mtv.corp.google.com","threadId":"46102","inReplyTo":"20170610064102.w4kp6omxdznfe7fa@sigill.intra.peff.net","subject":"Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-12T20:36:08Z","receivedAt":"2017-06-12T20:36:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Jun 09, 2017 at 08:17:29PM +0200, SZEDER Gábor wrote:\n>\n>> rev_info.early_output started out as an unsigned int in cdcefbc97 (Add\n>> \"--early-output\" log flag for interactive GUI use, 2007-11-03), but\n>> later it was turned into a single bit in a bit field in cc243c3ce\n>> (show: --ignore-missing, 2011-05-18) without explanation, though its\n>> users still expect it to be a regular integer type.  Consequently, any\n>> even number given via '--early-output=<N>' effectively disabled the\n>> feature.\n>> \n>> Turn it back into an unsigned int, restoring its original data type.\n>\n> This confused me for a moment, as on my first read it seems like the\n> obvious solution is to normalize the input to a bit-field, like:\n>\n>   revs->early_output = !!atoi(optarg);\n>\n> But the \"users still expect\" bit was a bit subtle to me, as I thought\n> you meant users of Git. But you mean that the feature itself is not a\n> boolean, but rather an integer count of how much early output to show.\n>\n> I'm not sure if I was just being thick or if that point (and the fact\n> that --early-output has basically been a noop since 2011!) should be\n> made more explicit.\n>\n> Given that nobody noticed, I kind of wonder if we should consider\n> ripping the feature out entirely.\n\nYes, we may want to think about deprecating it, especially given\nthat it is not advertised anywhere.\n\nIn any case, the patch looks correct ;-)\n"},{"id":"321997","messageId":"20170612213002.d3t2tamct3vkz7un@sigill.intra.peff.net","threadId":"46102","inReplyTo":"20170610114101.21026-1-szeder.dev@gmail.com","subject":"Re: [PATCHv2.1] revision.h: turn rev_info.early_output back into an unsigned int","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-12T21:30:02Z","receivedAt":"2017-06-12T21:30:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 10, 2017 at 01:41:01PM +0200, SZEDER Gábor wrote:\n\n> rev_info.early_output started out as an unsigned int in cdcefbc97 (Add\n> \"--early-output\" log flag for interactive GUI use, 2007-11-03), but\n> later it was turned into a single bit in a bit field in cc243c3ce\n> (show: --ignore-missing, 2011-05-18) without explanation, though the\n> code using it still expects it to be a regular integer type and uses\n> it as a counter.  Consequently, any even number given via\n> '--early-output=<N>', or indeed a plain '--early-output' defaulting to\n> 100 effectively disabled the feature.\n> \n> Turn rev_info.early_output back into its origin unsigned int data\n> type, making '--early-output' work again.\n> \n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n> \n> > But the \"users still expect\" bit was a bit subtle to me, as I thought\n> > you meant users of Git. But you mean that the feature itself is not a\n> > boolean, but rather an integer count of how much early output to show.\n> \n> Yeah, I wrote \"callsites\" first, but then realized it's not a\n> function...\n> \n> Here is the same patch with an updated commit message now saying \"code\n> using it\" and \"used as a counter\" to make it clearer.  It also\n> mentions that an argumentless '--early-output' turns off the feature,\n> too.\n> \n> I won't resend the rest of the series.\n\nThanks, this version is much more clear.\n\n-Peff\n"},{"id":"322007","messageId":"20170612215902.62cnfxdivv76337p@sigill.intra.peff.net","threadId":"46102","inReplyTo":"xmqq60g1ndtz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-12T21:59:02Z","receivedAt":"2017-06-12T21:59:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 12, 2017 at 01:36:08PM -0700, Junio C Hamano wrote:\n\n> > I'm not sure if I was just being thick or if that point (and the fact\n> > that --early-output has basically been a noop since 2011!) should be\n> > made more explicit.\n> >\n> > Given that nobody noticed, I kind of wonder if we should consider\n> > ripping the feature out entirely.\n> \n> Yes, we may want to think about deprecating it, especially given\n> that it is not advertised anywhere.\n> \n> In any case, the patch looks correct ;-)\n\nYeah, it's definitely orthogonal to Gábor's patches.\n\nI also wondered who might be using the feature. I assumed it was written\nwith gitk in mind. Digging in the list archive that seems to be the\ncase, and there was even an RFC patch for gitk to use it:\n\n  http://public-inbox.org/git/18221.2285.259487.655684@cargo.ozlabs.ibm.com/\n\nbut AFAICT it was never merged.\n\nIt looks like QGit had a patch around that time, too, but left\nthe line using \"--early-output\" commented out until Git merged the\npatches. And then it never got uncommented. ;)\n\nTig doesn't seem to use it at all. I don't know about other GUIs. I'd\nkind of doubt, given the obscurity of the feature (both gitk and qgit\nauthors were involved in the discussion way back in 2007).\n\nThe nice thing about deprecating it is that I think callers need to be\nprepared to handle the case already that it does nothing. So if we just\nripped out the code and treated it as a silent noop, everything would\njust work.\n\n-Peff\n"},{"id":"322034","messageId":"20170613005037.cz4udzfzkpuvdent@sigill.intra.peff.net","threadId":"46102","inReplyTo":"20170612215902.62cnfxdivv76337p@sigill.intra.peff.net","subject":"Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-13T00:50:37Z","receivedAt":"2017-06-13T00:50:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 12, 2017 at 05:59:02PM -0400, Jeff King wrote:\n\n> The nice thing about deprecating it is that I think callers need to be\n> prepared to handle the case already that it does nothing. So if we just\n> ripped out the code and treated it as a silent noop, everything would\n> just work.\n\nActually, I take that back. We might or might not have early output, but\nwe always say \"Final output\". So in theory a caller would be confused if\nwe no longer print that.\n\n(I do still suspect that there are no callers).\n\n-Peff\n"}]}