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

[Jonathan Nieder <jrnieder@gmail.com>]

René Scharfe wrote:

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

Sounds promising.

[...]
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int ofs)
>  
>  static int git_branch_config(const char *var, const char *value, void *cb)
>  {
> +	const char *slot_name;
> +
>  	if (starts_with(var, "column."))
>  		return git_column_config(var, value, "branch", &colopts);
>  	if (!strcmp(var, "color.branch")) {
>  		branch_use_color = git_config_colorbool(var, value);
>  		return 0;
>  	}
> -	if (starts_with(var, "color.branch.")) {
> -		int slot = parse_branch_color_slot(var, 13);
> +	if (skip_prefix(var, "color.branch.", &slot_name)) {
> +		int slot = parse_branch_color_slot(var, slot_name - var);

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

At first glance I thought this should be

		int slot = parse_branch_color_slot(slot_name, 0);

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

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

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

[...]
> --- a/pretty.c
> +++ b/pretty.c
[...]
> @@ -809,18 +808,19 @@ static void parse_commit_header(struct format_commit_context *context)
>  	int i;
>  
>  	for (i = 0; msg[i]; i++) {
> +		const char *name;

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

[...]
> @@ -1522,7 +1518,7 @@ static void pp_header(struct pretty_print_context *pp,
>  	int parents_shown = 0;
>  
>  	for (;;) {
> -		const char *line = *msg_p;
> +		const char *name, *line = *msg_p;

Likewise.

With or without the changes below,
Reviewed-by: Jonathan Nieder <jrnieder@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 = STRING_LIST_INIT_DUP;
 static unsigned int colopts;
 
-static int parse_branch_color_slot(const char *var, int ofs)
+static int parse_branch_color_slot(const char *slot)
 {
-	if (!strcasecmp(var+ofs, "plain"))
+	if (!strcasecmp(slot, "plain"))
 		return BRANCH_COLOR_PLAIN;
-	if (!strcasecmp(var+ofs, "reset"))
+	if (!strcasecmp(slot, "reset"))
 		return BRANCH_COLOR_RESET;
-	if (!strcasecmp(var+ofs, "remote"))
+	if (!strcasecmp(slot, "remote"))
 		return BRANCH_COLOR_REMOTE;
-	if (!strcasecmp(var+ofs, "local"))
+	if (!strcasecmp(slot, "local"))
 		return BRANCH_COLOR_LOCAL;
-	if (!strcasecmp(var+ofs, "current"))
+	if (!strcasecmp(slot, "current"))
 		return BRANCH_COLOR_CURRENT;
-	if (!strcasecmp(var+ofs, "upstream"))
+	if (!strcasecmp(slot, "upstream"))
 		return BRANCH_COLOR_UPSTREAM;
 	return -1;
 }
@@ -90,7 +90,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (skip_prefix(var, "color.branch.", &slot_name)) {
-		int slot = parse_branch_color_slot(var, slot_name - var);
+		int slot = parse_branch_color_slot(slot_name);
 		if (slot < 0)
 			return 0;
 		if (!value)
diff --git i/builtin/log.c w/builtin/log.c
index 1202eba..68d5d30 100644
--- i/builtin/log.c
+++ w/builtin/log.c
@@ -391,7 +391,7 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (skip_prefix(var, "color.decorate.", &slot_name))
-		return parse_decorate_color_config(var, slot_name - var, value);
+		return parse_decorate_color_config(var, slot_name, value);
 	if (!strcmp(var, "log.mailmap")) {
 		use_mailmap_config = git_config_bool(var, value);
 		return 0;
diff --git i/log-tree.c w/log-tree.c
index cff7ac1..6402c19 100644
--- i/log-tree.c
+++ w/log-tree.c
@@ -56,9 +56,9 @@ static int parse_decorate_color_slot(const char *slot)
 	return -1;
 }
 
-int parse_decorate_color_config(const char *var, const int ofs, const char *value)
+int parse_decorate_color_config(const char *var, const char *slot_name, const char *value)
 {
-	int slot = parse_decorate_color_slot(var + ofs);
+	int slot = parse_decorate_color_slot(slot_name);
 	if (slot < 0)
 		return 0;
 	if (!value)
diff --git i/log-tree.h w/log-tree.h
index b26160c..d637b04 100644
--- i/log-tree.h
+++ w/log-tree.h
@@ -7,7 +7,8 @@ struct log_info {
 	struct commit *commit, *parent;
 };
 
-int parse_decorate_color_config(const char *var, const int ofs, const char *value);
+int parse_decorate_color_config(const char *var, const char *slot_name,
+				const char *value);
 void init_log_tree_opt(struct rev_info *);
 int log_tree_diff_flush(struct rev_info *);
 int log_tree_commit(struct rev_info *, struct commit *);
diff --git i/pretty.c w/pretty.c
index a181ac6..e2c59ef 100644
--- i/pretty.c
+++ w/pretty.c
@@ -808,19 +808,19 @@ static void parse_commit_header(struct format_commit_context *context)
 	int i;
 
 	for (i = 0; msg[i]; i++) {
-		const char *name;
+		const char *ident;
 		int eol;
 		for (eol = i; msg[eol] && msg[eol] != '\n'; eol++)
 			; /* do nothing */
 
 		if (i == eol) {
 			break;
-		} else if (skip_prefix(msg + i, "author ", &name)) {
-			context->author.off = name - msg;
-			context->author.len = msg + eol - name;
-		} else if (skip_prefix(msg + i, "committer ", &name)) {
-			context->committer.off = name - msg;
-			context->committer.len = msg + eol - name;
+		} else if (skip_prefix(msg + i, "author ", &ident)) {
+			context->author.off = ident - msg;
+			context->author.len = msg + eol - ident;
+		} else if (skip_prefix(msg + i, "committer ", &ident)) {
+			context->committer.off = ident - msg;
+			context->committer.len = msg + eol - ident;
 		}
 		i = eol;
 	}
@@ -1518,7 +1518,7 @@ static void pp_header(struct pretty_print_context *pp,
 	int parents_shown = 0;
 
 	for (;;) {
-		const char *name, *line = *msg_p;
+		const char *ident, *line = *msg_p;
 		int linelen = get_one_line(*msg_p);
 
 		if (!linelen)
@@ -1553,14 +1553,14 @@ static void pp_header(struct pretty_print_context *pp,
 		 * FULL shows both authors but not dates.
 		 * FULLER shows both authors and dates.
 		 */
-		if (skip_prefix(line, "author ", &name)) {
+		if (skip_prefix(line, "author ", &ident)) {
 			strbuf_grow(sb, linelen + 80);
-			pp_user_info(pp, "Author", sb, name, encoding);
+			pp_user_info(pp, "Author", sb, ident, encoding);
 		}
-		if (skip_prefix(line, "committer ", &name) &&
+		if (skip_prefix(line, "committer ", &ident) &&
 		    (pp->fmt == CMIT_FMT_FULL || pp->fmt == CMIT_FMT_FULLER)) {
 			strbuf_grow(sb, linelen + 80);
-			pp_user_info(pp, "Commit", sb, name, encoding);
+			pp_user_info(pp, "Commit", sb, ident, encoding);
 		}
 	}
 }