Re: [PATCH] pretty: respect color settings for %C placeholders

From: Jeff King <peff@peff.net>
Date: Oct 10, 2016, 17:42 UTC
Message-ID: <20161010174257.b4uxplavefjyr6rl@sigill.intra.peff.net>
In-Reply-To: <42b3be14-fadb-4801-f377-59fb42b7ccdf@web.de>

On Mon, Oct 10, 2016 at 07:09:12PM +0200, René Scharfe wrote:

Show 16 quoted lines

> > diff --git a/pretty.c b/pretty.c
> > index 25efbca..73e58b5 100644
> > --- a/pretty.c
> > +++ b/pretty.c
> > @@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
> > 
> >  		if (!end)
> >  			return 0;
> > -		if (skip_prefix(begin, "auto,", &begin)) {
> > +
> > +		if (!skip_prefix(begin, "always,", &begin)) {
> >  			if (!want_color(c->pretty_ctx->color))
> >  				return end - placeholder + 1;
> >  		}
> 
> Shouldn't we have an "else" here?

I'm not sure what you mean; can you write it out?

Show 21 quoted lines

> > -	if (skip_prefix(placeholder + 1, "red", &rest))
> > +	if (skip_prefix(placeholder + 1, "red", &rest) &&
> > +	    want_color(c->pretty_ctx->color))
> >  		strbuf_addstr(sb, GIT_COLOR_RED);
> > -	else if (skip_prefix(placeholder + 1, "green", &rest))
> > +	else if (skip_prefix(placeholder + 1, "green", &rest) &&
> > +		 want_color(c->pretty_ctx->color))
> >  		strbuf_addstr(sb, GIT_COLOR_GREEN);
> > -	else if (skip_prefix(placeholder + 1, "blue", &rest))
> > +	else if (skip_prefix(placeholder + 1, "blue", &rest) &&
> > +		 want_color(c->pretty_ctx->color))
> >  		strbuf_addstr(sb, GIT_COLOR_BLUE);
> > -	else if (skip_prefix(placeholder + 1, "reset", &rest))
> > +	else if (skip_prefix(placeholder + 1, "reset", &rest) &&
> > +		 want_color(c->pretty_ctx->color))
> >  		strbuf_addstr(sb, GIT_COLOR_RESET);
> >  	return rest - placeholder;
> >  }
> 
> Perhaps it's a funtion like add_color(sb, ctx, color) or similar would be
> nice?

I actually wrote it that way first (I called it "maybe_add_color()"), but it felt a little funny to have a separate function that people might be tempted to reuse (the right solution is generally to check want_color() early as above, but we can't do that here because we have to find the end of each placeholder).

What I have here is a little funny, too, though, as it keeps trying other color names if it finds "red" but want_color() returns 0.

-Peff