diff mbox series

[v7] help: colorize man pages if man.color=true under less(1)

Message ID patch-1.1-a950ef49e28-20210621T083254Z-avarab@gmail.com (mailing list archive)
State New
Headers show
Series [v7] help: colorize man pages if man.color=true under less(1) | expand

Commit Message

Ævar Arnfjörð Bjarmason June 21, 2021, 8:34 a.m. UTC
From: Felipe Contreras <felipe.contreras@gmail.com>

We already colorize tools traditionally not colorized by default, like

Comments

Phillip Wood June 21, 2021, 10:17 a.m. UTC | #1
On 21/06/2021 09:34, Ævar Arnfjörð Bjarmason wrote:
> From: Felipe Contreras <felipe.contreras@gmail.com>
> 
> We already colorize tools traditionally not colorized by default, like
> diff and grep. Let's do the same for man, but only if `color.man` is
> explicitly set to "true".
> 
> Unlike other `color.*` output this colorization is not enabled by
> `color.ui` being true, the user needs to explicitly set the
> `color.man` variable to `true.
> 
> When it was proposed to treat `color.man` like any other `color.*`
> variable some thought that git opting in coloring for an external
> program such as man(1) was a step too far[1], even if the user invoked
> it via the "git help <topic>" wrapper.
> 
> So let's make this explicitly opt-in for now. As noted in the
> documentation we're leaving ourselves an out to turn this on by
> default in the future, or e.g. putting it under the
> feature.experimental umbrella. We probably won't, but let's not
> promise users that `color.man` will forever be a special-case.
> 
> As for what this actually does the effect of having this enabled is
> that a documentation blurb like (some parts elided with "[...]"):
> 
> 	NAME
> 	----
> 	git-config - Get and set [...]
> 
> 	SYNOPSIS
> 	--------
> 	[...]
> 	'git config' [<file-option>] [...]
> 	[...]
> 	The `--type=<type>` option instructs 'git config' to ensure [...]
> 
> Will have "NAME" and "SECTION" shown as BOLD RED instead of BOLD, "git
> config" and other '-quoted parts in BLUE UNDERLINE instead of
> UNDERLINE, and `--type=<type>` and other `-quoted parts in RED BOLD
> instead of BOLD. The "Standout" setting is then used for the user's
> own search bar (invoked with "/") and prompt. See [2] for more
> examples
> 
> Normally check_auto_color() would check the value of `color.pager`, but
> in this particular case it's not git the one executing the pager, but
> man. Therefore we need to check pager_use_color ourselves.
> 
> We do not need to support `color.man` being set to `always`; The `git
> help` command is always run for a tty (it would be very strange for a
> user to do `git help $page > output`, but in fact, that works anyway,
> we don't even need to check if stdout is a tty, but just to be
> consistent we do). So it's simply a boolean in our case.
> 
> So, in order for this change to have any effect:
> 
>   1. color.man=true must be set in the config
>   2. The user must use less
>   3. Not have the same LESS_TERMCAP variables set (we call setenv(3) with overwrite=0)
>   4. Have color.ui enabled
>   5. Not have color.pager disabled
>   6. Not have git with stdout directed to a file
> 
> 1. https://lore.kernel.org/git/87tun1qp91.fsf@evledraar.gmail.com/
> 2. https://unix.stackexchange.com/questions/119/colors-in-man-pages/147

Thanks for taking the time to write such a clear and comprehensive 
message, I think making this setting opt in for now makes sense. I've 
left a couple of comments below.

> Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> On Tue, Jun 08 2021, Junio C Hamano wrote:
> 
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> I've been running with this on my personal git build since May 26th. I
>>> haven't had any issues with it, and I like the new coloring.
>>> ...
>>> I think this is a good example of a change that we're better off just
>>> merging down and then reverting if the wider audience of git users hates
>>> it, rather than trying to come to some perfect consensus here
>>> on-list.
>>
>> My impression was tht we already had a rough consensus here on-list
>> that it may be good to educate users who like this "new coloring"
>> like you do to configure their "less", so that they consistently get
>> the "new coloring" they like whether they are doing "git help git",
>> "man git", or even "man ls", and the approach the posted patch takes
>> will not help (it only affects "git help git" among these).
>>
>> I'd rather not to take it.
> 
> Fair enough, here's a version I think you and others will find
> acceptable then. It allows users like me who like this to explicitly
> opt-in via color.man=true.
> 
> I also took the liberty of making some changes to the commit message
> to reword it for this new behavior, and to show an example of the sort
> of colors that change with this patch.
> 
> The only change to the patch itself is that now color_man is set to 0
> by default, not 1. I also moved its declaration so that it's with the
> one other config variable, insted of with the other command-line
> options to "git help".
> 
> Range-diff against v6:
> 1:  e021ca1da21 ! 1:  a950ef49e28 help: colorize man pages
>      @@ Metadata
>       Author: Felipe Contreras <felipe.contreras@gmail.com>
>       
>        ## Commit message ##
>      -    help: colorize man pages
>      +    help: colorize man pages if man.color=true under less(1)
>       
>           We already colorize tools traditionally not colorized by default, like
>      -    diff and grep. Let's do the same for man.
>      -
>      -    Our man pages don't contain many useful colors (just blue links),
>      -    moreover, many people have groff SGR disabled, so they don't see any
>      -    colors with man pages.
>      -
>      -    We can set LESS_TERMCAP variables to render bold and underlined text
>      -    with colors in the pager; a common trick[1].
>      -
>      -    Bold is rendered as red, underlined as blue, and standout (prompt and
>      -    highlighted search) as inverse cyan.
>      -
>      -    Obviously this only works when the less pager is used.
>      -
>      -    If the user already has LESS_TERMCAP variables set in his/her
>      -    environment, those are respected and nothing changes.
>      -
>      -    A new color configuration is added: `color.man` for the people that want
>      -    to turn this feature off, otherwise `color.ui` is respected.
>      -    Additionally, if color.pager is not enabled, this is disregarded.
>      +    diff and grep. Let's do the same for man, but only if `color.man` is
>      +    explicitly set to "true".
>      +
>      +    Unlike other `color.*` output this colorization is not enabled by
>      +    `color.ui` being true, the user needs to explicitly set the
>      +    `color.man` variable to `true.
>      +
>      +    When it was proposed to treat `color.man` like any other `color.*`
>      +    variable some thought that git opting in coloring for an external
>      +    program such as man(1) was a step too far[1], even if the user invoked
>      +    it via the "git help <topic>" wrapper.
>      +
>      +    So let's make this explicitly opt-in for now. As noted in the
>      +    documentation we're leaving ourselves an out to turn this on by
>      +    default in the future, or e.g. putting it under the
>      +    feature.experimental umbrella. We probably won't, but let's not
>      +    promise users that `color.man` will forever be a special-case.
>      +
>      +    As for what this actually does the effect of having this enabled is
>      +    that a documentation blurb like (some parts elided with "[...]"):
>      +
>      +            NAME
>      +            ----
>      +            git-config - Get and set [...]
>      +
>      +            SYNOPSIS
>      +            --------
>      +            [...]
>      +            'git config' [<file-option>] [...]
>      +            [...]
>      +            The `--type=<type>` option instructs 'git config' to ensure [...]
>      +
>      +    Will have "NAME" and "SECTION" shown as BOLD RED instead of BOLD, "git
>      +    config" and other '-quoted parts in BLUE UNDERLINE instead of
>      +    UNDERLINE, and `--type=<type>` and other `-quoted parts in RED BOLD
>      +    instead of BOLD. The "Standout" setting is then used for the user's
>      +    own search bar (invoked with "/") and prompt. See [2] for more
>      +    examples
>       
>           Normally check_auto_color() would check the value of `color.pager`, but
>           in this particular case it's not git the one executing the pager, but
>           man. Therefore we need to check pager_use_color ourselves.
>       
>      -    Also--unlike other color.* configurations--color.man=always does not
>      -    make any sense here; `git help` is always run for a tty (it would be very
>      -    strange for a user to do `git help $page > output`, but in fact, that
>      -    works anyway, we don't even need to check if stdout is a tty, but just
>      -    to be consistent we do). So it's simply a boolean in our case.
>      +    We do not need to support `color.man` being set to `always`; The `git
>      +    help` command is always run for a tty (it would be very strange for a
>      +    user to do `git help $page > output`, but in fact, that works anyway,
>      +    we don't even need to check if stdout is a tty, but just to be
>      +    consistent we do). So it's simply a boolean in our case.
>       
>           So, in order for this change to have any effect:
>       
>      -     1. The user must use less
>      -     2. Not have the same LESS_TERMCAP variables set
>      -     3. Have color.ui enabled
>      -     4. Not have color.pager disabled
>      -     5. Not have color.man disabled
>      -     7. Not have git with stdout directed to a file
>      -
>      -    Fortunately the vast majority of our users meet all of the above, and
>      -    anybody who doesn't would not be affected negatively (plus very likely
>      -    comprises a very tiny minority).
>      +     1. color.man=true must be set in the config
>      +     2. The user must use less
>      +     3. Not have the same LESS_TERMCAP variables set (we call setenv(3) with overwrite=0)
>      +     4. Have color.ui enabled
>      +     5. Not have color.pager disabled
>      +     6. Not have git with stdout directed to a file
>       
>      -    [1] https://unix.stackexchange.com/questions/119/colors-in-man-pages/147
>      +    1. https://lore.kernel.org/git/87tun1qp91.fsf@evledraar.gmail.com/
>      +    2. https://unix.stackexchange.com/questions/119/colors-in-man-pages/147
>       
>           Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>      -    Phillip Wood <phillip.wood123@gmail.com>
>      -    Comments-by: Jeff King <peff@peff.net>
>           Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>      +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>        ## Documentation/config/color.txt ##
>       @@ Documentation/config/color.txt: color.interactive.<slot>::
>      @@ Documentation/config/color.txt: color.interactive.<slot>::
>        	interactive commands.
>        
>       +color.man::
>      -+	This flag can be used to disable the automatic colorizaton of man
>      -+	pages when using the less pager. It's activated only when color.ui
>      -+	allows it, and also when color.pager is on. (`true` by default).
>      ++	This flag can be used to enable the automatic colorizaton of man
>      ++	pages when using the less pager, `false` by default. When set to
>      ++	`true` it's activated only when `color.ui` allows it, and if
>      ++	`color.pager` enable (which it is by default).
>       +
>        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
>      +@@ Documentation/config/color.txt: color.ui::
>      + 	output not intended for machine consumption to use color, to
>      + 	`true` or `auto` (this is the default since Git 1.8.4) if you
>      + 	want such output to use color when written to the terminal.
>      +++
>      ++When set to `true` certain other `color.*` variables may still not be
>      ++turned on unless explicitly enabled. Currently this only applies to
>      ++`color.man`, see above. Such opt-in variables may be moved under the
>      ++default `color.ui` umbrella in the future.
>       
>        ## builtin/help.c ##
>       @@
>      @@ builtin/help.c
>        
>        #ifndef DEFAULT_HELP_FORMAT
>        #define DEFAULT_HELP_FORMAT "man"
>      -@@ builtin/help.c: static int verbose = 1;
>      - static unsigned int colopts;
>      - static enum help_format help_format = HELP_FORMAT_NONE;
>      - static int exclude_guides;
>      -+static int man_color = 1;
>      - static struct option builtin_help_options[] = {
>      - 	OPT_BOOL('a', "all", &show_all, N_("print all available commands")),
>      - 	OPT_HIDDEN_BOOL(0, "exclude-guides", &exclude_guides, N_("exclude guides")),
>      +@@ builtin/help.c: enum help_format {
>      + 	HELP_FORMAT_WEB
>      + };
>      +
>      ++static int man_color;
>      + static const char *html_path;
>      +
>      + static int show_all = 0;
>       @@ builtin/help.c: static void exec_man_konqueror(const char *path, const char *page)
>        	}
>        }
> 
>   Documentation/config/color.txt | 11 +++++++++++
>   builtin/help.c                 | 32 +++++++++++++++++++++++++++++++-
>   color.h                        |  1 +
>   3 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
> index e05d520a867..2f12ae3386d 100644
> --- a/Documentation/config/color.txt
> +++ b/Documentation/config/color.txt
> @@ -126,6 +126,12 @@ color.interactive.<slot>::
>   	or `error`, for four distinct types of normal output from
>   	interactive commands.
>   
> +color.man::
> +	This flag can be used to enable the automatic colorizaton of man
> +	pages when using the less pager, `false` by default. When set to
> +	`true` it's activated only when `color.ui` allows it, and if
> +	`color.pager` enable (which it is by default).

s/and if `color.pager` enable/and `color.pager` is enabled/?

