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

From: Jeff King <peff@peff.net>
Date: Oct 10, 2016, 20:04 UTC
Message-ID: <20161010200457.6dzcrkhoi5id3z4x@sigill.intra.peff.net>
In-Reply-To: <19e59db7-f3dd-35ec-8cf1-b070b1c05abe@web.de>

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

Show 20 quoted lines

> > > Shouldn't we have an "else" here?
> > 
> > I'm not sure what you mean; can you write it out?
> 
> > -		if (skip_prefix(begin, "auto,", &begin)) {
> > +
> > +		if (!skip_prefix(begin, "always,", &begin)) {
> >  			if (!want_color(c->pretty_ctx->color))
> >  				return end - placeholder + 1;
> >  		}
> 
> 		else {	/* here */
> 
> > +		/* this is a historical noop */
> > +		skip_prefix(begin, "auto,", &begin);
> 
> 		}
> 
> Otherwise "always,auto," would be allowed and mean the same as "always,",
> which just seems wrong.  Not a biggie.

I don't think that will parse "%C(auto,foo)", as we hit the !skip_prefix() of the conditional, and do not look for "auto," at all.

I think you'd have to move the check for "auto," inside the if block.

I'm leaning towards just writing it out the long way, though, as I did in my reply to Junio.

Show 18 quoted lines

> > > 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).
> 
> OK.  A variable then?  Lazy pseudo-code:
> 
> 	if (RED)
> 		color = red;
> 	else if (GREEN)
> 		...
> 
> 	if (want_color())
> 		strbuf_addstr(sb, color);

Yeah, that is a bit more clear (the final conditional just needs to check that we actually found a color).

-Peff