Skip to contentSkip to conversation

use skip_prefix() to avoid more magic numbers

PATCH In review René Scharfe 4 participants 13 messages Jun 15, 2016 Kbuild

lore
Share & tools
lore

René Scharfe

author
#0 lore raw

PATCH 1/1 use skip_prefix() to avoid more magic numbers

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

Signed-off-by Rene Scharfe <[email protected]>
---
 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(-)
builtin/apply.c +1 -1
index 8714a88..97f7e8e 100644
@@ -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
builtin/branch.c +13 -16
index 9e4666f..6785097 100644
@@ -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);
builtin/cat-file.c +3 -2
index 7073304..f8d8129 100644
@@ -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
builtin/checkout.c +2 -4
index 8afdf2b..cef1996 100644
@@ -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"));
builtin/clean.c +4 -3
index 3beeea6..c35505e 100644
@@ -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)
builtin/commit.c +9 -9
index b0fe784..cff7802 100644
@@ -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;
builtin/get-tar-commit-id.c +3 -2
index aa72596..6f4147a 100644
@@ -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");
builtin/log.c +4 -2
index 2fb34c7..1202eba 100644
@@ -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;
builtin/remote-ext.c +4 -6
index d699d28..3b8c22c 100644
@@ -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);
pretty.c +26 -30

123 lines not on this page · raw message

Signature · 1 line
2.1.2
Reviewed-by on v1 1/1
#1 lore raw
Quoted 3 lines · 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.

Sounds promising.

[...]

Quoted 8 lines
builtin/branch.c +2 -0
@@ -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;
+

builtin/branch.c static int parse_branch_color_slot(const char *var, int ofs) · line 93

Quoted 10 lines
builtin/branch.c +2 -2
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "branch", &colopts);
 	if (!strcmp(var, "color.branch")) {
 		branch_use_color = git_config_colorbool(var, value);
 		return 0;
 	}
-	if (starts_with(var, "color.branch.")) {
-		int slot = parse_branch_color_slot(var, 13);
+	if (skip_prefix(var, "color.branch.", &slot_name)) {
+		int slot = parse_branch_color_slot(var, slot_name - var);

I wonder why the separate var and offset parameters exist in the first place. Wouldn't taking a single const char * so the old code could use 'var + 13' instead of 'var, 13' have worked?

At first glance I thought this should be

		int slot = parse_branch_color_slot(slot_name, 0);

to be simpler. At second glance, how about something like the following on top?

[...]

Quoted 2 lines
builtin/log.c +0 -0

[...]

builtin/log.c static int git_log_config(const char *var, const char *value, void *cb) · line 394

Quoted 8 lines
builtin/log.c +2 -2
@@ -388,8 +390,8 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		default_show_root = git_config_bool(var, value);
 		return 0;
 	}
-	if (starts_with(var, "color.decorate."))
-		return parse_decorate_color_config(var, 15, value);
+	if (skip_prefix(var, "color.decorate.", &slot_name))
+		return parse_decorate_color_config(var, slot_name - var, value);

Likewise: the offset-based API seems odd now here, too.

[...]

Quoted 2 lines
pretty.c +0 -0

[...]

pretty.c static void parse_commit_header(struct format_commit_context *context) · line 811

Quoted 5 lines
pretty.c +1 -0
@@ -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)

[...]

