[PATCH] use skip_prefix() to avoid more magic numbers

[René Scharfe]

Continue where ae021d87 (use skip_prefix to avoid magic numbers) left off
and use skip_prefix() in more places for determining the lengths of prefix
strings to avoid using dependent constants and other indirect methods.

Signed-off-by: Rene Scharfe <***@web.de>
---
builtin/apply.c | 2 +-
builtin/branch.c | 29 +++++++++++------------
builtin/cat-file.c | 5 ++--
builtin/checkout.c | 6 ++---
builtin/clean.c | 7 +++---
builtin/commit.c | 18 +++++++--------
builtin/get-tar-commit-id.c | 5 ++--
builtin/log.c | 6 +++--
builtin/remote-ext.c | 10 ++++----
pretty.c | 56 +++++++++++++++++++++------------------------
10 files changed, 69 insertions(+), 75 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 8714a88..97f7e8e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -435,7 +435,7 @@ static unsigned long linelen(const char *buffer, unsigned long size)

static int is_dev_null(const char *str)
{
- return !memcmp("/dev/null", str, 9) && isspace(str[9]);
+ return skip_prefix(str, "/dev/null", &str) && isspace(*str);
}

#define TERM_SPACE 1
diff --git a/builtin/branch.c b/builtin/branch.c
index 9e4666f..6785097 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int ofs)

