Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Feb 27, 2010, 11:43 UTC
Message-ID: <4B890572.5040604@lsrfire.ath.cx>
In-Reply-To: <1267246670-19118-5-git-send-email-lodatom@gmail.com>

Am 27.02.2010 05:57, schrieb Mark Lodato:

Show 11 quoted lines

> Colorize the filename, line number, and separator in git grep output, as
> GNU grep does.  The colors are customizable through color.grep.<slot>.
> The default is to only color the separator (in cyan), since this gives
> the biggest legibility increase without overwhelming the user with
> colors.  GNU grep also defaults cyan for the separator, but defaults to
> magenta for the filename and to green for the line number, as well.
> 
> There are a few differences from GNU grep:
> 
> 1. With --name-only, GNU grep colors the filenames, but we do not.  I do
>    not see any point to making everything the same color.

I guess they did it for consistency, so when you see "magenta" you think "filename", and because it can be turned off with a switch. With your patch all filenames are coloured the same, too, by the way: using the default foreground colour. :)

Show 40 quoted lines

> diff --git a/builtin-grep.c b/builtin-grep.c
> index dcc3d48..43b952b 100644
> --- a/builtin-grep.c
> +++ b/builtin-grep.c
> @@ -289,6 +289,7 @@ static int wait_all(void)
>  static int grep_config(const char *var, const char *value, void *cb)
>  {
>  	struct grep_opt *opt = cb;
> +	char *color = NULL;
>  
>  	switch (userdiff_config(var, value)) {
>  	case 0: break;
> @@ -296,17 +297,24 @@ static int grep_config(const char *var, const char *value, void *cb)
>  	default: return 0;
>  	}
>  
> -	if (!strcmp(var, "color.grep")) {
> +	if (!strcmp(var, "color.grep"))
>  		opt->color = git_config_colorbool(var, value, -1);
> -		return 0;
> -	}
> -	if (!strcmp(var, "color.grep.match")) {
> -		if (!value)
> -			return config_error_nonbool(var);
> -		color_parse(value, var, opt->color_match);
> -		return 0;
> -	}
> -	return git_color_default_config(var, value, cb);
> +	else if (!strcmp(var, "color.grep.filename"))
> +		color = opt->color_filename;
> +	else if (!strcmp(var, "color.grep.linenumber"))
> +		color = opt->color_lineno;
> +	else if (!strcmp(var, "color.grep.match"))
> +		color = opt->color_match;
> +	else if (!strcmp(var, "color.grep.separator"))
> +		color = opt->color_sep;
> +	else
> +		return git_color_default_config(var, value, cb);
> +	if (!value)
> +		return config_error_nonbool(var);

color.grep without a value used to turn on colourization, now it seems to error out.

> +	color_parse(value, var, color);
> +	if (!strcmp(color, GIT_COLOR_RESET))
> +		color[0] = '\0';

This turns off colouring if the user specified "reset" as the colour, right? Interesting optimization, but is it really needed? Perhaps it's just me, but I'd give the user the requested "<reset>text<reset>" sequence if she asked for it, even if it's longer than and looks the same as "text" alone.

Show 17 quoted lines

> diff --git a/grep.c b/grep.c
> index a0864f1..132798d 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -506,35 +506,52 @@ static int next_match(struct grep_opt *opt, char *bol, char *eol,
>  	return hit;
>  }
>  
> +static void output_color(struct grep_opt *opt, const void *data, size_t size,
> +			 const char *color)
> +{
> +	if (opt->color && color && color[0]) {
> +		opt->output(opt, color, strlen(color));
> +		opt->output(opt, data, size);
> +		opt->output(opt, GIT_COLOR_RESET, strlen(GIT_COLOR_RESET));
> +	}
> +	else

	} else

Show 9 quoted lines

> +		opt->output(opt, data, size);
> +}
> +
> +static void output_sep(struct grep_opt *opt, char sign)
> +{
> +	if (opt->null_following_name) {
> +		sign = '\0';
> +		opt->output(opt, &sign, 1);
> +	} else

	if (opt->null_following_name)
		opt->output(opt, "", 1);
	else

> +		output_color(opt, &sign, 1, opt->color_sep);
> +}