Re: [PATCH] use skip_prefix() to avoid more magic numbers
Am 07.10.2014 um 20:23 schrieb Junio C Hamano:
René Scharfe l@web.de writes:
@@ -335,20 +337,18 @@ static int append_ref(const char *refname, const
unsigned char *sha1, int flags,
static struct {
int kind;
const char *prefix;
-int pfxlen;
} ref_kind[] = {
-{ REF_LOCAL_BRANCH, refs/heads/, 11 },
-{ REF_REMOTE_BRANCH, refs/remotes/, 13 },
+{ REF_LOCAL_BRANCH, refs/heads/ },
+{ REF_REMOTE_BRANCH, refs/remotes/ },
};
/* Detect kind */
for (i = 0; i ARRAY_SIZE(ref_kind); i++) {
prefix = ref_kind[i].prefix;
-if (strncmp(refname, prefix, ref_kind[i].pfxlen))
-continue;
-kind = ref_kind[i].kind;
-refname += ref_kind[i].pfxlen;
-break;
+if (skip_prefix(refname, prefix, refname)) {
+kind = ref_kind[i].kind;
+break;
+}
This certainly makes it easier to read.
I suspect that the original was done as a (possibly premature)
optimization to avoid having to do strlen(3) on a variable that
points at constant strings for each and every ref we iterate with
for_each_rawref(), and it is somewhat sad to see it lost because
skip_prefix() assumes that the caller never knows the length of the
prefix, though.
I didn't think much about the performance implications. skip_prefix()
doesn't call strlen(3), though. Your reply made me curious. The
synthetic test program below can be used to call the old and the new
code numerous times. I called it like this:
for a in strncmp skip_prefix
do
for b in refs/heads/x refs/remotes/y refs/of/the/third/kind
do
time ./test-prefix $a $b
done
done
And got the following results:
1x strncmp for refs/heads/x, which is a local branch
real0m2.423s
user0m2.420s
sys 0m0.000s
1x strncmp for refs/remotes/y, which is a remote branch
real0m4.331s
user0m4.328s
sys 0m0.000s
1x strncmp for refs/of/the/third/kind, which is no branch
real0m3.878s
user0m3.872s
sys 0m0.000s
1x skip_prefix for refs/heads/x, which is a local branch
real0m0.891s
user0m0.888s
sys 0m0.000s
1x skip_prefix for refs/remotes/y, which is a remote branch
real0m1.345s
user0m1.340s
sys 0m0.000s
1x skip_prefix for refs/of/the/third/kind, which is no branch
real0m1.080s
user0m1.076s
sys 0m0.000s
The code handles millions of ref strings per second before and after
the change, and with the change it's faster. I hope the results are
reproducible and make it easier to say goodbye to pfxlen. :)
René
---
.gitignore| 1 +
Makefile | 1 +
test-prefix.c | 87 +++
3 files changed, 89 insertions(+)
create mode 100644 test-prefix.c
diff --git a/.gitignore b/.gitignore
index 5bfb234..8416c5e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -193,6 +193,7 @@
/test-mktemp
/test-parse-options
/test-path-utils
+/test-prefix
/test-prio-queue
/test-read-cache
/test-regex
diff --git a/Makefile b/Makefile
index f34a2d4..c09b59e 100644
--- a/Makefile
+++ b/Makefile
@@ -561,6 +561,7 @@ TEST_PROGRAMS_NEED_X += test-mergesort
TEST_PROGRAMS_NEED_X += test-mktemp
TEST_PROGRAMS_NEED_X += test-parse-options
TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-prefix
TEST_PROGRAMS_NEED_X += test-prio-queue
TEST_PROGRAMS_NEED_X += test-read-cache
TEST_PROGRAMS_NEED_X += test-regex
diff --git a/test-prefix.c b/test-prefix.c
new file mode 100644
index 000..ddc63af
--- /dev/null
+++ b/test-prefix.c
@@ -0,0 +1,87 @@
+#include cache.h
+
+#define ROUNDS 1
+
+#define REF_LOCAL_BRANCH0x01
+#define REF_REMOTE_BRANCH 0x02
+
+static int test_skip_prefix(const char *refname)
+{
+ int kind, i;
+ const char *prefix;
+ static struct {
+ int kind;
+ const char *prefix;
+ } ref_kind[] = {
+ { REF_LOCAL_BRANCH, refs/heads/ },
+ { REF_REMOTE_BRANCH, refs/remotes/ },
+ };
+
+ for (i = 0; i ARRAY_SIZE(ref_kind); i++) {
+ prefix = ref_kind[i].prefix;
+ if (skip_prefix(refname, prefix, refname)) {
+ kind = ref_kind[i].kind;
+ break;
+ }
+ }
+ if (ARRAY_SIZE(ref_kind) = i)
+ return 0;
+ return kind;
+}
+
+static int test_strncmp(const char *refname)
+{
+ int kind, i;
+ const char *prefix;
+ static struct {
+ int kind;
+ const char *prefix;
+ int pfxlen;
+ } ref_kind[] = {
+ { REF_LOCAL_BRANCH, refs/heads/, 11 },
+ { REF_REMOTE_BRANCH, refs/remotes/, 13 },
+ };
+
+ for (i = 0; i ARRAY_SIZE(ref_kind);
Re: [PATCH] use skip_prefix() to avoid more magic numbers
René Scharfe l@web.de writes: I didn't think much about the performance implications. skip_prefix() doesn't call strlen(3), though. Ah, OK. The code handles millions of ref strings per second before and after the change, and with the change it's faster. I hope the results are reproducible and make it easier to say goodbye to pfxlen. :) ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] use skip_prefix() to avoid more magic numbers
Jeff King p...@peff.net writes:
On Sun, Oct 05, 2014 at 03:49:19PM -0700, Jonathan Nieder wrote:
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var,
int ofs)
static int git_branch_config(const char *var, const char *value, void *cb)
{
+ const char *slot_name;
+
if (starts_with(var, column.))
return git_column_config(var, value, branch, colopts);
if (!strcmp(var, color.branch)) {
branch_use_color = git_config_colorbool(var, value);
return 0;
}
- if (starts_with(var, color.branch.)) {
- int slot = parse_branch_color_slot(var, 13);
+ if (skip_prefix(var, color.branch., slot_name)) {
+ int slot = parse_branch_color_slot(var, slot_name - var);
I wonder why the separate var and offset parameters exist in the first
place. Wouldn't taking a single const char * so the old code could use
'var + 13' instead of 'var, 13' have worked?
I think this is in the same boat as parse_diff_color_slot, which I fixed
in 9e1a5eb (parse_diff_color_slot: drop ofs parameter, 2014-06-18). The
short of it is that the function used to want both the full name and the
slot name, but now needs only the latter.
The fix you proposed below is along the same line, and looks good to me
(and grepping for 'var *+ *ofs' shows only the two sites you found, so
hopefully that is the last of it).
So perhaps we would want this change as a preliminary separate
patch?
Thanks
@@ -809,18 +808,19 @@ static void parse_commit_header(struct
format_commit_context *context)
int i;
for (i = 0; msg[i]; i++) {
+ const char *name;
ident instead of name, maybe? (since it's a 'name email timestamp'
field)
Yeah, agreed.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] use skip_prefix() to avoid more magic numbers
Jeff King p...@peff.net writes:
On Sun, Oct 05, 2014 at 03:49:19PM -0700, Jonathan Nieder wrote:
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var,
int ofs)
static int git_branch_config(const char *var, const char *value, void *cb)
{
+ const char *slot_name;
+
if (starts_with(var, column.))
return git_column_config(var, value, branch, colopts);
if (!strcmp(var, color.branch)) {
branch_use_color = git_config_colorbool(var, value);
return 0;
}
- if (starts_with(var, color.branch.)) {
- int slot = parse_branch_color_slot(var, 13);
+ if (skip_prefix(var, color.branch., slot_name)) {
+ int slot = parse_branch_color_slot(var, slot_name - var);
I wonder why the separate var and offset parameters exist in the first
place. Wouldn't taking a single const char * so the old code could use
'var + 13' instead of 'var, 13' have worked?
I think this is in the same boat as parse_diff_color_slot, which I fixed
in 9e1a5eb (parse_diff_color_slot: drop ofs parameter, 2014-06-18). The
short of it is that the function used to want both the full name and the
slot name, but now needs only the latter.
The fix you proposed below is along the same line, and looks good to me
(and grepping for 'var *+ *ofs' shows only the two sites you found, so
hopefully that is the last of it).
builtin/commit.c::parse_status_slot() has the same construct.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] use skip_prefix() to avoid more magic numbers
René Scharfe l@web.de writes:
@@ -335,20 +337,18 @@ static int append_ref(const char *refname, const
unsigned char *sha1, int flags,
static struct {
int kind;
const char *prefix;
- int pfxlen;
} ref_kind[] = {
- { REF_LOCAL_BRANCH, refs/heads/, 11 },
- { REF_REMOTE_BRANCH, refs/remotes/, 13 },
+ { REF_LOCAL_BRANCH, refs/heads/ },
+ { REF_REMOTE_BRANCH, refs/remotes/ },
};
/* Detect kind */
for (i = 0; i ARRAY_SIZE(ref_kind); i++) {
prefix = ref_kind[i].prefix;
- if (strncmp(refname, prefix, ref_kind[i].pfxlen))
- continue;
- kind = ref_kind[i].kind;
- refname += ref_kind[i].pfxlen;
- break;
+ if (skip_prefix(refname, prefix, refname)) {
+ kind = ref_kind[i].kind;
+ break;
+ }
This certainly makes it easier to read.
I suspect that the original was done as a (possibly premature)
optimization to avoid having to do strlen(3) on a variable that
points at constant strings for each and every ref we iterate with
for_each_rawref(), and it is somewhat sad to see it lost because
skip_prefix() assumes that the caller never knows the length of the
prefix, though.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] use skip_prefix() to avoid more magic numbers
On Tue, Oct 07, 2014 at 11:21:58AM -0700, Junio C Hamano wrote: The fix you proposed below is along the same line, and looks good to me (and grepping for 'var *+ *ofs' shows only the two sites you found, so hopefully that is the last of it). builtin/commit.c::parse_status_slot() has the same construct. Thanks, I missed it because it uses offset instead of ofs. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] use skip_prefix() to avoid more magic numbers
On Tue, Oct 07, 2014 at 11:14:35AM -0700, Junio C Hamano wrote:
The fix you proposed below is along the same line, and looks good to me
(and grepping for 'var *+ *ofs' shows only the two sites you found, so
hopefully that is the last of it).
So perhaps we would want this change as a preliminary separate
patch?
Here it is on top of master, with a commit message (and including the
other case you found). I've attributed it to Jonathan, who did most of
the work. We'd need his signoff and approval, of course.
-- 8 --
From: Jonathan Nieder jrnie...@gmail.com
Subject: pass config slots as pointers instead of offsets
Many config-parsing helpers, like parse_branch_color_slot,
take the name of a config variable and an offset to the
slot name (e.g., color.branch.plain is passed along with
13 to effectively pass plain). This is leftover from the
time that these functions would die() on error, and would
want the full variable name for error reporting.
These days they do not use the full variable name at all.
Passing a single pointer to the slot name is more natural,
and lets us more easily adjust the callers to use skip_prefix
to avoid manually writing offset numbers.
This is effectively a continuation of 9e1a5eb, which did the
same for parse_diff_color_slot. This patch covers all of the
remaining similar constructs.
Signed-off-by: Jeff King p...@peff.net
---
Actually, parse_decorate_color_config is slightly odd in that it takes
the variable and the slot_name after this patch. That's because it
passes the full variable name to color_parse, which does still die() on
error. Perhaps it should be converted to return an error, too.
builtin/branch.c | 16
builtin/commit.c | 19 +--
builtin/log.c| 2 +-
log-tree.c | 4 ++--
log-tree.h | 2 +-
5 files changed, 21 insertions(+), 22 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 9e4666f..2c97141 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -62,19 +62,19 @@ static unsigned char merge_filter_ref[20];
static struct string_list output = STRING_LIST_INIT_DUP;
static unsigned int colopts;
-static int parse_branch_color_slot(const char *var, int ofs)
+static int parse_branch_color_slot(const char *slot)
{
- if (!strcasecmp(var+ofs, plain))
+ if (!strcasecmp(slot, plain))
return BRANCH_COLOR_PLAIN;
- if (!strcasecmp(var+ofs, reset))
+ if (!strcasecmp(slot, reset))
return BRANCH_COLOR_RESET;
- if (!strcasecmp(var+ofs, remote))
+ if (!strcasecmp(slot, remote))
return BRANCH_COLOR_REMOTE;
- if (!strcasecmp(var+ofs, local))
+ if (!strcasecmp(slot, local))
return BRANCH_COLOR_LOCAL;
- if (!strcasecmp(var+ofs, current))
+ if (!strcasecmp(slot, current))
return BRANCH_COLOR_CURRENT;
- if (!strcasecmp(var+ofs, upstream))
+ if (!strcasecmp(slot, upstream))
return BRANCH_COLOR_UPSTREAM;
return -1;
}
@@ -88,7 +88,7 @@ static int git_branch_config(const char *var, const char
*value, void *cb)
return 0;
}
if (starts_with(var, color.branch.)) {
- int slot = parse_branch_color_slot(var, 13);
+ int slot = parse_branch_color_slot(var + 13);
if (slot 0)
return 0;
if (!value)
diff --git a/builtin/commit.c b/builtin/commit.c
index b0fe784..c45613a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1271,22 +1271,21 @@ static int dry_run_commit(int argc, const char **argv,
const char *prefix,
return commitable ? 0 : 1;
}
-static int parse_status_slot(const char *var, int offset)
+static int parse_status_slot(const char *slot)
{
- if (!strcasecmp(var+offset, header))
+ if (!strcasecmp(slot, header))
return WT_STATUS_HEADER;
- if (!strcasecmp(var+offset, branch))
+ if (!strcasecmp(slot, branch))
return WT_STATUS_ONBRANCH;
- if (!strcasecmp(var+offset, updated)
- || !strcasecmp(var+offset, added))
+ if (!strcasecmp(slot, updated) || !strcasecmp(slot, added))
return WT_STATUS_UPDATED;
- if (!strcasecmp(var+offset, changed))
+ if (!strcasecmp(slot, changed))
return WT_STATUS_CHANGED;
- if (!strcasecmp(var+offset, untracked))
+ if (!strcasecmp(slot, untracked))
return WT_STATUS_UNTRACKED;
- if (!strcasecmp(var+offset, nobranch))
+ if (!strcasecmp(slot, nobranch))
return WT_STATUS_NOBRANCH;
- if (!strcasecmp(var+offset, unmerged))
+ if (!strcasecmp(slot, unmerged))
return WT_STATUS_UNMERGED;
return -1;
}
@@ -1324,7 +1323,7 @@ static int git_status_config(const char *k, const char
*v, void *cb)
return 0;
}
if (starts_with(k, status.color.) ||
Re: [PATCH] use skip_prefix() to avoid more magic numbers
On Tue, Oct 07, 2014 at 03:16:56PM -0400, Jeff King wrote:
Actually, parse_decorate_color_config is slightly odd in that it takes
the variable and the slot_name after this patch. That's because it
passes the full variable name to color_parse, which does still die() on
error. Perhaps it should be converted to return an error, too.
Actually, this doesn't let us get rid of the var in
parse_decorate_color_config, because it also wants to call
config_error_nonbool. However, while looking at this, I did notice that
some of the error messages generated by color_parse are a bit ugly, and
came up with the patch below (on top of what I just sent, but it could
be separate).
-- 8 --
Subject: color_parse: do not mention variable name in error message
Originally the color-parsing function was used only for
config variables. It made sense to pass the variable name so
that the die() message could be something like:
$ git -c color.branch.plain=bogus branch
fatal: bad color value 'bogus' for variable 'color.branch.plain'
These days we call it in other contexts, and the resulting
error messages are a little confusing:
$ git log --pretty='%C(bogus)'
fatal: bad color value 'bogus' for variable '--pretty format'
$ git config --get-color foo.bar bogus
fatal: bad color value 'bogus' for variable 'command line'
This patch teaches color_parse to complain only about the
value, and then return an error code. Config callers can
then propagate that up to the config parser, which mentions
the variable name. Other callers can provide a custom
message. After this patch these three cases now look like:
$ git -c color.branch.plain=bogus branch
error: invalid color value: bogus
fatal: unable to parse 'color.branch.plain' from command-line config
$ git log --pretty='%C(bogus)'
error: invalid color value: bogus
fatal: unable to parse --pretty format
$ git config --get-color foo.bar bogus
error: invalid color value: bogus
fatal: unable to parse default color value
Signed-off-by: Jeff King p...@peff.net
---
I think the two-line errors are kind of ugly. It does match how
config_error_nonbool works, which also propagates its errors, and looks
like:
$ git -c color.branch.plain branch
error: Missing value for 'color.branch.plain'
fatal: unable to parse 'color.branch.plain' from command-line config
We could instead make color_parse silently return -1, and leave it up to
the caller to complain (and probably add die_bad_color_config() or
similar to avoid repetition in the config callers).
builtin/branch.c | 3 +--
builtin/clean.c| 3 +--
builtin/commit.c | 3 +--
builtin/config.c | 9 ++---
builtin/for-each-ref.c | 6 --
color.c| 13 ++---
color.h| 4 ++--
diff.c | 3 +--
grep.c | 2 +-
log-tree.c | 3 +--
pretty.c | 5 ++---
11 files changed, 26 insertions(+), 28 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2c97141..35c786d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -93,8 +93,7 @@ static int git_branch_config(const char *var, const char
*value, void *cb)
return 0;
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, branch_colors[slot]);
- return 0;
+ return color_parse(value, branch_colors[slot]);
}
return git_color_default_config(var, value, cb);
}
diff --git a/builtin/clean.c b/builtin/clean.c
index 3beeea6..a7e7b0b 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -116,8 +116,7 @@ static int git_clean_config(const char *var, const char
*value, void *cb)
return 0;
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, clean_colors[slot]);
- return 0;
+ return color_parse(value, clean_colors[slot]);
}
if (!strcmp(var, clean.requireforce)) {
diff --git a/builtin/commit.c b/builtin/commit.c
index c45613a..407566d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1328,8 +1328,7 @@ static int git_status_config(const char *k, const char
*v, void *cb)
return 0;
if (!v)
return config_error_nonbool(k);
- color_parse(v, k, s-color_palette[slot]);
- return 0;
+ return color_parse(v, s-color_palette[slot]);
}
if (!strcmp(k, status.relativepaths)) {
s-relative_paths = git_config_bool(k, v);
diff --git a/builtin/config.c b/builtin/config.c
index 37305e9..8cc2604 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -296,7 +296,8 @@ static int git_get_color_config(const char *var, const char
*value, void *cb)
if (!strcmp(var, get_color_slot)) {
if (!value)
Re: [PATCH] use skip_prefix() to avoid more magic numbers
Jeff King p...@peff.net writes:
Subject: color_parse: do not mention variable name in error message
...
I think the two-line errors are kind of ugly. It does match how
config_error_nonbool works, which also propagates its errors, and looks
like:
$ git -c color.branch.plain branch
error: Missing value for 'color.branch.plain'
fatal: unable to parse 'color.branch.plain' from command-line config
We could instead make color_parse silently return -1, and leave it up to
the caller to complain (and probably add die_bad_color_config() or
similar to avoid repetition in the config callers).
Yeah, in the longer term we would want to do something like that to
fix both nonbool and this one, but for now this should help avoid
confusion.
Thanks for a nice analysis, write-up and patch.
builtin/branch.c | 3 +--
builtin/clean.c| 3 +--
builtin/commit.c | 3 +--
builtin/config.c | 9 ++---
builtin/for-each-ref.c | 6 --
color.c| 13 ++---
color.h| 4 ++--
diff.c | 3 +--
grep.c | 2 +-
log-tree.c | 3 +--
pretty.c | 5 ++---
11 files changed, 26 insertions(+), 28 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2c97141..35c786d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -93,8 +93,7 @@ static int git_branch_config(const char *var, const char
*value, void *cb)
return 0;
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, branch_colors[slot]);
- return 0;
+ return color_parse(value, branch_colors[slot]);
}
return git_color_default_config(var, value, cb);
}
diff --git a/builtin/clean.c b/builtin/clean.c
index 3beeea6..a7e7b0b 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -116,8 +116,7 @@ static int git_clean_config(const char *var, const char
*value, void *cb)
return 0;
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, clean_colors[slot]);
- return 0;
+ return color_parse(value, clean_colors[slot]);
}
if (!strcmp(var, clean.requireforce)) {
diff --git a/builtin/commit.c b/builtin/commit.c
index c45613a..407566d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1328,8 +1328,7 @@ static int git_status_config(const char *k, const char
*v, void *cb)
return 0;
if (!v)
return config_error_nonbool(k);
- color_parse(v, k, s-color_palette[slot]);
- return 0;
+ return color_parse(v, s-color_palette[slot]);
}
if (!strcmp(k, status.relativepaths)) {
s-relative_paths = git_config_bool(k, v);
diff --git a/builtin/config.c b/builtin/config.c
index 37305e9..8cc2604 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -296,7 +296,8 @@ static int git_get_color_config(const char *var, const
char *value, void *cb)
if (!strcmp(var, get_color_slot)) {
if (!value)
config_error_nonbool(var);
- color_parse(value, var, parsed_color);
+ if (color_parse(value, parsed_color) 0)
+ return -1;
get_color_found = 1;
}
return 0;
@@ -309,8 +310,10 @@ static void get_color(const char *def_color)
git_config_with_options(git_get_color_config, NULL,
given_config_source, respect_includes);
- if (!get_color_found def_color)
- color_parse(def_color, command line, parsed_color);
+ if (!get_color_found def_color) {
+ if (color_parse(def_color, parsed_color) 0)
+ die(_(unable to parse default color value));
+ }
fputs(parsed_color, stdout);
}
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index fda0f04..7ee86b3 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -671,7 +671,8 @@ static void populate_value(struct refinfo *ref)
} else if (starts_with(name, color:)) {
char color[COLOR_MAXLEN] = ;
- color_parse(name + 6, --format, color);
+ if (color_parse(name + 6, color) 0)
+ die(_(unable to parse format));
v-s = xstrdup(color);
continue;
} else if (!strcmp(name, flag)) {
@@ -1004,7 +1005,8 @@ static void show_ref(struct refinfo *info, const char
*format, int quote_style)
struct atom_value resetv;
char color[COLOR_MAXLEN] = ;
- color_parse(reset, --format, color);
+ if (color_parse(reset, color) 0)
+
Re: [PATCH] use skip_prefix() to avoid more magic numbers
René Scharfe wrote:
Continue where ae021d87 (use skip_prefix to avoid magic numbers) left off
and use skip_prefix() in more places for determining the lengths of prefix
strings to avoid using dependent constants and other indirect methods.
Sounds promising.
[...]
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int
ofs)
static int git_branch_config(const char *var, const char *value, void *cb)
{
+ const char *slot_name;
+
if (starts_with(var, column.))
return git_column_config(var, value, branch, colopts);
if (!strcmp(var, color.branch)) {
branch_use_color = git_config_colorbool(var, value);
return 0;
}
- if (starts_with(var, color.branch.)) {
- int slot = parse_branch_color_slot(var, 13);
+ if (skip_prefix(var, color.branch., slot_name)) {
+ int slot = parse_branch_color_slot(var, slot_name - var);
I wonder why the separate var and offset parameters exist in the first
place. Wouldn't taking a single const char * so the old code could use
'var + 13' instead of 'var, 13' have worked?
At first glance I thought this should be
int slot = parse_branch_color_slot(slot_name, 0);
to be simpler. At second glance, how about something like the following
on top?
[...]
--- a/builtin/log.c
+++ b/builtin/log.c
[...]
@@ -388,8 +390,8 @@ static int git_log_config(const char *var, const char
*value, void *cb)
default_show_root = git_config_bool(var, value);
return 0;
}
- if (starts_with(var, color.decorate.))
- return parse_decorate_color_config(var, 15, value);
+ if (skip_prefix(var, color.decorate., slot_name))
+ return parse_decorate_color_config(var, slot_name - var, value);
Likewise: the offset-based API seems odd now here, too.
[...]
--- a/pretty.c
+++ b/pretty.c
[...]
@@ -809,18 +808,19 @@ static void parse_commit_header(struct
format_commit_context *context)
int i;
for (i = 0; msg[i]; i++) {
+ const char *name;
ident instead of name, maybe? (since it's a 'name email timestamp'
field)
[...]
@@ -1522,7 +1518,7 @@ static void pp_header(struct pretty_print_context *pp,
int parents_shown = 0;
for (;;) {
- const char *line = *msg_p;
+ const char *name, *line = *msg_p;
Likewise.
With or without the changes below,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/builtin/branch.c w/builtin/branch.c
index 6785097..022a88e 100644
--- i/builtin/branch.c
+++ w/builtin/branch.c
@@ -62,19 +62,19 @@ static unsigned char merge_filter_ref[20];
static struct string_list output = STRING_LIST_INIT_DUP;
static unsigned int colopts;
-static int parse_branch_color_slot(const char *var, int ofs)
+static int parse_branch_color_slot(const char *slot)
{
- if (!strcasecmp(var+ofs, plain))
+ if (!strcasecmp(slot, plain))
return BRANCH_COLOR_PLAIN;
- if (!strcasecmp(var+ofs, reset))
+ if (!strcasecmp(slot, reset))
return BRANCH_COLOR_RESET;
- if (!strcasecmp(var+ofs, remote))
+ if (!strcasecmp(slot, remote))
return BRANCH_COLOR_REMOTE;
- if (!strcasecmp(var+ofs, local))
+ if (!strcasecmp(slot, local))
return BRANCH_COLOR_LOCAL;
- if (!strcasecmp(var+ofs, current))
+ if (!strcasecmp(slot, current))
return BRANCH_COLOR_CURRENT;
- if (!strcasecmp(var+ofs, upstream))
+ if (!strcasecmp(slot, upstream))
return BRANCH_COLOR_UPSTREAM;
return -1;
}
@@ -90,7 +90,7 @@ static int git_branch_config(const char *var, const char
*value, void *cb)
return 0;
}
if (skip_prefix(var, color.branch., slot_name)) {
- int slot = parse_branch_color_slot(var, slot_name - var);
+ int slot = parse_branch_color_slot(slot_name);
if (slot 0)
return 0;
if (!value)
diff --git i/builtin/log.c w/builtin/log.c
index 1202eba..68d5d30 100644
--- i/builtin/log.c
+++ w/builtin/log.c
@@ -391,7 +391,7 @@ static int git_log_config(const char *var, const char
*value, void *cb)
return 0;
}
if (skip_prefix(var, color.decorate., slot_name))
- return parse_decorate_color_config(var, slot_name - var, value);
+ return parse_decorate_color_config(var, slot_name, value);
if (!strcmp(var, log.mailmap)) {
use_mailmap_config = git_config_bool(var, value);
return 0;
diff --git i/log-tree.c w/log-tree.c
index cff7ac1..6402c19 100644
--- i/log-tree.c
+++ w/log-tree.c
@@ -56,9 +56,9 @@ static int parse_decorate_color_slot(const char *slot)
return -1;
}
-int parse_decorate_color_config(const char *var, const int
Re: [PATCH] use skip_prefix() to avoid more magic numbers
On Sun, Oct 05, 2014 at 03:49:19PM -0700, Jonathan Nieder wrote:
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int
ofs)
static int git_branch_config(const char *var, const char *value, void *cb)
{
+ const char *slot_name;
+
if (starts_with(var, column.))
return git_column_config(var, value, branch, colopts);
if (!strcmp(var, color.branch)) {
branch_use_color = git_config_colorbool(var, value);
return 0;
}
- if (starts_with(var, color.branch.)) {
- int slot = parse_branch_color_slot(var, 13);
+ if (skip_prefix(var, color.branch., slot_name)) {
+ int slot = parse_branch_color_slot(var, slot_name - var);
I wonder why the separate var and offset parameters exist in the first
place. Wouldn't taking a single const char * so the old code could use
'var + 13' instead of 'var, 13' have worked?
I think this is in the same boat as parse_diff_color_slot, which I fixed
in 9e1a5eb (parse_diff_color_slot: drop ofs parameter, 2014-06-18). The
short of it is that the function used to want both the full name and the
slot name, but now needs only the latter.
The fix you proposed below is along the same line, and looks good to me
(and grepping for 'var *+ *ofs' shows only the two sites you found, so
hopefully that is the last of it).
@@ -809,18 +808,19 @@ static void parse_commit_header(struct
format_commit_context *context)
int i;
for (i = 0; msg[i]; i++) {
+ const char *name;
ident instead of name, maybe? (since it's a 'name email timestamp'
field)
Yeah, agreed.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] use skip_prefix() to avoid more magic numbers
On Sat, Oct 04, 2014 at 08:54:50PM +0200, René Scharfe wrote: Continue where ae021d87 (use skip_prefix to avoid magic numbers) left off and use skip_prefix() in more places for determining the lengths of prefix strings to avoid using dependent constants and other indirect methods. Thanks, these all look like nice improvements. I think both of the suggestions made my Jonathan are good on top, though. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] use skip_prefix() to avoid more magic numbers
Continue where ae021d87 (use skip_prefix to avoid magic numbers) left off
and use skip_prefix() in more places for determining the lengths of prefix
strings to avoid using dependent constants and other indirect methods.
Signed-off-by: Rene Scharfe l@web.de
---
builtin/apply.c | 2 +-
builtin/branch.c| 29 +++
builtin/cat-file.c | 5 ++--
builtin/checkout.c | 6 ++---
builtin/clean.c | 7 +++---
builtin/commit.c| 18 +++
builtin/get-tar-commit-id.c | 5 ++--
builtin/log.c | 6 +++--
builtin/remote-ext.c| 10
pretty.c| 56 +
10 files changed, 69 insertions(+), 75 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 8714a88..97f7e8e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -435,7 +435,7 @@ static unsigned long linelen(const char *buffer, unsigned
long size)
static int is_dev_null(const char *str)
{
- return !memcmp(/dev/null, str, 9) isspace(str[9]);
+ return skip_prefix(str, /dev/null, str) isspace(*str);
}
#define TERM_SPACE 1
diff --git a/builtin/branch.c b/builtin/branch.c
index 9e4666f..6785097 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int ofs)
static int git_branch_config(const char *var, const char *value, void *cb)
{
+ const char *slot_name;
+
if (starts_with(var, column.))
return git_column_config(var, value, branch, colopts);
if (!strcmp(var, color.branch)) {
branch_use_color = git_config_colorbool(var, value);
return 0;
}
- if (starts_with(var, color.branch.)) {
- int slot = parse_branch_color_slot(var, 13);
+ if (skip_prefix(var, color.branch., slot_name)) {
+ int slot = parse_branch_color_slot(var, slot_name - var);
if (slot 0)
return 0;
if (!value)
@@ -335,20 +337,18 @@ static int append_ref(const char *refname, const unsigned
char *sha1, int flags,
static struct {
int kind;
const char *prefix;
- int pfxlen;
} ref_kind[] = {
- { REF_LOCAL_BRANCH, refs/heads/, 11 },
- { REF_REMOTE_BRANCH, refs/remotes/, 13 },
+ { REF_LOCAL_BRANCH, refs/heads/ },
+ { REF_REMOTE_BRANCH, refs/remotes/ },
};
/* Detect kind */
for (i = 0; i ARRAY_SIZE(ref_kind); i++) {
prefix = ref_kind[i].prefix;
- if (strncmp(refname, prefix, ref_kind[i].pfxlen))
- continue;
- kind = ref_kind[i].kind;
- refname += ref_kind[i].pfxlen;
- break;
+ if (skip_prefix(refname, prefix, refname)) {
+ kind = ref_kind[i].kind;
+ break;
+ }
}
if (ARRAY_SIZE(ref_kind) = i)
return 0;
@@ -872,13 +872,10 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
head = resolve_refdup(HEAD, head_sha1, 0, NULL);
if (!head)
die(_(Failed to resolve HEAD as a valid ref.));
- if (!strcmp(head, HEAD)) {
+ if (!strcmp(head, HEAD))
detached = 1;
- } else {
- if (!starts_with(head, refs/heads/))
- die(_(HEAD not found below refs/heads!));
- head += 11;
- }
+ else if (!skip_prefix(head, refs/heads/, head))
+ die(_(HEAD not found below refs/heads!));
hashcpy(merge_filter_ref, head_sha1);
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 7073304..f8d8129 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -82,8 +82,9 @@ static int cat_one_file(int opt, const char *exp_type, const
char *obj_name)
enum object_type type;
unsigned long size;
char *buffer = read_sha1_file(sha1, type,
size);
- if (memcmp(buffer, object , 7) ||
- get_sha1_hex(buffer + 7, blob_sha1))
+ const char *target;
+ if (!skip_prefix(buffer, object , target) ||
+ get_sha1_hex(target, blob_sha1))
die(%s not a valid tag,
sha1_to_hex(sha1));
free(buffer);
} else
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8afdf2b..cef1996 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1150,10 +1150,8 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
const char *argv0 = argv[0];
