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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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

>
>  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;
>  	}