From 8852117a603c5ed5131233a80453db37c0958871 Mon Sep 17 00:00:00 2001
From: Jonathan Nieder <jrnieder@gmail.com>
Date: Tue, 7 Oct 2014 15:16:57 -0400
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: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 0591b22a48..b2e1895ca9 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 5ed60364ce..5a8a29e075 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1238,22 +1238,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;
 }
@@ -1291,7 +1290,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 4389722b4b..4c5fc4bff5 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 95e9b1da25..479b1d2a5b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -66,9 +66,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 d6ecd4dc46..8cbefac573 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 *);
-- 
cgit v1.2.3


From f6c5a2968c103621adf6928a29e4895361eaa23b Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Tue, 7 Oct 2014 15:33:09 -0400
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@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 b2e1895ca9..0a8ed9d22a 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 1032563e5f..035ea391a2 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 5a8a29e075..8dab44d933 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1295,8 +1295,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 7bba516383..842809b0b3 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 47bd624696..d41920d382 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -673,7 +673,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")) {
@@ -1007,7 +1008,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 f672885b71..7941e932d2 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 9a8495bb7f..f5beab1ed7 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 867f034b8f..4493dde749 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 99217dc04f..4dc31ea386 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 479b1d2a5b..a21ef30d0c 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -73,8 +73,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 31fc76b2fd..6182ca9aed 100644
--- a/pretty.c
+++ b/pretty.c
@@ -979,9 +979,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;
 	}
-- 
cgit v1.2.3