static int git_branch_config(const char *var, const char *value, void *cb)
{
+ const char *slot_name;
+
if (starts_with(var, "column."))
return git_column_config(var, value, "branch", &colopts);
if (!strcmp(var, "color.branch")) {
branch_use_color = git_config_colorbool(var, value);
return 0;
}
- if (starts_with(var, "color.branch.")) {
- int slot = parse_branch_color_slot(var, 13);
+ if (skip_prefix(var, "color.branch.", &slot_name)) {
+ int slot = parse_branch_color_slot(var, slot_name - var);
if (slot < 0)
return 0;
if (!value)
@@ -335,20 +337,18 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
static struct {
int kind;
const char *prefix;
- int pfxlen;
} ref_kind[] = {
- { REF_LOCAL_BRANCH, "refs/heads/", 11 },
- { REF_REMOTE_BRANCH, "refs/remotes/", 13 },
+ { REF_LOCAL_BRANCH, "refs/heads/" },
+ { REF_REMOTE_BRANCH, "refs/remotes/" },
};

/* Detect kind */
for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
prefix = ref_kind[i].prefix;
- if (strncmp(refname, prefix, ref_kind[i].pfxlen))
- continue;
- kind = ref_kind[i].kind;
- refname += ref_kind[i].pfxlen;
- break;
+ if (skip_prefix(refname, prefix, &refname)) {
+ kind = ref_kind[i].kind;
+ break;
+ }
}
if (ARRAY_SIZE(ref_kind) <= i)
return 0;
@@ -872,13 +872,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
head = resolve_refdup("HEAD", head_sha1, 0, NULL);
if (!head)
die(_("Failed to resolve HEAD as a valid ref."));
- if (!strcmp(head, "HEAD")) {
+ if (!strcmp(head, "HEAD"))
detached = 1;
- } else {
- if (!starts_with(head, "refs/heads/"))
- die(_("HEAD not found below refs/heads!"));
- head += 11;
- }
+ else if (!skip_prefix(head, "refs/heads/", &head))
+ die(_("HEAD not found below refs/heads!"));
hashcpy(merge_filter_ref, head_sha1);


diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 7073304..f8d8129 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -82,8 +82,9 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
enum object_type type;
unsigned long size;
char *buffer = read_sha1_file(sha1, &type, &size);
- if (memcmp(buffer, "object ", 7) ||
- get_sha1_hex(buffer + 7, blob_sha1))
+ const char *target;
+ if (!skip_prefix(buffer, "object ", &target) ||
+ get_sha1_hex(target, blob_sha1))
die("%s not a valid tag", sha1_to_hex(sha1));
free(buffer);
} else
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8afdf2b..cef1996 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1150,10 +1150,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
const char *argv0 = argv[0];
if (!argc || !strcmp(argv0, "--"))
die (_("--track needs a branch name"));
- if (starts_with(argv0, "refs/"))
- argv0 += 5;
- if (starts_with(argv0, "remotes/"))
- argv0 += 8;
+ skip_prefix(argv0, "refs/", &argv0);
+ skip_prefix(argv0, "remotes/", &argv0);
argv0 = strchr(argv0, '/');
if (!argv0 || !argv0[1])
die (_("Missing branch name; try -b"));
diff --git a/builtin/clean.c b/builtin/clean.c
index 3beeea6..c35505e 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -100,6 +100,8 @@ static int parse_clean_color_slot(const char *var)

static int git_clean_config(const char *var, const char *value, void *cb)
{
+ const char *slot_name;
+
if (starts_with(var, "column."))
return git_column_config(var, value, "clean", &colopts);

@@ -109,9 +111,8 @@ static int git_clean_config(const char *var, const char *value, void *cb)
clean_use_color = git_config_colorbool(var, value);
return 0;
}
- if (starts_with(var, "color.interactive.")) {
- int slot = parse_clean_color_slot(var +
- strlen("color.interactive."));
+ if (skip_prefix(var, "color.interactive.", &slot_name)) {
+ int slot = parse_clean_color_slot(slot_name);
if (slot < 0)
return 0;
if (!value)
diff --git a/builtin/commit.c b/builtin/commit.c
index b0fe784..cff7802 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1294,6 +1294,7 @@ static int parse_status_slot(const char *var, int offset)
static int git_status_config(const char *k, const char *v, void *cb)
{
struct wt_status *s = cb;
+ const char *slot_name;

if (starts_with(k, "column."))
return git_column_config(k, v, "status", &s->colopts);
@@ -1323,8 +1324,9 @@ static int git_status_config(const char *k, const char *v, void *cb)
s->display_comment_prefix = git_config_bool(k, v);
return 0;
}
- if (starts_with(k, "status.color.") || starts_with(k, "color.status.")) {
- int slot = parse_status_slot(k, 13);
+ if (skip_prefix(k, "status.color.", &slot_name) ||
+ skip_prefix(k, "color.status.", &slot_name)) {
+ int slot = parse_status_slot(k, slot_name - k);
if (slot < 0)
return 0;
if (!v)
@@ -1513,13 +1515,11 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
diff_setup_done(&rev.diffopt);

head = resolve_ref_unsafe("HEAD", junk_sha1, 0, NULL);
- printf("[%s%s ",
- starts_with(head, "refs/heads/") ?
- head + 11 :
- !strcmp(head, "HEAD") ?
- _("detached HEAD") :
- head,
- initial_commit ? _(" (root-commit)") : "");
+ if (!strcmp(head, "HEAD"))
+ head = _("detached HEAD");
+ else
+ skip_prefix(head, "refs/heads/", &head);
+ printf("[%s%s ", head, initial_commit ? _(" (root-commit)") : "");

if (!log_tree_commit(&rev, commit)) {
rev.always_show_header = 1;
diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c
index aa72596..6f4147a 100644
--- a/builtin/get-tar-commit-id.c
+++ b/builtin/get-tar-commit-id.c
@@ -19,6 +19,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)
char buffer[HEADERSIZE];
struct ustar_header *header = (struct ustar_header *)buffer;
char *content = buffer + RECORDSIZE;
+ const char *comment;
ssize_t n;

if (argc != 1)
@@ -29,10 +30,10 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)
die("git get-tar-commit-id: read error");
if (header->typeflag[0] != 'g')
return 1;
- if (memcmp(content, "52 comment=", 11))
+ if (!skip_prefix(content, "52 comment=", &comment))
return 1;

- n = write_in_full(1, content + 11, 41);
+ n = write_in_full(1, comment, 41);
if (n < 41)
die_errno("git get-tar-commit-id: write error");

diff --git a/builtin/log.c b/builtin/log.c
index 2fb34c7..1202eba 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -368,6 +368,8 @@ static int cmd_log_walk(struct rev_info *rev)

static int git_log_config(const char *var, const char *value, void *cb)
{
+ const char *slot_name;
+
if (!strcmp(var, "format.pretty"))
return git_config_string(&fmt_pretty, var, value);
if (!strcmp(var, "format.subjectprefix"))
@@ -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);
if (!strcmp(var, "log.mailmap")) {
use_mailmap_config = git_config_bool(var, value);
return 0;
diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index d699d28..3b8c22c 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -30,16 +30,14 @@ static char *strip_escapes(const char *str, const char *service,
size_t rpos = 0;
int escape = 0;
char special = 0;
- size_t psoff = 0;
+ const char *service_noprefix = service;
struct strbuf ret = STRBUF_INIT;

- /* Calculate prefix length for \s and lengths for \s and \S */
- if (!strncmp(service, "git-", 4))
- psoff = 4;
+ skip_prefix(service_noprefix, "git-", &service_noprefix);

/* Pass the service to command. */
setenv("GIT_EXT_SERVICE", service, 1);
- setenv("GIT_EXT_SERVICE_NOPREFIX", service + psoff, 1);
+ setenv("GIT_EXT_SERVICE_NOPREFIX", service_noprefix, 1);

/* Scan the length of argument. */
while (str[rpos] && (escape || str[rpos] != ' ')) {
@@ -85,7 +83,7 @@ static char *strip_escapes(const char *str, const char *service,
strbuf_addch(&ret, str[rpos]);
break;
case 's':
- strbuf_addstr(&ret, service + psoff);
+ strbuf_addstr(&ret, service_noprefix);
break;
case 'S':
strbuf_addstr(&ret, service);
diff --git a/pretty.c b/pretty.c
index 5fd9de6..a181ac6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -73,10 +73,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c
if (git_config_string(&fmt, var, value))
return -1;

- if (starts_with(fmt, "format:") || starts_with(fmt, "tformat:")) {
- commit_format->is_tformat = fmt[0] == 't';
- fmt = strchr(fmt, ':') + 1;
- } else if (strchr(fmt, '%'))
+ if (skip_prefix(fmt, "format:", &fmt))
+ commit_format->is_tformat = 0;
+ else if (skip_prefix(fmt, "tformat:", &fmt) || strchr(fmt, '%'))
commit_format->is_tformat = 1;
else
commit_format->is_alias = 1;
@@ -157,12 +156,12 @@ void get_commit_format(const char *arg, struct rev_info *rev)
rev->commit_format = CMIT_FMT_DEFAULT;
return;
}
- if (starts_with(arg, "format:") || starts_with(arg, "tformat:")) {
- save_user_format(rev, strchr(arg, ':') + 1, arg[0] == 't');
+ if (skip_prefix(arg, "format:", &arg)) {
+ save_user_format(rev, arg, 0);
return;
}

- if (!*arg || strchr(arg, '%')) {
+ if (!*arg || skip_prefix(arg, "tformat:", &arg) || strchr(arg, '%')) {
save_user_format(rev, arg, 1);
return;
}
@@ -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;
int eol;
for (eol = i; msg[eol] && msg[eol] != '\n'; eol++)
; /* do nothing */

if (i == eol) {
break;
- } else if (starts_with(msg + i, "author ")) {
- context->author.off = i + 7;
- context->author.len = eol - i - 7;
- } else if (starts_with(msg + i, "committer ")) {
- context->committer.off = i + 10;
- context->committer.len = eol - i - 10;
+ } 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;
}
i = eol;
}
@@ -951,6 +951,8 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
struct format_commit_context *c)
{
+ const char *rest = placeholder;
+
if (placeholder[1] == '(') {
const char *begin = placeholder + 2;
const char *end = strchr(begin, ')');
@@ -958,10 +960,9 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */

if (!end)
return 0;
- if (starts_with(begin, "auto,")) {
+ if (skip_prefix(begin, "auto,", &begin)) {
if (!want_color(c->pretty_ctx->color))
return end - placeholder + 1;
- begin += 5;
}
color_parse_mem(begin,
end - begin,
@@ -969,20 +970,15 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
strbuf_addstr(sb, color);
return end - placeholder + 1;
}
- if (starts_with(placeholder + 1, "red")) {
+ if (skip_prefix(placeholder + 1, "red", &rest))
strbuf_addstr(sb, GIT_COLOR_RED);
- return 4;
- } else if (starts_with(placeholder + 1, "green")) {
+ else if (skip_prefix(placeholder + 1, "green", &rest))
strbuf_addstr(sb, GIT_COLOR_GREEN);
- return 6;
- } else if (starts_with(placeholder + 1, "blue")) {
+ else if (skip_prefix(placeholder + 1, "blue", &rest))
strbuf_addstr(sb, GIT_COLOR_BLUE);
- return 5;
- } else if (starts_with(placeholder + 1, "reset")) {
+ else if (skip_prefix(placeholder + 1, "reset", &rest))
strbuf_addstr(sb, GIT_COLOR_RESET);
- return 6;
- } else
- return 0;
+ return rest - placeholder;
}

static size_t parse_padding_placeholder(struct strbuf *sb,
@@ -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;
int linelen = get_one_line(*msg_p);

if (!linelen)
@@ -1557,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 (starts_with(line, "author ")) {
+ if (skip_prefix(line, "author ", &name)) {
strbuf_grow(sb, linelen + 80);
- pp_user_info(pp, "Author", sb, line + 7, encoding);
+ pp_user_info(pp, "Author", sb, name, encoding);
}
- if (starts_with(line, "committer ") &&
+ if (skip_prefix(line, "committer ", &name) &&
(pp->fmt == CMIT_FMT_FULL || pp->fmt == CMIT_FMT_FULLER)) {
strbuf_grow(sb, linelen + 80);
- pp_user_info(pp, "Commit", sb, line + 10, encoding);
+ pp_user_info(pp, "Commit", sb, name, encoding);
}
}
}

[Jonathan Nieder]

Continue where ae021d87 (use skip_prefix to avoid magic numbers) left=

off

and use skip_prefix() in more places for determining the lengths of p=

refix

strings to avoid using dependent constants and other indirect methods=

=2E

Sounds promising.

[...]

--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *va=

r, int ofs)

=20
static int git_branch_config(const char *var, const char *value, voi=

d *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 =3D git_config_colorbool(var, value);
return 0;
}
- if (starts_with(var, "color.branch.")) {
- int slot =3D parse_branch_color_slot(var, 13);
+ if (skip_prefix(var, "color.branch.", &slot_name)) {
+ int slot =3D 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 =3D parse_branch_color_slot(slot_name, 0);

to be simpler. At second glance, how about something like the followin=
g
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 =3D 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_c=

ommit_context *context)

int i;
=20
for (i =3D 0; msg[i]; i++) {
+ const char *name;

ident instead of name, maybe? (since it's a 'name <email> timestamp'
field)

[...]

@@ -1522,7 +1518,7 @@ static void pp_header(struct pretty_print_conte=

xt *pp,

int parents_shown =3D 0;
=20
for (;;) {
- const char *line =3D *msg_p;
+ const char *name, *line =3D *msg_p;

Likewise.

With or without the changes below,
Reviewed-by: Jonathan Nieder <***@gmail.com>

diff --git i/builtin/branch.c w/builtin/branch.c
index 6785097..022a88e 100644
--- i/builtin/branch.c
+++ w/builtin/branch.c
@@ -62,19 +62,19 @@ static unsigned char merge_filter_ref[20];
static struct string_list output =3D STRING_LIST_INIT_DUP;
static unsigned int colopts;
=20
-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 c=
har *value, void *cb)
return 0;
}
if (skip_prefix(var, "color.branch.", &slot_name)) {
- int slot =3D parse_branch_color_slot(var, slot_name - var);
+ int slot =3D 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 ch=
ar *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 =3D 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;
}
=20
-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 =3D parse_decorate_color_slot(var + ofs);
+ int slot =3D 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;
};
=20
-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_com=
mit_context *context)
int i;
=20
for (i =3D 0; msg[i]; i++) {
- const char *name;
+ const char *ident;
int eol;
for (eol =3D i; msg[eol] && msg[eol] !=3D '\n'; eol++)
; /* do nothing */
=20
if (i =3D=3D eol) {
break;
- } else if (skip_prefix(msg + i, "author ", &name)) {
- context->author.off =3D name - msg;
- context->author.len =3D msg + eol - name;
- } else if (skip_prefix(msg + i, "committer ", &name)) {
- context->committer.off =3D name - msg;
- context->committer.len =3D msg + eol - name;
+ } else if (skip_prefix(msg + i, "author ", &ident)) {
+ context->author.off =3D ident - msg;
+ context->author.len =3D msg + eol - ident;
+ } else if (skip_prefix(msg + i, "committer ", &ident)) {
+ context->committer.off =3D ident - msg;
+ context->committer.len =3D msg + eol - ident;
}
i =3D eol;
}
@@ -1518,7 +1518,7 @@ static void pp_header(struct pretty_print_context=
*pp,
int parents_shown =3D 0;
=20
for (;;) {
- const char *name, *line =3D *msg_p;
+ const char *ident, *line =3D *msg_p;
int linelen =3D get_one_line(*msg_p);
=20
if (!linelen)
@@ -1553,14 +1553,14 @@ static void pp_header(struct pretty_print_conte=
xt *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 =3D=3D CMIT_FMT_FULL || pp->fmt =3D=3D CMIT_FMT_FULLER)=
) {
strbuf_grow(sb, linelen + 80);
- pp_user_info(pp, "Commit", sb, name, encoding);
+ pp_user_info(pp, "Commit", sb, ident, encoding);
}
}
}

[Jeff King]

Post by Jonathan Nieder

Post by René Scharfe
--- 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).

Post by Jonathan Nieder

Post by René Scharfe
@@ -809,18 +808,19 @@ static void parse_commit_header(struct format_commit_context *context)
int i;
for (i = 0; msg[i]; i++) {
+ const char *name;

ident instead of name, maybe? (since it's a 'name <email> timestamp'
field)

Yeah, agreed.

-Peff

[Junio C Hamano]

Post by Jeff King

Post by Jonathan Nieder

Post by René Scharfe
--- 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

Post by Jeff King

Post by Jonathan Nieder

Post by René Scharfe
@@ -809,18 +808,19 @@ static void parse_commit_header(struct
format_commit_context *context)
int i;
for (i = 0; msg[i]; i++) {
+ const char *name;

ident instead of name, maybe? (since it's a 'name <email> timestamp'
field)

Yeah, agreed.
-Peff

[Jeff King]

Post by Junio C Hamano

Post by Jeff King
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 <***@gmail.com>
Subject: pass config slots as pointers instead of offsets

Many config-parsing helpers, like parse_branch_color_slot,
take the name of a config variable and an offset to the
"slot" name (e.g., "color.branch.plain" is passed along with
"13" to effectively pass "plain"). This is leftover from the
time that these functions would die() on error, and would
want the full variable name for error reporting.

These days they do not use the full variable name at all.
Passing a single pointer to the slot name is more natural,
and lets us more easily adjust the callers to use skip_prefix
to avoid manually writing offset numbers.

This is effectively a continuation of 9e1a5eb, which did the
same for parse_diff_color_slot. This patch covers all of the
remaining similar constructs.

Signed-off-by: Jeff King <***@peff.net>
---
Actually, parse_decorate_color_config is slightly odd in that it takes
the variable and the slot_name after this patch. That's because it
passes the full variable name to color_parse, which does still die() on
error. Perhaps it should be converted to return an error, too.

builtin/branch.c | 16 ++++++++--------
builtin/commit.c | 19 +++++++++----------
builtin/log.c | 2 +-
log-tree.c | 4 ++--
log-tree.h | 2 +-
5 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 9e4666f..2c97141 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -62,19 +62,19 @@ static unsigned char merge_filter_ref[20];
static struct string_list output = STRING_LIST_INIT_DUP;
static unsigned int colopts;

-static int parse_branch_color_slot(const char *var, int ofs)
+static int parse_branch_color_slot(const char *slot)
{
- if (!strcasecmp(var+ofs, "plain"))
+ if (!strcasecmp(slot, "plain"))
return BRANCH_COLOR_PLAIN;
- if (!strcasecmp(var+ofs, "reset"))
+ if (!strcasecmp(slot, "reset"))
return BRANCH_COLOR_RESET;
- if (!strcasecmp(var+ofs, "remote"))
+ if (!strcasecmp(slot, "remote"))
return BRANCH_COLOR_REMOTE;
- if (!strcasecmp(var+ofs, "local"))
+ if (!strcasecmp(slot, "local"))
return BRANCH_COLOR_LOCAL;
- if (!strcasecmp(var+ofs, "current"))
+ if (!strcasecmp(slot, "current"))
return BRANCH_COLOR_CURRENT;
- if (!strcasecmp(var+ofs, "upstream"))
+ if (!strcasecmp(slot, "upstream"))
return BRANCH_COLOR_UPSTREAM;
return -1;
}
@@ -88,7 +88,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)
return 0;
}
if (starts_with(var, "color.branch.")) {
- int slot = parse_branch_color_slot(var, 13);
+ int slot = parse_branch_color_slot(var + 13);
if (slot < 0)
return 0;
if (!value)
diff --git a/builtin/commit.c b/builtin/commit.c
index b0fe784..c45613a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1271,22 +1271,21 @@ static int dry_run_commit(int argc, const char **argv, const char *prefix,
return commitable ? 0 : 1;
}

-static int parse_status_slot(const char *var, int offset)
+static int parse_status_slot(const char *slot)
{
- if (!strcasecmp(var+offset, "header"))
+ if (!strcasecmp(slot, "header"))
return WT_STATUS_HEADER;
- if (!strcasecmp(var+offset, "branch"))
+ if (!strcasecmp(slot, "branch"))
return WT_STATUS_ONBRANCH;
- if (!strcasecmp(var+offset, "updated")
- || !strcasecmp(var+offset, "added"))
+ if (!strcasecmp(slot, "updated") || !strcasecmp(slot, "added"))
return WT_STATUS_UPDATED;
- if (!strcasecmp(var+offset, "changed"))
+ if (!strcasecmp(slot, "changed"))
return WT_STATUS_CHANGED;
- if (!strcasecmp(var+offset, "untracked"))
+ if (!strcasecmp(slot, "untracked"))
return WT_STATUS_UNTRACKED;
- if (!strcasecmp(var+offset, "nobranch"))
+ if (!strcasecmp(slot, "nobranch"))
return WT_STATUS_NOBRANCH;
- if (!strcasecmp(var+offset, "unmerged"))
+ if (!strcasecmp(slot, "unmerged"))
return WT_STATUS_UNMERGED;
return -1;
}
@@ -1324,7 +1323,7 @@ static int git_status_config(const char *k, const char *v, void *cb)
return 0;
}
if (starts_with(k, "status.color.") || starts_with(k, "color.status.")) {
- int slot = parse_status_slot(k, 13);
+ int slot = parse_status_slot(k + 13);
if (slot < 0)
return 0;
if (!v)
diff --git a/builtin/log.c b/builtin/log.c
index 2fb34c7..9c75447 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -389,7 +389,7 @@ static int git_log_config(const char *var, const char *value, void *cb)
return 0;
}
if (starts_with(var, "color.decorate."))
- return parse_decorate_color_config(var, 15, value);
+ return parse_decorate_color_config(var, var + 15, value);
if (!strcmp(var, "log.mailmap")) {
use_mailmap_config = git_config_bool(var, value);
return 0;
diff --git a/log-tree.c b/log-tree.c
index bcee7c5..877d976 100644
--- a/log-tree.c
+++ b/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 a/log-tree.h b/log-tree.h
index d6ecd4d..8cbefac 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -7,7 +7,7 @@ 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 *);

[Jeff King]

Post 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.

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 <***@peff.net>
---
I think the two-line errors are kind of ugly. It does match how
config_error_nonbool works, which also propagates its errors, and looks
like:

$ git -c color.branch.plain branch
error: Missing value for 'color.branch.plain'
fatal: unable to parse 'color.branch.plain' from command-line config

We could instead make color_parse silently return -1, and leave it up to
the caller to complain (and probably add die_bad_color_config() or
similar to avoid repetition in the config callers).

builtin/branch.c | 3 +--
builtin/clean.c | 3 +--
builtin/commit.c | 3 +--
builtin/config.c | 9 ++++++---
builtin/for-each-ref.c | 6 ++++--
color.c | 13 ++++++-------
color.h | 4 ++--
diff.c | 3 +--
grep.c | 2 +-
log-tree.c | 3 +--
pretty.c | 5 ++---
11 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 2c97141..35c786d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -93,8 +93,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)
return 0;
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, branch_colors[slot]);
- return 0;
+ return color_parse(value, branch_colors[slot]);
}
return git_color_default_config(var, value, cb);
}
diff --git a/builtin/clean.c b/builtin/clean.c
index 3beeea6..a7e7b0b 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -116,8 +116,7 @@ static int git_clean_config(const char *var, const char *value, void *cb)
return 0;
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, clean_colors[slot]);
- return 0;
+ return color_parse(value, clean_colors[slot]);
}

