Re: Git's config core.pager doesn't respect color.pager

[Jeff King <peff@peff.net>]

On Tue, Jul 22, 2008 at 10:10:17PM -0400, Benjamin Kudria wrote:

> [core]
> 	pager = tig
> 
> [color]
> 	diff = auto
> 	pager = false
[...]
> So, with the above config, when I do:
> 
> git diff | tig
> 
> Everything works correctly - git sends no color codes, because of
> color.pager = false.

Actually, it works because of color.diff = auto. Stdout is not a tty,
therefore color does not kick in. The point of color.pager is to say
"since git started a pager on behalf of the user, the tty test is
pointless. What we really want to know is whether the pager can handle
colors."

> However, if I do just:
> 
> git diff
> 
> git uses core.pager to display the output, but still sends color
> codes, which is OK for, say, less, bit not so good for tig, which does
> it's own colorizing, and displays the color codes git sends as-is.

And this is a bug, but one that only affects "diff". It's an ordering
problem in looking at the config and starting the pager (diff is unlike
most other commands in that we cannot decide immediately if we want the
pager, in the case that --exit-code is used). So we make the color
decision before we have started the pager.

Unfortunately, it is not quite as simple as just flipping the order.
Starting the pager depends on knowing that we are using --exit-code,
which depends on doing the diff options parsing and setup, which depends
on reading the config, which then depends on the pager setup.

The patch below should fix it, but it really leaves a bad taste in my
mouth. However, breaking the dependency chain would require some pretty
major surgery, I think. It is a little worrisome to me that
git_config_colorbool depends on the value of pager_use_color, another
config variable, at all; that means that ordering in the config file
could change the outcome. It happens to work because we read the config
several times, and we pick up pager.color on a previous read. I think
the "right" solution would be refactoring the color stuff to make the
decision closer to the point of use. But that is definitely not -rc
material, so perhaps this should go in, ugly as it is.

---
 builtin-diff.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/builtin-diff.c b/builtin-diff.c
index 7ffea97..564984e 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -301,6 +301,21 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		setup_pager();
 
 	/*
+	 * Special case: we have already examined the config for
+	 * color, but we didn't know at that point whether we were
+	 * going to start a pager. The only case that we care about is
+	 * when we turned on color, but shouldn't have (we will never
+	 * "should have but didn't" because of the pager, since
+	 * a lack of a pager implies either the tty is stdout, in
+	 * which case we do turn on color, or it is not, in which
+	 * case we don't start a pager).
+	 */
+	if (DIFF_OPT_TST(&rev.diffopt, COLOR_DIFF) &&
+	    pager_in_use() &&
+	    !pager_use_color)
+		DIFF_OPT_CLR(&rev.diffopt, COLOR_DIFF);
+
+	/*
 	 * Do we have --cached and not have a pending object, then
 	 * default to HEAD by hand.  Eek.
 	 */
-- 
1.6.0.rc1.155.gd3310.dirty