pretty.c static void pp_header(struct pretty_print_context *pp, · line 1521

Quoted 6 lines
pretty.c +1 -1
@@ -1522,7 +1518,7 @@ static void pp_header(struct pretty_print_context *pp,
 	int parents_shown = 0;
 
 	for (;;) {
-		const char *line = *msg_p;
+		const char *name, *line = *msg_p;

Likewise.

With or without the changes below,

Reviewed-by Jonathan Nieder <[email protected]>
builtin/branch.c +8 -8
index 6785097..022a88e 100644
@@ -62,19 +62,19 @@ static unsigned char merge_filter_ref[20];
 static struct string_list output = STRING_LIST_INIT_DUP;
 static unsigned int colopts;
 
-static int parse_branch_color_slot(const char *var, int ofs)
+static int parse_branch_color_slot(const char *slot)
 {
-	if (!strcasecmp(var+ofs, "plain"))
+	if (!strcasecmp(slot, "plain"))
 		return BRANCH_COLOR_PLAIN;
-	if (!strcasecmp(var+ofs, "reset"))
+	if (!strcasecmp(slot, "reset"))
 		return BRANCH_COLOR_RESET;
-	if (!strcasecmp(var+ofs, "remote"))
+	if (!strcasecmp(slot, "remote"))
 		return BRANCH_COLOR_REMOTE;
-	if (!strcasecmp(var+ofs, "local"))
+	if (!strcasecmp(slot, "local"))
 		return BRANCH_COLOR_LOCAL;
-	if (!strcasecmp(var+ofs, "current"))
+	if (!strcasecmp(slot, "current"))
 		return BRANCH_COLOR_CURRENT;
-	if (!strcasecmp(var+ofs, "upstream"))
+	if (!strcasecmp(slot, "upstream"))
 		return BRANCH_COLOR_UPSTREAM;
 	return -1;
 }

@@ -90,7 +90,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (skip_prefix(var, "color.branch.", &slot_name)) {
-		int slot = parse_branch_color_slot(var, slot_name - var);
+		int slot = parse_branch_color_slot(slot_name);
 		if (slot < 0)
 			return 0;
 		if (!value)
builtin/log.c +1 -1
index 1202eba..68d5d30 100644
@@ -391,7 +391,7 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (skip_prefix(var, "color.decorate.", &slot_name))
-		return parse_decorate_color_config(var, slot_name - var, value);
+		return parse_decorate_color_config(var, slot_name, value);
 	if (!strcmp(var, "log.mailmap")) {
 		use_mailmap_config = git_config_bool(var, value);
 		return 0;
log-tree.c +2 -2
index cff7ac1..6402c19 100644
@@ -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)
log-tree.h +2 -1
index b26160c..d637b04 100644
@@ -7,7 +7,8 @@ struct log_info {
 	struct commit *commit, *parent;
 };
 
-int parse_decorate_color_config(const char *var, const int ofs, const char *value);
+int parse_decorate_color_config(const char *var, const char *slot_name,
+				const char *value);
 void init_log_tree_opt(struct rev_info *);
 int log_tree_diff_flush(struct rev_info *);
 int log_tree_commit(struct rev_info *, struct commit *);
pretty.c +12 -12
index a181ac6..e2c59ef 100644
@@ -808,19 +808,19 @@ static void parse_commit_header(struct format_commit_context *context)
 	int i;
 
 	for (i = 0; msg[i]; i++) {
-		const char *name;
+		const char *ident;
 		int eol;
 		for (eol = i; msg[eol] && msg[eol] != '\n'; eol++)
 			; /* do nothing */
 
 		if (i == eol) {
 			break;
-		} else if (skip_prefix(msg + i, "author ", &name)) {
-			context->author.off = name - msg;
-			context->author.len = msg + eol - name;
-		} else if (skip_prefix(msg + i, "committer ", &name)) {
-			context->committer.off = name - msg;
-			context->committer.len = msg + eol - name;
+		} else if (skip_prefix(msg + i, "author ", &ident)) {
+			context->author.off = ident - msg;
+			context->author.len = msg + eol - ident;
+		} else if (skip_prefix(msg + i, "committer ", &ident)) {
+			context->committer.off = ident - msg;
+			context->committer.len = msg + eol - ident;
 		}
 		i = eol;
 	}

@@ -1518,7 +1518,7 @@ static void pp_header(struct pretty_print_context *pp,
 	int parents_shown = 0;
 
 	for (;;) {
-		const char *name, *line = *msg_p;
+		const char *ident, *line = *msg_p;
 		int linelen = get_one_line(*msg_p);
 
 		if (!linelen)

@@ -1553,14 +1553,14 @@ static void pp_header(struct pretty_print_context *pp,
 		 * FULL shows both authors but not dates.
 		 * FULLER shows both authors and dates.
 		 */
-		if (skip_prefix(line, "author ", &name)) {
+		if (skip_prefix(line, "author ", &ident)) {
 			strbuf_grow(sb, linelen + 80);
-			pp_user_info(pp, "Author", sb, name, encoding);
+			pp_user_info(pp, "Author", sb, ident, encoding);
 		}
-		if (skip_prefix(line, "committer ", &name) &&
+		if (skip_prefix(line, "committer ", &ident) &&
 		    (pp->fmt == CMIT_FMT_FULL || pp->fmt == CMIT_FMT_FULLER)) {
 			strbuf_grow(sb, linelen + 80);
-			pp_user_info(pp, "Commit", sb, name, encoding);
+			pp_user_info(pp, "Commit", sb, ident, encoding);
 		}
 	}
 }
#2 lore raw
Quoted 14 lines · Jonathan Nieder
Quoted 14 lines
builtin/branch.c +2 -0
@@ -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;
 	}
Quoted 8 lines

builtin/branch.c static int parse_branch_color_slot(const char *var, int ofs) · line 93

Quoted 4 lines
builtin/branch.c +2 -2
-	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).

Quoted 8 lines

builtin/branch.c static void parse_commit_header(struct format_commit_context *context) · line 811

Quoted 5 lines
builtin/branch.c +1 -0
@@ -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

#3 lore raw
Quoted 25 lines · Jeff King
Quoted 14 lines · Jonathan Nieder
Quoted 14 lines
builtin/branch.c +2 -0
@@ -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;
 	}
Quoted 8 lines

builtin/branch.c static int parse_branch_color_slot(const char *var, int ofs) · line 93

Quoted 4 lines
builtin/branch.c +2 -2
-	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?

Quoted 8 lines

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.

#4 lore raw
Quoted 5 lines · Junio C Hamano
Quoted 3 lines

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

#5 lore raw
In reply to Jeff King
Quoted 25 lines · Jeff King
Quoted 14 lines · Jonathan Nieder
Quoted 14 lines
builtin/branch.c +2 -0
@@ -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;
 	}
Quoted 8 lines

builtin/branch.c static int parse_branch_color_slot(const char *var, int ofs) · line 93

Quoted 4 lines
builtin/branch.c +2 -2
-	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?

Quoted 8 lines

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

Quoted 14 lines
Quoted 10 lines

builtin/branch.c static void parse_commit_header(struct · line 812

Quoted 7 lines
builtin/branch.c +1 -0
@@ -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

#6 lore raw
Quoted 6 lines · Junio C Hamano
Quoted 3 lines

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 <[email protected]>
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 <[email protected]>

---
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(-)
builtin/branch.c +8 -8
index 9e4666f..2c97141 100644
@@ -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)
builtin/commit.c +9 -10
index b0fe784..c45613a 100644
@@ -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)
builtin/log.c +1 -1
index 2fb34c7..9c75447 100644
@@ -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;
log-tree.c +2 -2
index bcee7c5..877d976 100644
@@ -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)
log-tree.h +1 -1
index d6ecd4d..8cbefac 100644
@@ -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 *);
Signature · 1 line
2.1.1.566.gdb1f904
#7 lore raw
Quoted 4 lines · 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 <[email protected]>