if (!strcmp(var, "clean.requireforce")) {
diff --git a/builtin/commit.c b/builtin/commit.c
index c45613a..407566d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1328,8 +1328,7 @@ static int git_status_config(const char *k, const char *v, void *cb)
return 0;
if (!v)
return config_error_nonbool(k);
- color_parse(v, k, s->color_palette[slot]);
- return 0;
+ return color_parse(v, s->color_palette[slot]);
}
if (!strcmp(k, "status.relativepaths")) {
s->relative_paths = git_config_bool(k, v);
diff --git a/builtin/config.c b/builtin/config.c
index 37305e9..8cc2604 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -296,7 +296,8 @@ static int git_get_color_config(const char *var, const char *value, void *cb)
if (!strcmp(var, get_color_slot)) {
if (!value)
config_error_nonbool(var);
- color_parse(value, var, parsed_color);
+ if (color_parse(value, parsed_color) < 0)
+ return -1;
get_color_found = 1;
}
return 0;
@@ -309,8 +310,10 @@ static void get_color(const char *def_color)
git_config_with_options(git_get_color_config, NULL,
&given_config_source, respect_includes);

- if (!get_color_found && def_color)
- color_parse(def_color, "command line", parsed_color);
+ if (!get_color_found && def_color) {
+ if (color_parse(def_color, parsed_color) < 0)
+ die(_("unable to parse default color value"));
+ }

fputs(parsed_color, stdout);
}
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index fda0f04..7ee86b3 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -671,7 +671,8 @@ static void populate_value(struct refinfo *ref)
} else if (starts_with(name, "color:")) {
char color[COLOR_MAXLEN] = "";

- color_parse(name + 6, "--format", color);
+ if (color_parse(name + 6, color) < 0)
+ die(_("unable to parse format"));
v->s = xstrdup(color);
continue;
} else if (!strcmp(name, "flag")) {
@@ -1004,7 +1005,8 @@ static void show_ref(struct refinfo *info, const char *format, int quote_style)
struct atom_value resetv;
char color[COLOR_MAXLEN] = "";

- color_parse("reset", "--format", color);
+ if (color_parse("reset", color) < 0)
+ die("BUG: couldn't parse 'reset' as a color");
resetv.s = color;
print_value(&resetv, quote_style);
}
diff --git a/color.c b/color.c
index f672885..7941e93 100644
--- a/color.c
+++ b/color.c
@@ -60,13 +60,12 @@ static int parse_attr(const char *name, int len)
return -1;
}