> +
>   color.pager::
>   	A boolean to specify whether `auto` color modes should colorize
>   	output going to the pager. Defaults to true; set this to false
> @@ -200,3 +206,8 @@ color.ui::
>   	output not intended for machine consumption to use color, to
>   	`true` or `auto` (this is the default since Git 1.8.4) if you
>   	want such output to use color when written to the terminal.
> ++
> +When set to `true` certain other `color.*` variables may still not be
> +turned on unless explicitly enabled. Currently this only applies to
> +`color.man`, see above. Such opt-in variables may be moved under the
> +default `color.ui` umbrella in the future.
> diff --git a/builtin/help.c b/builtin/help.c
> index bb339f0fc80..c607da88d78 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -11,6 +11,7 @@
>   #include "config-list.h"
>   #include "help.h"
>   #include "alias.h"
> +#include "color.h"
>   
>   #ifndef DEFAULT_HELP_FORMAT
>   #define DEFAULT_HELP_FORMAT "man"
> @@ -34,6 +35,7 @@ enum help_format {
>   	HELP_FORMAT_WEB
>   };
>   
> +static int man_color;
>   static const char *html_path;
>   
>   static int show_all = 0;
> @@ -253,10 +255,33 @@ static void exec_man_konqueror(const char *path, const char *page)
>   	}
>   }
>   
> +static void colorize_man(void)
> +{
> +	if (!man_color || !want_color(GIT_COLOR_UNKNOWN) || !pager_use_color)
> +		return;
> +
> +	/* Disable groff colors */
> +	setenv("GROFF_NO_SGR", "1", 0);
> +
> +	/* Bold */
> +	setenv("LESS_TERMCAP_md", GIT_COLOR_BOLD_RED, 0);
> +	setenv("LESS_TERMCAP_me", GIT_COLOR_RESET, 0);

We take care not to override the user's preference if these variables 
are set individually, but there is a possible corner case where the user 
sets a subset of these LESS_TERMCAP_* because they like the defaults for 
some of them but not others, in that case we wouldn't want to set any of 
them. Whether this is likely to be a problem in practice or not I'm not 
sure. Version 5 [1] of this patch took care not to override the users 
settings in this case.

Best Wishes

Phillip

[1] 
https://lore.kernel.org/git/20210522011718.541986-1-felipe.contreras@gmail.com/

> +
> +	/* Underline */
> +	setenv("LESS_TERMCAP_us", GIT_COLOR_BLUE GIT_COLOR_UNDERLINE, 0);
> +	setenv("LESS_TERMCAP_ue", GIT_COLOR_RESET, 0);
> +
> +	/* Standout */
> +	setenv("LESS_TERMCAP_so", GIT_COLOR_CYAN GIT_COLOR_REVERSE, 0);
> +	setenv("LESS_TERMCAP_se", GIT_COLOR_RESET, 0);
> +}
> +
>   static void exec_man_man(const char *path, const char *page)
>   {
>   	if (!path)
>   		path = "man";
> +
> +	colorize_man();
>   	execlp(path, "man", page, (char *)NULL);
>   	warning_errno(_("failed to exec '%s'"), path);
>   }
> @@ -264,6 +289,7 @@ static void exec_man_man(const char *path, const char *page)
>   static void exec_man_cmd(const char *cmd, const char *page)
>   {
>   	struct strbuf shell_cmd = STRBUF_INIT;
> +	colorize_man();
>   	strbuf_addf(&shell_cmd, "%s %s", cmd, page);
>   	execl(SHELL_PATH, SHELL_PATH, "-c", shell_cmd.buf, (char *)NULL);
>   	warning(_("failed to exec '%s'"), cmd);
> @@ -371,8 +397,12 @@ static int git_help_config(const char *var, const char *value, void *cb)
>   	}
>   	if (starts_with(var, "man."))
>   		return add_man_viewer_info(var, value);
> +	if (!strcmp(var, "color.man")) {
> +		man_color = git_config_bool(var, value);
> +		return 0;
> +	}
>   
> -	return git_default_config(var, value, cb);
> +	return git_color_default_config(var, value, cb);
>   }
>   
>   static struct cmdnames main_cmds, other_cmds;
> diff --git a/color.h b/color.h
> index 98894d6a175..d012add4e8a 100644
> --- a/color.h
> +++ b/color.h
> @@ -51,6 +51,7 @@ struct strbuf;
>   #define GIT_COLOR_FAINT		"\033[2m"
>   #define GIT_COLOR_FAINT_ITALIC	"\033[2;3m"
>   #define GIT_COLOR_REVERSE	"\033[7m"
> +#define GIT_COLOR_UNDERLINE	"\033[4m"
>   
>   /* A special value meaning "no color selected" */
>   #define GIT_COLOR_NIL "NIL"
>
Junio C Hamano June 21, 2021, 10:28 a.m. UTC | #2
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> So, in order for this change to have any effect:
>
>  1. color.man=true must be set in the config
>  2. The user must use less
>  3. Not have the same LESS_TERMCAP variables set (we call setenv(3) with overwrite=0)
>  4. Have color.ui enabled
>  5. Not have color.pager disabled
>  6. Not have git with stdout directed to a file
>
> 1. https://lore.kernel.org/git/87tun1qp91.fsf@evledraar.gmail.com/
> 2. https://unix.stackexchange.com/questions/119/colors-in-man-pages/147
>
> Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> On Tue, Jun 08 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> I've been running with this on my personal git build since May 26th. I
>>> haven't had any issues with it, and I like the new coloring.
>>> ...
>>> I think this is a good example of a change that we're better off just
>>> merging down and then reverting if the wider audience of git users hates
>>> it, rather than trying to come to some perfect consensus here
>>> on-list.
>>
>> My impression was tht we already had a rough consensus here on-list
>> that it may be good to educate users who like this "new coloring"
>> like you do to configure their "less", so that they consistently get
>> the "new coloring" they like whether they are doing "git help git",
>> "man git", or even "man ls", and the approach the posted patch takes
>> will not help (it only affects "git help git" among these).
>>
>> I'd rather not to take it.
>
> Fair enough, here's a version I think you and others will find
> acceptable then. It allows users like me who like this to explicitly
> opt-in via color.man=true.

Not really.

Since the implementation of the posted patch, as I understand it,
does not aim to affect both "git help -m foo" and "man git-foo"
identically, I think it would be easier to understand to end-users
if this were exposed as a new "mode", like "git help --web" and "git
help --info" are different modes from the "git help --man",
something like "git help --fancy-man" (or whatever is easy to type
and explain, and also add it to the variants help.format knows about
to make it easy to set the default).

One advantage of doing so is that we do not have to worry about "ah,
user has LESS_BLAH environment variable so we should disable this
new mode here" etc.  As long as the new mode is requested either via
the command line option or help.format configuration, it can
completely take it over.  That simplifies the necessary explanation
given to the users quite a lot, no?

