From ef87cc79dff1fa0d16ad4d67646afeba9ce11a6c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 15 Mar 2017 16:05:36 -0400 Subject: [PATCH 1/3] rev-parse: use skip_prefix when parsing options Using skip_prefix lets us avoid manually-counted offsets into the argument string. This patch converts the simple and obvious cases. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/rev-parse.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index ff13e59e1d..48338ac760 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -719,8 +719,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) for_each_ref(show_reference, NULL); continue; } - if (starts_with(arg, "--disambiguate=")) { - for_each_abbrev(arg + 15, show_abbrev, NULL); + if (skip_prefix(arg, "--disambiguate=", &arg)) { + for_each_abbrev(arg, show_abbrev, NULL); continue; } if (!strcmp(arg, "--bisect")) { @@ -728,8 +728,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) for_each_ref_in("refs/bisect/good", anti_reference, NULL); continue; } - if (starts_with(arg, "--branches=")) { - for_each_glob_ref_in(show_reference, arg + 11, + if (skip_prefix(arg, "--branches=", &arg)) { + for_each_glob_ref_in(show_reference, arg, "refs/heads/", NULL); clear_ref_exclusion(&ref_excludes); continue; @@ -739,8 +739,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) clear_ref_exclusion(&ref_excludes); continue; } - if (starts_with(arg, "--tags=")) { - for_each_glob_ref_in(show_reference, arg + 7, + if (skip_prefix(arg, "--tags=", &arg)) { + for_each_glob_ref_in(show_reference, arg, "refs/tags/", NULL); clear_ref_exclusion(&ref_excludes); continue; @@ -750,13 +750,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) clear_ref_exclusion(&ref_excludes); continue; } - if (starts_with(arg, "--glob=")) { - for_each_glob_ref(show_reference, arg + 7, NULL); + if (skip_prefix(arg, "--glob=", &arg)) { + for_each_glob_ref(show_reference, arg, NULL); clear_ref_exclusion(&ref_excludes); continue; } - if (starts_with(arg, "--remotes=")) { - for_each_glob_ref_in(show_reference, arg + 10, + if (skip_prefix(arg, "--remotes=", &arg)) { + for_each_glob_ref_in(show_reference, arg, "refs/remotes/", NULL); clear_ref_exclusion(&ref_excludes); continue; @@ -766,8 +766,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) clear_ref_exclusion(&ref_excludes); continue; } - if (starts_with(arg, "--exclude=")) { - add_ref_exclusion(&ref_excludes, arg + 10); + if (skip_prefix(arg, "--exclude=", &arg)) { + add_ref_exclusion(&ref_excludes, arg); continue; } if (!strcmp(arg, "--show-toplevel")) { @@ -849,20 +849,20 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) } continue; } - if (starts_with(arg, "--since=")) { - show_datestring("--max-age=", arg+8); + if (skip_prefix(arg, "--since=", &arg)) { + show_datestring("--max-age=", arg); continue; } - if (starts_with(arg, "--after=")) { - show_datestring("--max-age=", arg+8); + if (skip_prefix(arg, "--after=", &arg)) { + show_datestring("--max-age=", arg); continue; } - if (starts_with(arg, "--before=")) { - show_datestring("--min-age=", arg+9); + if (skip_prefix(arg, "--before=", &arg)) { + show_datestring("--min-age=", arg); continue; } - if (starts_with(arg, "--until=")) { - show_datestring("--min-age=", arg+8); + if (skip_prefix(arg, "--until=", &arg)) { + show_datestring("--min-age=", arg); continue; } if (show_flag(arg) && verify) From 9d16ca65bbe45d4a28cbb37c3299b544c81ff2e8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 15 Mar 2017 16:06:53 -0400 Subject: [PATCH 2/3] rev-parse: add helper for parsing "--foo/--foo=" We can't just use a bare skip_prefix() for these cases, because we need to match both the "--foo" form and the "--foo=" form (and tell the difference between the two in the caller). We can wrap this in a simple helper which has two obvious callsites, and will gain some more in the next patch. Note that the error output for abbrev-ref changes slightly, as we don't keep our original "arg" pointer. However, the new output should hopefully be more clear: [before] fatal: unknown mode for --abbrev-ref=foo [after] fatal: unknown mode for --abbrev-ref: foo Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/rev-parse.c | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 48338ac760..a6a009152f 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -535,6 +535,25 @@ N_("git rev-parse --parseopt [] -- [...]\n" "\n" "Run \"git rev-parse --parseopt -h\" for more information on the first usage."); +/* + * Parse "opt" or "opt=", setting value respectively to either + * NULL or the string after "=". + */ +static int opt_with_value(const char *arg, const char *opt, const char **value) +{ + if (skip_prefix(arg, opt, &arg)) { + if (!*arg) { + *value = NULL; + return 1; + } + if (*arg++ == '=') { + *value = arg; + return 1; + } + } + return 0; +} + int cmd_rev_parse(int argc, const char **argv, const char *prefix) { int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0; @@ -671,14 +690,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) flags |= GET_SHA1_QUIETLY; continue; } - if (!strcmp(arg, "--short") || - starts_with(arg, "--short=")) { + if (opt_with_value(arg, "--short", &arg)) { filter &= ~(DO_FLAGS|DO_NOREV); verify = 1; abbrev = DEFAULT_ABBREV; - if (!arg[7]) + if (!arg) continue; - abbrev = strtoul(arg + 8, NULL, 10); + abbrev = strtoul(arg, NULL, 10); if (abbrev < MINIMUM_ABBREV) abbrev = MINIMUM_ABBREV; else if (40 <= abbrev) @@ -701,17 +719,17 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) symbolic = SHOW_SYMBOLIC_FULL; continue; } - if (starts_with(arg, "--abbrev-ref") && - (!arg[12] || arg[12] == '=')) { + if (opt_with_value(arg, "--abbrev-ref", &arg)) { abbrev_ref = 1; abbrev_ref_strict = warn_ambiguous_refs; - if (arg[12] == '=') { - if (!strcmp(arg + 13, "strict")) + if (arg) { + if (!strcmp(arg, "strict")) abbrev_ref_strict = 1; - else if (!strcmp(arg + 13, "loose")) + else if (!strcmp(arg, "loose")) abbrev_ref_strict = 0; else - die("unknown mode for %s", arg); + die("unknown mode for --abbrev-ref: %s", + arg); } continue; } From ffddfc6328fd3c8b0e8ee69144ec2dda5c766364 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 15 Mar 2017 16:08:02 -0400 Subject: [PATCH 3/3] rev-parse: simplify parsing of ref options All of these options do the same thing "--foo" iterates over the "foo" refs, and "--foo=" does the same with a glob. We can factor this into its own function to avoid repeating ourselves. There are two subtleties to note: - the original called for_each_branch_ref(), etc, in the non-glob case. Now we will call for_each_ref_in("refs/heads/") which is exactly what for_each_branch_ref() did under the hood. - for --glob, we'll call for_each_glob_ref_in() with a NULL "prefix" argument. Which is exactly what for_each_glob_ref() was doing already. So both cases should behave identically, and it seems reasonable to assume that this will remain the same. The functions we are calling now are the more-generic ones, and the ones we are dropping are just convenience wrappers. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/rev-parse.c | 45 ++++++++++++++++----------------------------- 1 file changed, 16 insertions(+), 29 deletions(-) diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index a6a009152f..ed8819b451 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -554,6 +554,15 @@ static int opt_with_value(const char *arg, const char *opt, const char **value) return 0; } +static void handle_ref_opt(const char *pattern, const char *prefix) +{ + if (pattern) + for_each_glob_ref_in(show_reference, pattern, prefix, NULL); + else + for_each_ref_in(prefix, show_reference, NULL); + clear_ref_exclusion(&ref_excludes); +} + int cmd_rev_parse(int argc, const char **argv, const char *prefix) { int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0; @@ -746,42 +755,20 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) for_each_ref_in("refs/bisect/good", anti_reference, NULL); continue; } - if (skip_prefix(arg, "--branches=", &arg)) { - for_each_glob_ref_in(show_reference, arg, - "refs/heads/", NULL); - clear_ref_exclusion(&ref_excludes); + if (opt_with_value(arg, "--branches", &arg)) { + handle_ref_opt(arg, "refs/heads/"); continue; } - if (!strcmp(arg, "--branches")) { - for_each_branch_ref(show_reference, NULL); - clear_ref_exclusion(&ref_excludes); - continue; - } - if (skip_prefix(arg, "--tags=", &arg)) { - for_each_glob_ref_in(show_reference, arg, - "refs/tags/", NULL); - clear_ref_exclusion(&ref_excludes); - continue; - } - if (!strcmp(arg, "--tags")) { - for_each_tag_ref(show_reference, NULL); - clear_ref_exclusion(&ref_excludes); + if (opt_with_value(arg, "--tags", &arg)) { + handle_ref_opt(arg, "refs/tags/"); continue; } if (skip_prefix(arg, "--glob=", &arg)) { - for_each_glob_ref(show_reference, arg, NULL); - clear_ref_exclusion(&ref_excludes); + handle_ref_opt(arg, NULL); continue; } - if (skip_prefix(arg, "--remotes=", &arg)) { - for_each_glob_ref_in(show_reference, arg, - "refs/remotes/", NULL); - clear_ref_exclusion(&ref_excludes); - continue; - } - if (!strcmp(arg, "--remotes")) { - for_each_remote_ref(show_reference, NULL); - clear_ref_exclusion(&ref_excludes); + if (opt_with_value(arg, "--remotes", &arg)) { + handle_ref_opt(arg, "refs/remotes/"); continue; } if (skip_prefix(arg, "--exclude=", &arg)) {