{"thread":{"id":"45389","subject":"[PATCH 2/3] rev-parse: add helper for parsing \"--foo/--foo=\"","startedAt":"2017-03-15T20:07:02Z","lastAt":"2017-03-15T21:38:25Z","messageCount":6,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"314164","messageId":"20170315200653.ougdpmv2oombefrt@sigill.intra.peff.net","threadId":"45389","inReplyTo":"20170315200521.qwcm5tqcdzkbwp3x@sigill.intra.peff.net","subject":"[PATCH 2/3] rev-parse: add helper for parsing \"--foo/--foo=\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T20:06:53Z","receivedAt":"2017-03-15T20:07:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We can't just use a bare skip_prefix() for these cases,\nbecause we need to match both the \"--foo\" form and the\n\"--foo=<value>\" form (and tell the difference between the\ntwo in the caller).\n\nWe can wrap this in a simple helper which has two obvious\ncallsites, and will gain some more in the next patch.\n\nNote that the error output for abbrev-ref changes slightly,\nas we don't keep our original \"arg\" pointer. However, the\nnew output should hopefully be more clear:\n\n  [before]\n  fatal: unknown mode for --abbrev-ref=foo\n\n  [after]\n  fatal: unknown mode for --abbrev-ref: foo\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rev-parse.c | 38 ++++++++++++++++++++++++++++----------\n 1 file changed, 28 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 836ccf00e..db1417160 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -535,6 +535,25 @@ N_(\"git rev-parse --parseopt [<options>] -- [<args>...]\\n\"\n    \"\\n\"\n    \"Run \\\"git rev-parse --parseopt -h\\\" for more information on the first usage.\");\n \n+/*\n+ * Parse \"opt\" or \"opt=<value>\", setting value respectively to either\n+ * NULL or the string after \"=\".\n+ */\n+static int opt_with_value(const char *arg, const char *opt, const char **value)\n+{\n+\tif (skip_prefix(arg, opt, &arg)) {\n+\t\tif (!*arg) {\n+\t\t\t*value = NULL;\n+\t\t\treturn 1;\n+\t\t}\n+\t\tif (*arg++ == '=') {\n+\t\t\t*value = arg;\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n {\n \tint i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;\n@@ -674,14 +693,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tflags |= GET_SHA1_QUIETLY;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (!strcmp(arg, \"--short\") ||\n-\t\t\t    starts_with(arg, \"--short=\")) {\n+\t\t\tif (opt_with_value(arg, \"--short\", &arg)) {\n \t\t\t\tfilter &= ~(DO_FLAGS|DO_NOREV);\n \t\t\t\tverify = 1;\n \t\t\t\tabbrev = DEFAULT_ABBREV;\n-\t\t\t\tif (!arg[7])\n+\t\t\t\tif (!arg)\n \t\t\t\t\tcontinue;\n-\t\t\t\tabbrev = strtoul(arg + 8, NULL, 10);\n+\t\t\t\tabbrev = strtoul(arg, NULL, 10);\n \t\t\t\tif (abbrev < MINIMUM_ABBREV)\n \t\t\t\t\tabbrev = MINIMUM_ABBREV;\n \t\t\t\telse if (40 <= abbrev)\n@@ -704,17 +722,17 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tsymbolic = SHOW_SYMBOLIC_FULL;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (starts_with(arg, \"--abbrev-ref\") &&\n-\t\t\t    (!arg[12] || arg[12] == '=')) {\n+\t\t\tif (opt_with_value(arg, \"--abbrev-ref\", &arg)) {\n \t\t\t\tabbrev_ref = 1;\n \t\t\t\tabbrev_ref_strict = warn_ambiguous_refs;\n-\t\t\t\tif (arg[12] == '=') {\n-\t\t\t\t\tif (!strcmp(arg + 13, \"strict\"))\n+\t\t\t\tif (arg) {\n+\t\t\t\t\tif (!strcmp(arg, \"strict\"))\n \t\t\t\t\t\tabbrev_ref_strict = 1;\n-\t\t\t\t\telse if (!strcmp(arg + 13, \"loose\"))\n+\t\t\t\t\telse if (!strcmp(arg, \"loose\"))\n \t\t\t\t\t\tabbrev_ref_strict = 0;\n \t\t\t\t\telse\n-\t\t\t\t\t\tdie(\"unknown mode for %s\", arg);\n+\t\t\t\t\t\tdie(\"unknown mode for --abbrev-ref: %s\",\n+\t\t\t\t\t\t    arg);\n \t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n-- \n2.12.0.613.g6e7c52a0d\n\n"},{"id":"314165","messageId":"20170315200802.r4un6p5admfu4h34@sigill.intra.peff.net","threadId":"45389","inReplyTo":"20170315200521.qwcm5tqcdzkbwp3x@sigill.intra.peff.net","subject":"[PATCH 3/3] rev-parse: simplify parsing of ref options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T20:08:02Z","receivedAt":"2017-03-15T20:08:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of these options do the same thing \"--foo\" iterates over\nthe \"foo\" refs, and \"--foo=<glob>\" does the same with a\nglob. We can factor this into its own function to avoid\nrepeating ourselves.\n\nThere are two subtleties to note:\n\n  - the original called for_each_branch_ref(), etc, in the\n    non-glob case. Now we will call for_each_ref_in(\"refs/heads/\")\n    which is exactly what for_each_branch_ref() did under\n    the hood.\n\n  - for --glob, we'll call for_each_glob_ref_in() with a\n    NULL \"prefix\" argument. Which is exactly what\n    for_each_glob_ref() was doing already.\n\nSo both cases should behave identically, and it seems\nreasonable to assume that this will remain the same. The\nfunctions we are calling now are the more-generic ones, and\nthe ones we are dropping are just convenience wrappers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis actually drops the last caller for for_each_branch_ref(). I'm not\nsure if we shoulder consider cleaning up the proliferation of\nfor_each_ref() helpers.\n\n builtin/rev-parse.c | 45 ++++++++++++++++-----------------------------\n 1 file changed, 16 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex db1417160..76be91350 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -554,6 +554,15 @@ static int opt_with_value(const char *arg, const char *opt, const char **value)\n \treturn 0;\n }\n \n+static void handle_ref_opt(const char *pattern, const char *prefix)\n+{\n+\tif (pattern)\n+\t\tfor_each_glob_ref_in(show_reference, pattern, prefix, NULL);\n+\telse\n+\t\tfor_each_ref_in(prefix, show_reference, NULL);\n+\tclear_ref_exclusion(&ref_excludes);\n+}\n+\n int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n {\n \tint i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;\n@@ -749,42 +758,20 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tfor_each_ref_in(\"refs/bisect/good\", anti_reference, NULL);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (skip_prefix(arg, \"--branches=\", &arg)) {\n-\t\t\t\tfor_each_glob_ref_in(show_reference, arg,\n-\t\t\t\t\t\"refs/heads/\", NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(arg, \"--branches\")) {\n-\t\t\t\tfor_each_branch_ref(show_reference, NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (skip_prefix(arg, \"--tags=\", &arg)) {\n-\t\t\t\tfor_each_glob_ref_in(show_reference, arg,\n-\t\t\t\t\t\"refs/tags/\", NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n+\t\t\tif (opt_with_value(arg, \"--branches\", &arg)) {\n+\t\t\t\thandle_ref_opt(arg, \"refs/heads/\");\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (!strcmp(arg, \"--tags\")) {\n-\t\t\t\tfor_each_tag_ref(show_reference, NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n+\t\t\tif (opt_with_value(arg, \"--tags\", &arg)) {\n+\t\t\t\thandle_ref_opt(arg, \"refs/tags/\");\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (skip_prefix(arg, \"--glob=\", &arg)) {\n-\t\t\t\tfor_each_glob_ref(show_reference, arg, NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (skip_prefix(arg, \"--remotes=\", &arg)) {\n-\t\t\t\tfor_each_glob_ref_in(show_reference, arg,\n-\t\t\t\t\t\"refs/remotes/\", NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n+\t\t\t\thandle_ref_opt(arg, NULL);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (!strcmp(arg, \"--remotes\")) {\n-\t\t\t\tfor_each_remote_ref(show_reference, NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n+\t\t\tif (opt_with_value(arg, \"--remotes\", &arg)) {\n+\t\t\t\thandle_ref_opt(arg, \"refs/remotes/\");\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (skip_prefix(arg, \"--exclude=\", &arg)) {\n-- \n2.12.0.613.g6e7c52a0d\n"},{"id":"314166","messageId":"20170315200521.qwcm5tqcdzkbwp3x@sigill.intra.peff.net","threadId":"45389","inReplyTo":null,"subject":"[PATCH 0/3] minor rev-parse option-parsing cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T20:05:21Z","receivedAt":"2017-03-15T20:12:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I happened to be looking at these yesterday and got annoyed by the\ntangled logic. No bug-fixes, but the result is less repetitive and has\nfewer magic numbers.\n\n  [1/3]: rev-parse: use skip_prefix when parsing options\n  [2/3]: rev-parse: add helper for parsing \"--foo/--foo=\"\n  [3/3]: rev-parse: simplify parsing of ref options\n\n builtin/rev-parse.c | 109 +++++++++++++++++++++++++++-------------------------\n 1 file changed, 57 insertions(+), 52 deletions(-)\n\n-Peff\n"},{"id":"314167","messageId":"20170315200535.rdno2keqnynfzmyj@sigill.intra.peff.net","threadId":"45389","inReplyTo":"20170315200521.qwcm5tqcdzkbwp3x@sigill.intra.peff.net","subject":"[PATCH 1/3] rev-parse: use skip_prefix when parsing options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T20:05:36Z","receivedAt":"2017-03-15T20:12:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Using skip_prefix lets us avoid manually-counted offsets\ninto the argument string. This patch converts the simple and\nobvious cases.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rev-parse.c | 40 ++++++++++++++++++++--------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex e08677e55..836ccf00e 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -722,8 +722,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tfor_each_ref(show_reference, NULL);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (starts_with(arg, \"--disambiguate=\")) {\n-\t\t\t\tfor_each_abbrev(arg + 15, show_abbrev, NULL);\n+\t\t\tif (skip_prefix(arg, \"--disambiguate=\", &arg)) {\n+\t\t\t\tfor_each_abbrev(arg, show_abbrev, NULL);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--bisect\")) {\n@@ -731,8 +731,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tfor_each_ref_in(\"refs/bisect/good\", anti_reference, NULL);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (starts_with(arg, \"--branches=\")) {\n-\t\t\t\tfor_each_glob_ref_in(show_reference, arg + 11,\n+\t\t\tif (skip_prefix(arg, \"--branches=\", &arg)) {\n+\t\t\t\tfor_each_glob_ref_in(show_reference, arg,\n \t\t\t\t\t\"refs/heads/\", NULL);\n \t\t\t\tclear_ref_exclusion(&ref_excludes);\n \t\t\t\tcontinue;\n@@ -742,8 +742,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tclear_ref_exclusion(&ref_excludes);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (starts_with(arg, \"--tags=\")) {\n-\t\t\t\tfor_each_glob_ref_in(show_reference, arg + 7,\n+\t\t\tif (skip_prefix(arg, \"--tags=\", &arg)) {\n+\t\t\t\tfor_each_glob_ref_in(show_reference, arg,\n \t\t\t\t\t\"refs/tags/\", NULL);\n \t\t\t\tclear_ref_exclusion(&ref_excludes);\n \t\t\t\tcontinue;\n@@ -753,13 +753,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tclear_ref_exclusion(&ref_excludes);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (starts_with(arg, \"--glob=\")) {\n-\t\t\t\tfor_each_glob_ref(show_reference, arg + 7, NULL);\n+\t\t\tif (skip_prefix(arg, \"--glob=\", &arg)) {\n+\t\t\t\tfor_each_glob_ref(show_reference, arg, NULL);\n \t\t\t\tclear_ref_exclusion(&ref_excludes);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (starts_with(arg, \"--remotes=\")) {\n-\t\t\t\tfor_each_glob_ref_in(show_reference, arg + 10,\n+\t\t\tif (skip_prefix(arg, \"--remotes=\", &arg)) {\n+\t\t\t\tfor_each_glob_ref_in(show_reference, arg,\n \t\t\t\t\t\"refs/remotes/\", NULL);\n \t\t\t\tclear_ref_exclusion(&ref_excludes);\n \t\t\t\tcontinue;\n@@ -769,8 +769,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tclear_ref_exclusion(&ref_excludes);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (starts_with(arg, \"--exclude=\")) {\n-\t\t\t\tadd_ref_exclusion(&ref_excludes, arg + 10);\n+\t\t\tif (skip_prefix(arg, \"--exclude=\", &arg)) {\n+\t\t\t\tadd_ref_exclusion(&ref_excludes, arg);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--show-toplevel\")) {\n@@ -865,20 +865,20 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (starts_with(arg, \"--since=\")) {\n-\t\t\t\tshow_datestring(\"--max-age=\", arg+8);\n+\t\t\tif (skip_prefix(arg, \"--since=\", &arg)) {\n+\t\t\t\tshow_datestring(\"--max-age=\", arg);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (starts_with(arg, \"--after=\")) {\n-\t\t\t\tshow_datestring(\"--max-age=\", arg+8);\n+\t\t\tif (skip_prefix(arg, \"--after=\", &arg)) {\n+\t\t\t\tshow_datestring(\"--max-age=\", arg);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (starts_with(arg, \"--before=\")) {\n-\t\t\t\tshow_datestring(\"--min-age=\", arg+9);\n+\t\t\tif (skip_prefix(arg, \"--before=\", &arg)) {\n+\t\t\t\tshow_datestring(\"--min-age=\", arg);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (starts_with(arg, \"--until=\")) {\n-\t\t\t\tshow_datestring(\"--min-age=\", arg+8);\n+\t\t\tif (skip_prefix(arg, \"--until=\", &arg)) {\n+\t\t\t\tshow_datestring(\"--min-age=\", arg);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (show_flag(arg) && verify)\n-- \n2.12.0.613.g6e7c52a0d\n\n"},{"id":"314173","messageId":"xmqq8to6dz76.fsf@gitster.mtv.corp.google.com","threadId":"45389","inReplyTo":"20170315200802.r4un6p5admfu4h34@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] rev-parse: simplify parsing of ref options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-15T21:09:33Z","receivedAt":"2017-03-15T21:11:07Z","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> This actually drops the last caller for for_each_branch_ref(). I'm not\n> sure if we shoulder consider cleaning up the proliferation of\n> for_each_ref() helpers.\n\nThat is certainly a good thing to do (but outside this series).\n\nI am wondering if \"git diff\" could have chosen a better way to\narrange deleted and inserted lines.  The first two if statements\nin the preimage corresponds to the new opt-with-value(branches)\nthing, the next two are repalced by opt-with-value(tags), an\nodd-man-out \"--glob=\" thing is replaced with handle_ref_opt(NULL),\nand then two \"--remotes\" thing are paired with the final\nopt-with-value(remotes) thing.  \n\nThat would break the usual expectation of seeing all \"-\" first and\nthen \"+\" when there is no intervening \" \" context lines, so such an\noutput might break tools, though (I do not think \"git apply\" would\nchoke).\n\n>  builtin/rev-parse.c | 45 ++++++++++++++++-----------------------------\n>  1 file changed, 16 insertions(+), 29 deletions(-)\n>\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index db1417160..76be91350 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -554,6 +554,15 @@ static int opt_with_value(const char *arg, const char *opt, const char **value)\n>  \treturn 0;\n>  }\n>  \n> +static void handle_ref_opt(const char *pattern, const char *prefix)\n> +{\n> +\tif (pattern)\n> +\t\tfor_each_glob_ref_in(show_reference, pattern, prefix, NULL);\n> +\telse\n> +\t\tfor_each_ref_in(prefix, show_reference, NULL);\n> +\tclear_ref_exclusion(&ref_excludes);\n> +}\n> +\n>  int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;\n> @@ -749,42 +758,20 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>  \t\t\t\tfor_each_ref_in(\"refs/bisect/good\", anti_reference, NULL);\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n> -\t\t\tif (skip_prefix(arg, \"--branches=\", &arg)) {\n> -\t\t\t\tfor_each_glob_ref_in(show_reference, arg,\n> -\t\t\t\t\t\"refs/heads/\", NULL);\n> -\t\t\t\tclear_ref_exclusion(&ref_excludes);\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> -\t\t\tif (!strcmp(arg, \"--branches\")) {\n> -\t\t\t\tfor_each_branch_ref(show_reference, NULL);\n> -\t\t\t\tclear_ref_exclusion(&ref_excludes);\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> -\t\t\tif (skip_prefix(arg, \"--tags=\", &arg)) {\n> -\t\t\t\tfor_each_glob_ref_in(show_reference, arg,\n> -\t\t\t\t\t\"refs/tags/\", NULL);\n> -\t\t\t\tclear_ref_exclusion(&ref_excludes);\n> +\t\t\tif (opt_with_value(arg, \"--branches\", &arg)) {\n> +\t\t\t\thandle_ref_opt(arg, \"refs/heads/\");\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n> -\t\t\tif (!strcmp(arg, \"--tags\")) {\n> -\t\t\t\tfor_each_tag_ref(show_reference, NULL);\n> -\t\t\t\tclear_ref_exclusion(&ref_excludes);\n> +\t\t\tif (opt_with_value(arg, \"--tags\", &arg)) {\n> +\t\t\t\thandle_ref_opt(arg, \"refs/tags/\");\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n>  \t\t\tif (skip_prefix(arg, \"--glob=\", &arg)) {\n> -\t\t\t\tfor_each_glob_ref(show_reference, arg, NULL);\n> -\t\t\t\tclear_ref_exclusion(&ref_excludes);\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> -\t\t\tif (skip_prefix(arg, \"--remotes=\", &arg)) {\n> -\t\t\t\tfor_each_glob_ref_in(show_reference, arg,\n> -\t\t\t\t\t\"refs/remotes/\", NULL);\n> -\t\t\t\tclear_ref_exclusion(&ref_excludes);\n> +\t\t\t\thandle_ref_opt(arg, NULL);\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n> -\t\t\tif (!strcmp(arg, \"--remotes\")) {\n> -\t\t\t\tfor_each_remote_ref(show_reference, NULL);\n> -\t\t\t\tclear_ref_exclusion(&ref_excludes);\n> +\t\t\tif (opt_with_value(arg, \"--remotes\", &arg)) {\n> +\t\t\t\thandle_ref_opt(arg, \"refs/remotes/\");\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n>  \t\t\tif (skip_prefix(arg, \"--exclude=\", &arg)) {\n"},{"id":"314186","messageId":"20170315213818.qfccs2atfijngaxr@sigill.intra.peff.net","threadId":"45389","inReplyTo":"xmqq8to6dz76.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] rev-parse: simplify parsing of ref options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T21:38:18Z","receivedAt":"2017-03-15T21:38:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 15, 2017 at 02:09:33PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This actually drops the last caller for for_each_branch_ref(). I'm not\n> > sure if we shoulder consider cleaning up the proliferation of\n> > for_each_ref() helpers.\n> \n> That is certainly a good thing to do (but outside this series).\n> \n> I am wondering if \"git diff\" could have chosen a better way to\n> arrange deleted and inserted lines.  The first two if statements\n> in the preimage corresponds to the new opt-with-value(branches)\n> thing, the next two are repalced by opt-with-value(tags), an\n> odd-man-out \"--glob=\" thing is replaced with handle_ref_opt(NULL),\n> and then two \"--remotes\" thing are paired with the final\n> opt-with-value(remotes) thing.  \n> \n> That would break the usual expectation of seeing all \"-\" first and\n> then \"+\" when there is no intervening \" \" context lines, so such an\n> output might break tools, though (I do not think \"git apply\" would\n> choke).\n\nI think in this case you could still show it better with context lines\nbetween. Something like:\n\n@@ -749,42 +758,20 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tfor_each_ref_in(\"refs/bisect/good\", anti_reference, NULL);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (skip_prefix(arg, \"--branches=\", &arg)) {\n-\t\t\t\tfor_each_glob_ref_in(show_reference, arg,\n-\t\t\t\t\t\"refs/heads/\", NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(arg, \"--branches\")) {\n-\t\t\t\tfor_each_branch_ref(show_reference, NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n+\t\t\tif (opt_with_value(arg, \"--branches\", &arg)) {\n+\t\t\t\thandle_ref_opt(arg, \"refs/heads/\");\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (skip_prefix(arg, \"--tags=\", &arg)) {\n-\t\t\t\tfor_each_glob_ref_in(show_reference, arg,\n-\t\t\t\t\t\"refs/tags/\", NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n-\t\t\tif (!strcmp(arg, \"--tags\")) {\n-\t\t\t\tfor_each_tag_ref(show_reference, NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n+\t\t\tif (opt_with_value(arg, \"--tags\", &arg)) {\n+\t\t\t\thandle_ref_opt(arg, \"refs/tags/\");\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (skip_prefix(arg, \"--glob=\", &arg)) {\n-\t\t\t\tfor_each_glob_ref(show_reference, arg, NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n+\t\t\t\thandle_ref_opt(arg, NULL);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (skip_prefix(arg, \"--remotes=\", &arg)) {\n-\t\t\t\tfor_each_glob_ref_in(show_reference, arg,\n-\t\t\t\t\t\"refs/remotes/\", NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n-\t\t\tif (!strcmp(arg, \"--remotes\")) {\n-\t\t\t\tfor_each_remote_ref(show_reference, NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n+\t\t\tif (opt_with_value(arg, \"--remotes\", &arg)) {\n+\t\t\t\thandle_ref_opt(arg, \"refs/remotes/\");\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (skip_prefix(arg, \"--exclude=\", &arg)) {\n\nI have no idea what heuristic might generate that diff, though. Using\n--patience and --histogram does provided different results than Myers\nfor this case, but none produces the output above. The other two try to\npush the new \"--branches\" lines up as a replacement for \"--branches=\",\nbut then you get a clump of \"--branches\", \"--tags=\", and \"--tags\".\n\n-Peff\n"}]}