> 	----
> 	git-config - Get and set [...]
>
> 	SYNOPSIS
> 	--------
> 	[...]
> 	'git config' [<file-option>] [...]
> 	[...]
> 	The `--type=<type>` option instructs 'git config' to ensure [...]
>
> Will have "NAME" and "SECTION" shown as BOLD RED instead of BOLD, "git
> config" and other '-quoted parts in BLUE UNDERLINE instead of
> UNDERLINE, and `--type=<type>` and other `-quoted parts in RED BOLD
> instead of BOLD. The "Standout" setting is then used for the user's
> own search bar (invoked with "/") and prompt. See [2] for more
> examples

There are BOLD RED and READ BOLD; are they differently rendered?
Felipe Contreras June 21, 2021, 3:59 p.m. UTC | #3
Ævar Arnfjörð Bjarmason wrote:
> From: Felipe Contreras <felipe.contreras@gmail.com>
> 
> We already colorize tools traditionally not colorized by default, like
> diff and grep. Let's do the same for man, but only if `color.man` is
> explicitly set to "true".
> 
> Unlike other `color.*` output this colorization is not enabled by
> `color.ui` being true, the user needs to explicitly set the
> `color.man` variable to `true.
> 
> When it was proposed to treat `color.man` like any other `color.*`
> variable some thought that git opting in coloring for an external
> program such as man(1) was a step too far[1], even if the user invoked
> it via the "git help <topic>" wrapper.
> 
> So let's make this explicitly opt-in for now. As noted in the
> documentation we're leaving ourselves an out to turn this on by
> default in the future, or e.g. putting it under the
> feature.experimental umbrella. We probably won't, but let's not
> promise users that `color.man` will forever be a special-case.
> 
> As for what this actually does the effect of having this enabled is
> that a documentation blurb like (some parts elided with "[...]"):
> 
> 	NAME
> 	----
> 	git-config - Get and set [...]
> 
> 	SYNOPSIS
> 	--------
> 	[...]
> 	'git config' [<file-option>] [...]
> 	[...]
> 	The `--type=<type>` option instructs 'git config' to ensure [...]
> 
> Will have "NAME" and "SECTION" shown as BOLD RED instead of BOLD, "git
> config" and other '-quoted parts in BLUE UNDERLINE instead of
> UNDERLINE, and `--type=<type>` and other `-quoted parts in RED BOLD
> instead of BOLD. The "Standout" setting is then used for the user's
> own search bar (invoked with "/") and prompt. See [2] for more
> examples
> 
> Normally check_auto_color() would check the value of `color.pager`, but
> in this particular case it's not git the one executing the pager, but
> man. Therefore we need to check pager_use_color ourselves.
> 
> We do not need to support `color.man` being set to `always`; The `git
> help` command is always run for a tty (it would be very strange for a
> user to do `git help $page > output`, but in fact, that works anyway,
> we don't even need to check if stdout is a tty, but just to be
> consistent we do). So it's simply a boolean in our case.
> 
> So, in order for this change to have any effect:
> 
>  1. color.man=true must be set in the config
>  2. The user must use less
>  3. Not have the same LESS_TERMCAP variables set (we call setenv(3) with overwrite=0)
>  4. Have color.ui enabled
>  5. Not have color.pager disabled
>  6. Not have git with stdout directed to a file
> 
> 1. https://lore.kernel.org/git/87tun1qp91.fsf@evledraar.gmail.com/
> 2. https://unix.stackexchange.com/questions/119/colors-in-man-pages/147
> 
> Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Looks good to me.
Felipe Contreras June 21, 2021, 6:41 p.m. UTC | #4
Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> > Fair enough, here's a version I think you and others will find
> > acceptable then. It allows users like me who like this to explicitly
> > opt-in via color.man=true.
> 
> Not really.
> 
> Since the implementation of the posted patch, as I understand it,
> does not aim to affect both "git help -m foo" and "man git-foo"
> identically,

It cannot aim to do what is not possible.

> I think it would be easier to understand to end-users
> if this were exposed as a new "mode", like "git help --web" and "git
> help --info" are different modes from the "git help --man",
> something like "git help --fancy-man" (or whatever is easy to type
> and explain, and also add it to the variants help.format knows about
> to make it easy to set the default).

But it is not a new mode.

I presume you mean format, since man, info, and web are formats,
controlled by help.format.

But no, it's not a format either, because we still want to see the same
format (man).

Perhaps you meant a man viewer (controlled with man.viewer), but there's
no command line option to launch help with for example emacs woman.

But still, it's not a new viewer; it's an improvement of an already
existing viewer.

> One advantage of doing so is that we do not have to worry about "ah,
> user has LESS_BLAH environment variable so we should disable this
> new mode here" etc.

We don't have to worry about that with the current patch.

> As long as the new mode is requested either via
> the command line option or help.format configuration, it can
> completely take it over.

That is already the case.

> That simplifies the necessary explanation given to the users quite a
> lot, no?

No, it would still be:

 1. man.viewer=fancyman must be set in the config
 2. The user must use less
 3. Not have the same LESS_TERMCAP variables set (we call setenv(3) with overwrite=0)
 4. Have color.ui enabled
 5. Not have color.pager disabled
 6. Not have git with stdout directed to a file

Moreover, this explanation is for developers.

Realistically all the user needs to know is that color.man=true turns
this on (man.viewer=fancyman is not better in any way).
Ævar Arnfjörð Bjarmason June 21, 2021, 7:08 p.m. UTC | #5
On Mon, Jun 21 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> So, in order for this change to have any effect:
>>
>>  1. color.man=true must be set in the config
>>  2. The user must use less
>>  3. Not have the same LESS_TERMCAP variables set (we call setenv(3) with overwrite=0)
>>  4. Have color.ui enabled
>>  5. Not have color.pager disabled
>>  6. Not have git with stdout directed to a file
>>
>> 1. https://lore.kernel.org/git/87tun1qp91.fsf@evledraar.gmail.com/
>> 2. https://unix.stackexchange.com/questions/119/colors-in-man-pages/147
>>
>> Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> On Tue, Jun 08 2021, Junio C Hamano wrote:
>>
>>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>>
>>>> I've been running with this on my personal git build since May 26th. I
>>>> haven't had any issues with it, and I like the new coloring.
>>>> ...
>>>> I think this is a good example of a change that we're better off just
>>>> merging down and then reverting if the wider audience of git users hates
>>>> it, rather than trying to come to some perfect consensus here
>>>> on-list.
>>>
>>> My impression was tht we already had a rough consensus here on-list
>>> that it may be good to educate users who like this "new coloring"
>>> like you do to configure their "less", so that they consistently get
>>> the "new coloring" they like whether they are doing "git help git",
>>> "man git", or even "man ls", and the approach the posted patch takes
>>> will not help (it only affects "git help git" among these).
>>>
>>> I'd rather not to take it.
>>
>> Fair enough, here's a version I think you and others will find
>> acceptable then. It allows users like me who like this to explicitly
>> opt-in via color.man=true.
>
> Not really.
>
> [snip] I think it would be easier to understand to end-users
> if this were exposed as a new "mode", like "git help --web" and "git
> help --info" are different modes from the "git help --man",
> something like "git help --fancy-man" (or whatever is easy to type
> and explain, and also add it to the variants help.format knows about
> to make it easy to set the default).
>
> One advantage of doing so is that we do not have to worry about "ah,
> user has LESS_BLAH environment variable so we should disable this
> new mode here" etc.  As long as the new mode is requested either via
> the command line option or help.format configuration, it can
> completely take it over.  That simplifies the necessary explanation
> given to the users quite a lot, no?

The interaction between "git help" and "man"/"less" doesn't really have
an equivalent in the rest of git as far as color output goes. Usually we
emit colors via our own programs.

But no, I think it makes the most sense to consider this orthagonal to
help.format=man or man.viewer=<cmd>.

We're not invoking a different man viewer or command, we're just
expecting that mode to invoke the pager, and if that pager is less to
have these variables tweak our color preferences.

> [unsnip] Since the implementation of the posted patch, as I understand it,
> does not aim to affect both "git help -m foo" and "man git-foo"
> identically, 

Aside from this patch I don't think it makes sense to view git's UI and
interaction with the pager like that.

To probably >95% of our users the "we invoke the pager" is just a
technical implementation details they're not aware of. Git just has
pretty colors by default, so I don't think it's jarring that "git help
xyz" and "man git-xyz" look different.

We also don't try to maintain the UI that:

    git cmd >file &&
    pager file

Gives you the same UX as:

    # Invokes pager(1) for you
    git cmd

Since we set e.g. PAGER_ENV already, I believe this was brought up in
past discussions. So we're already past the point of git adding its own
magic custom options to feed to the pager.

So I don't see the problem with having "a bit more like PAGER_ENV"
hidden behind a color.man=true config option in this case.

>> 	----
>> 	git-config - Get and set [...]
>>
>> 	SYNOPSIS
>> 	--------
>> 	[...]
>> 	'git config' [<file-option>] [...]
>> 	[...]
>> 	The `--type=<type>` option instructs 'git config' to ensure [...]
>>
>> Will have "NAME" and "SECTION" shown as BOLD RED instead of BOLD, "git
>> config" and other '-quoted parts in BLUE UNDERLINE instead of
>> UNDERLINE, and `--type=<type>` and other `-quoted parts in RED BOLD
>> instead of BOLD. The "Standout" setting is then used for the user's
>> own search bar (invoked with "/") and prompt. See [2] for more
>> examples
>
> There are BOLD RED and READ BOLD; are they differently rendered?

