Re: [PATCH] use skip_prefix() to avoid more magic numbers
From: Jonathan Nieder <hidden>
Date: 2016-06-15 23:02:39
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. [...]
quoted hunk ↗ jump to hunk
--- 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? [...]
quoted hunk ↗ jump to hunk
--- a/builtin/log.c +++ b/builtin/log.c
[...]
quoted hunk ↗ jump to hunk
@@ -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. [...]
quoted hunk ↗ jump to hunk
--- a/pretty.c +++ b/pretty.c
[...]
quoted hunk ↗ jump to hunk
@@ -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) [...]
quoted hunk ↗ jump to hunk
@@ -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 <redacted>
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 ofs, const char *value) +int parse_decorate_color_config(const char *var, const char *slot_name, const char *value) { - int slot = parse_decorate_color_slot(var + ofs); + int slot = parse_decorate_color_slot(slot_name); if (slot < 0) return 0; if (!value)
diff --git i/log-tree.h w/log-tree.h
index b26160c..d637b04 100644
--- i/log-tree.h
+++ w/log-tree.h@@ -7,7 +7,8 @@ struct log_info { struct commit *commit, *parent; }; -int parse_decorate_color_config(const char *var, const int ofs, const char *value); +int parse_decorate_color_config(const char *var, const char *slot_name, + const char *value); void init_log_tree_opt(struct rev_info *); int log_tree_diff_flush(struct rev_info *); int log_tree_commit(struct rev_info *, struct commit *);
diff --git i/pretty.c w/pretty.c
index a181ac6..e2c59ef 100644
--- i/pretty.c
+++ w/pretty.c@@ -808,19 +808,19 @@ static void parse_commit_header(struct format_commit_context *context) int i; for (i = 0; msg[i]; i++) { - const char *name; + const char *ident; int eol; for (eol = i; msg[eol] && msg[eol] != '\n'; eol++) ; /* do nothing */ if (i == eol) { break; - } else if (skip_prefix(msg + i, "author ", &name)) { - context->author.off = name - msg; - context->author.len = msg + eol - name; - } else if (skip_prefix(msg + i, "committer ", &name)) { - context->committer.off = name - msg; - context->committer.len = msg + eol - name; + } else if (skip_prefix(msg + i, "author ", &ident)) { + context->author.off = ident - msg; + context->author.len = msg + eol - ident; + } else if (skip_prefix(msg + i, "committer ", &ident)) { + context->committer.off = ident - msg; + context->committer.len = msg + eol - ident; } i = eol; }
@@ -1518,7 +1518,7 @@ static void pp_header(struct pretty_print_context *pp, int parents_shown = 0; for (;;) { - const char *name, *line = *msg_p; + const char *ident, *line = *msg_p; int linelen = get_one_line(*msg_p); if (!linelen)
@@ -1553,14 +1553,14 @@ static void pp_header(struct pretty_print_context *pp, * FULL shows both authors but not dates. * FULLER shows both authors and dates. */ - if (skip_prefix(line, "author ", &name)) { + if (skip_prefix(line, "author ", &ident)) { strbuf_grow(sb, linelen + 80); - pp_user_info(pp, "Author", sb, name, encoding); + pp_user_info(pp, "Author", sb, ident, encoding); } - if (skip_prefix(line, "committer ", &name) && + if (skip_prefix(line, "committer ", &ident) && (pp->fmt == CMIT_FMT_FULL || pp->fmt == CMIT_FMT_FULLER)) { strbuf_grow(sb, linelen + 80); - pp_user_info(pp, "Commit", sb, name, encoding); + pp_user_info(pp, "Commit", sb, ident, encoding); } } }