Felipe Contreras [off-list ref] writes:
Junio C Hamano wrote:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
This looks much better.
I wonder a good follow-up (hint, hint! :) would be to have
exec_man_man() and exec_man_cmd() in builtin/help.c set this depending
on color.ui (so we'd do it by default with "auto").
Then e.g. "git help git" would look prettier than "man git".
As long as color.man.ui can be used to override the blanket
color.ui, I think it is a good idea.
Why not use color.pager?
I dug a bit to refresh my memory and it turns out that the reason we
should not do so is because it means something totally different.
color.[ch] defines want_color() that applications like "diff" and
"log" can use to see if the application is configured to paint its
output in colors.
When that layer says for that particular application it should be
decided automatically, then we call into color.c::check_auto_color()
which is the only user of pager_use_color (which is set from the
color.pager configuration variable). The purpose of that call is to
ask if the pager is capable of colors.
So in short, the color.pager is about "is the pager capable of
colors?" and the color.ui (and color.<cmd>) is about "does the user
wants output from <cmd> in color?" We need to use color.help or
something that controls whether the user wants help/man in colors,
and perhaps default it to "auto" like color.ui defaults to, which
then in turn would consult "color.pager". Tying it directly to
"color.pager" is wrong.
Junio C Hamano wrote:
Felipe Contreras [off-list ref] writes:
quoted
Why not use color.pager?
I dug a bit to refresh my memory and it turns out that the reason we
should not do so is because it means something totally different.
color.[ch] defines want_color() that applications like "diff" and
"log" can use to see if the application is configured to paint its
output in colors.
When that layer says for that particular application it should be
decided automatically, then we call into color.c::check_auto_color()
which is the only user of pager_use_color (which is set from the
color.pager configuration variable). The purpose of that call is to
ask if the pager is capable of colors.
That is not true. check_auto_color() returns 1 when the fd is a tty.
For example advice() would indirectly call check_auto_color() and no
pager is involved.
Inside the help built-in want_color(GIT_COLOR_UNKNOWN) will always
return 1 (unless you do something like `git help git > file`).
pager_use_color is completely ignored in this case.
It's used only when 1) the fd is not a tty, 2) it's stdout, 3) git is
using a pager... Only _then_ it checks for pager_use_color
(color.pager).
So no; the first purpose of check_auto_color() is to find out if the fd
is capable of showing colors, secondary, if we are using a pager, then
check the user configuration.
In the case we are using a pager it's not check if the pager is capable
of colors, the pager could be capable of colors and yet the user would
disable them: it's the check for the user preference.
So in short, the color.pager is about "is the pager capable of
colors?"
That's not the case.
Even the documentation says so:
color.pager::
A boolean to enable/disable colored output when the pager is in
use (default is true).
and the color.ui (and color.<cmd>) is about "does the user
wants output from <cmd> in color?"
Tying it directly to "color.pager" is wrong.
Why? color.pager is to enable/disable colored output when a pager is in
use.
The fact that it's man executing the pager and not git makes no
difference to the user.
Cheers.
--
Felipe Contreras
On Mon, May 17, 2021 at 11:27:23PM -0500, Felipe Contreras wrote:
quoted
So in short, the color.pager is about "is the pager capable of
colors?"
That's not the case.
Even the documentation says so:
color.pager::
A boolean to enable/disable colored output when the pager is in
use (default is true).
I think that documentation misses the reason you'd want to use it.
Likewise, the commit message introducing it (aa086eb813d) sucks, but the
motivation (from [0]) was:
When I use a pager that escapes the escape character or highlights the
content itself the output of git diff without the pager should have
colors but not with the pager. For example using git diff with a
pathspec is quite short most of the time. For git diff I have to
enable paging manually and run git diff | $PAGER usually but git log
uses the pager automatically and should not use colors with it.
For a more concrete example, my pager _does_ understand colors, and I
would not want to set pager.color to "false" (because then "git log",
etc, would not show me any colors). But I don't like the man colors you
are suggesting. I want to be able to turn them off by setting
"color.man" or similar to false, not by disabling color for everything
that is paged.
So color.pager being true is _necessary_ for showing colors in paged
outputs, but by itself is not sufficient. We have other per-context
color options (color.diff, color.branch, and so on).
And so likewise, we would want to avoid turning on colors if the user
has set color.pager=false. Usually this is done automatically because
want_color() checks, which knows if we are using the pager or not. But
if we are going to call out to "man" which will invoke another pager,
that caller would have to check pager_use_color themselves (it's yet
another question of whether "the pager can handle color" applies equally
to the pager that Git will run versus the one that man will run).
-Peff
[0] https://lore.kernel.org/git/E1G6zPH-00062L-Je@moooo.ath.cx/
Jeff King wrote:
On Mon, May 17, 2021 at 11:27:23PM -0500, Felipe Contreras wrote:
quoted
quoted
So in short, the color.pager is about "is the pager capable of
colors?"
That's not the case.
Even the documentation says so:
color.pager::
A boolean to enable/disable colored output when the pager is in
use (default is true).
I think that documentation misses the reason you'd want to use it.
Likewise, the commit message introducing it (aa086eb813d) sucks,
Well, it was 2006. Many of the best practices of today were not followed
back then.
but the
motivation (from [0]) was:
When I use a pager that escapes the escape character or highlights the
content itself the output of git diff without the pager should have
colors but not with the pager. For example using git diff with a
pathspec is quite short most of the time. For git diff I have to
enable paging manually and run git diff | $PAGER usually but git log
uses the pager automatically and should not use colors with it.
This is aligned with what I said: the uswer wants to disable colors when
using a pager.
Yes, in this instance it's because the pager doesn't support colors, but
that's not always necessarily so. A person with sight problems may use
less (perfectly capable of colors), but yet not want to exercise that
capability.
It's still a preference.
For a more concrete example, my pager _does_ understand colors, and I
would not want to set pager.color to "false" (because then "git log",
etc, would not show me any colors). But I don't like the man colors you
are suggesting.
You can change them in your environment.
I want to be able to turn them off by setting "color.man" or similar
to false, not by disabling color for everything that is paged.
Sure, for that particular case it does make sense. I'll add that.
So color.pager being true is _necessary_ for showing colors in paged
outputs, but by itself is not sufficient. We have other per-context
color options (color.diff, color.branch, and so on).
And so likewise, we would want to avoid turning on colors if the user
has set color.pager=false. Usually this is done automatically because
want_color() checks, which knows if we are using the pager or not. But
if we are going to call out to "man" which will invoke another pager,
that caller would have to check pager_use_color themselves (it's yet
another question of whether "the pager can handle color" applies equally
to the pager that Git will run versus the one that man will run).
Yes, but we still need to check pager_use_color.
Except... Maybe a user has GIT_PAGER set to a colorless pager, and
MANPAGER to something fancier, in which case color.pager should be
ignored. But that's probably a corner-case nobody is ever going to hit.
Anyway, I've sent an update version with color.man.
--
Felipe Contreras