Yes, that's just an omission/mistake. It should be the "RED BOLD",
i.e. "<COLOR> <ATTR>".
Jeff King June 23, 2021, 11:58 p.m. UTC | #6
On Mon, Jun 21, 2021 at 09:08:20PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > [snip] I think it would be easier to understand to end-users
> > if this were exposed as a new "mode", like "git help --web" and "git
> > help --info" are different modes from the "git help --man",
> > something like "git help --fancy-man" (or whatever is easy to type
> > and explain, and also add it to the variants help.format knows about
> > to make it easy to set the default).
> >
> > One advantage of doing so is that we do not have to worry about "ah,
> > user has LESS_BLAH environment variable so we should disable this
> > new mode here" etc.  As long as the new mode is requested either via
> > the command line option or help.format configuration, it can
> > completely take it over.  That simplifies the necessary explanation
> > given to the users quite a lot, no?
> 
> The interaction between "git help" and "man"/"less" doesn't really have
> an equivalent in the rest of git as far as color output goes. Usually we
> emit colors via our own programs.
> 
> But no, I think it makes the most sense to consider this orthagonal to
> help.format=man or man.viewer=<cmd>.
> 
> We're not invoking a different man viewer or command, we're just
> expecting that mode to invoke the pager, and if that pager is less to
> have these variables tweak our color preferences.

FWIW, if we are going to do this, then just having it as "color.man"
makes the most sense to me. It is easily explained as "when we invoke
man, set up some environment variables that may enable colors in the
output".

I'm still entirely unconvinced that this should be in Git at all;
pointing GIT_MAN_VIEWER or man.*.cmd at a color-man wrapper seems like
it would be sufficient. But it feels like that conversation was not
going anywhere productive; I mention it here only to indicate that my
response above is not an endorsement of the concept.

-Peff
Jeff King June 24, 2021, 12:08 a.m. UTC | #7
On Mon, Jun 21, 2021 at 10:34:00AM +0200, Ævar Arnfjörð Bjarmason wrote:

> diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
> index e05d520a867..2f12ae3386d 100644
> --- a/Documentation/config/color.txt
> +++ b/Documentation/config/color.txt
> @@ -126,6 +126,12 @@ color.interactive.<slot>::
>  	or `error`, for four distinct types of normal output from
>  	interactive commands.
>  
> +color.man::
> +	This flag can be used to enable the automatic colorizaton of man
> +	pages when using the less pager, `false` by default. When set to
> +	`true` it's activated only when `color.ui` allows it, and if
> +	`color.pager` enable (which it is by default).

A few typos here:

diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
index 2f12ae3386..fcc12df508 100644
--- a/Documentation/config/color.txt
+++ b/Documentation/config/color.txt
@@ -127,10 +127,10 @@ color.interactive.<slot>::
 	interactive commands.
 
 color.man::
-	This flag can be used to enable the automatic colorizaton of man
+	This flag can be used to enable the automatic colorization of man
 	pages when using the less pager, `false` by default. When set to
 	`true` it's activated only when `color.ui` allows it, and if
-	`color.pager` enable (which it is by default).
+	`color.pager` is enabled (which it is by default).
 
 color.pager::
 	A boolean to specify whether `auto` color modes should colorize

The interaction with color.ui seems unusual. Normally it is not a
gate-keeper for specific colorizations, but rather a fallback when
more-specific color config is unspecified. E.g.:

  [color]
  ui = false
  branch = true

