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 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 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) retur
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 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
Re: [PATCH] use skip_prefix() to avoid more magic numbers
Jeff King 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 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 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 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 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 --- 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
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 --- 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 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
Re: [PATCH] use skip_prefix() to avoid more magic numbers
Am 07.10.2014 um 20:23 schrieb Junio C Hamano: > René Scharfe 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/remot
Re: [PATCH] use skip_prefix() to avoid more magic numbers
René Scharfe 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