-void color_parse(const char *value, const char *var, char *dst)
+int color_parse(const char *value, char *dst)
{
- color_parse_mem(value, strlen(value), var, dst);
+ return color_parse_mem(value, strlen(value), dst);
}

-void color_parse_mem(const char *value, int value_len, const char *var,
- char *dst)
+int color_parse_mem(const char *value, int value_len, char *dst)
{
const char *ptr = value;
int len = value_len;
@@ -76,7 +75,7 @@ void color_parse_mem(const char *value, int value_len, const char *var,

if (!strncasecmp(value, "reset", len)) {
strcpy(dst, GIT_COLOR_RESET);
- return;
+ return 0;
}

/* [fg [bg]] [attr]... */
@@ -153,9 +152,9 @@ void color_parse_mem(const char *value, int value_len, const char *var,
*dst++ = 'm';
}
*dst = 0;
- return;
+ return 0;
bad:
- die("bad color value '%.*s' for variable '%s'", value_len, value, var);
+ return error(_("invalid color value: %.*s"), value_len, value);
}

int git_config_colorbool(const char *var, const char *value)
diff --git a/color.h b/color.h
index 9a8495b..f5beab1 100644
--- a/color.h
+++ b/color.h
@@ -77,8 +77,8 @@ int git_color_default_config(const char *var, const char *value, void *cb);

int git_config_colorbool(const char *var, const char *value);
int want_color(int var);
-void color_parse(const char *value, const char *var, char *dst);
-void color_parse_mem(const char *value, int len, const char *var, char *dst);
+int color_parse(const char *value, char *dst);
+int color_parse_mem(const char *value, int len, char *dst);
__attribute__((format (printf, 3, 4)))
int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
__attribute__((format (printf, 3, 4)))
diff --git a/diff.c b/diff.c
index d7a5c81..d1bd534 100644
--- a/diff.c
+++ b/diff.c
@@ -248,8 +248,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
return 0;
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, diff_colors[slot]);
- return 0;
+ return color_parse(value, diff_colors[slot]);
}