---
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(-)
builtin/branch.c +1 -2
index 2c97141..35c786d 100644
@@ -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);
 }
builtin/clean.c +1 -2
index 3beeea6..a7e7b0b 100644
@@ -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")) {
builtin/commit.c +1 -2
index c45613a..407566d 100644
@@ -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);
builtin/config.c +6 -3
index 37305e9..8cc2604 100644
@@ -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);
 }
builtin/for-each-ref.c +4 -2
index fda0f04..7ee86b3 100644
@@ -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);
 	}
color.c +6 -7
index f672885..7941e93 100644
@@ -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)
color.h +2 -2
index 9a8495b..f5beab1 100644
@@ -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.c +1 -2
index d7a5c81..d1bd534 100644
@@ -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  */
grep.c +1 -1
index 99217dc..4dc31ea 100644
@@ -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;
 }
log-tree.c +1 -2
index 877d976..7844f33 100644
@@ -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]);
 }
 
 /*
pretty.c +2 -3
index 31f2ede..59de1dc 100644
@@ -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;
 	}
Signature · 1 line
2.1.1.566.gdb1f904
#8 lore raw
Quoted 6 lines · 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 like:

Quoted 7 lines
  $ 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.

Quoted 221 lines
 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(-)
builtin/branch.c +1 -2
index 2c97141..35c786d 100644
@@ -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);
 }
builtin/clean.c +1 -2
index 3beeea6..a7e7b0b 100644
@@ -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")) {
builtin/commit.c +1 -2
index c45613a..407566d 100644
@@ -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);
builtin/config.c +6 -3
index 37305e9..8cc2604 100644
@@ -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);
 }
builtin/for-each-ref.c +4 -2
index fda0f04..7ee86b3 100644
@@ -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);
 	}
color.c +6 -7
index f672885..7941e93 100644
@@ -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)
color.h +2 -2
index 9a8495b..f5beab1 100644
@@ -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.c +1 -2
index d7a5c81..d1bd534 100644
@@ -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  */
grep.c +1 -1
index 99217dc..4dc31ea 100644
@@ -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;
 }
