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

From: Jeff King <peff@peff.net>
Date: Oct 10, 2016, 18:59 UTC
Message-ID: <20161010185935.r3wnfglpi7hjjpdk@sigill.intra.peff.net>
In-Reply-To: <xmqq1szof34x.fsf@gitster.mtv.corp.google.com>

On Mon, Oct 10, 2016 at 11:52:14AM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> > +  By default, colors are shown only when enabled for log output (by
> > +  `color.diff`, `color.ui`, or `--color`, and respecting the `auto`
> > +  settings of the former if we are going to a terminal). `%C(auto,...)`
> > +  is accepted as a historical synonym for the default. Specifying
> > +  `%C(always,...) will show the colors always, even when colors are not
> > +  otherwise enabled (to enable this behavior for the whole format, use
> > +  `--color=always`).
> 
> It is not just "for the whole format", but also affects other parts
> of the output, no?  I am thinking about "git log -p --format=...".

True. I consider that a feature and more likely to be pointing the user in the right direction, but it should probably be s/format/output/ in the last sentence.

Show 18 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;
> >  		}
> 
> As a way to say "when color is not enabled, ignore everything unless
> it begins with 'always,'", this was a bit hard to read.  Perhaps an
> in-code comment is in order?

I'll see what I can do. The most readable one is probably:

  if (skip_prefix(begin, "auto,", &begin)) {
	if (!want_color(c->pretty_ctx->color))
		return end - placeholder + 1;
  } else if (skip_prefix(begin, "always,", &begin)) {
	/* nothing to do; we do not respect want_color at all */
  } else {
	/* the default is now the same as "auto" */
	if (!want_color(c->pretty_ctx->color))
		return end - placeholder + 1;
  }

I avoided that because of the repetition, but it probably is not too bad.

Show 11 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);
> 
> Hmm.  If we are in "no I do not want color" mode and "always,red"
> was given, we countermanded !want_color() check up above and come
> here.  Then we check want_color() again and refuse to paint it red?
> 
> I must be reading the patch incorrectly, but I cannot quite tell
> where I want astray...

No, we don't come here from %C() at all. This is for bare "%Cred", which cannot have "always" (or "auto"), as there is no syntactic spot for it. It is mostly historical (it _only_ supports red, green, and blue). We could actually leave this as-is to show the colors unconditionally. I changed it to keep the new behavior consistent, but I doubt anybody cares much either way.

-Peff