would colorize branch output, but nothing else. But from your
description (and I think the code matches this), doing:

  [color]
  ui = false
  man = true

would still disable the man-colors. So there's no way to enable this
feature without enabling colors everywhere else. I think it should
simply be independent of color.ui (with the exception that it may
eventually use it as a fallback like all the other color.* booleans,
_if_ we want to move it to default-to-on).

-Peff
Felipe Contreras June 24, 2021, 1:03 a.m. UTC | #8
Jeff King wrote:
> On Mon, Jun 21, 2021 at 09:08:20PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > > [snip] I think it would be easier to understand to end-users
> > > if this were exposed as a new "mode", like "git help --web" and "git
> > > help --info" are different modes from the "git help --man",
> > > something like "git help --fancy-man" (or whatever is easy to type
> > > and explain, and also add it to the variants help.format knows about
> > > to make it easy to set the default).
> > >
> > > One advantage of doing so is that we do not have to worry about "ah,
> > > user has LESS_BLAH environment variable so we should disable this
> > > new mode here" etc.  As long as the new mode is requested either via
> > > the command line option or help.format configuration, it can
> > > completely take it over.  That simplifies the necessary explanation
> > > given to the users quite a lot, no?
> > 
> > The interaction between "git help" and "man"/"less" doesn't really have
> > an equivalent in the rest of git as far as color output goes. Usually we
> > emit colors via our own programs.
> > 
> > But no, I think it makes the most sense to consider this orthagonal to
> > help.format=man or man.viewer=<cmd>.
> > 
> > We're not invoking a different man viewer or command, we're just
> > expecting that mode to invoke the pager, and if that pager is less to
> > have these variables tweak our color preferences.
> 
> FWIW, if we are going to do this, then just having it as "color.man"
> makes the most sense to me. It is easily explained as "when we invoke
> man, set up some environment variables that may enable colors in the
> output".
> 
> I'm still entirely unconvinced that this should be in Git at all;

That's OK, you don't need to be convinced for this change to be a
positive one.

> pointing GIT_MAN_VIEWER or man.*.cmd at a color-man wrapper seems like
> it would be sufficient.

What color-man wrapper?

> But it feels like that conversation was not going anywhere productive;

Feelings are not facts.

Bailing from a discussion doesn't resolve the discussion, and the
question "how is a user supposed to configure this properly?" remains
unanswered by you, or anyone [1].

This patch is the closest to a convenient solution anybody has come up
with.

If anybody has any other proposal it would be good to hear them.

[1] https://lore.kernel.org/git/60bfadc0aca09_1abb8f208fd@natae.notmuch/
Junio C Hamano June 28, 2021, 5:37 p.m. UTC | #9
Jeff King <peff@peff.net> writes:

> I'm still entirely unconvinced that this should be in Git at all;
> pointing GIT_MAN_VIEWER or man.*.cmd at a color-man wrapper seems like
> it would be sufficient. But it feels like that conversation was not
> going anywhere productive; I mention it here only to indicate that my
> response above is not an endorsement of the concept.

I have the same reaction to the patch.
Felipe Contreras June 28, 2021, 6:05 p.m. UTC | #10
Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > I'm still entirely unconvinced that this should be in Git at all;
> > pointing GIT_MAN_VIEWER or man.*.cmd at a color-man wrapper seems like
> > it would be sufficient. But it feels like that conversation was not
> > going anywhere productive; I mention it here only to indicate that my
> > response above is not an endorsement of the concept.
> 
> I have the same reaction to the patch.

Do you care to explain why you think so?

What is the alternative in your view? What configuration should users
use instead?
Junio C Hamano June 29, 2021, 1:42 a.m. UTC | #11
Jeff King <peff@peff.net> writes:

> .... But from your
> description (and I think the code matches this), doing:
>
>   [color]
>   ui = false
>   man = true
>
> would still disable the man-colors. So there's no way to enable this
> feature without enabling colors everywhere else. I think it should
> simply be independent of color.ui (with the exception that it may
> eventually use it as a fallback like all the other color.* booleans,
> _if_ we want to move it to default-to-on).

That matches my perception.  Thanks.
Felipe Contreras June 29, 2021, 1:48 a.m. UTC | #12
Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > .... But from your
> > description (and I think the code matches this), doing:
> >
> >   [color]
> >   ui = false
> >   man = true
> >
> > would still disable the man-colors. So there's no way to enable this
> > feature without enabling colors everywhere else. I think it should
> > simply be independent of color.ui (with the exception that it may
> > eventually use it as a fallback like all the other color.* booleans,
> > _if_ we want to move it to default-to-on).
> 
> That matches my perception.  Thanks.

This is already integrated into v8.

https://lore.kernel.org/git/20210626025040.104428-1-felipe.contreras@gmail.com/
diff mbox series

Patch

diff and grep. Let's do the same for man, but only if `color.man` is
explicitly set to "true".

Unlike other `color.*` output this colorization is not enabled by
`color.ui` being true, the user needs to explicitly set the
`color.man` variable to `true.

When it was proposed to treat `color.man` like any other `color.*`
variable some thought that git opting in coloring for an external
program such as man(1) was a step too far[1], even if the user invoked
it via the "git help <topic>" wrapper.

So let's make this explicitly opt-in for now. As noted in the
documentation we're leaving ourselves an out to turn this on by
default in the future, or e.g. putting it under the
feature.experimental umbrella. We probably won't, but let's not
promise users that `color.man` will forever be a special-case.

As for what this actually does the effect of having this enabled is
that a documentation blurb like (some parts elided with "[...]"):

	NAME
	----
	git-config - Get and set [...]

	SYNOPSIS
	--------
	[...]
	'git config' [<file-option>] [...]
	[...]
	The `--type=<type>` option instructs 'git config' to ensure [...]

Will have "NAME" and "SECTION" shown as BOLD RED instead of BOLD, "git
config" and other '-quoted parts in BLUE UNDERLINE instead of
UNDERLINE, and `--type=<type>` and other `-quoted parts in RED BOLD
instead of BOLD. The "Standout" setting is then used for the user's
own search bar (invoked with "/") and prompt. See [2] for more
examples

Normally check_auto_color() would check the value of `color.pager`, but
in this particular case it's not git the one executing the pager, but
man. Therefore we need to check pager_use_color ourselves.

We do not need to support `color.man` being set to `always`; The `git
help` command is always run for a tty (it would be very strange for a
user to do `git help $page > output`, but in fact, that works anyway,
we don't even need to check if stdout is a tty, but just to be
consistent we do). So it's simply a boolean in our case.

