{"thread":{"id":"14611","subject":"Git's config core.pager doesn't respect color.pager","startedAt":"2008-07-23T02:10:17Z","lastAt":"2008-07-28T07:25:02Z","messageCount":2,"participants":["Benjamin Kudria","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"84435","messageId":"d129c0140807221910j37f40e72se9f411d6424077de@mail.gmail.com","threadId":"14611","inReplyTo":null,"subject":"Git's config core.pager doesn't respect color.pager","fromName":"Benjamin Kudria","fromEmail":"ben@kudria.net","sentAt":"2008-07-23T02:10:17Z","receivedAt":"2008-07-23T02:10:17Z","isPatch":false,"sender":{"key":"ben@kudria.net","avatar":null},"body":"I'm seeing a problem with git.  It's easier to demonstrate than\nexplain.  The relevant parts of my .gitconfig:\n\n[core]\n\tpager = tig\n\n[color]\n\tdiff = auto\n\tpager = false\n\ntig is a console-based git client that can also act as a pager,\ncolorizing git output.\n\nSo, with the above config, when I do:\n\ngit diff | tig\n\nEverything works correctly - git sends no color codes, because of\ncolor.pager = false.\n\nHowever, if I do just:\n\ngit diff\n\ngit uses core.pager to display the output, but still sends color\ncodes, which is OK for, say, less, bit not so good for tig, which does\nit's own colorizing, and displays the color codes git sends as-is.\n\nShouldn't core.pager respect color.pager, and not send the color codes?\n\nBenjamin Kudria\n\n-- \nhttp://ben.kudria.net | Jabber: ben@kudria.net\n"},{"id":"85260","messageId":"20080728072502.GA13040@sigill.intra.peff.net","threadId":"14611","inReplyTo":"d129c0140807221910j37f40e72se9f411d6424077de@mail.gmail.com","subject":"Re: Git's config core.pager doesn't respect color.pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-28T07:25:02Z","receivedAt":"2008-07-28T07:25:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 22, 2008 at 10:10:17PM -0400, Benjamin Kudria wrote:\n\n> [core]\n> \tpager = tig\n> \n> [color]\n> \tdiff = auto\n> \tpager = false\n[...]\n> So, with the above config, when I do:\n> \n> git diff | tig\n> \n> Everything works correctly - git sends no color codes, because of\n> color.pager = false.\n\nActually, it works because of color.diff = auto. Stdout is not a tty,\ntherefore color does not kick in. The point of color.pager is to say\n\"since git started a pager on behalf of the user, the tty test is\npointless. What we really want to know is whether the pager can handle\ncolors.\"\n\n> However, if I do just:\n> \n> git diff\n> \n> git uses core.pager to display the output, but still sends color\n> codes, which is OK for, say, less, bit not so good for tig, which does\n> it's own colorizing, and displays the color codes git sends as-is.\n\nAnd this is a bug, but one that only affects \"diff\". It's an ordering\nproblem in looking at the config and starting the pager (diff is unlike\nmost other commands in that we cannot decide immediately if we want the\npager, in the case that --exit-code is used). So we make the color\ndecision before we have started the pager.\n\nUnfortunately, it is not quite as simple as just flipping the order.\nStarting the pager depends on knowing that we are using --exit-code,\nwhich depends on doing the diff options parsing and setup, which depends\non reading the config, which then depends on the pager setup.\n\nThe patch below should fix it, but it really leaves a bad taste in my\nmouth. However, breaking the dependency chain would require some pretty\nmajor surgery, I think. It is a little worrisome to me that\ngit_config_colorbool depends on the value of pager_use_color, another\nconfig variable, at all; that means that ordering in the config file\ncould change the outcome. It happens to work because we read the config\nseveral times, and we pick up pager.color on a previous read. I think\nthe \"right\" solution would be refactoring the color stuff to make the\ndecision closer to the point of use. But that is definitely not -rc\nmaterial, so perhaps this should go in, ugly as it is.\n\n---\n builtin-diff.c |   15 +++++++++++++++\n 1 files changed, 15 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 7ffea97..564984e 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -301,6 +301,21 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tsetup_pager();\n \n \t/*\n+\t * Special case: we have already examined the config for\n+\t * color, but we didn't know at that point whether we were\n+\t * going to start a pager. The only case that we care about is\n+\t * when we turned on color, but shouldn't have (we will never\n+\t * \"should have but didn't\" because of the pager, since\n+\t * a lack of a pager implies either the tty is stdout, in\n+\t * which case we do turn on color, or it is not, in which\n+\t * case we don't start a pager).\n+\t */\n+\tif (DIFF_OPT_TST(&rev.diffopt, COLOR_DIFF) &&\n+\t    pager_in_use() &&\n+\t    !pager_use_color)\n+\t\tDIFF_OPT_CLR(&rev.diffopt, COLOR_DIFF);\n+\n+\t/*\n \t * Do we have --cached and not have a pending object, then\n \t * default to HEAD by hand.  Eek.\n \t */\n-- \n1.6.0.rc1.155.gd3310.dirty\n"}]}