Re: [PATCH v2 1/3] pretty: add conditional %C?colorname placeholders

From: Jeff King <peff@peff.net>
Date: Apr 26, 2010, 03:26 UTC
Message-ID: <20100426032642.GA14421@coredump.intra.peff.net>
In-Reply-To: <20100426021347.GA29669@progeny.tock>

On Sun, Apr 25, 2010 at 09:13:47PM -0500, Jonathan Nieder wrote:

Show 6 quoted lines

> This is nice because in certain situations (e.g. different background
> colors), the default colors might not be suitable.  As an example of
> this, the ‘commit ’ line of ‘git log’ output uses color.diff.commit.
> 
> So it would be nice to be able to use %C(diff.commit) and
> automatically use the right color, if color is enabled.

FWIW, I like this idea very much. And it would be a bit more natural for %C(diff.commit) to respect diff.color, whereas we could perhaps keep %Cred as "always on" for backwards compatibility.

However:

> Why not make %C always check?  I can understand that it would be
> annoying when first trying to use %C.  On the other hand, it would be

I am a little nervous that we would be breaking scripts that pipe the colorized output for later display to the user. Right now doing something like[1]:

  log=`git log --format=%Cred%h`
  test "x$log" != x && echo "foo: $log"

colorizes, but we would be breaking it. And for new values like %C(diff.commit), we are not breaking anything (because it is a new syntax), but a script like the one above may want to convert, and there is no way to say "respect the color _config_, but don't respect isatty(1)". Saying "--color" doesn't work, because it overrides the color config. We can cheat a little with GIT_PAGER_IN_USE, but that will have funny interactions with pager.color.

[1] Yes, I know this snippet is contrived, but it seems within the realm of possibility. git-add--interactive reads the diff output in both regular and color forms, though it takes some care to look at the user's config and decide whether to show the color.

-Peff