{"thread":{"id":"37673","subject":"[PATCH] use skip_prefix() to avoid more magic numbers","startedAt":"2014-10-04T18:54:50Z","lastAt":"2014-10-09T20:12:08Z","messageCount":13,"participants":["René Scharfe","Jonathan Nieder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"250250","messageId":"5430427A.5080800@web.de","threadId":"37673","inReplyTo":null,"subject":"[PATCH] use skip_prefix() to avoid more magic numbers","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-10-04T18:54:50Z","receivedAt":"2014-10-04T18:54:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Continue where ae021d87 (use skip_prefix to avoid magic numbers) left off\nand use skip_prefix() in more places for determining the lengths of prefix\nstrings to avoid using dependent constants and other indirect methods.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/apply.c             |  2 +-\n builtin/branch.c            | 29 +++++++++++------------\n builtin/cat-file.c          |  5 ++--\n builtin/checkout.c          |  6 ++---\n builtin/clean.c             |  7 +++---\n builtin/commit.c            | 18 +++++++--------\n builtin/get-tar-commit-id.c |  5 ++--\n builtin/log.c               |  6 +++--\n builtin/remote-ext.c        | 10 ++++----\n pretty.c                    | 56 +++++++++++++++++++++------------------------\n 10 files changed, 69 insertions(+), 75 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 8714a88..97f7e8e 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -435,7 +435,7 @@ static unsigned long linelen(const char *buffer, unsigned long size)\n \n static int is_dev_null(const char *str)\n {\n-\treturn !memcmp(\"/dev/null\", str, 9) && isspace(str[9]);\n+\treturn skip_prefix(str, \"/dev/null\", &str) && isspace(*str);\n }\n \n #define TERM_SPACE\t1\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 9e4666f..6785097 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int ofs)\n \n static int git_branch_config(const char *var, const char *value, void *cb)\n {\n+\tconst char *slot_name;\n+\n \tif (starts_with(var, \"column.\"))\n \t\treturn git_column_config(var, value, \"branch\", &colopts);\n \tif (!strcmp(var, \"color.branch\")) {\n \t\tbranch_use_color = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n-\tif (starts_with(var, \"color.branch.\")) {\n-\t\tint slot = parse_branch_color_slot(var, 13);\n+\tif (skip_prefix(var, \"color.branch.\", &slot_name)) {\n+\t\tint slot = parse_branch_color_slot(var, slot_name - var);\n \t\tif (slot < 0)\n \t\t\treturn 0;\n \t\tif (!value)\n@@ -335,20 +337,18 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,\n \tstatic struct {\n \t\tint kind;\n \t\tconst char *prefix;\n-\t\tint pfxlen;\n \t} ref_kind[] = {\n-\t\t{ REF_LOCAL_BRANCH, \"refs/heads/\", 11 },\n-\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/\", 13 },\n+\t\t{ REF_LOCAL_BRANCH, \"refs/heads/\" },\n+\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/\" },\n \t};\n \n \t/* Detect kind */\n \tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n \t\tprefix = ref_kind[i].prefix;\n-\t\tif (strncmp(refname, prefix, ref_kind[i].pfxlen))\n-\t\t\tcontinue;\n-\t\tkind = ref_kind[i].kind;\n-\t\trefname += ref_kind[i].pfxlen;\n-\t\tbreak;\n+\t\tif (skip_prefix(refname, prefix, &refname)) {\n+\t\t\tkind = ref_kind[i].kind;\n+\t\t\tbreak;\n+\t\t}\n \t}\n \tif (ARRAY_SIZE(ref_kind) <= i)\n \t\treturn 0;\n@@ -872,13 +872,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \thead = resolve_refdup(\"HEAD\", head_sha1, 0, NULL);\n \tif (!head)\n \t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n-\tif (!strcmp(head, \"HEAD\")) {\n+\tif (!strcmp(head, \"HEAD\"))\n \t\tdetached = 1;\n-\t} else {\n-\t\tif (!starts_with(head, \"refs/heads/\"))\n-\t\t\tdie(_(\"HEAD not found below refs/heads!\"));\n-\t\thead += 11;\n-\t}\n+\telse if (!skip_prefix(head, \"refs/heads/\", &head))\n+\t\tdie(_(\"HEAD not found below refs/heads!\"));\n \thashcpy(merge_filter_ref, head_sha1);\n \n \ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 7073304..f8d8129 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -82,8 +82,9 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \t\t\t\tenum object_type type;\n \t\t\t\tunsigned long size;\n \t\t\t\tchar *buffer = read_sha1_file(sha1, &type, &size);\n-\t\t\t\tif (memcmp(buffer, \"object \", 7) ||\n-\t\t\t\t    get_sha1_hex(buffer + 7, blob_sha1))\n+\t\t\t\tconst char *target;\n+\t\t\t\tif (!skip_prefix(buffer, \"object \", &target) ||\n+\t\t\t\t    get_sha1_hex(target, blob_sha1))\n \t\t\t\t\tdie(\"%s not a valid tag\", sha1_to_hex(sha1));\n \t\t\t\tfree(buffer);\n \t\t\t} else\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8afdf2b..cef1996 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1150,10 +1150,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tconst char *argv0 = argv[0];\n \t\tif (!argc || !strcmp(argv0, \"--\"))\n \t\t\tdie (_(\"--track needs a branch name\"));\n-\t\tif (starts_with(argv0, \"refs/\"))\n-\t\t\targv0 += 5;\n-\t\tif (starts_with(argv0, \"remotes/\"))\n-\t\t\targv0 += 8;\n+\t\tskip_prefix(argv0, \"refs/\", &argv0);\n+\t\tskip_prefix(argv0, \"remotes/\", &argv0);\n \t\targv0 = strchr(argv0, '/');\n \t\tif (!argv0 || !argv0[1])\n \t\t\tdie (_(\"Missing branch name; try -b\"));\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 3beeea6..c35505e 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -100,6 +100,8 @@ static int parse_clean_color_slot(const char *var)\n \n static int git_clean_config(const char *var, const char *value, void *cb)\n {\n+\tconst char *slot_name;\n+\n \tif (starts_with(var, \"column.\"))\n \t\treturn git_column_config(var, value, \"clean\", &colopts);\n \n@@ -109,9 +111,8 @@ static int git_clean_config(const char *var, const char *value, void *cb)\n \t\tclean_use_color = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n-\tif (starts_with(var, \"color.interactive.\")) {\n-\t\tint slot = parse_clean_color_slot(var +\n-\t\t\t\t\t\t  strlen(\"color.interactive.\"));\n+\tif (skip_prefix(var, \"color.interactive.\", &slot_name)) {\n+\t\tint slot = parse_clean_color_slot(slot_name);\n \t\tif (slot < 0)\n \t\t\treturn 0;\n \t\tif (!value)\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b0fe784..cff7802 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1294,6 +1294,7 @@ static int parse_status_slot(const char *var, int offset)\n static int git_status_config(const char *k, const char *v, void *cb)\n {\n \tstruct wt_status *s = cb;\n+\tconst char *slot_name;\n \n \tif (starts_with(k, \"column.\"))\n \t\treturn git_column_config(k, v, \"status\", &s->colopts);\n@@ -1323,8 +1324,9 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t\ts->display_comment_prefix = git_config_bool(k, v);\n \t\treturn 0;\n \t}\n-\tif (starts_with(k, \"status.color.\") || starts_with(k, \"color.status.\")) {\n-\t\tint slot = parse_status_slot(k, 13);\n+\tif (skip_prefix(k, \"status.color.\", &slot_name) ||\n+\t    skip_prefix(k, \"color.status.\", &slot_name)) {\n+\t\tint slot = parse_status_slot(k, slot_name - k);\n \t\tif (slot < 0)\n \t\t\treturn 0;\n \t\tif (!v)\n@@ -1513,13 +1515,11 @@ static void print_summary(const char *prefix, const unsigned char *sha1,\n \tdiff_setup_done(&rev.diffopt);\n \n \thead = resolve_ref_unsafe(\"HEAD\", junk_sha1, 0, NULL);\n-\tprintf(\"[%s%s \",\n-\t\tstarts_with(head, \"refs/heads/\") ?\n-\t\t\thead + 11 :\n-\t\t\t!strcmp(head, \"HEAD\") ?\n-\t\t\t\t_(\"detached HEAD\") :\n-\t\t\t\thead,\n-\t\tinitial_commit ? _(\" (root-commit)\") : \"\");\n+\tif (!strcmp(head, \"HEAD\"))\n+\t\thead = _(\"detached HEAD\");\n+\telse\n+\t\tskip_prefix(head, \"refs/heads/\", &head);\n+\tprintf(\"[%s%s \", head, initial_commit ? _(\" (root-commit)\") : \"\");\n \n \tif (!log_tree_commit(&rev, commit)) {\n \t\trev.always_show_header = 1;\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex aa72596..6f4147a 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -19,6 +19,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)\n \tchar buffer[HEADERSIZE];\n \tstruct ustar_header *header = (struct ustar_header *)buffer;\n \tchar *content = buffer + RECORDSIZE;\n+\tconst char *comment;\n \tssize_t n;\n \n \tif (argc != 1)\n@@ -29,10 +30,10 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)\n \t\tdie(\"git get-tar-commit-id: read error\");\n \tif (header->typeflag[0] != 'g')\n \t\treturn 1;\n-\tif (memcmp(content, \"52 comment=\", 11))\n+\tif (!skip_prefix(content, \"52 comment=\", &comment))\n \t\treturn 1;\n \n-\tn = write_in_full(1, content + 11, 41);\n+\tn = write_in_full(1, comment, 41);\n \tif (n < 41)\n \t\tdie_errno(\"git get-tar-commit-id: write error\");\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex 2fb34c7..1202eba 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -368,6 +368,8 @@ static int cmd_log_walk(struct rev_info *rev)\n \n static int git_log_config(const char *var, const char *value, void *cb)\n {\n+\tconst char *slot_name;\n+\n \tif (!strcmp(var, \"format.pretty\"))\n \t\treturn git_config_string(&fmt_pretty, var, value);\n \tif (!strcmp(var, \"format.subjectprefix\"))\n@@ -388,8 +390,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\tdefault_show_root = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n-\tif (starts_with(var, \"color.decorate.\"))\n-\t\treturn parse_decorate_color_config(var, 15, value);\n+\tif (skip_prefix(var, \"color.decorate.\", &slot_name))\n+\t\treturn parse_decorate_color_config(var, slot_name - var, value);\n \tif (!strcmp(var, \"log.mailmap\")) {\n \t\tuse_mailmap_config = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/builtin/remote-ext.c b/builtin/remote-ext.c\nindex d699d28..3b8c22c 100644\n--- a/builtin/remote-ext.c\n+++ b/builtin/remote-ext.c\n@@ -30,16 +30,14 @@ static char *strip_escapes(const char *str, const char *service,\n \tsize_t rpos = 0;\n \tint escape = 0;\n \tchar special = 0;\n-\tsize_t psoff = 0;\n+\tconst char *service_noprefix = service;\n \tstruct strbuf ret = STRBUF_INIT;\n \n-\t/* Calculate prefix length for \\s and lengths for \\s and \\S */\n-\tif (!strncmp(service, \"git-\", 4))\n-\t\tpsoff = 4;\n+\tskip_prefix(service_noprefix, \"git-\", &service_noprefix);\n \n \t/* Pass the service to command. */\n \tsetenv(\"GIT_EXT_SERVICE\", service, 1);\n-\tsetenv(\"GIT_EXT_SERVICE_NOPREFIX\", service + psoff, 1);\n+\tsetenv(\"GIT_EXT_SERVICE_NOPREFIX\", service_noprefix, 1);\n \n \t/* Scan the length of argument. */\n \twhile (str[rpos] && (escape || str[rpos] != ' ')) {\n@@ -85,7 +83,7 @@ static char *strip_escapes(const char *str, const char *service,\n \t\t\t\tstrbuf_addch(&ret, str[rpos]);\n \t\t\t\tbreak;\n \t\t\tcase 's':\n-\t\t\t\tstrbuf_addstr(&ret, service + psoff);\n+\t\t\t\tstrbuf_addstr(&ret, service_noprefix);\n \t\t\t\tbreak;\n \t\t\tcase 'S':\n \t\t\t\tstrbuf_addstr(&ret, service);\ndiff --git a/pretty.c b/pretty.c\nindex 5fd9de6..a181ac6 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -73,10 +73,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c\n \tif (git_config_string(&fmt, var, value))\n \t\treturn -1;\n \n-\tif (starts_with(fmt, \"format:\") || starts_with(fmt, \"tformat:\")) {\n-\t\tcommit_format->is_tformat = fmt[0] == 't';\n-\t\tfmt = strchr(fmt, ':') + 1;\n-\t} else if (strchr(fmt, '%'))\n+\tif (skip_prefix(fmt, \"format:\", &fmt))\n+\t\tcommit_format->is_tformat = 0;\n+\telse if (skip_prefix(fmt, \"tformat:\", &fmt) || strchr(fmt, '%'))\n \t\tcommit_format->is_tformat = 1;\n \telse\n \t\tcommit_format->is_alias = 1;\n@@ -157,12 +156,12 @@ void get_commit_format(const char *arg, struct rev_info *rev)\n \t\trev->commit_format = CMIT_FMT_DEFAULT;\n \t\treturn;\n \t}\n-\tif (starts_with(arg, \"format:\") || starts_with(arg, \"tformat:\")) {\n-\t\tsave_user_format(rev, strchr(arg, ':') + 1, arg[0] == 't');\n+\tif (skip_prefix(arg, \"format:\", &arg)) {\n+\t\tsave_user_format(rev, arg, 0);\n \t\treturn;\n \t}\n \n-\tif (!*arg || strchr(arg, '%')) {\n+\tif (!*arg || skip_prefix(arg, \"tformat:\", &arg) || strchr(arg, '%')) {\n \t\tsave_user_format(rev, arg, 1);\n \t\treturn;\n \t}\n@@ -809,18 +808,19 @@ static void parse_commit_header(struct format_commit_context *context)\n \tint i;\n \n \tfor (i = 0; msg[i]; i++) {\n+\t\tconst char *name;\n \t\tint eol;\n \t\tfor (eol = i; msg[eol] && msg[eol] != '\\n'; eol++)\n \t\t\t; /* do nothing */\n \n \t\tif (i == eol) {\n \t\t\tbreak;\n-\t\t} else if (starts_with(msg + i, \"author \")) {\n-\t\t\tcontext->author.off = i + 7;\n-\t\t\tcontext->author.len = eol - i - 7;\n-\t\t} else if (starts_with(msg + i, \"committer \")) {\n-\t\t\tcontext->committer.off = i + 10;\n-\t\t\tcontext->committer.len = eol - i - 10;\n+\t\t} else if (skip_prefix(msg + i, \"author \", &name)) {\n+\t\t\tcontext->author.off = name - msg;\n+\t\t\tcontext->author.len = msg + eol - name;\n+\t\t} else if (skip_prefix(msg + i, \"committer \", &name)) {\n+\t\t\tcontext->committer.off = name - msg;\n+\t\t\tcontext->committer.len = msg + eol - name;\n \t\t}\n \t\ti = eol;\n \t}\n@@ -951,6 +951,8 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n \t\t\t  const char *placeholder,\n \t\t\t  struct format_commit_context *c)\n {\n+\tconst char *rest = placeholder;\n+\n \tif (placeholder[1] == '(') {\n \t\tconst char *begin = placeholder + 2;\n \t\tconst char *end = strchr(begin, ')');\n@@ -958,10 +960,9 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n \n \t\tif (!end)\n \t\t\treturn 0;\n-\t\tif (starts_with(begin, \"auto,\")) {\n+\t\tif (skip_prefix(begin, \"auto,\", &begin)) {\n \t\t\tif (!want_color(c->pretty_ctx->color))\n \t\t\t\treturn end - placeholder + 1;\n-\t\t\tbegin += 5;\n \t\t}\n \t\tcolor_parse_mem(begin,\n \t\t\t\tend - begin,\n@@ -969,20 +970,15 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n \t\tstrbuf_addstr(sb, color);\n \t\treturn end - placeholder + 1;\n \t}\n-\tif (starts_with(placeholder + 1, \"red\")) {\n+\tif (skip_prefix(placeholder + 1, \"red\", &rest))\n \t\tstrbuf_addstr(sb, GIT_COLOR_RED);\n-\t\treturn 4;\n-\t} else if (starts_with(placeholder + 1, \"green\")) {\n+\telse if (skip_prefix(placeholder + 1, \"green\", &rest))\n \t\tstrbuf_addstr(sb, GIT_COLOR_GREEN);\n-\t\treturn 6;\n-\t} else if (starts_with(placeholder + 1, \"blue\")) {\n+\telse if (skip_prefix(placeholder + 1, \"blue\", &rest))\n \t\tstrbuf_addstr(sb, GIT_COLOR_BLUE);\n-\t\treturn 5;\n-\t} else if (starts_with(placeholder + 1, \"reset\")) {\n+\telse if (skip_prefix(placeholder + 1, \"reset\", &rest))\n \t\tstrbuf_addstr(sb, GIT_COLOR_RESET);\n-\t\treturn 6;\n-\t} else\n-\t\treturn 0;\n+\treturn rest - placeholder;\n }\n \n static size_t parse_padding_placeholder(struct strbuf *sb,\n@@ -1522,7 +1518,7 @@ static void pp_header(struct pretty_print_context *pp,\n \tint parents_shown = 0;\n \n \tfor (;;) {\n-\t\tconst char *line = *msg_p;\n+\t\tconst char *name, *line = *msg_p;\n \t\tint linelen = get_one_line(*msg_p);\n \n \t\tif (!linelen)\n@@ -1557,14 +1553,14 @@ static void pp_header(struct pretty_print_context *pp,\n \t\t * FULL shows both authors but not dates.\n \t\t * FULLER shows both authors and dates.\n \t\t */\n-\t\tif (starts_with(line, \"author \")) {\n+\t\tif (skip_prefix(line, \"author \", &name)) {\n \t\t\tstrbuf_grow(sb, linelen + 80);\n-\t\t\tpp_user_info(pp, \"Author\", sb, line + 7, encoding);\n+\t\t\tpp_user_info(pp, \"Author\", sb, name, encoding);\n \t\t}\n-\t\tif (starts_with(line, \"committer \") &&\n+\t\tif (skip_prefix(line, \"committer \", &name) &&\n \t\t    (pp->fmt == CMIT_FMT_FULL || pp->fmt == CMIT_FMT_FULLER)) {\n \t\t\tstrbuf_grow(sb, linelen + 80);\n-\t\t\tpp_user_info(pp, \"Commit\", sb, line + 10, encoding);\n+\t\t\tpp_user_info(pp, \"Commit\", sb, name, encoding);\n \t\t}\n \t}\n }\n-- \n2.1.2\n"},{"id":"250261","messageId":"20141005224919.GA19998@google.com","threadId":"37673","inReplyTo":"5430427A.5080800@web.de","subject":"Re: [PATCH] use skip_prefix() to avoid more magic numbers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-10-05T22:49:19Z","receivedAt":"2014-10-05T22:49:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"René Scharfe wrote:\n\n> Continue where ae021d87 (use skip_prefix to avoid magic numbers) left off\n> and use skip_prefix() in more places for determining the lengths of prefix\n> strings to avoid using dependent constants and other indirect methods.\n\nSounds promising.\n\n[...]\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int ofs)\n>  \n>  static int git_branch_config(const char *var, const char *value, void *cb)\n>  {\n> +\tconst char *slot_name;\n> +\n>  \tif (starts_with(var, \"column.\"))\n>  \t\treturn git_column_config(var, value, \"branch\", &colopts);\n>  \tif (!strcmp(var, \"color.branch\")) {\n>  \t\tbranch_use_color = git_config_colorbool(var, value);\n>  \t\treturn 0;\n>  \t}\n> -\tif (starts_with(var, \"color.branch.\")) {\n> -\t\tint slot = parse_branch_color_slot(var, 13);\n> +\tif (skip_prefix(var, \"color.branch.\", &slot_name)) {\n> +\t\tint slot = parse_branch_color_slot(var, slot_name - var);\n\nI wonder why the separate var and offset parameters exist in the first\nplace.  Wouldn't taking a single const char * so the old code could use\n'var + 13' instead of 'var, 13' have worked?\n\nAt first glance I thought this should be\n\n\t\tint slot = parse_branch_color_slot(slot_name, 0);\n\nto be simpler.  At second glance, how about something like the following\non top?\n\n[...]\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n[...]\n> @@ -388,8 +390,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n>  \t\tdefault_show_root = git_config_bool(var, value);\n>  \t\treturn 0;\n>  \t}\n> -\tif (starts_with(var, \"color.decorate.\"))\n> -\t\treturn parse_decorate_color_config(var, 15, value);\n> +\tif (skip_prefix(var, \"color.decorate.\", &slot_name))\n> +\t\treturn parse_decorate_color_config(var, slot_name - var, value);\n\nLikewise: the offset-based API seems odd now here, too.\n\n[...]\n> --- a/pretty.c\n> +++ b/pretty.c\n[...]\n> @@ -809,18 +808,19 @@ static void parse_commit_header(struct format_commit_context *context)\n>  \tint i;\n>  \n>  \tfor (i = 0; msg[i]; i++) {\n> +\t\tconst char *name;\n\nident instead of name, maybe? (since it's a 'name <email> timestamp'\nfield)\n\n[...]\n> @@ -1522,7 +1518,7 @@ static void pp_header(struct pretty_print_context *pp,\n>  \tint parents_shown = 0;\n>  \n>  \tfor (;;) {\n> -\t\tconst char *line = *msg_p;\n> +\t\tconst char *name, *line = *msg_p;\n\nLikewise.\n\nWith or without the changes below,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\ndiff --git i/builtin/branch.c w/builtin/branch.c\nindex 6785097..022a88e 100644\n--- i/builtin/branch.c\n+++ w/builtin/branch.c\n@@ -62,19 +62,19 @@ static unsigned char merge_filter_ref[20];\n static struct string_list output = STRING_LIST_INIT_DUP;\n static unsigned int colopts;\n \n-static int parse_branch_color_slot(const char *var, int ofs)\n+static int parse_branch_color_slot(const char *slot)\n {\n-\tif (!strcasecmp(var+ofs, \"plain\"))\n+\tif (!strcasecmp(slot, \"plain\"))\n \t\treturn BRANCH_COLOR_PLAIN;\n-\tif (!strcasecmp(var+ofs, \"reset\"))\n+\tif (!strcasecmp(slot, \"reset\"))\n \t\treturn BRANCH_COLOR_RESET;\n-\tif (!strcasecmp(var+ofs, \"remote\"))\n+\tif (!strcasecmp(slot, \"remote\"))\n \t\treturn BRANCH_COLOR_REMOTE;\n-\tif (!strcasecmp(var+ofs, \"local\"))\n+\tif (!strcasecmp(slot, \"local\"))\n \t\treturn BRANCH_COLOR_LOCAL;\n-\tif (!strcasecmp(var+ofs, \"current\"))\n+\tif (!strcasecmp(slot, \"current\"))\n \t\treturn BRANCH_COLOR_CURRENT;\n-\tif (!strcasecmp(var+ofs, \"upstream\"))\n+\tif (!strcasecmp(slot, \"upstream\"))\n \t\treturn BRANCH_COLOR_UPSTREAM;\n \treturn -1;\n }\n@@ -90,7 +90,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \tif (skip_prefix(var, \"color.branch.\", &slot_name)) {\n-\t\tint slot = parse_branch_color_slot(var, slot_name - var);\n+\t\tint slot = parse_branch_color_slot(slot_name);\n \t\tif (slot < 0)\n \t\t\treturn 0;\n \t\tif (!value)\ndiff --git i/builtin/log.c w/builtin/log.c\nindex 1202eba..68d5d30 100644\n--- i/builtin/log.c\n+++ w/builtin/log.c\n@@ -391,7 +391,7 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \tif (skip_prefix(var, \"color.decorate.\", &slot_name))\n-\t\treturn parse_decorate_color_config(var, slot_name - var, value);\n+\t\treturn parse_decorate_color_config(var, slot_name, value);\n \tif (!strcmp(var, \"log.mailmap\")) {\n \t\tuse_mailmap_config = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git i/log-tree.c w/log-tree.c\nindex cff7ac1..6402c19 100644\n--- i/log-tree.c\n+++ w/log-tree.c\n@@ -56,9 +56,9 @@ static int parse_decorate_color_slot(const char *slot)\n \treturn -1;\n }\n \n-int parse_decorate_color_config(const char *var, const int ofs, const char *value)\n+int parse_decorate_color_config(const char *var, const char *slot_name, const char *value)\n {\n-\tint slot = parse_decorate_color_slot(var + ofs);\n+\tint slot = parse_decorate_color_slot(slot_name);\n \tif (slot < 0)\n \t\treturn 0;\n \tif (!value)\ndiff --git i/log-tree.h w/log-tree.h\nindex b26160c..d637b04 100644\n--- i/log-tree.h\n+++ w/log-tree.h\n@@ -7,7 +7,8 @@ struct log_info {\n \tstruct commit *commit, *parent;\n };\n \n-int parse_decorate_color_config(const char *var, const int ofs, const char *value);\n+int parse_decorate_color_config(const char *var, const char *slot_name,\n+\t\t\t\tconst char *value);\n void init_log_tree_opt(struct rev_info *);\n int log_tree_diff_flush(struct rev_info *);\n int log_tree_commit(struct rev_info *, struct commit *);\ndiff --git i/pretty.c w/pretty.c\nindex a181ac6..e2c59ef 100644\n--- i/pretty.c\n+++ w/pretty.c\n@@ -808,19 +808,19 @@ static void parse_commit_header(struct format_commit_context *context)\n \tint i;\n \n \tfor (i = 0; msg[i]; i++) {\n-\t\tconst char *name;\n+\t\tconst char *ident;\n \t\tint eol;\n \t\tfor (eol = i; msg[eol] && msg[eol] != '\\n'; eol++)\n \t\t\t; /* do nothing */\n \n \t\tif (i == eol) {\n \t\t\tbreak;\n-\t\t} else if (skip_prefix(msg + i, \"author \", &name)) {\n-\t\t\tcontext->author.off = name - msg;\n-\t\t\tcontext->author.len = msg + eol - name;\n-\t\t} else if (skip_prefix(msg + i, \"committer \", &name)) {\n-\t\t\tcontext->committer.off = name - msg;\n-\t\t\tcontext->committer.len = msg + eol - name;\n+\t\t} else if (skip_prefix(msg + i, \"author \", &ident)) {\n+\t\t\tcontext->author.off = ident - msg;\n+\t\t\tcontext->author.len = msg + eol - ident;\n+\t\t} else if (skip_prefix(msg + i, \"committer \", &ident)) {\n+\t\t\tcontext->committer.off = ident - msg;\n+\t\t\tcontext->committer.len = msg + eol - ident;\n \t\t}\n \t\ti = eol;\n \t}\n@@ -1518,7 +1518,7 @@ static void pp_header(struct pretty_print_context *pp,\n \tint parents_shown = 0;\n \n \tfor (;;) {\n-\t\tconst char *name, *line = *msg_p;\n+\t\tconst char *ident, *line = *msg_p;\n \t\tint linelen = get_one_line(*msg_p);\n \n \t\tif (!linelen)\n@@ -1553,14 +1553,14 @@ static void pp_header(struct pretty_print_context *pp,\n \t\t * FULL shows both authors but not dates.\n \t\t * FULLER shows both authors and dates.\n \t\t */\n-\t\tif (skip_prefix(line, \"author \", &name)) {\n+\t\tif (skip_prefix(line, \"author \", &ident)) {\n \t\t\tstrbuf_grow(sb, linelen + 80);\n-\t\t\tpp_user_info(pp, \"Author\", sb, name, encoding);\n+\t\t\tpp_user_info(pp, \"Author\", sb, ident, encoding);\n \t\t}\n-\t\tif (skip_prefix(line, \"committer \", &name) &&\n+\t\tif (skip_prefix(line, \"committer \", &ident) &&\n \t\t    (pp->fmt == CMIT_FMT_FULL || pp->fmt == CMIT_FMT_FULLER)) {\n \t\t\tstrbuf_grow(sb, linelen + 80);\n-\t\t\tpp_user_info(pp, \"Commit\", sb, name, encoding);\n+\t\t\tpp_user_info(pp, \"Commit\", sb, ident, encoding);\n \t\t}\n \t}\n }\n"},{"id":"250262","messageId":"20141006011827.GA11027@peff.net","threadId":"37673","inReplyTo":"20141005224919.GA19998@google.com","subject":"Re: [PATCH] use skip_prefix() to avoid more magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-06T01:18:28Z","receivedAt":"2014-10-06T01:18:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 05, 2014 at 03:49:19PM -0700, Jonathan Nieder wrote:\n\n> > --- a/builtin/branch.c\n> > +++ b/builtin/branch.c\n> > @@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int ofs)\n> >  \n> >  static int git_branch_config(const char *var, const char *value, void *cb)\n> >  {\n> > +\tconst char *slot_name;\n> > +\n> >  \tif (starts_with(var, \"column.\"))\n> >  \t\treturn git_column_config(var, value, \"branch\", &colopts);\n> >  \tif (!strcmp(var, \"color.branch\")) {\n> >  \t\tbranch_use_color = git_config_colorbool(var, value);\n> >  \t\treturn 0;\n> >  \t}\n> > -\tif (starts_with(var, \"color.branch.\")) {\n> > -\t\tint slot = parse_branch_color_slot(var, 13);\n> > +\tif (skip_prefix(var, \"color.branch.\", &slot_name)) {\n> > +\t\tint slot = parse_branch_color_slot(var, slot_name - var);\n> \n> I wonder why the separate var and offset parameters exist in the first\n> place.  Wouldn't taking a single const char * so the old code could use\n> 'var + 13' instead of 'var, 13' have worked?\n\nI think this is in the same boat as parse_diff_color_slot, which I fixed\nin 9e1a5eb (parse_diff_color_slot: drop ofs parameter, 2014-06-18). The\nshort of it is that the function used to want both the full name and the\nslot name, but now needs only the latter.\n\nThe fix you proposed below is along the same line, and looks good to me\n(and grepping for 'var *+ *ofs' shows only the two sites you found, so\nhopefully that is the last of it).\n\n> > @@ -809,18 +808,19 @@ static void parse_commit_header(struct format_commit_context *context)\n> >  \tint i;\n> >  \n> >  \tfor (i = 0; msg[i]; i++) {\n> > +\t\tconst char *name;\n> \n> ident instead of name, maybe? (since it's a 'name <email> timestamp'\n> field)\n\nYeah, agreed.\n\n-Peff\n"},{"id":"250263","messageId":"20141006013459.GC11027@peff.net","threadId":"37673","inReplyTo":"5430427A.5080800@web.de","subject":"Re: [PATCH] use skip_prefix() to avoid more magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-06T01:35:00Z","receivedAt":"2014-10-06T01:35:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 04, 2014 at 08:54:50PM +0200, René Scharfe wrote:\n\n> Continue where ae021d87 (use skip_prefix to avoid magic numbers) left off\n> and use skip_prefix() in more places for determining the lengths of prefix\n> strings to avoid using dependent constants and other indirect methods.\n\nThanks, these all look like nice improvements. I think both of the\nsuggestions made my Jonathan are good on top, though.\n\n-Peff\n"},{"id":"250291","messageId":"xmqqtx3fg31w.fsf@gitster.dls.corp.google.com","threadId":"37673","inReplyTo":"20141006011827.GA11027@peff.net","subject":"Re: [PATCH] use skip_prefix() to avoid more magic numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-07T18:14:35Z","receivedAt":"2014-10-07T18:14:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, Oct 05, 2014 at 03:49:19PM -0700, Jonathan Nieder wrote:\n>\n>> > --- a/builtin/branch.c\n>> > +++ b/builtin/branch.c\n>> > @@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int ofs)\n>> >  \n>> >  static int git_branch_config(const char *var, const char *value, void *cb)\n>> >  {\n>> > +\tconst char *slot_name;\n>> > +\n>> >  \tif (starts_with(var, \"column.\"))\n>> >  \t\treturn git_column_config(var, value, \"branch\", &colopts);\n>> >  \tif (!strcmp(var, \"color.branch\")) {\n>> >  \t\tbranch_use_color = git_config_colorbool(var, value);\n>> >  \t\treturn 0;\n>> >  \t}\n>> > -\tif (starts_with(var, \"color.branch.\")) {\n>> > -\t\tint slot = parse_branch_color_slot(var, 13);\n>> > +\tif (skip_prefix(var, \"color.branch.\", &slot_name)) {\n>> > +\t\tint slot = parse_branch_color_slot(var, slot_name - var);\n>> \n>> I wonder why the separate var and offset parameters exist in the first\n>> place.  Wouldn't taking a single const char * so the old code could use\n>> 'var + 13' instead of 'var, 13' have worked?\n>\n> I think this is in the same boat as parse_diff_color_slot, which I fixed\n> in 9e1a5eb (parse_diff_color_slot: drop ofs parameter, 2014-06-18). The\n> short of it is that the function used to want both the full name and the\n> slot name, but now needs only the latter.\n>\n> The fix you proposed below is along the same line, and looks good to me\n> (and grepping for 'var *+ *ofs' shows only the two sites you found, so\n> hopefully that is the last of it).\n\nSo perhaps we would want this change as a preliminary separate\npatch?\n\nThanks\n\n>\n>> > @@ -809,18 +808,19 @@ static void parse_commit_header(struct\n>> > format_commit_context *context)\n>> >  \tint i;\n>> >  \n>> >  \tfor (i = 0; msg[i]; i++) {\n>> > +\t\tconst char *name;\n>> \n>> ident instead of name, maybe? (since it's a 'name <email> timestamp'\n>> field)\n>\n> Yeah, agreed.\n>\n> -Peff\n"},{"id":"250297","messageId":"xmqqfvezg2pl.fsf@gitster.dls.corp.google.com","threadId":"37673","inReplyTo":"20141006011827.GA11027@peff.net","subject":"Re: [PATCH] use skip_prefix() to avoid more magic numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-07T18:21:58Z","receivedAt":"2014-10-07T18:21:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, Oct 05, 2014 at 03:49:19PM -0700, Jonathan Nieder wrote:\n>\n>> > --- a/builtin/branch.c\n>> > +++ b/builtin/branch.c\n>> > @@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int ofs)\n>> >  \n>> >  static int git_branch_config(const char *var, const char *value, void *cb)\n>> >  {\n>> > +\tconst char *slot_name;\n>> > +\n>> >  \tif (starts_with(var, \"column.\"))\n>> >  \t\treturn git_column_config(var, value, \"branch\", &colopts);\n>> >  \tif (!strcmp(var, \"color.branch\")) {\n>> >  \t\tbranch_use_color = git_config_colorbool(var, value);\n>> >  \t\treturn 0;\n>> >  \t}\n>> > -\tif (starts_with(var, \"color.branch.\")) {\n>> > -\t\tint slot = parse_branch_color_slot(var, 13);\n>> > +\tif (skip_prefix(var, \"color.branch.\", &slot_name)) {\n>> > +\t\tint slot = parse_branch_color_slot(var, slot_name - var);\n>> \n>> I wonder why the separate var and offset parameters exist in the first\n>> place.  Wouldn't taking a single const char * so the old code could use\n>> 'var + 13' instead of 'var, 13' have worked?\n>\n> I think this is in the same boat as parse_diff_color_slot, which I fixed\n> in 9e1a5eb (parse_diff_color_slot: drop ofs parameter, 2014-06-18). The\n> short of it is that the function used to want both the full name and the\n> slot name, but now needs only the latter.\n>\n> The fix you proposed below is along the same line, and looks good to me\n> (and grepping for 'var *+ *ofs' shows only the two sites you found, so\n> hopefully that is the last of it).\n\nbuiltin/commit.c::parse_status_slot() has the same construct.\n"},{"id":"250298","messageId":"xmqqd2a3g2mf.fsf@gitster.dls.corp.google.com","threadId":"37673","inReplyTo":"5430427A.5080800@web.de","subject":"Re: [PATCH] use skip_prefix() to avoid more magic numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-07T18:23:52Z","receivedAt":"2014-10-07T18:23:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> @@ -335,20 +337,18 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,\n>  \tstatic struct {\n>  \t\tint kind;\n>  \t\tconst char *prefix;\n> -\t\tint pfxlen;\n>  \t} ref_kind[] = {\n> -\t\t{ REF_LOCAL_BRANCH, \"refs/heads/\", 11 },\n> -\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/\", 13 },\n> +\t\t{ REF_LOCAL_BRANCH, \"refs/heads/\" },\n> +\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/\" },\n>  \t};\n>  \n>  \t/* Detect kind */\n>  \tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n>  \t\tprefix = ref_kind[i].prefix;\n> -\t\tif (strncmp(refname, prefix, ref_kind[i].pfxlen))\n> -\t\t\tcontinue;\n> -\t\tkind = ref_kind[i].kind;\n> -\t\trefname += ref_kind[i].pfxlen;\n> -\t\tbreak;\n> +\t\tif (skip_prefix(refname, prefix, &refname)) {\n> +\t\t\tkind = ref_kind[i].kind;\n> +\t\t\tbreak;\n> +\t\t}\n\nThis certainly makes it easier to read.\n\nI suspect that the original was done as a (possibly premature)\noptimization to avoid having to do strlen(3) on a variable that\npoints at constant strings for each and every ref we iterate with\nfor_each_rawref(), and it is somewhat sad to see it lost because\nskip_prefix() assumes that the caller never knows the length of the\nprefix, though.\n\nThanks.\n"},{"id":"250293","messageId":"20141007183136.GA32374@peff.net","threadId":"37673","inReplyTo":"xmqqfvezg2pl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] use skip_prefix() to avoid more magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-07T18:31:36Z","receivedAt":"2014-10-07T18:31:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 07, 2014 at 11:21:58AM -0700, Junio C Hamano wrote:\n\n> > The fix you proposed below is along the same line, and looks good to me\n> > (and grepping for 'var *+ *ofs' shows only the two sites you found, so\n> > hopefully that is the last of it).\n> \n> builtin/commit.c::parse_status_slot() has the same construct.\n\nThanks, I missed it because it uses \"offset\" instead of \"ofs\".\n\n-Peff\n"},{"id":"250303","messageId":"20141007191656.GB32374@peff.net","threadId":"37673","inReplyTo":"xmqqtx3fg31w.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] use skip_prefix() to avoid more magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-07T19:16:57Z","receivedAt":"2014-10-07T19:16:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 07, 2014 at 11:14:35AM -0700, Junio C Hamano wrote:\n\n> > The fix you proposed below is along the same line, and looks good to me\n> > (and grepping for 'var *+ *ofs' shows only the two sites you found, so\n> > hopefully that is the last of it).\n> \n> So perhaps we would want this change as a preliminary separate\n> patch?\n\nHere it is on top of master, with a commit message (and including the\nother case you found). I've attributed it to Jonathan, who did most of\nthe work. We'd need his signoff and approval, of course.\n\n-- >8 --\nFrom: Jonathan Nieder <jrnieder@gmail.com>\nSubject: pass config slots as pointers instead of offsets\n\nMany config-parsing helpers, like parse_branch_color_slot,\ntake the name of a config variable and an offset to the\n\"slot\" name (e.g., \"color.branch.plain\" is passed along with\n\"13\" to effectively pass \"plain\"). This is leftover from the\ntime that these functions would die() on error, and would\nwant the full variable name for error reporting.\n\nThese days they do not use the full variable name at all.\nPassing a single pointer to the slot name is more natural,\nand lets us more easily adjust the callers to use skip_prefix\nto avoid manually writing offset numbers.\n\nThis is effectively a continuation of 9e1a5eb, which did the\nsame for parse_diff_color_slot. This patch covers all of the\nremaining similar constructs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nActually, parse_decorate_color_config is slightly odd in that it takes\nthe variable and the slot_name after this patch. That's because it\npasses the full variable name to color_parse, which does still die() on\nerror. Perhaps it should be converted to return an error, too.\n\n builtin/branch.c | 16 ++++++++--------\n builtin/commit.c | 19 +++++++++----------\n builtin/log.c    |  2 +-\n log-tree.c       |  4 ++--\n log-tree.h       |  2 +-\n 5 files changed, 21 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 9e4666f..2c97141 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -62,19 +62,19 @@ static unsigned char merge_filter_ref[20];\n static struct string_list output = STRING_LIST_INIT_DUP;\n static unsigned int colopts;\n \n-static int parse_branch_color_slot(const char *var, int ofs)\n+static int parse_branch_color_slot(const char *slot)\n {\n-\tif (!strcasecmp(var+ofs, \"plain\"))\n+\tif (!strcasecmp(slot, \"plain\"))\n \t\treturn BRANCH_COLOR_PLAIN;\n-\tif (!strcasecmp(var+ofs, \"reset\"))\n+\tif (!strcasecmp(slot, \"reset\"))\n \t\treturn BRANCH_COLOR_RESET;\n-\tif (!strcasecmp(var+ofs, \"remote\"))\n+\tif (!strcasecmp(slot, \"remote\"))\n \t\treturn BRANCH_COLOR_REMOTE;\n-\tif (!strcasecmp(var+ofs, \"local\"))\n+\tif (!strcasecmp(slot, \"local\"))\n \t\treturn BRANCH_COLOR_LOCAL;\n-\tif (!strcasecmp(var+ofs, \"current\"))\n+\tif (!strcasecmp(slot, \"current\"))\n \t\treturn BRANCH_COLOR_CURRENT;\n-\tif (!strcasecmp(var+ofs, \"upstream\"))\n+\tif (!strcasecmp(slot, \"upstream\"))\n \t\treturn BRANCH_COLOR_UPSTREAM;\n \treturn -1;\n }\n@@ -88,7 +88,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \tif (starts_with(var, \"color.branch.\")) {\n-\t\tint slot = parse_branch_color_slot(var, 13);\n+\t\tint slot = parse_branch_color_slot(var + 13);\n \t\tif (slot < 0)\n \t\t\treturn 0;\n \t\tif (!value)\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b0fe784..c45613a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1271,22 +1271,21 @@ static int dry_run_commit(int argc, const char **argv, const char *prefix,\n \treturn commitable ? 0 : 1;\n }\n \n-static int parse_status_slot(const char *var, int offset)\n+static int parse_status_slot(const char *slot)\n {\n-\tif (!strcasecmp(var+offset, \"header\"))\n+\tif (!strcasecmp(slot, \"header\"))\n \t\treturn WT_STATUS_HEADER;\n-\tif (!strcasecmp(var+offset, \"branch\"))\n+\tif (!strcasecmp(slot, \"branch\"))\n \t\treturn WT_STATUS_ONBRANCH;\n-\tif (!strcasecmp(var+offset, \"updated\")\n-\t\t|| !strcasecmp(var+offset, \"added\"))\n+\tif (!strcasecmp(slot, \"updated\") || !strcasecmp(slot, \"added\"))\n \t\treturn WT_STATUS_UPDATED;\n-\tif (!strcasecmp(var+offset, \"changed\"))\n+\tif (!strcasecmp(slot, \"changed\"))\n \t\treturn WT_STATUS_CHANGED;\n-\tif (!strcasecmp(var+offset, \"untracked\"))\n+\tif (!strcasecmp(slot, \"untracked\"))\n \t\treturn WT_STATUS_UNTRACKED;\n-\tif (!strcasecmp(var+offset, \"nobranch\"))\n+\tif (!strcasecmp(slot, \"nobranch\"))\n \t\treturn WT_STATUS_NOBRANCH;\n-\tif (!strcasecmp(var+offset, \"unmerged\"))\n+\tif (!strcasecmp(slot, \"unmerged\"))\n \t\treturn WT_STATUS_UNMERGED;\n \treturn -1;\n }\n@@ -1324,7 +1323,7 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t\treturn 0;\n \t}\n \tif (starts_with(k, \"status.color.\") || starts_with(k, \"color.status.\")) {\n-\t\tint slot = parse_status_slot(k, 13);\n+\t\tint slot = parse_status_slot(k + 13);\n \t\tif (slot < 0)\n \t\t\treturn 0;\n \t\tif (!v)\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 2fb34c7..9c75447 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -389,7 +389,7 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \tif (starts_with(var, \"color.decorate.\"))\n-\t\treturn parse_decorate_color_config(var, 15, value);\n+\t\treturn parse_decorate_color_config(var, var + 15, value);\n \tif (!strcmp(var, \"log.mailmap\")) {\n \t\tuse_mailmap_config = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/log-tree.c b/log-tree.c\nindex bcee7c5..877d976 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -56,9 +56,9 @@ static int parse_decorate_color_slot(const char *slot)\n \treturn -1;\n }\n \n-int parse_decorate_color_config(const char *var, const int ofs, const char *value)\n+int parse_decorate_color_config(const char *var, const char *slot_name, const char *value)\n {\n-\tint slot = parse_decorate_color_slot(var + ofs);\n+\tint slot = parse_decorate_color_slot(slot_name);\n \tif (slot < 0)\n \t\treturn 0;\n \tif (!value)\ndiff --git a/log-tree.h b/log-tree.h\nindex d6ecd4d..8cbefac 100644\n--- a/log-tree.h\n+++ b/log-tree.h\n@@ -7,7 +7,7 @@ struct log_info {\n \tstruct commit *commit, *parent;\n };\n \n-int parse_decorate_color_config(const char *var, const int ofs, const char *value);\n+int parse_decorate_color_config(const char *var, const char *slot_name, const char *value);\n void init_log_tree_opt(struct rev_info *);\n int log_tree_diff_flush(struct rev_info *);\n int log_tree_commit(struct rev_info *, struct commit *);\n-- \n2.1.1.566.gdb1f904\n"},{"id":"250305","messageId":"20141007193309.GA22179@peff.net","threadId":"37673","inReplyTo":"20141007191656.GB32374@peff.net","subject":"Re: [PATCH] use skip_prefix() to avoid more magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-07T19:33:09Z","receivedAt":"2014-10-07T19:33:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 07, 2014 at 03:16:56PM -0400, Jeff King wrote:\n\n> Actually, parse_decorate_color_config is slightly odd in that it takes\n> the variable and the slot_name after this patch. That's because it\n> passes the full variable name to color_parse, which does still die() on\n> error. Perhaps it should be converted to return an error, too.\n\nActually, this doesn't let us get rid of the \"var\" in\nparse_decorate_color_config, because it also wants to call\nconfig_error_nonbool. However, while looking at this, I did notice that\nsome of the error messages generated by color_parse are a bit ugly, and\ncame up with the patch below (on top of what I just sent, but it could\nbe separate).\n\n-- >8 --\nSubject: color_parse: do not mention variable name in error message\n\nOriginally the color-parsing function was used only for\nconfig variables. It made sense to pass the variable name so\nthat the die() message could be something like:\n\n  $ git -c color.branch.plain=bogus branch\n  fatal: bad color value 'bogus' for variable 'color.branch.plain'\n\nThese days we call it in other contexts, and the resulting\nerror messages are a little confusing:\n\n  $ git log --pretty='%C(bogus)'\n  fatal: bad color value 'bogus' for variable '--pretty format'\n\n  $ git config --get-color foo.bar bogus\n  fatal: bad color value 'bogus' for variable 'command line'\n\nThis patch teaches color_parse to complain only about the\nvalue, and then return an error code. Config callers can\nthen propagate that up to the config parser, which mentions\nthe variable name. Other callers can provide a custom\nmessage. After this patch these three cases now look like:\n\n  $ git -c color.branch.plain=bogus branch\n  error: invalid color value: bogus\n  fatal: unable to parse 'color.branch.plain' from command-line config\n\n  $ git log --pretty='%C(bogus)'\n  error: invalid color value: bogus\n  fatal: unable to parse --pretty format\n\n  $ git config --get-color foo.bar bogus\n  error: invalid color value: bogus\n  fatal: unable to parse default color value\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think the two-line errors are kind of ugly. It does match how\nconfig_error_nonbool works, which also propagates its errors, and looks\nlike:\n\n  $ git -c color.branch.plain branch\n  error: Missing value for 'color.branch.plain'\n  fatal: unable to parse 'color.branch.plain' from command-line config\n\nWe could instead make color_parse silently return -1, and leave it up to\nthe caller to complain (and probably add die_bad_color_config() or\nsimilar to avoid repetition in the config callers).\n\n builtin/branch.c       |  3 +--\n builtin/clean.c        |  3 +--\n builtin/commit.c       |  3 +--\n builtin/config.c       |  9 ++++++---\n builtin/for-each-ref.c |  6 ++++--\n color.c                | 13 ++++++-------\n color.h                |  4 ++--\n diff.c                 |  3 +--\n grep.c                 |  2 +-\n log-tree.c             |  3 +--\n pretty.c               |  5 ++---\n 11 files changed, 26 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 2c97141..35c786d 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -93,8 +93,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)\n \t\t\treturn 0;\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tcolor_parse(value, var, branch_colors[slot]);\n-\t\treturn 0;\n+\t\treturn color_parse(value, branch_colors[slot]);\n \t}\n \treturn git_color_default_config(var, value, cb);\n }\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 3beeea6..a7e7b0b 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -116,8 +116,7 @@ static int git_clean_config(const char *var, const char *value, void *cb)\n \t\t\treturn 0;\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tcolor_parse(value, var, clean_colors[slot]);\n-\t\treturn 0;\n+\t\treturn color_parse(value, clean_colors[slot]);\n \t}\n \n \tif (!strcmp(var, \"clean.requireforce\")) {\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex c45613a..407566d 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1328,8 +1328,7 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t\t\treturn 0;\n \t\tif (!v)\n \t\t\treturn config_error_nonbool(k);\n-\t\tcolor_parse(v, k, s->color_palette[slot]);\n-\t\treturn 0;\n+\t\treturn color_parse(v, s->color_palette[slot]);\n \t}\n \tif (!strcmp(k, \"status.relativepaths\")) {\n \t\ts->relative_paths = git_config_bool(k, v);\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 37305e9..8cc2604 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -296,7 +296,8 @@ static int git_get_color_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, get_color_slot)) {\n \t\tif (!value)\n \t\t\tconfig_error_nonbool(var);\n-\t\tcolor_parse(value, var, parsed_color);\n+\t\tif (color_parse(value, parsed_color) < 0)\n+\t\t\treturn -1;\n \t\tget_color_found = 1;\n \t}\n \treturn 0;\n@@ -309,8 +310,10 @@ static void get_color(const char *def_color)\n \tgit_config_with_options(git_get_color_config, NULL,\n \t\t\t\t&given_config_source, respect_includes);\n \n-\tif (!get_color_found && def_color)\n-\t\tcolor_parse(def_color, \"command line\", parsed_color);\n+\tif (!get_color_found && def_color) {\n+\t\tif (color_parse(def_color, parsed_color) < 0)\n+\t\t\tdie(_(\"unable to parse default color value\"));\n+\t}\n \n \tfputs(parsed_color, stdout);\n }\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex fda0f04..7ee86b3 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -671,7 +671,8 @@ static void populate_value(struct refinfo *ref)\n \t\t} else if (starts_with(name, \"color:\")) {\n \t\t\tchar color[COLOR_MAXLEN] = \"\";\n \n-\t\t\tcolor_parse(name + 6, \"--format\", color);\n+\t\t\tif (color_parse(name + 6, color) < 0)\n+\t\t\t\tdie(_(\"unable to parse format\"));\n \t\t\tv->s = xstrdup(color);\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"flag\")) {\n@@ -1004,7 +1005,8 @@ static void show_ref(struct refinfo *info, const char *format, int quote_style)\n \t\tstruct atom_value resetv;\n \t\tchar color[COLOR_MAXLEN] = \"\";\n \n-\t\tcolor_parse(\"reset\", \"--format\", color);\n+\t\tif (color_parse(\"reset\", color) < 0)\n+\t\t\tdie(\"BUG: couldn't parse 'reset' as a color\");\n \t\tresetv.s = color;\n \t\tprint_value(&resetv, quote_style);\n \t}\ndiff --git a/color.c b/color.c\nindex f672885..7941e93 100644\n--- a/color.c\n+++ b/color.c\n@@ -60,13 +60,12 @@ static int parse_attr(const char *name, int len)\n \treturn -1;\n }\n \n-void color_parse(const char *value, const char *var, char *dst)\n+int color_parse(const char *value, char *dst)\n {\n-\tcolor_parse_mem(value, strlen(value), var, dst);\n+\treturn color_parse_mem(value, strlen(value), dst);\n }\n \n-void color_parse_mem(const char *value, int value_len, const char *var,\n-\t\tchar *dst)\n+int color_parse_mem(const char *value, int value_len, char *dst)\n {\n \tconst char *ptr = value;\n \tint len = value_len;\n@@ -76,7 +75,7 @@ void color_parse_mem(const char *value, int value_len, const char *var,\n \n \tif (!strncasecmp(value, \"reset\", len)) {\n \t\tstrcpy(dst, GIT_COLOR_RESET);\n-\t\treturn;\n+\t\treturn 0;\n \t}\n \n \t/* [fg [bg]] [attr]... */\n@@ -153,9 +152,9 @@ void color_parse_mem(const char *value, int value_len, const char *var,\n \t\t*dst++ = 'm';\n \t}\n \t*dst = 0;\n-\treturn;\n+\treturn 0;\n bad:\n-\tdie(\"bad color value '%.*s' for variable '%s'\", value_len, value, var);\n+\treturn error(_(\"invalid color value: %.*s\"), value_len, value);\n }\n \n int git_config_colorbool(const char *var, const char *value)\ndiff --git a/color.h b/color.h\nindex 9a8495b..f5beab1 100644\n--- a/color.h\n+++ b/color.h\n@@ -77,8 +77,8 @@ int git_color_default_config(const char *var, const char *value, void *cb);\n \n int git_config_colorbool(const char *var, const char *value);\n int want_color(int var);\n-void color_parse(const char *value, const char *var, char *dst);\n-void color_parse_mem(const char *value, int len, const char *var, char *dst);\n+int color_parse(const char *value, char *dst);\n+int color_parse_mem(const char *value, int len, char *dst);\n __attribute__((format (printf, 3, 4)))\n int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);\n __attribute__((format (printf, 3, 4)))\ndiff --git a/diff.c b/diff.c\nindex d7a5c81..d1bd534 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -248,8 +248,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\t\treturn 0;\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tcolor_parse(value, var, diff_colors[slot]);\n-\t\treturn 0;\n+\t\treturn color_parse(value, diff_colors[slot]);\n \t}\n \n \t/* like GNU diff's --suppress-blank-empty option  */\ndiff --git a/grep.c b/grep.c\nindex 99217dc..4dc31ea 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -111,7 +111,7 @@ int grep_config(const char *var, const char *value, void *cb)\n \tif (color) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tcolor_parse(value, var, color);\n+\t\treturn color_parse(value, color);\n \t}\n \treturn 0;\n }\ndiff --git a/log-tree.c b/log-tree.c\nindex 877d976..7844f33 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -63,8 +63,7 @@ int parse_decorate_color_config(const char *var, const char *slot_name, const ch\n \t\treturn 0;\n \tif (!value)\n \t\treturn config_error_nonbool(var);\n-\tcolor_parse(value, var, decoration_colors[slot]);\n-\treturn 0;\n+\treturn color_parse(value, decoration_colors[slot]);\n }\n \n /*\ndiff --git a/pretty.c b/pretty.c\nindex 31f2ede..59de1dc 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -963,9 +963,8 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\treturn end - placeholder + 1;\n \t\t\tbegin += 5;\n \t\t}\n-\t\tcolor_parse_mem(begin,\n-\t\t\t\tend - begin,\n-\t\t\t\t\"--pretty format\", color);\n+\t\tif (color_parse_mem(begin, end - begin, color) < 0)\n+\t\t\tdie(_(\"unable to parse --pretty format\"));\n \t\tstrbuf_addstr(sb, color);\n \t\treturn end - placeholder + 1;\n \t}\n-- \n2.1.1.566.gdb1f904\n"},{"id":"250307","messageId":"xmqqmw97ek6u.fsf@gitster.dls.corp.google.com","threadId":"37673","inReplyTo":"20141007193309.GA22179@peff.net","subject":"Re: [PATCH] use skip_prefix() to avoid more magic numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-07T19:47:21Z","receivedAt":"2014-10-07T19:47:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: color_parse: do not mention variable name in error message\n> ...\n> I think the two-line errors are kind of ugly. It does match how\n> config_error_nonbool works, which also propagates its errors, and looks\n> like:\n>\n>   $ git -c color.branch.plain branch\n>   error: Missing value for 'color.branch.plain'\n>   fatal: unable to parse 'color.branch.plain' from command-line config\n>\n> We could instead make color_parse silently return -1, and leave it up to\n> the caller to complain (and probably add die_bad_color_config() or\n> similar to avoid repetition in the config callers).\n\nYeah, in the longer term we would want to do something like that to\nfix both nonbool and this one, but for now this should help avoid\nconfusion.\n\nThanks for a nice analysis, write-up and patch.\n\n>\n>  builtin/branch.c       |  3 +--\n>  builtin/clean.c        |  3 +--\n>  builtin/commit.c       |  3 +--\n>  builtin/config.c       |  9 ++++++---\n>  builtin/for-each-ref.c |  6 ++++--\n>  color.c                | 13 ++++++-------\n>  color.h                |  4 ++--\n>  diff.c                 |  3 +--\n>  grep.c                 |  2 +-\n>  log-tree.c             |  3 +--\n>  pretty.c               |  5 ++---\n>  11 files changed, 26 insertions(+), 28 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 2c97141..35c786d 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -93,8 +93,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)\n>  \t\t\treturn 0;\n>  \t\tif (!value)\n>  \t\t\treturn config_error_nonbool(var);\n> -\t\tcolor_parse(value, var, branch_colors[slot]);\n> -\t\treturn 0;\n> +\t\treturn color_parse(value, branch_colors[slot]);\n>  \t}\n>  \treturn git_color_default_config(var, value, cb);\n>  }\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index 3beeea6..a7e7b0b 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -116,8 +116,7 @@ static int git_clean_config(const char *var, const char *value, void *cb)\n>  \t\t\treturn 0;\n>  \t\tif (!value)\n>  \t\t\treturn config_error_nonbool(var);\n> -\t\tcolor_parse(value, var, clean_colors[slot]);\n> -\t\treturn 0;\n> +\t\treturn color_parse(value, clean_colors[slot]);\n>  \t}\n>  \n>  \tif (!strcmp(var, \"clean.requireforce\")) {\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index c45613a..407566d 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1328,8 +1328,7 @@ static int git_status_config(const char *k, const char *v, void *cb)\n>  \t\t\treturn 0;\n>  \t\tif (!v)\n>  \t\t\treturn config_error_nonbool(k);\n> -\t\tcolor_parse(v, k, s->color_palette[slot]);\n> -\t\treturn 0;\n> +\t\treturn color_parse(v, s->color_palette[slot]);\n>  \t}\n>  \tif (!strcmp(k, \"status.relativepaths\")) {\n>  \t\ts->relative_paths = git_config_bool(k, v);\n> diff --git a/builtin/config.c b/builtin/config.c\n> index 37305e9..8cc2604 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -296,7 +296,8 @@ static int git_get_color_config(const char *var, const char *value, void *cb)\n>  \tif (!strcmp(var, get_color_slot)) {\n>  \t\tif (!value)\n>  \t\t\tconfig_error_nonbool(var);\n> -\t\tcolor_parse(value, var, parsed_color);\n> +\t\tif (color_parse(value, parsed_color) < 0)\n> +\t\t\treturn -1;\n>  \t\tget_color_found = 1;\n>  \t}\n>  \treturn 0;\n> @@ -309,8 +310,10 @@ static void get_color(const char *def_color)\n>  \tgit_config_with_options(git_get_color_config, NULL,\n>  \t\t\t\t&given_config_source, respect_includes);\n>  \n> -\tif (!get_color_found && def_color)\n> -\t\tcolor_parse(def_color, \"command line\", parsed_color);\n> +\tif (!get_color_found && def_color) {\n> +\t\tif (color_parse(def_color, parsed_color) < 0)\n> +\t\t\tdie(_(\"unable to parse default color value\"));\n> +\t}\n>  \n>  \tfputs(parsed_color, stdout);\n>  }\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index fda0f04..7ee86b3 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -671,7 +671,8 @@ static void populate_value(struct refinfo *ref)\n>  \t\t} else if (starts_with(name, \"color:\")) {\n>  \t\t\tchar color[COLOR_MAXLEN] = \"\";\n>  \n> -\t\t\tcolor_parse(name + 6, \"--format\", color);\n> +\t\t\tif (color_parse(name + 6, color) < 0)\n> +\t\t\t\tdie(_(\"unable to parse format\"));\n>  \t\t\tv->s = xstrdup(color);\n>  \t\t\tcontinue;\n>  \t\t} else if (!strcmp(name, \"flag\")) {\n> @@ -1004,7 +1005,8 @@ static void show_ref(struct refinfo *info, const char *format, int quote_style)\n>  \t\tstruct atom_value resetv;\n>  \t\tchar color[COLOR_MAXLEN] = \"\";\n>  \n> -\t\tcolor_parse(\"reset\", \"--format\", color);\n> +\t\tif (color_parse(\"reset\", color) < 0)\n> +\t\t\tdie(\"BUG: couldn't parse 'reset' as a color\");\n>  \t\tresetv.s = color;\n>  \t\tprint_value(&resetv, quote_style);\n>  \t}\n> diff --git a/color.c b/color.c\n> index f672885..7941e93 100644\n> --- a/color.c\n> +++ b/color.c\n> @@ -60,13 +60,12 @@ static int parse_attr(const char *name, int len)\n>  \treturn -1;\n>  }\n>  \n> -void color_parse(const char *value, const char *var, char *dst)\n> +int color_parse(const char *value, char *dst)\n>  {\n> -\tcolor_parse_mem(value, strlen(value), var, dst);\n> +\treturn color_parse_mem(value, strlen(value), dst);\n>  }\n>  \n> -void color_parse_mem(const char *value, int value_len, const char *var,\n> -\t\tchar *dst)\n> +int color_parse_mem(const char *value, int value_len, char *dst)\n>  {\n>  \tconst char *ptr = value;\n>  \tint len = value_len;\n> @@ -76,7 +75,7 @@ void color_parse_mem(const char *value, int value_len, const char *var,\n>  \n>  \tif (!strncasecmp(value, \"reset\", len)) {\n>  \t\tstrcpy(dst, GIT_COLOR_RESET);\n> -\t\treturn;\n> +\t\treturn 0;\n>  \t}\n>  \n>  \t/* [fg [bg]] [attr]... */\n> @@ -153,9 +152,9 @@ void color_parse_mem(const char *value, int value_len, const char *var,\n>  \t\t*dst++ = 'm';\n>  \t}\n>  \t*dst = 0;\n> -\treturn;\n> +\treturn 0;\n>  bad:\n> -\tdie(\"bad color value '%.*s' for variable '%s'\", value_len, value, var);\n> +\treturn error(_(\"invalid color value: %.*s\"), value_len, value);\n>  }\n>  \n>  int git_config_colorbool(const char *var, const char *value)\n> diff --git a/color.h b/color.h\n> index 9a8495b..f5beab1 100644\n> --- a/color.h\n> +++ b/color.h\n> @@ -77,8 +77,8 @@ int git_color_default_config(const char *var, const char *value, void *cb);\n>  \n>  int git_config_colorbool(const char *var, const char *value);\n>  int want_color(int var);\n> -void color_parse(const char *value, const char *var, char *dst);\n> -void color_parse_mem(const char *value, int len, const char *var, char *dst);\n> +int color_parse(const char *value, char *dst);\n> +int color_parse_mem(const char *value, int len, char *dst);\n>  __attribute__((format (printf, 3, 4)))\n>  int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);\n>  __attribute__((format (printf, 3, 4)))\n> diff --git a/diff.c b/diff.c\n> index d7a5c81..d1bd534 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -248,8 +248,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n>  \t\t\treturn 0;\n>  \t\tif (!value)\n>  \t\t\treturn config_error_nonbool(var);\n> -\t\tcolor_parse(value, var, diff_colors[slot]);\n> -\t\treturn 0;\n> +\t\treturn color_parse(value, diff_colors[slot]);\n>  \t}\n>  \n>  \t/* like GNU diff's --suppress-blank-empty option  */\n> diff --git a/grep.c b/grep.c\n> index 99217dc..4dc31ea 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -111,7 +111,7 @@ int grep_config(const char *var, const char *value, void *cb)\n>  \tif (color) {\n>  \t\tif (!value)\n>  \t\t\treturn config_error_nonbool(var);\n> -\t\tcolor_parse(value, var, color);\n> +\t\treturn color_parse(value, color);\n>  \t}\n>  \treturn 0;\n>  }\n> diff --git a/log-tree.c b/log-tree.c\n> index 877d976..7844f33 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -63,8 +63,7 @@ int parse_decorate_color_config(const char *var, const char *slot_name, const ch\n>  \t\treturn 0;\n>  \tif (!value)\n>  \t\treturn config_error_nonbool(var);\n> -\tcolor_parse(value, var, decoration_colors[slot]);\n> -\treturn 0;\n> +\treturn color_parse(value, decoration_colors[slot]);\n>  }\n>  \n>  /*\n> diff --git a/pretty.c b/pretty.c\n> index 31f2ede..59de1dc 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -963,9 +963,8 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n>  \t\t\t\treturn end - placeholder + 1;\n>  \t\t\tbegin += 5;\n>  \t\t}\n> -\t\tcolor_parse_mem(begin,\n> -\t\t\t\tend - begin,\n> -\t\t\t\t\"--pretty format\", color);\n> +\t\tif (color_parse_mem(begin, end - begin, color) < 0)\n> +\t\t\tdie(_(\"unable to parse --pretty format\"));\n>  \t\tstrbuf_addstr(sb, color);\n>  \t\treturn end - placeholder + 1;\n>  \t}\n"},{"id":"250454","messageId":"5436EAB5.2070809@web.de","threadId":"37673","inReplyTo":"xmqqd2a3g2mf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] use skip_prefix() to avoid more magic numbers","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-10-09T20:06:13Z","receivedAt":"2014-10-09T20:06:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.10.2014 um 20:23 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n> \n>> @@ -335,20 +337,18 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,\n>>   \tstatic struct {\n>>   \t\tint kind;\n>>   \t\tconst char *prefix;\n>> -\t\tint pfxlen;\n>>   \t} ref_kind[] = {\n>> -\t\t{ REF_LOCAL_BRANCH, \"refs/heads/\", 11 },\n>> -\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/\", 13 },\n>> +\t\t{ REF_LOCAL_BRANCH, \"refs/heads/\" },\n>> +\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/\" },\n>>   \t};\n>>   \n>>   \t/* Detect kind */\n>>   \tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n>>   \t\tprefix = ref_kind[i].prefix;\n>> -\t\tif (strncmp(refname, prefix, ref_kind[i].pfxlen))\n>> -\t\t\tcontinue;\n>> -\t\tkind = ref_kind[i].kind;\n>> -\t\trefname += ref_kind[i].pfxlen;\n>> -\t\tbreak;\n>> +\t\tif (skip_prefix(refname, prefix, &refname)) {\n>> +\t\t\tkind = ref_kind[i].kind;\n>> +\t\t\tbreak;\n>> +\t\t}\n> \n> This certainly makes it easier to read.\n> \n> I suspect that the original was done as a (possibly premature)\n> optimization to avoid having to do strlen(3) on a variable that\n> points at constant strings for each and every ref we iterate with\n> for_each_rawref(), and it is somewhat sad to see it lost because\n> skip_prefix() assumes that the caller never knows the length of the\n> prefix, though.\n\nI didn't think much about the performance implications.  skip_prefix()\ndoesn't call strlen(3), though.  Your reply made me curious.  The\nsynthetic test program below can be used to call the old and the new\ncode numerous times.  I called it like this:\n\n    for a in strncmp skip_prefix\n    do\n        for b in refs/heads/x refs/remotes/y refs/of/the/third/kind\n        do\n            time ./test-prefix $a $b\n        done\n    done\n\nAnd got the following results:\n\n100000000x strncmp for refs/heads/x, which is a local branch\n\nreal    0m2.423s\nuser    0m2.420s\nsys     0m0.000s\n100000000x strncmp for refs/remotes/y, which is a remote branch\n\nreal    0m4.331s\nuser    0m4.328s\nsys     0m0.000s\n100000000x strncmp for refs/of/the/third/kind, which is no branch\n\nreal    0m3.878s\nuser    0m3.872s\nsys     0m0.000s\n100000000x skip_prefix for refs/heads/x, which is a local branch\n\nreal    0m0.891s\nuser    0m0.888s\nsys     0m0.000s\n100000000x skip_prefix for refs/remotes/y, which is a remote branch\n\nreal    0m1.345s\nuser    0m1.340s\nsys     0m0.000s\n100000000x skip_prefix for refs/of/the/third/kind, which is no branch\n\nreal    0m1.080s\nuser    0m1.076s\nsys     0m0.000s\n\n\nThe code handles millions of ref strings per second before and after\nthe change, and with the change it's faster.  I hope the results are\nreproducible and make it easier to say goodbye to pfxlen. :)\n\nRené\n\n---\n .gitignore    |  1 +\n Makefile      |  1 +\n test-prefix.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 89 insertions(+)\n create mode 100644 test-prefix.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 5bfb234..8416c5e 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -193,6 +193,7 @@\n /test-mktemp\n /test-parse-options\n /test-path-utils\n+/test-prefix\n /test-prio-queue\n /test-read-cache\n /test-regex\ndiff --git a/Makefile b/Makefile\nindex f34a2d4..c09b59e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -561,6 +561,7 @@ TEST_PROGRAMS_NEED_X += test-mergesort\n TEST_PROGRAMS_NEED_X += test-mktemp\n TEST_PROGRAMS_NEED_X += test-parse-options\n TEST_PROGRAMS_NEED_X += test-path-utils\n+TEST_PROGRAMS_NEED_X += test-prefix\n TEST_PROGRAMS_NEED_X += test-prio-queue\n TEST_PROGRAMS_NEED_X += test-read-cache\n TEST_PROGRAMS_NEED_X += test-regex\ndiff --git a/test-prefix.c b/test-prefix.c\nnew file mode 100644\nindex 0000000..ddc63af\n--- /dev/null\n+++ b/test-prefix.c\n@@ -0,0 +1,87 @@\n+#include \"cache.h\"\n+\n+#define ROUNDS 100000000\n+\n+#define REF_LOCAL_BRANCH    0x01\n+#define REF_REMOTE_BRANCH   0x02\n+\n+static int test_skip_prefix(const char *refname)\n+{\n+\tint kind, i;\n+\tconst char *prefix;\n+\tstatic struct {\n+\t\tint kind;\n+\t\tconst char *prefix;\n+\t} ref_kind[] = {\n+\t\t{ REF_LOCAL_BRANCH, \"refs/heads/\" },\n+\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/\" },\n+\t};\n+\n+\tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n+\t\tprefix = ref_kind[i].prefix;\n+\t\tif (skip_prefix(refname, prefix, &refname)) {\n+\t\t\tkind = ref_kind[i].kind;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tif (ARRAY_SIZE(ref_kind) <= i)\n+\t\treturn 0;\n+\treturn kind;\n+}\n+\n+static int test_strncmp(const char *refname)\n+{\n+\tint kind, i;\n+\tconst char *prefix;\n+\tstatic struct {\n+\t\tint kind;\n+\t\tconst char *prefix;\n+\t\tint pfxlen;\n+\t} ref_kind[] = {\n+\t\t{ REF_LOCAL_BRANCH, \"refs/heads/\", 11 },\n+\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/\", 13 },\n+\t};\n+\n+\tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n+\t\tprefix = ref_kind[i].prefix;\n+\t\tif (strncmp(refname, prefix, ref_kind[i].pfxlen))\n+\t\t\tcontinue;\n+\t\tkind = ref_kind[i].kind;\n+\t\trefname += ref_kind[i].pfxlen;\n+\t\tbreak;\n+\t}\n+\tif (ARRAY_SIZE(ref_kind) <= i)\n+\t\treturn 0;\n+\treturn kind;\n+}\n+\n+int main(int argc, char **argv)\n+{\n+\tif (argc == 3) {\n+\t\tint (*fn)(const char *) = NULL;\n+\t\tprintf(\"%dx %s for %s, which is \", ROUNDS, argv[1], argv[2]);\n+\t\tif (!strcmp(argv[1], \"skip_prefix\"))\n+\t\t\tfn = test_skip_prefix;\n+\t\tif (!strcmp(argv[1], \"strncmp\"))\n+\t\t\tfn = test_strncmp;\n+\t\tif (fn) {\n+\t\t\tint i, kind = 0;\n+\t\t\tfor (i = 0; i < ROUNDS; i++)\n+\t\t\t\tkind |= fn(argv[2]);\n+\t\t\tswitch (kind) {\n+\t\t\tcase 0:\n+\t\t\t\tputs(\"no branch\");\n+\t\t\t\tbreak;\n+\t\t\tcase REF_LOCAL_BRANCH:\n+\t\t\t\tputs(\"a local branch\");\n+\t\t\t\tbreak;\n+\t\t\tcase REF_REMOTE_BRANCH:\n+\t\t\t\tputs(\"a remote branch\");\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\tputs(\"invalid\");\n+\t\t\t}\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n-- \n2.1.2\n"},{"id":"250455","messageId":"xmqqsiix57fr.fsf@gitster.dls.corp.google.com","threadId":"37673","inReplyTo":"5436EAB5.2070809@web.de","subject":"Re: [PATCH] use skip_prefix() to avoid more magic numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-09T20:12:08Z","receivedAt":"2014-10-09T20:12:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> I didn't think much about the performance implications.  skip_prefix()\n> doesn't call strlen(3), though.\n\nAh, OK.\n\n> The code handles millions of ref strings per second before and after\n> the change, and with the change it's faster.  I hope the results are\n> reproducible and make it easier to say goodbye to pfxlen. :)\n\n;-)\n"}]}