So, in order for this change to have any effect:

 1. color.man=true must be set in the config
 2. The user must use less
 3. Not have the same LESS_TERMCAP variables set (we call setenv(3) with overwrite=0)
 4. Have color.ui enabled
 5. Not have color.pager disabled
 6. Not have git with stdout directed to a file

1. https://lore.kernel.org/git/87tun1qp91.fsf@evledraar.gmail.com/
2. https://unix.stackexchange.com/questions/119/colors-in-man-pages/147

Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Tue, Jun 08 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> I've been running with this on my personal git build since May 26th. I
>> haven't had any issues with it, and I like the new coloring.
>> ...
>> I think this is a good example of a change that we're better off just
>> merging down and then reverting if the wider audience of git users hates
>> it, rather than trying to come to some perfect consensus here
>> on-list.
>
> My impression was tht we already had a rough consensus here on-list
> that it may be good to educate users who like this "new coloring"
> like you do to configure their "less", so that they consistently get
> the "new coloring" they like whether they are doing "git help git",
> "man git", or even "man ls", and the approach the posted patch takes
> will not help (it only affects "git help git" among these).
>
> I'd rather not to take it.

Fair enough, here's a version I think you and others will find
acceptable then. It allows users like me who like this to explicitly
opt-in via color.man=true.

I also took the liberty of making some changes to the commit message
to reword it for this new behavior, and to show an example of the sort
of colors that change with this patch.

The only change to the patch itself is that now color_man is set to 0
by default, not 1. I also moved its declaration so that it's with the
one other config variable, insted of with the other command-line
options to "git help".

Range-diff against v6:
1:  e021ca1da21 ! 1:  a950ef49e28 help: colorize man pages
    @@ Metadata
     Author: Felipe Contreras <felipe.contreras@gmail.com>
     
      ## Commit message ##
    -    help: colorize man pages
    +    help: colorize man pages if man.color=true under less(1)
     
         We already colorize tools traditionally not colorized by default, like
    -    diff and grep. Let's do the same for man.
    -
    -    Our man pages don't contain many useful colors (just blue links),
    -    moreover, many people have groff SGR disabled, so they don't see any
    -    colors with man pages.
    -
    -    We can set LESS_TERMCAP variables to render bold and underlined text
    -    with colors in the pager; a common trick[1].
    -
    -    Bold is rendered as red, underlined as blue, and standout (prompt and
    -    highlighted search) as inverse cyan.
    -
    -    Obviously this only works when the less pager is used.
    -
    -    If the user already has LESS_TERMCAP variables set in his/her
    -    environment, those are respected and nothing changes.
    -
    -    A new color configuration is added: `color.man` for the people that want
    -    to turn this feature off, otherwise `color.ui` is respected.
    -    Additionally, if color.pager is not enabled, this is disregarded.
    +    diff and grep. Let's do the same for man, but only if `color.man` is
    +    explicitly set to "true".
    +
    +    Unlike other `color.*` output this colorization is not enabled by
    +    `color.ui` being true, the user needs to explicitly set the
    +    `color.man` variable to `true.
    +
    +    When it was proposed to treat `color.man` like any other `color.*`
    +    variable some thought that git opting in coloring for an external
    +    program such as man(1) was a step too far[1], even if the user invoked
    +    it via the "git help <topic>" wrapper.
    +
    +    So let's make this explicitly opt-in for now. As noted in the
    +    documentation we're leaving ourselves an out to turn this on by
    +    default in the future, or e.g. putting it under the
    +    feature.experimental umbrella. We probably won't, but let's not
    +    promise users that `color.man` will forever be a special-case.
    +
    +    As for what this actually does the effect of having this enabled is
    +    that a documentation blurb like (some parts elided with "[...]"):
    +
    +            NAME
    +            ----
    +            git-config - Get and set [...]
    +
    +            SYNOPSIS
    +            --------
    +            [...]
    +            'git config' [<file-option>] [...]
    +            [...]
    +            The `--type=<type>` option instructs 'git config' to ensure [...]
    +
    +    Will have "NAME" and "SECTION" shown as BOLD RED instead of BOLD, "git
    +    config" and other '-quoted parts in BLUE UNDERLINE instead of
    +    UNDERLINE, and `--type=<type>` and other `-quoted parts in RED BOLD
    +    instead of BOLD. The "Standout" setting is then used for the user's
    +    own search bar (invoked with "/") and prompt. See [2] for more
    +    examples
     
         Normally check_auto_color() would check the value of `color.pager`, but
         in this particular case it's not git the one executing the pager, but
         man. Therefore we need to check pager_use_color ourselves.
     
    -    Also--unlike other color.* configurations--color.man=always does not
    -    make any sense here; `git help` is always run for a tty (it would be very
    -    strange for a user to do `git help $page > output`, but in fact, that
    -    works anyway, we don't even need to check if stdout is a tty, but just
    -    to be consistent we do). So it's simply a boolean in our case.
    +    We do not need to support `color.man` being set to `always`; The `git
    +    help` command is always run for a tty (it would be very strange for a
    +    user to do `git help $page > output`, but in fact, that works anyway,
    +    we don't even need to check if stdout is a tty, but just to be
    +    consistent we do). So it's simply a boolean in our case.
     
         So, in order for this change to have any effect:
     
    -     1. The user must use less
    -     2. Not have the same LESS_TERMCAP variables set
    -     3. Have color.ui enabled
    -     4. Not have color.pager disabled
    -     5. Not have color.man disabled
    -     7. Not have git with stdout directed to a file
    -
    -    Fortunately the vast majority of our users meet all of the above, and
    -    anybody who doesn't would not be affected negatively (plus very likely
    -    comprises a very tiny minority).
    +     1. color.man=true must be set in the config
    +     2. The user must use less
    +     3. Not have the same LESS_TERMCAP variables set (we call setenv(3) with overwrite=0)
    +     4. Have color.ui enabled
    +     5. Not have color.pager disabled
    +     6. Not have git with stdout directed to a file
     
    -    [1] https://unix.stackexchange.com/questions/119/colors-in-man-pages/147
    +    1. https://lore.kernel.org/git/87tun1qp91.fsf@evledraar.gmail.com/
    +    2. https://unix.stackexchange.com/questions/119/colors-in-man-pages/147
     
         Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    -    Phillip Wood <phillip.wood123@gmail.com>
    -    Comments-by: Jeff King <peff@peff.net>
         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Documentation/config/color.txt ##
     @@ Documentation/config/color.txt: color.interactive.<slot>::
    @@ Documentation/config/color.txt: color.interactive.<slot>::
      	interactive commands.
      
     +color.man::
    -+	This flag can be used to disable the automatic colorizaton of man
    -+	pages when using the less pager. It's activated only when color.ui
    -+	allows it, and also when color.pager is on. (`true` by default).
    ++	This flag can be used to enable the automatic colorizaton of man
    ++	pages when using the less pager, `false` by default. When set to
    ++	`true` it's activated only when `color.ui` allows it, and if
    ++	`color.pager` enable (which it is by default).
     +
      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
    +@@ Documentation/config/color.txt: color.ui::
    + 	output not intended for machine consumption to use color, to
    + 	`true` or `auto` (this is the default since Git 1.8.4) if you
    + 	want such output to use color when written to the terminal.
    +++
    ++When set to `true` certain other `color.*` variables may still not be
    ++turned on unless explicitly enabled. Currently this only applies to
    ++`color.man`, see above. Such opt-in variables may be moved under the
    ++default `color.ui` umbrella in the future.
     
      ## builtin/help.c ##
     @@
    @@ builtin/help.c
      
      #ifndef DEFAULT_HELP_FORMAT
      #define DEFAULT_HELP_FORMAT "man"
    -@@ builtin/help.c: static int verbose = 1;
    - static unsigned int colopts;
    - static enum help_format help_format = HELP_FORMAT_NONE;
    - static int exclude_guides;
    -+static int man_color = 1;
    - static struct option builtin_help_options[] = {
    - 	OPT_BOOL('a', "all", &show_all, N_("print all available commands")),
    - 	OPT_HIDDEN_BOOL(0, "exclude-guides", &exclude_guides, N_("exclude guides")),
    +@@ builtin/help.c: enum help_format {
    + 	HELP_FORMAT_WEB
    + };
    + 
    ++static int man_color;
    + static const char *html_path;
    + 
    + static int show_all = 0;
     @@ builtin/help.c: static void exec_man_konqueror(const char *path, const char *page)
      	}
      }

 Documentation/config/color.txt | 11 +++++++++++
 builtin/help.c                 | 32 +++++++++++++++++++++++++++++++-
 color.h                        |  1 +
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
index e05d520a867..2f12ae3386d 100644
--- a/Documentation/config/color.txt
+++ b/Documentation/config/color.txt
@@ -126,6 +126,12 @@  color.interactive.<slot>::
 	or `error`, for four distinct types of normal output from
 	interactive commands.
 
