[PATCH] doc: explain the use of color.pager

The current documentation for color.pager is technically correct, but
slightly misleading and doesn't really clarify the purpose of the
variable. As explained in the original thread which added it:

  https://lore.kernel.org/git/E1G6zPH-00062L-Je@moooo.ath.cx/

the point is deal with pagers that don't understand colors. And hence it
being set to "true" is necessary for colorizing output to the pager, but
not sufficient by itself (you must also have enabled one of the other
color options, though note that these are set to "auto" by default these
days).

Signed-off-by: Jeff King <redacted>
---
Just a loose end from:

  https://lore.kernel.org/git/xmqqlf8c2k2y.fsf@gitster.g/

that I think is worth tying up before we forget.

 Documentation/config/color.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
index d5daacb13a..e05d520a86 100644
--- a/Documentation/config/color.txt
+++ b/Documentation/config/color.txt

@@ -127,8 +127,9 @@ color.interactive.<slot>::
 	interactive commands.
 
 color.pager::
-	A boolean to enable/disable colored output when the pager is in
-	use (default is true).
+	A boolean to specify whether `auto` color modes should colorize
+	output going to the pager. Defaults to true; set this to false
+	if your pager does not understand ANSI color codes.
 
 color.push::
 	A boolean to enable/disable color in push errors. May be set to

-- 
2.32.0.rc0.424.g95d3bde94f

Jeff King wrote:

This sounds correct to me, short, and complete. I like it.

Acked-by: Felipe Contreras <redacted>

-- 
Felipe Contreras

Jeff King wrote:

The current documentation for color.pager is technically correct, but
slightly misleading and doesn't really clarify the purpose of the
variable. As explained in the original thread which added it:

  https://lore.kernel.org/git/E1G6zPH-00062L-Je@moooo.ath.cx/

the point is deal with pagers that don't understand colors.

Missing "to" before "deal".  More importantly, I think I'd find a
reference to the commit or a quotation from the affected user more
helpful than a reference to the mailing list archive, since that would
make this a bit more self-contained.  (Especially since I think this
is un-subtle enough that chasing through the mailing list thread
doesn't add much to my life. :))

[...]

I quite like the "set this to false if your pager does not understand
ANSI color codes" part --- short and to the point.

The sentence before takes me long enough to understand that I don't
think we've gotten the wording right yet.  Before I suggest some
wording, let's make sure I understand the behavior correctly:

- unlike other color.* settings, this can only be "true" or "false".
  It cannot be "auto".

- in other color.* settings, "auto" means "colors are used only when
  stderr goes to a terminal".  A pager typically ultimately writes to
  a terminal, but (1) it's not guaranteed to (e.g., xless writes to
  its own window instead) and (2) more importantly for us, it's not
  guaranteed to write terminal escapes as is.

- so this setting can be used to answer "for the sake of evaluating
  color settings, should we treat output that is going to a pager as
  going to a terminal?"

If I understood correctly, how about some text like the following?

	A boolean to specify whether `auto` color modes should colorize
	output going to a pager, in addition to their behavior of
	colorizing output going to a terminal. Defaults to true; [etc]

Side note, not about this patch: we treat pager.color as a synonym for
color.pager.  Is that something we want to document, or is that an
instance of being extra friendly when the user makes a typo?

Thanks,
Jonathan

Jonathan Nieder wrote:

More importantly, I think I'd find a reference to the commit or a
quotation from the affected user more helpful than a reference to the
mailing list archive, since that would make this a bit more
self-contained.

And for me it's the opposite; I find what one user found at one point in
time long ago not particularly important. At best it's a footnote.

[...]

quoted

--- a/Documentation/config/color.txt
+++ b/Documentation/config/color.txt

@@ -127,8 +127,9 @@ color.interactive.<slot>::
 	interactive commands.
 
 color.pager::
-	A boolean to enable/disable colored output when the pager is in
-	use (default is true).
+	A boolean to specify whether `auto` color modes should colorize
+	output going to the pager. Defaults to true; set this to false
+	if your pager does not understand ANSI color codes.

I quite like the "set this to false if your pager does not understand
ANSI color codes" part --- short and to the point.

The sentence before takes me long enough to understand that I don't
think we've gotten the wording right yet.  Before I suggest some
wording, let's make sure I understand the behavior correctly:

- unlike other color.* settings, this can only be "true" or "false".
  It cannot be "auto".

- in other color.* settings, "auto" means "colors are used only when
  stderr goes to a terminal".  A pager typically ultimately writes to
  a terminal, but (1) it's not guaranteed to (e.g., xless writes to
  its own window instead) and (2) more importantly for us, it's not
  guaranteed to write terminal escapes as is.

- so this setting can be used to answer "for the sake of evaluating
  color settings, should we treat output that is going to a pager as
  going to a terminal?"

Correct.

If I understood correctly, how about some text like the following?

	A boolean to specify whether `auto` color modes should colorize
	output going to a pager, in addition to their behavior of
	colorizing output going to a terminal.

The "in adittion" part is unnecessary. You don't say "the rest of git's
behavior remains unafffected" on every single configuration. That is
obvious.

We are specifying what happens when output is going to a pager, that's
it.

Everything else--including what happens when the output going to
different programs--it not relevant.

In addition, as far as I can recall there are no commands that send
output both to a pager and to a terminal.

Side note, not about this patch: we treat pager.color as a synonym for
color.pager.  Is that something we want to document, or is that an
instance of being extra friendly when the user makes a typo?

I suspect pager.color is a remnant from an ancient suboptimal name choice.

-- 
Felipe Contreras