log-tree.c +1 -2
index 877d976..7844f33 100644
@@ -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]);
 }
 
 /*
pretty.c +2 -3
index 31f2ede..59de1dc 100644
@@ -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;
 	}
#9 lore raw
In reply to René Scharfe
Quoted 3 lines · 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.

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

-Peff

#10 lore raw
In reply to René Scharfe
Quoted 14 lines · René Scharfe
builtin/branch.c +2 -3
@@ -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++) {

builtin/branch.c static int append_ref(const char *refname, const unsigned char *sha1, int flags, · line 351

Quoted 10 lines
builtin/branch.c +4 -5
 		prefix = ref_kind[i].prefix;
-		if (strncmp(refname, prefix, ref_kind[i].pfxlen))
-			continue;
-		kind = ref_kind[i].kind;
-		refname += ref_kind[i].pfxlen;
-		break;
+		if (skip_prefix(refname, prefix, &refname)) {
+			kind = ref_kind[i].kind;
+			break;
+		}

This certainly makes it easier to read.

I suspect that the original was done as a (possibly premature) optimization to avoid having to do strlen(3) on a variable that points at constant strings for each and every ref we iterate with for_each_rawref(), and it is somewhat sad to see it lost because skip_prefix() assumes that the caller never knows the length of the prefix, though.

Thanks.

author
#11 lore raw

Am 07.10.2014 um 20:23 schrieb Junio C Hamano:

Quoted 29 lines
Quoted 14 lines · René Scharfe
(quoted diff) +2 -3
@@ -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++) {

Review static int append_ref(const char *refname, const unsigned char *sha1, int flags, · line 351

Quoted 10 lines
(quoted diff) +4 -5
  		prefix = ref_kind[i].prefix;
-		if (strncmp(refname, prefix, ref_kind[i].pfxlen))
-			continue;
-		kind = ref_kind[i].kind;
-		refname += ref_kind[i].pfxlen;
-		break;
+		if (skip_prefix(refname, prefix, &refname)) {
+			kind = ref_kind[i].kind;
+			break;
+		}

This certainly makes it easier to read.

Quoted 6 lines

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é

---
 .gitignore    |  1 +
 Makefile      |  1 +
 test-prefix.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+)
 create mode 100644 test-prefix.c
.gitignore +1 -0
index 5bfb234..8416c5e 100644
@@ -193,6 +193,7 @@
 /test-mktemp
 /test-parse-options
 /test-path-utils
+/test-prefix
 /test-prio-queue
 /test-read-cache
 /test-regex
Makefile +1 -0
index f34a2d4..c09b59e 100644
@@ -561,6 +561,7 @@ TEST_PROGRAMS_NEED_X += test-mergesort
 TEST_PROGRAMS_NEED_X += test-mktemp
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-prefix
 TEST_PROGRAMS_NEED_X += test-prio-queue
 TEST_PROGRAMS_NEED_X += test-read-cache
 TEST_PROGRAMS_NEED_X += test-regex
test-prefix.c new file +87 -0

87 lines not on this page · raw message

Signature · 1 line
2.1.2
#12 lore raw
Quoted 2 lines · René Scharfe

I didn't think much about the performance implications. skip_prefix() doesn't call strlen(3), though.

Ah, OK.

Quoted 3 lines

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

;-)