+color.man::
+	This flag can be used to enable the automatic colorizaton of man
+	pages when using the less pager, `false` by default. When set to
+	`true` it's activated only when `color.ui` allows it, and if
+	`color.pager` enable (which it is by default).
+
 color.pager::
 	A boolean to specify whether `auto` color modes should colorize
 	output going to the pager. Defaults to true; set this to false
@@ -200,3 +206,8 @@  color.ui::
 	output not intended for machine consumption to use color, to
 	`true` or `auto` (this is the default since Git 1.8.4) if you
 	want such output to use color when written to the terminal.
++
+When set to `true` certain other `color.*` variables may still not be
+turned on unless explicitly enabled. Currently this only applies to
+`color.man`, see above. Such opt-in variables may be moved under the
+default `color.ui` umbrella in the future.
diff --git a/builtin/help.c b/builtin/help.c
index bb339f0fc80..c607da88d78 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -11,6 +11,7 @@ 
 #include "config-list.h"
 #include "help.h"
 #include "alias.h"
+#include "color.h"
 
 #ifndef DEFAULT_HELP_FORMAT
 #define DEFAULT_HELP_FORMAT "man"
@@ -34,6 +35,7 @@  enum help_format {
 	HELP_FORMAT_WEB
 };
 
+static int man_color;
 static const char *html_path;
 
 static int show_all = 0;
@@ -253,10 +255,33 @@  static void exec_man_konqueror(const char *path, const char *page)
 	}
 }
 
+static void colorize_man(void)
+{
+	if (!man_color || !want_color(GIT_COLOR_UNKNOWN) || !pager_use_color)
+		return;
+
+	/* Disable groff colors */
+	setenv("GROFF_NO_SGR", "1", 0);
+
+	/* Bold */
+	setenv("LESS_TERMCAP_md", GIT_COLOR_BOLD_RED, 0);
+	setenv("LESS_TERMCAP_me", GIT_COLOR_RESET, 0);
+
+	/* Underline */
+	setenv("LESS_TERMCAP_us", GIT_COLOR_BLUE GIT_COLOR_UNDERLINE, 0);
+	setenv("LESS_TERMCAP_ue", GIT_COLOR_RESET, 0);
+
+	/* Standout */
+	setenv("LESS_TERMCAP_so", GIT_COLOR_CYAN GIT_COLOR_REVERSE, 0);
+	setenv("LESS_TERMCAP_se", GIT_COLOR_RESET, 0);
+}
+
 static void exec_man_man(const char *path, const char *page)
 {
 	if (!path)
 		path = "man";
+
+	colorize_man();
 	execlp(path, "man", page, (char *)NULL);
 	warning_errno(_("failed to exec '%s'"), path);
 }
@@ -264,6 +289,7 @@  static void exec_man_man(const char *path, const char *page)
 static void exec_man_cmd(const char *cmd, const char *page)
 {
 	struct strbuf shell_cmd = STRBUF_INIT;
+	colorize_man();
 	strbuf_addf(&shell_cmd, "%s %s", cmd, page);
 	execl(SHELL_PATH, SHELL_PATH, "-c", shell_cmd.buf, (char *)NULL);
 	warning(_("failed to exec '%s'"), cmd);
@@ -371,8 +397,12 @@  static int git_help_config(const char *var, const char *value, void *cb)
 	}
 	if (starts_with(var, "man."))
 		return add_man_viewer_info(var, value);
+	if (!strcmp(var, "color.man")) {
+		man_color = git_config_bool(var, value);
+		return 0;
+	}
 
-	return git_default_config(var, value, cb);
+	return git_color_default_config(var, value, cb);
 }
 
 static struct cmdnames main_cmds, other_cmds;
diff --git a/color.h b/color.h
index 98894d6a175..d012add4e8a 100644
--- a/color.h
+++ b/color.h
@@ -51,6 +51,7 @@  struct strbuf;
 #define GIT_COLOR_FAINT		"\033[2m"
 #define GIT_COLOR_FAINT_ITALIC	"\033[2;3m"
 #define GIT_COLOR_REVERSE	"\033[7m"
+#define GIT_COLOR_UNDERLINE	"\033[4m"
 
 /* A special value meaning "no color selected" */
 #define GIT_COLOR_NIL "NIL"