* [PATCH 0/3] minor rev-parse option-parsing cleanups
@ 2017-03-15 20:05 Jeff King
2017-03-15 20:05 ` [PATCH 1/3] rev-parse: use skip_prefix when parsing options Jeff King
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jeff King @ 2017-03-15 20:05 UTC (permalink / raw)
To: git
I happened to be looking at these yesterday and got annoyed by the
tangled logic. No bug-fixes, but the result is less repetitive and has
fewer magic numbers.
[1/3]: rev-parse: use skip_prefix when parsing options
[2/3]: rev-parse: add helper for parsing "--foo/--foo="
[3/3]: rev-parse: simplify parsing of ref options
builtin/rev-parse.c | 109 +++++++++++++++++++++++++++-------------------------
1 file changed, 57 insertions(+), 52 deletions(-)
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] rev-parse: use skip_prefix when parsing options
2017-03-15 20:05 [PATCH 0/3] minor rev-parse option-parsing cleanups Jeff King
@ 2017-03-15 20:05 ` Jeff King
2017-03-15 20:06 ` [PATCH 2/3] rev-parse: add helper for parsing "--foo/--foo=" Jeff King
2017-03-15 20:08 ` [PATCH 3/3] rev-parse: simplify parsing of ref options Jeff King
2 siblings, 0 replies; 6+ messages in thread
From: Jeff King @ 2017-03-15 20:05 UTC (permalink / raw)
To: git
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 <peff@peff.net>
---
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 e08677e55..836ccf00e 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -722,8 +722,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")) {
@@ -731,8 +731,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;
@@ -742,8 +742,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;
@@ -753,13 +753,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;
@@ -769,8 +769,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")) {
@@ -865,20 +865,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)
--
2.12.0.613.g6e7c52a0d
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] rev-parse: add helper for parsing "--foo/--foo="
2017-03-15 20:05 [PATCH 0/3] minor rev-parse option-parsing cleanups Jeff King
2017-03-15 20:05 ` [PATCH 1/3] rev-parse: use skip_prefix when parsing options Jeff King
@ 2017-03-15 20:06 ` Jeff King
2017-03-15 20:08 ` [PATCH 3/3] rev-parse: simplify parsing of ref options Jeff King
2 siblings, 0 replies; 6+ messages in thread
From: Jeff King @ 2017-03-15 20:06 UTC (permalink / raw)
To: git
We can't just use a bare skip_prefix() for these cases,
because we need to match both the "--foo" form and the
"--foo=<value>" 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 <peff@peff.net>
---
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 836ccf00e..db1417160 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -535,6 +535,25 @@ N_("git rev-parse --parseopt [<options>] -- [<args>...]\n"
"\n"
"Run \"git rev-parse --parseopt -h\" for more information on the first usage.");
+/*
+ * Parse "opt" or "opt=<value>", 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;
@@ -674,14 +693,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)
@@ -704,17 +722,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;
}
--
2.12.0.613.g6e7c52a0d
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] rev-parse: simplify parsing of ref options
2017-03-15 20:05 [PATCH 0/3] minor rev-parse option-parsing cleanups Jeff King
2017-03-15 20:05 ` [PATCH 1/3] rev-parse: use skip_prefix when parsing options Jeff King
2017-03-15 20:06 ` [PATCH 2/3] rev-parse: add helper for parsing "--foo/--foo=" Jeff King
@ 2017-03-15 20:08 ` Jeff King
2017-03-15 21:09 ` Junio C Hamano
2 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2017-03-15 20:08 UTC (permalink / raw)
To: git
All of these options do the same thing "--foo" iterates over
the "foo" refs, and "--foo=<glob>" 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 <peff@peff.net>
---
This actually drops the last caller for for_each_branch_ref(). I'm not
sure if we shoulder consider cleaning up the proliferation of
for_each_ref() helpers.
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 db1417160..76be91350 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;
@@ -749,42 +758,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);
- 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);
+ if (opt_with_value(arg, "--branches", &arg)) {
+ handle_ref_opt(arg, "refs/heads/");
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);
- continue;
- }
- if (skip_prefix(arg, "--remotes=", &arg)) {
- for_each_glob_ref_in(show_reference, arg,
- "refs/remotes/", NULL);
- clear_ref_exclusion(&ref_excludes);
+ handle_ref_opt(arg, NULL);
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)) {
--
2.12.0.613.g6e7c52a0d
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] rev-parse: simplify parsing of ref options
2017-03-15 20:08 ` [PATCH 3/3] rev-parse: simplify parsing of ref options Jeff King
@ 2017-03-15 21:09 ` Junio C Hamano
2017-03-15 21:38 ` Jeff King
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2017-03-15 21:09 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> This actually drops the last caller for for_each_branch_ref(). I'm not
> sure if we shoulder consider cleaning up the proliferation of
> for_each_ref() helpers.
That is certainly a good thing to do (but outside this series).
I am wondering if "git diff" could have chosen a better way to
arrange deleted and inserted lines. The first two if statements
in the preimage corresponds to the new opt-with-value(branches)
thing, the next two are repalced by opt-with-value(tags), an
odd-man-out "--glob=" thing is replaced with handle_ref_opt(NULL),
and then two "--remotes" thing are paired with the final
opt-with-value(remotes) thing.
That would break the usual expectation of seeing all "-" first and
then "+" when there is no intervening " " context lines, so such an
output might break tools, though (I do not think "git apply" would
choke).
> 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 db1417160..76be91350 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;
> @@ -749,42 +758,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);
> - 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);
> + if (opt_with_value(arg, "--branches", &arg)) {
> + handle_ref_opt(arg, "refs/heads/");
> 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);
> - continue;
> - }
> - if (skip_prefix(arg, "--remotes=", &arg)) {
> - for_each_glob_ref_in(show_reference, arg,
> - "refs/remotes/", NULL);
> - clear_ref_exclusion(&ref_excludes);
> + handle_ref_opt(arg, NULL);
> 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)) {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] rev-parse: simplify parsing of ref options
2017-03-15 21:09 ` Junio C Hamano
@ 2017-03-15 21:38 ` Jeff King
0 siblings, 0 replies; 6+ messages in thread
From: Jeff King @ 2017-03-15 21:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Wed, Mar 15, 2017 at 02:09:33PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > This actually drops the last caller for for_each_branch_ref(). I'm not
> > sure if we shoulder consider cleaning up the proliferation of
> > for_each_ref() helpers.
>
> That is certainly a good thing to do (but outside this series).
>
> I am wondering if "git diff" could have chosen a better way to
> arrange deleted and inserted lines. The first two if statements
> in the preimage corresponds to the new opt-with-value(branches)
> thing, the next two are repalced by opt-with-value(tags), an
> odd-man-out "--glob=" thing is replaced with handle_ref_opt(NULL),
> and then two "--remotes" thing are paired with the final
> opt-with-value(remotes) thing.
>
> That would break the usual expectation of seeing all "-" first and
> then "+" when there is no intervening " " context lines, so such an
> output might break tools, though (I do not think "git apply" would
> choke).
I think in this case you could still show it better with context lines
between. Something like:
@@ -749,42 +758,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);
- continue;
- }
- if (!strcmp(arg, "--branches")) {
- for_each_branch_ref(show_reference, NULL);
- clear_ref_exclusion(&ref_excludes);
+ if (opt_with_value(arg, "--branches", &arg)) {
+ handle_ref_opt(arg, "refs/heads/");
continue;
}
- if (skip_prefix(arg, "--tags=", &arg)) {
- for_each_glob_ref_in(show_reference, arg,
- "refs/tags/", NULL);
- clear_ref_exclusion(&ref_excludes);
- 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);
- 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)) {
I have no idea what heuristic might generate that diff, though. Using
--patience and --histogram does provided different results than Myers
for this case, but none produces the output above. The other two try to
push the new "--branches" lines up as a replacement for "--branches=",
but then you get a clump of "--branches", "--tags=", and "--tags".
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-15 21:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-15 20:05 [PATCH 0/3] minor rev-parse option-parsing cleanups Jeff King
2017-03-15 20:05 ` [PATCH 1/3] rev-parse: use skip_prefix when parsing options Jeff King
2017-03-15 20:06 ` [PATCH 2/3] rev-parse: add helper for parsing "--foo/--foo=" Jeff King
2017-03-15 20:08 ` [PATCH 3/3] rev-parse: simplify parsing of ref options Jeff King
2017-03-15 21:09 ` Junio C Hamano
2017-03-15 21:38 ` Jeff King
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).