/* like GNU diff's --suppress-blank-empty option */
diff --git a/grep.c b/grep.c
index 99217dc..4dc31ea 100644
--- a/grep.c
+++ b/grep.c
@@ -111,7 +111,7 @@ int grep_config(const char *var, const char *value, void *cb)
if (color) {
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, color);
+ return color_parse(value, color);
}
return 0;
}
diff --git a/log-tree.c b/log-tree.c
index 877d976..7844f33 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -63,8 +63,7 @@ int parse_decorate_color_config(const char *var, const char *slot_name, const ch
return 0;
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, decoration_colors[slot]);
- return 0;
+ return color_parse(value, decoration_colors[slot]);
}

/*
diff --git a/pretty.c b/pretty.c
index 31f2ede..59de1dc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -963,9 +963,8 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
return end - placeholder + 1;
begin += 5;
}
- color_parse_mem(begin,
- end - begin,
- "--pretty format", color);
+ if (color_parse_mem(begin, end - begin, color) < 0)
+ die(_("unable to parse --pretty format"));
strbuf_addstr(sb, color);
return end - placeholder + 1;
}

[Junio C Hamano]

Post by Jeff King
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
$ 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.

Post by Jeff King
builtin/branch.c | 3 +--
builtin/clean.c | 3 +--
builtin/commit.c | 3 +--
builtin/config.c | 9 ++++++---
builtin/for-each-ref.c | 6 ++++--
color.c | 13 ++++++-------
color.h | 4 ++--
diff.c | 3 +--
grep.c | 2 +-
log-tree.c | 3 +--
pretty.c | 5 ++---
11 files changed, 26 insertions(+), 28 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2c97141..35c786d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -93,8 +93,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)
return 0;
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, branch_colors[slot]);
- return 0;
+ return color_parse(value, branch_colors[slot]);
}
return git_color_default_config(var, value, cb);
}
diff --git a/builtin/clean.c b/builtin/clean.c
index 3beeea6..a7e7b0b 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -116,8 +116,7 @@ static int git_clean_config(const char *var, const char *value, void *cb)
return 0;
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, clean_colors[slot]);
- return 0;
+ return color_parse(value, clean_colors[slot]);
}
if (!strcmp(var, "clean.requireforce")) {
diff --git a/builtin/commit.c b/builtin/commit.c
index c45613a..407566d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1328,8 +1328,7 @@ static int git_status_config(const char *k, const char *v, void *cb)
return 0;
if (!v)
return config_error_nonbool(k);
- color_parse(v, k, s->color_palette[slot]);
- return 0;
+ return color_parse(v, s->color_palette[slot]);
}
if (!strcmp(k, "status.relativepaths")) {
s->relative_paths = git_config_bool(k, v);
diff --git a/builtin/config.c b/builtin/config.c
index 37305e9..8cc2604 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -296,7 +296,8 @@ static int git_get_color_config(const char *var, const char *value, void *cb)
if (!strcmp(var, get_color_slot)) {
if (!value)
config_error_nonbool(var);
- color_parse(value, var, parsed_color);
+ if (color_parse(value, parsed_color) < 0)
+ return -1;
get_color_found = 1;
}
return 0;
@@ -309,8 +310,10 @@ static void get_color(const char *def_color)
git_config_with_options(git_get_color_config, NULL,
&given_config_source, respect_includes);
- if (!get_color_found && def_color)
- color_parse(def_color, "command line", parsed_color);
+ if (!get_color_found && def_color) {
+ if (color_parse(def_color, parsed_color) < 0)
+ die(_("unable to parse default color value"));
+ }
fputs(parsed_color, stdout);
}
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index fda0f04..7ee86b3 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -671,7 +671,8 @@ static void populate_value(struct refinfo *ref)
} else if (starts_with(name, "color:")) {
char color[COLOR_MAXLEN] = "";
- color_parse(name + 6, "--format", color);
+ if (color_parse(name + 6, color) < 0)
+ die(_("unable to parse format"));
v->s = xstrdup(color);
continue;
} else if (!strcmp(name, "flag")) {
@@ -1004,7 +1005,8 @@ static void show_ref(struct refinfo *info, const char *format, int quote_style)
struct atom_value resetv;
char color[COLOR_MAXLEN] = "";
- color_parse("reset", "--format", color);
+ if (color_parse("reset", color) < 0)
+ die("BUG: couldn't parse 'reset' as a color");
resetv.s = color;
print_value(&resetv, quote_style);
}
diff --git a/color.c b/color.c
index f672885..7941e93 100644
--- a/color.c
+++ b/color.c
@@ -60,13 +60,12 @@ static int parse_attr(const char *name, int len)
return -1;
}
-void color_parse(const char *value, const char *var, char *dst)
+int color_parse(const char *value, char *dst)
{
- color_parse_mem(value, strlen(value), var, dst);
+ return color_parse_mem(value, strlen(value), dst);
}
-void color_parse_mem(const char *value, int value_len, const char *var,
- char *dst)
+int color_parse_mem(const char *value, int value_len, char *dst)
{
const char *ptr = value;
int len = value_len;
@@ -76,7 +75,7 @@ void color_parse_mem(const char *value, int value_len, const char *var,
if (!strncasecmp(value, "reset", len)) {
strcpy(dst, GIT_COLOR_RESET);
- return;
+ return 0;
}
/* [fg [bg]] [attr]... */
@@ -153,9 +152,9 @@ void color_parse_mem(const char *value, int value_len, const char *var,
*dst++ = 'm';
}
*dst = 0;
- return;
+ return 0;
- die("bad color value '%.*s' for variable '%s'", value_len, value, var);
+ return error(_("invalid color value: %.*s"), value_len, value);
}
int git_config_colorbool(const char *var, const char *value)
diff --git a/color.h b/color.h
index 9a8495b..f5beab1 100644
--- a/color.h
+++ b/color.h
@@ -77,8 +77,8 @@ int git_color_default_config(const char *var, const char *value, void *cb);
int git_config_colorbool(const char *var, const char *value);
int want_color(int var);
-void color_parse(const char *value, const char *var, char *dst);
-void color_parse_mem(const char *value, int len, const char *var, char *dst);
+int color_parse(const char *value, char *dst);
+int color_parse_mem(const char *value, int len, char *dst);
__attribute__((format (printf, 3, 4)))
int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
__attribute__((format (printf, 3, 4)))
diff --git a/diff.c b/diff.c
index d7a5c81..d1bd534 100644
--- a/diff.c
+++ b/diff.c
@@ -248,8 +248,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
return 0;
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, diff_colors[slot]);
- return 0;
+ return color_parse(value, diff_colors[slot]);
}
/* like GNU diff's --suppress-blank-empty option */
diff --git a/grep.c b/grep.c
index 99217dc..4dc31ea 100644
--- a/grep.c
+++ b/grep.c
@@ -111,7 +111,7 @@ int grep_config(const char *var, const char *value, void *cb)
if (color) {
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, color);
+ return color_parse(value, color);
}
return 0;
}
diff --git a/log-tree.c b/log-tree.c
index 877d976..7844f33 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -63,8 +63,7 @@ int parse_decorate_color_config(const char *var, const char *slot_name, const ch
return 0;
if (!value)
return config_error_nonbool(var);
- color_parse(value, var, decoration_colors[slot]);
- return 0;
+ return color_parse(value, decoration_colors[slot]);
}
/*
diff --git a/pretty.c b/pretty.c
index 31f2ede..59de1dc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -963,9 +963,8 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
return end - placeholder + 1;
begin += 5;
}
- color_parse_mem(begin,
- end - begin,
- "--pretty format", color);
+ if (color_parse_mem(begin, end - begin, color) < 0)
+ die(_("unable to parse --pretty format"));
strbuf_addstr(sb, color);
return end - placeholder + 1;
}

[Jeff King]

Post by Jeff King
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

[Junio C Hamano]

Post by Jeff King

Post by Jonathan Nieder

Post by René Scharfe
--- 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.

[Jeff King]

Continue where ae021d87 (use skip_prefix to avoid magic numbers) left=

off

and use skip_prefix() in more places for determining the lengths of p=

refix

strings to avoid using dependent constants and other indirect methods=

=2E

Thanks, these all look like nice improvements. I think both of the
suggestions made my Jonathan are good on top, though.

-Peff

[Junio C Hamano]

@@ -335,20 +337,18 @@ static int append_ref(const char *refname, cons=

t unsigned char *sha1, int flags,

static struct {
int kind;
const char *prefix;
- int pfxlen;
} ref_kind[] =3D {
- { REF_LOCAL_BRANCH, "refs/heads/", 11 },
- { REF_REMOTE_BRANCH, "refs/remotes/", 13 },
+ { REF_LOCAL_BRANCH, "refs/heads/" },
+ { REF_REMOTE_BRANCH, "refs/remotes/" },
};
=20
/* Detect kind */
for (i =3D 0; i < ARRAY_SIZE(ref_kind); i++) {
prefix =3D ref_kind[i].prefix;
- if (strncmp(refname, prefix, ref_kind[i].pfxlen))
- continue;
- kind =3D ref_kind[i].kind;
- refname +=3D ref_kind[i].pfxlen;
- break;
+ if (skip_prefix(refname, prefix, &refname)) {
+ kind =3D 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.

[René Scharfe]

Post by Junio C Hamano
=20

@@ -335,20 +337,18 @@ static int append_ref(const char *refname, con=

st unsigned char *sha1, int flags,

Post by Junio C Hamano

static struct {
int kind;
const char *prefix;
- int pfxlen;
} ref_kind[] =3D {
- { REF_LOCAL_BRANCH, "refs/heads/", 11 },
- { REF_REMOTE_BRANCH, "refs/remotes/", 13 },
+ { REF_LOCAL_BRANCH, "refs/heads/" },
+ { REF_REMOTE_BRANCH, "refs/remotes/" },
};
=20
/* Detect kind */
for (i =3D 0; i < ARRAY_SIZE(ref_kind); i++) {
prefix =3D ref_kind[i].prefix;
- if (strncmp(refname, prefix, ref_kind[i].pfxlen))
- continue;
- kind =3D ref_kind[i].kind;
- refname +=3D ref_kind[i].pfxlen;
- break;
+ if (skip_prefix(refname, prefix, &refname)) {
+ kind =3D ref_kind[i].kind;
+ break;
+ }

=20
This certainly makes it easier to read.
=20
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:

100000000x strncmp for refs/heads/x, which is a local branch

real 0m2.423s
user 0m2.420s
sys 0m0.000s
100000000x strncmp for refs/remotes/y, which is a remote branch

real 0m4.331s
user 0m4.328s
sys 0m0.000s
100000000x strncmp for refs/of/the/third/kind, which is no branch

real 0m3.878s
user 0m3.872s
sys 0m0.000s
100000000x skip_prefix for refs/heads/x, which is a local branch

real 0m0.891s
user 0m0.888s
sys 0m0.000s
100000000x skip_prefix for refs/remotes/y, which is a remote branch

real 0m1.345s
user 0m1.340s
sys 0m0.000s
100000000x skip_prefix for refs/of/the/third/kind, which is no branch

real 0m1.080s
user 0m1.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=C3=A9

---
.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 +=3D test-mergesort
TEST_PROGRAMS_NEED_X +=3D test-mktemp
TEST_PROGRAMS_NEED_X +=3D test-parse-options
TEST_PROGRAMS_NEED_X +=3D test-path-utils
+TEST_PROGRAMS_NEED_X +=3D test-prefix
TEST_PROGRAMS_NEED_X +=3D test-prio-queue
TEST_PROGRAMS_NEED_X +=3D test-read-cache
TEST_PROGRAMS_NEED_X +=3D test-regex
diff --git a/test-prefix.c b/test-prefix.c
new file mode 100644
index 0000000..ddc63af
--- /dev/null
+++ b/test-prefix.c
@@ -0,0 +1,87 @@
+#include "cache.h"
+
+#define ROUNDS 100000000
+
+#define REF_LOCAL_BRANCH 0x01
+#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[] =3D {
+ { REF_LOCAL_BRANCH, "refs/heads/" },
+ { REF_REMOTE_BRANCH, "refs/remotes/" },
+ };
+
+ for (i =3D 0; i < ARRAY_SIZE(ref_kind); i++) {
+ prefix =3D ref_kind[i].prefix;
+ if (skip_prefix(refname, prefix, &refname)) {
+ kind =3D ref_kind[i].kind;
+ break;
+ }
+ }
+ if (ARRAY_SIZE(ref_kind) <=3D 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[] =3D {
+ { REF_LOCAL_BRANCH, "refs/heads/", 11 },
+ { REF_REMOTE_BRANCH, "refs/remotes/", 13 },
+ };
+
+ for (i =3D 0; i < ARRAY_SIZE(ref_kind); i++) {
+ prefix =3D ref_kind[i].prefix;
+ if (strncmp(refname, prefix, ref_kind[i].pfxlen))
+ continue;
+ kind =3D ref_kind[i].kind;
+ refname +=3D ref_kind[i].pfxlen;
+ break;
+ }
+ if (ARRAY_SIZE(ref_kind) <=3D i)
+ return 0;
+ return kind;
+}
+
+int main(int argc, char **argv)
+{
+ if (argc =3D=3D 3) {
+ int (*fn)(const char *) =3D NULL;
+ printf("%dx %s for %s, which is ", ROUNDS, argv[1], argv[2]);
+ if (!strcmp(argv[1], "skip_prefix"))
+ fn =3D test_skip_prefix;
+ if (!strcmp(argv[1], "strncmp"))
+ fn =3D test_strncmp;
+ if (fn) {
+ int i, kind =3D 0;
+ for (i =3D 0; i < ROUNDS; i++)
+ kind |=3D fn(argv[2]);
+ switch (kind) {
+ case 0:
+ puts("no branch");
+ break;
+ case REF_LOCAL_BRANCH:
+ puts("a local branch");
+ break;
+ case REF_REMOTE_BRANCH:
+ puts("a remote branch");
+ break;
+ default:
+ puts("invalid");
+ }
+ }
+ }
+ return 0;
+}
--=20
2.1.2

[Junio C Hamano]

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. :)

;-)