{"thread":{"id":"55731","subject":"[PATCH] doc: explain the use of color.pager","startedAt":"2021-05-19T09:17:13Z","lastAt":"2021-05-20T15:05:33Z","messageCount":8,"participants":["Jeff King","Felipe Contreras","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"424924","messageId":"YKTXlTjwOUi4W+R8@coredump.intra.peff.net","threadId":"55731","inReplyTo":null,"subject":"[PATCH] doc: explain the use of color.pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-19T09:17:09Z","receivedAt":"2021-05-19T09:17:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The current documentation for color.pager is technically correct, but\nslightly misleading and doesn't really clarify the purpose of the\nvariable. As explained in the original thread which added it:\n\n https://lore.kernel.org/git/E1G6zPH-00062L-Je@moooo.ath.cx/\n\nthe point is deal with pagers that don't understand colors. And hence it\nbeing set to \"true\" is necessary for colorizing output to the pager, but\nnot sufficient by itself (you must also have enabled one of the other\ncolor options, though note that these are set to \"auto\" by default these\ndays).\n\nSigned-off-by: Jeff King \n---\nJust a loose end from:\n\n https://lore.kernel.org/git/xmqqlf8c2k2y.fsf@gitster.g/\n\nthat I think is worth tying up before we forget.\n\n Documentation/config/color.txt | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/color.txt b/Documentation/config/color.txt\nindex d5daacb13a..e05d520a86 100644\n--- a/Documentation/config/color.txt\n+++ b/Documentation/config/color.txt\n@@ -127,8 +127,9 @@ color.interactive.::\n \tinteractive commands.\n \n color.pager::\n-\tA boolean to enable/disable colored output when the pager is in\n-\tuse (default is true).\n+\tA boolean to specify whether `auto` color modes should colorize\n+\toutput going to the pager. Defaults to true; set this to false\n+\tif your pager does not understand ANSI color codes.\n \n color.push::\n \tA boolean to enable/disable color in push errors. May be set to\n-- \n2.32.0.rc0.424.g95d3bde94f\n"},{"id":"424936","messageId":"60a4eb6d58c05_86a8208af@natae.notmuch","threadId":"55731","inReplyTo":"YKTXlTjwOUi4W+R8@coredump.intra.peff.net","subject":"RE: [PATCH] doc: explain the use of color.pager","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-05-19T10:41:49Z","receivedAt":"2021-05-19T10:41:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt\n> index d5daacb13a..e05d520a86 100644\n> --- a/Documentation/config/color.txt\n> +++ b/Documentation/config/color.txt\n> @@ -127,8 +127,9 @@ color.interactive.::\n> \tinteractive commands.\n> \n> color.pager::\n> -\tA boolean to enable/disable colored output when the pager is in\n> -\tuse (default is true).\n> +\tA boolean to specify whether `auto` color modes should colorize\n> +\toutput going to the pager. Defaults to true; set this to false\n> +\tif your pager does not understand ANSI color codes.\n\nThis sounds correct to me, short, and complete. I like it.\n\nAcked-by: Felipe Contreras \n\n-- \nFelipe Contreras\n"},{"id":"424987","messageId":"YKVsw3uqb66ifzvd@google.com","threadId":"55731","inReplyTo":"YKTXlTjwOUi4W+R8@coredump.intra.peff.net","subject":"Re: [PATCH] doc: explain the use of color.pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2021-05-19T19:53:39Z","receivedAt":"2021-05-19T19:53:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> The current documentation for color.pager is technically correct, but\n> slightly misleading and doesn't really clarify the purpose of the\n> variable. As explained in the original thread which added it:\n>\n> https://lore.kernel.org/git/E1G6zPH-00062L-Je@moooo.ath.cx/\n>\n> the point is deal with pagers that don't understand colors.\n\nMissing \"to\" before \"deal\". More importantly, I think I'd find a\nreference to the commit or a quotation from the affected user more\nhelpful than a reference to the mailing list archive, since that would\nmake this a bit more self-contained. (Especially since I think this\nis un-subtle enough that chasing through the mailing list thread\ndoesn't add much to my life. :))\n\n[...]\n> --- a/Documentation/config/color.txt\n> +++ b/Documentation/config/color.txt\n> @@ -127,8 +127,9 @@ color.interactive.::\n> \tinteractive commands.\n> \n> color.pager::\n> -\tA boolean to enable/disable colored output when the pager is in\n> -\tuse (default is true).\n> +\tA boolean to specify whether `auto` color modes should colorize\n> +\toutput going to the pager. Defaults to true; set this to false\n> +\tif your pager does not understand ANSI color codes.\n\nI quite like the \"set this to false if your pager does not understand\nANSI color codes\" part --- short and to the point.\n\nThe sentence before takes me long enough to understand that I don't\nthink we've gotten the wording right yet. Before I suggest some\nwording, let's make sure I understand the behavior correctly:\n\n- unlike other color.* settings, this can only be \"true\" or \"false\".\n It cannot be \"auto\".\n\n- in other color.* settings, \"auto\" means \"colors are used only when\n stderr goes to a terminal\". A pager typically ultimately writes to\n a terminal, but (1) it's not guaranteed to (e.g., xless writes to\n its own window instead) and (2) more importantly for us, it's not\n guaranteed to write terminal escapes as is.\n\n- so this setting can be used to answer \"for the sake of evaluating\n color settings, should we treat output that is going to a pager as\n going to a terminal?\"\n\nIf I understood correctly, how about some text like the following?\n\n\tA boolean to specify whether `auto` color modes should colorize\n\toutput going to a pager, in addition to their behavior of\n\tcolorizing output going to a terminal. Defaults to true; [etc]\n\nSide note, not about this patch: we treat pager.color as a synonym for\ncolor.pager. Is that something we want to document, or is that an\ninstance of being extra friendly when the user makes a typo?\n\nThanks,\nJonathan\n"},{"id":"424988","messageId":"60a5778992ddd_1b578208e3@natae.notmuch","threadId":"55731","inReplyTo":"YKVsw3uqb66ifzvd@google.com","subject":"Re: [PATCH] doc: explain the use of color.pager","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-05-19T20:39:37Z","receivedAt":"2021-05-19T20:39:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jonathan Nieder wrote:\n\n> More importantly, I think I'd find a reference to the commit or a\n> quotation from the affected user more helpful than a reference to the\n> mailing list archive, since that would make this a bit more\n> self-contained.\n\nAnd for me it's the opposite; I find what one user found at one point in\ntime long ago not particularly important. At best it's a footnote.\n\n> [...]\n> > --- a/Documentation/config/color.txt\n> > +++ b/Documentation/config/color.txt\n> > @@ -127,8 +127,9 @@ color.interactive.::\n> > \tinteractive commands.\n> > \n> > color.pager::\n> > -\tA boolean to enable/disable colored output when the pager is in\n> > -\tuse (default is true).\n> > +\tA boolean to specify whether `auto` color modes should colorize\n> > +\toutput going to the pager. Defaults to true; set this to false\n> > +\tif your pager does not understand ANSI color codes.\n> \n> I quite like the \"set this to false if your pager does not understand\n> ANSI color codes\" part --- short and to the point.\n> \n> The sentence before takes me long enough to understand that I don't\n> think we've gotten the wording right yet. Before I suggest some\n> wording, let's make sure I understand the behavior correctly:\n> \n> - unlike other color.* settings, this can only be \"true\" or \"false\".\n> It cannot be \"auto\".\n> \n> - in other color.* settings, \"auto\" means \"colors are used only when\n> stderr goes to a terminal\". A pager typically ultimately writes to\n> a terminal, but (1) it's not guaranteed to (e.g., xless writes to\n> its own window instead) and (2) more importantly for us, it's not\n> guaranteed to write terminal escapes as is.\n> \n> - so this setting can be used to answer \"for the sake of evaluating\n> color settings, should we treat output that is going to a pager as\n> going to a terminal?\"\n\nCorrect.\n\n> If I understood correctly, how about some text like the following?\n> \n> \tA boolean to specify whether `auto` color modes should colorize\n> \toutput going to a pager, in addition to their behavior of\n> \tcolorizing output going to a terminal.\n\nThe \"in adittion\" part is unnecessary. You don't say \"the rest of git's\nbehavior remains unafffected\" on every single configuration. That is\nobvious.\n\nWe are specifying what happens when output is going to a pager, that's\nit.\n\nEverything else--including what happens when the output going to\ndifferent programs--it not relevant.\n\nIn addition, as far as I can recall there are no commands that send\noutput both to a pager and to a terminal.\n\n> Side note, not about this patch: we treat pager.color as a synonym for\n> color.pager. Is that something we want to document, or is that an\n> instance of being extra friendly when the user makes a typo?\n\nI suspect pager.color is a remnant from an ancient suboptimal name choice.\n\n-- \nFelipe Contreras\n"},{"id":"424998","messageId":"xmqq7djuxrvk.fsf@gitster.g","threadId":"55731","inReplyTo":"YKVsw3uqb66ifzvd@google.com","subject":"Re: [PATCH] doc: explain the use of color.pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-19T22:47:59Z","receivedAt":"2021-05-19T22:48:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Side note, not about this patch: we treat pager.color as a synonym for\n> color.pager. Is that something we want to document, or is that an\n> instance of being extra friendly when the user makes a typo?\n\nIIUC, this is the same historical wart that comes from the same\nplace as diff.color that existed before color. wanted to be the\nway forward. We do not break existing configuration files by\nstarting to suddenly reject diff.color or pager.color, but we do not\nencourage new uses by not advertising them.\n\n"},{"id":"425051","messageId":"xmqq5yzdvrlg.fsf@gitster.g","threadId":"55731","inReplyTo":"YKVsw3uqb66ifzvd@google.com","subject":"Re: [PATCH] doc: explain the use of color.pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-20T06:36:59Z","receivedAt":"2021-05-20T06:37:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> The current documentation for color.pager is technically correct, but\n>> slightly misleading and doesn't really clarify the purpose of the\n>> variable. As explained in the original thread which added it:\n>>\n>> https://lore.kernel.org/git/E1G6zPH-00062L-Je@moooo.ath.cx/\n>>\n>> the point is deal with pagers that don't understand colors.\n>\n> Missing \"to\" before \"deal\".\n\nWill locally amend.\n\n> More importantly, I think I'd find a\n> reference to the commit or a quotation from the affected user more\n> helpful than a reference to the mailing list archive, since that would\n> make this a bit more self-contained.\n\nThe original commit and its log message we ended up with did not\nexplain the motivation behind well enough.\n\nThe motivation from the original thread:\n\n When I use a pager that escapes the escape character or highlights the\n content itself the output of git diff without the pager should have\n colors but not with the pager. For example using git diff with a\n pathspec is quite short most of the time. For git diff I have to\n enable paging manually and run git diff | $PAGER usually but git log\n uses the pager automatically and should not use colors with it.\n\ncan be quoted as a whole, but \"the point is to deal with ...\" is a\nsuccinct summary that is good enough for the purpose of this commit,\nI would think.\n\n>> +\tA boolean to specify whether `auto` color modes should colorize\n>> +\toutput going to the pager. Defaults to true; set this to false\n>> +\tif your pager does not understand ANSI color codes.\n>\n> I quite like the \"set this to false if your pager does not understand\n> ANSI color codes\" part --- short and to the point.\n>\n> The sentence before takes me long enough to understand that I don't\n> think we've gotten the wording right yet. Before I suggest some\n> wording, let's make sure I understand the behavior correctly:\n>\n> - unlike other color.* settings, this can only be \"true\" or \"false\".\n> It cannot be \"auto\".\n\nCorrect.\n\n> - in other color.* settings, \"auto\" means \"colors are used only when\n> stderr goes to a terminal\". A pager typically ultimately writes to\n> a terminal, but (1) it's not guaranteed to (e.g., xless writes to\n> its own window instead) and (2) more importantly for us, it's not\n> guaranteed to write terminal escapes as is.\n\nCorrect---color.pager is about telling Git about the pager's capability.\n\n> - so this setting can be used to answer \"for the sake of evaluating\n> color settings, should we treat output that is going to a pager as\n> going to a terminal?\"\n\nI am not sure if that is the easiest-to-explain way to view it.\nIt's more like \"color.diff says 'auto', so we'd color it when the\noutput is going to tty and the terminal is not dumb. We'd also\ncolor it when our output is not directly going to tty (because we\nare writing to a pipe whose other end is being read by the pager)\nbut we know we are talking to a pager, BUT the pager may not be able\nto handle coloured output correctly---so we need a way to say \"no,\neven though our output goes to the pager, we cannot colour the\noutput\".\n\n> If I understood correctly, how about some text like the following?\n>\n> \tA boolean to specify whether `auto` color modes should colorize\n> \toutput going to a pager, in addition to their behavior of\n> \tcolorizing output going to a terminal. Defaults to true; [etc]\n\nThe variable has no control over what happens to output that\ndirectly goes to a terminal, so while the additional phrase might\nnot be technically wrong per-se, I do not see why this is more clear\nthan the original.\n\nSo, in short, I think it would be sufficient to amend the proposed\nlog message with s/deal with/to deal with/ and nothing else.\n\nThanks.\n"},{"id":"425073","messageId":"YKYe9PZbTO1CyBOp@coredump.intra.peff.net","threadId":"55731","inReplyTo":"xmqq5yzdvrlg.fsf@gitster.g","subject":"Re: [PATCH] doc: explain the use of color.pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-20T08:33:56Z","receivedAt":"2021-05-20T08:33:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 20, 2021 at 03:36:59PM +0900, Junio C Hamano wrote:\n\n> > More importantly, I think I'd find a\n> > reference to the commit or a quotation from the affected user more\n> > helpful than a reference to the mailing list archive, since that would\n> > make this a bit more self-contained.\n> \n> The original commit and its log message we ended up with did not\n> explain the motivation behind well enough.\n> \n> The motivation from the original thread:\n> \n> When I use a pager that escapes the escape character or highlights the\n> content itself the output of git diff without the pager should have\n> colors but not with the pager. For example using git diff with a\n> pathspec is quite short most of the time. For git diff I have to\n> enable paging manually and run git diff | $PAGER usually but git log\n> uses the pager automatically and should not use colors with it.\n> \n> can be quoted as a whole, but \"the point is to deal with ...\" is a\n> succinct summary that is good enough for the purpose of this commit,\n> I would think.\n\nThanks, I was just preparing a near-identical response.\n\nI do think it's an important principle in general to summarize the\ncontent of things we link to. It's just that the summary in this case\nwas so short that it was easy to look past. :)\n\n> > If I understood correctly, how about some text like the following?\n> >\n> > \tA boolean to specify whether `auto` color modes should colorize\n> > \toutput going to a pager, in addition to their behavior of\n> > \tcolorizing output going to a terminal. Defaults to true; [etc]\n> \n> The variable has no control over what happens to output that\n> directly goes to a terminal, so while the additional phrase might\n> not be technically wrong per-se, I do not see why this is more clear\n> than the original.\n\nUnsurprisingly, that's my opinion, too. While writing it, I actually did\ntry some longer explanations to explain from the ground up, but I\nworried it was distracting from the main point of this variable. E.g.,\nsomething like:\n\n When color mode options (e.g., `color.ui`) are set to `auto`, Git by\n default will enable color when the output is going to a terminal or to\n a pager (since the pager itself is outputting to a terminal). This is\n a problem if your pager doesn't faithfully relay the color codes to\n the terminal. You can set this boolean to `false` to disable color\n when output is going to a terminal.\n\n> So, in short, I think it would be sufficient to amend the proposed\n> log message with s/deal with/to deal with/ and nothing else.\n\nI'm happy with that.\n\n-Peff\n"},{"id":"425108","messageId":"YKZ6tqP+e/iTxz2g@google.com","threadId":"55731","inReplyTo":"YKYe9PZbTO1CyBOp@coredump.intra.peff.net","subject":"Re: [PATCH] doc: explain the use of color.pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2021-05-20T15:05:26Z","receivedAt":"2021-05-20T15:05:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Thu, May 20, 2021 at 03:36:59PM +0900, Junio C Hamano wrote:\n\n>> The original commit and its log message we ended up with did not\n>> explain the motivation behind well enough.\n>>\n>> The motivation from the original thread:\n>>\n>> When I use a pager that escapes the escape character or highlights the\n>> content itself the output of git diff without the pager should have\n>> colors but not with the pager. For example using git diff with a\n>> pathspec is quite short most of the time. For git diff I have to\n>> enable paging manually and run git diff | $PAGER usually but git log\n>> uses the pager automatically and should not use colors with it.\n>>\n>> can be quoted as a whole, but \"the point is to deal with ...\" is a\n>> succinct summary that is good enough for the purpose of this commit,\n>> I would think.\n>\n> Thanks, I was just preparing a near-identical response.\n>\n> I do think it's an important principle in general to summarize the\n> content of things we link to. It's just that the summary in this case\n> was so short that it was easy to look past. :)\n\nI was making a different point: that the summary is so good that the\nlink is not even needed (or that if you are just trying to cite your\nsources, it should be a footnote). The current proposed commit\nmessage makes the link prominent, which tells the reader that reading\nthe thread is a good use of their time, when in this example it isn't.\n\n[...]\n> When color mode options (e.g., `color.ui`) are set to `auto`, Git by\n> default will enable color when the output is going to a terminal or to\n> a pager (since the pager itself is outputting to a terminal). This is\n> a problem if your pager doesn't faithfully relay the color codes to\n> the terminal. You can set this boolean to `false` to disable color\n> when output is going to a terminal.\n\nOh! FWIW, I like that and don't think it's overkill.\n\n> On Thu, May 20, 2021 at 03:36:59PM +0900, Junio C Hamano wrote:\n\n>> So, in short, I think it would be sufficient to amend the proposed\n>> log message with s/deal with/to deal with/ and nothing else.\n>\n> I'm happy with that.\n\nYes, the proposed patch is an improvement on the status quo, so I'd\nrather that it land as-is than not land at all. That said, the\nresulting text is still confusing. It can be easy to forget how much\ncontext we're assuming when we read it ourselves; the proposed text\nstill makes an uninitiated reader go \"huh?\" in a way that the \"When\ncolor mode options\" example above does not.\n\nThanks,\nJonathan\n"}]}