[PATCH v4] help: colorize man pages

[PATCH v4] help: colorize man pages

[Felipe Contreras]

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 the LESS variable to render bold, underlined, and standout
text with colors in the less pager.

Bold is rendered as red, underlined as blue, and standout (prompt and
highlighted search) as inverse magenta.

Obviously this only works when the less pager is used.

If the user has already set the LESS variable in his/her environment,
that is 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.

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 automatically [probably thanks to less being smart], 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.

Moreover, just to be painstakingly comprehensive with people who have
color-aversion; we honour NO_COLOR [1].

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

 1. The user must use less
 2. Not have the LESS variable set
 3. Have color.ui enabled
 4. Not have color.pager disabled
 5. Not have color.man disabled
 6. Not have NO_COLOR set
 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] https://no-color.org/

Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Comments-by: Jeff King <peff@peff.net>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
Range-diff against v3:
1:  db93bf432b ! 1:  7249785014 help: colorize man pages
    @@ Metadata
      ## Commit message ##
         help: colorize man pages
     
    +    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].
    +    We can set the LESS variable to render bold, underlined, and standout
    +    text with colors in the less pager.
     
    -    Bold is rendered as red, underlined as blue, and standout (messages and
    +    Bold is rendered as red, underlined as blue, and standout (prompt and
         highlighted search) as inverse magenta.
     
    -    This only works when the pager is less.
    +    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 not overwritten.
    +    If the user has already set the LESS variable in his/her environment,
    +    that is 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, and in
    -    addition color.pager needs to be turned on.
    +    to turn this feature off, otherwise `color.ui` is respected.
    +    Additionally, if color.pager is not enabled, this is disregarded.
     
         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; git help is always run for a tty (it would be very
    +    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 automatically [probably thanks to less being smart], 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 to have an effect:
    +    Moreover, just to be painstakingly comprehensive with people who have
    +    color-aversion; we honour NO_COLOR [1].
    +
    +    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
    +     2. Not have the LESS variable set
          3. Have color.ui enabled
    -     4. Have color.pager enabled
    +     4. Not have color.pager disabled
          5. Not have color.man disabled
    -     6. Run git with stdout on a tty
    +     6. Not have NO_COLOR set
    +     7. Not have git with stdout directed to a file
     
    -    Otherwise the current behavior remains.
    +    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] https://unix.stackexchange.com/questions/119/colors-in-man-pages/147
    +    [1] https://no-color.org/
     
         Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
         Comments-by: Jeff King <peff@peff.net>
    @@ builtin/help.c: static void exec_man_konqueror(const char *path, const char *pag
     +	if (!man_color || !want_color(GIT_COLOR_UNKNOWN) || !pager_use_color)
     +		return;
     +
    ++	/* See: https://no-color.org/ */
    ++	if (getenv("NO_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_MAGENTA GIT_COLOR_REVERSE, 0);
    -+	setenv("LESS_TERMCAP_se", GIT_COLOR_RESET, 0);
    ++	/* Add red to bold, blue to underline, and magenta to standout */
    ++	/* No visual information is lost */
    ++	setenv("LESS", "Dd+r$Du+b$Ds", 0);
     +}
     +
      static void exec_man_man(const char *path, const char *page)
    @@ builtin/help.c: static int git_help_config(const char *var, const char *value, v
      }
      
      static struct cmdnames main_cmds, other_cmds;
    -
    - ## color.h ##
    -@@ color.h: 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"

 Documentation/config/color.txt |  5 +++++
 builtin/help.c                 | 28 +++++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
index d5daacb13a..11278b7f72 100644
--- a/Documentation/config/color.txt
+++ b/Documentation/config/color.txt
@@ -126,6 +126,11 @@ color.interactive.<slot>::
 	or `error`, for four distinct types of normal output from
 	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).
+
 color.pager::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).
diff --git a/builtin/help.c b/builtin/help.c
index bb339f0fc8..298d97cc39 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"
@@ -43,6 +44,7 @@ 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")),
@@ -253,10 +255,29 @@ 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;
+
+	/* See: https://no-color.org/ */
+	if (getenv("NO_COLOR"))
+		return;
+
+	/* Disable groff colors */
+	setenv("GROFF_NO_SGR", "1", 0);
+
+	/* Add red to bold, blue to underline, and magenta to standout */
+	/* No visual information is lost */
+	setenv("LESS", "Dd+r$Du+b$Ds", 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 +285,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 +393,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;
-- 
2.31.1

Re: [PATCH v4] help: colorize man pages

[Phillip Wood]

On 20/05/2021 05:07, Felipe Contreras wrote:
> We already colorize tools traditionally not colorized by default, like
> diff and grep. Let's do the same for man.

I think there is a distinction between 'diff' and 'grep' where we are 
generating the content and help where we are running man - I would 
expect a man page to look the same whether it is displayed by 'man git 
foo' or 'git help foo'

> 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 the LESS variable to render bold, underlined, and standout
> text with colors in the less pager.
> 
> Bold is rendered as red, underlined as blue, and standout (prompt and
> highlighted search) as inverse magenta.
> 
> Obviously this only works when the less pager is used.
> 
> If the user has already set the LESS variable in his/her environment,
> that is respected, and nothing changes.

However if they have specified the colors they would like by using the 
LESS_TERMCAP_xx environment variables that the previous versions of this 
patch used their choice is overridden by this new patch.

I've got LESS_TERMCAP_xx set and running
	LESS='Dd+r$Du+b$Ds' man git add
changes the output colors

> 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.
> 
> 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

s/git the one/git is not/

Best Wishes

Phillip

> 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 automatically [probably thanks to less being smart], 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.
> 
> Moreover, just to be painstakingly comprehensive with people who have
> color-aversion; we honour NO_COLOR [1].
> 
> So, in order for this change to have any effect:
> 
>   1. The user must use less
>   2. Not have the LESS variable set
>   3. Have color.ui enabled
>   4. Not have color.pager disabled
>   5. Not have color.man disabled
>   6. Not have NO_COLOR set
>   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] https://no-color.org/
> 
> Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Comments-by: Jeff King <peff@peff.net>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> Range-diff against v3:
> 1:  db93bf432b ! 1:  7249785014 help: colorize man pages
>      @@ Metadata
>        ## Commit message ##
>           help: colorize man pages
>       
>      +    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].
>      +    We can set the LESS variable to render bold, underlined, and standout
>      +    text with colors in the less pager.
>       
>      -    Bold is rendered as red, underlined as blue, and standout (messages and
>      +    Bold is rendered as red, underlined as blue, and standout (prompt and
>           highlighted search) as inverse magenta.
>       
>      -    This only works when the pager is less.
>      +    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 not overwritten.
>      +    If the user has already set the LESS variable in his/her environment,
>      +    that is 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, and in
>      -    addition color.pager needs to be turned on.
>      +    to turn this feature off, otherwise `color.ui` is respected.
>      +    Additionally, if color.pager is not enabled, this is disregarded.
>       
>           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; git help is always run for a tty (it would be very
>      +    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 automatically [probably thanks to less being smart], 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 to have an effect:
>      +    Moreover, just to be painstakingly comprehensive with people who have
>      +    color-aversion; we honour NO_COLOR [1].
>      +
>      +    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
>      +     2. Not have the LESS variable set
>            3. Have color.ui enabled
>      -     4. Have color.pager enabled
>      +     4. Not have color.pager disabled
>            5. Not have color.man disabled
>      -     6. Run git with stdout on a tty
>      +     6. Not have NO_COLOR set
>      +     7. Not have git with stdout directed to a file
>       
>      -    Otherwise the current behavior remains.
>      +    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] https://unix.stackexchange.com/questions/119/colors-in-man-pages/147
>      +    [1] https://no-color.org/
>       
>           Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>           Comments-by: Jeff King <peff@peff.net>
>      @@ builtin/help.c: static void exec_man_konqueror(const char *path, const char *pag
>       +	if (!man_color || !want_color(GIT_COLOR_UNKNOWN) || !pager_use_color)
>       +		return;
>       +
>      ++	/* See: https://no-color.org/ */
>      ++	if (getenv("NO_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_MAGENTA GIT_COLOR_REVERSE, 0);
>      -+	setenv("LESS_TERMCAP_se", GIT_COLOR_RESET, 0);
>      ++	/* Add red to bold, blue to underline, and magenta to standout */
>      ++	/* No visual information is lost */
>      ++	setenv("LESS", "Dd+r$Du+b$Ds", 0);
>       +}
>       +
>        static void exec_man_man(const char *path, const char *page)
>      @@ builtin/help.c: static int git_help_config(const char *var, const char *value, v
>        }
>        
>        static struct cmdnames main_cmds, other_cmds;
>      -
>      - ## color.h ##
>      -@@ color.h: 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"
> 
>   Documentation/config/color.txt |  5 +++++
>   builtin/help.c                 | 28 +++++++++++++++++++++++++++-
>   2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
> index d5daacb13a..11278b7f72 100644
> --- a/Documentation/config/color.txt
> +++ b/Documentation/config/color.txt
> @@ -126,6 +126,11 @@ color.interactive.<slot>::
>   	or `error`, for four distinct types of normal output from
>   	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).
> +
>   color.pager::
>   	A boolean to enable/disable colored output when the pager is in
>   	use (default is true).
> diff --git a/builtin/help.c b/builtin/help.c
> index bb339f0fc8..298d97cc39 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"
> @@ -43,6 +44,7 @@ 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")),
> @@ -253,10 +255,29 @@ 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;
> +
> +	/* See: https://no-color.org/ */
> +	if (getenv("NO_COLOR"))
> +		return;
> +
> +	/* Disable groff colors */
> +	setenv("GROFF_NO_SGR", "1", 0);
> +
> +	/* Add red to bold, blue to underline, and magenta to standout */
> +	/* No visual information is lost */
> +	setenv("LESS", "Dd+r$Du+b$Ds", 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 +285,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 +393,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;
> 

Re: [PATCH v4] help: colorize man pages

[Felipe Contreras]

Phillip Wood wrote:
> On 20/05/2021 05:07, Felipe Contreras wrote:
> > We already colorize tools traditionally not colorized by default, like
> > diff and grep. Let's do the same for man.
> 
> I think there is a distinction between 'diff' and 'grep' where we are 
> generating the content and help where we are running man

It makes a difference for git developers, not for the user.

The user doesn't care how the output of `git grep` was generated, all
she sees is that it's different from `grep`. It's in fact more
surprising than a difference in `git help` because it's even the same
comand.

Maybe if the command was `git man` they would be equally surprising, but
it's not, in fact, `git help` can be used to 1) output directly to the
terminal 2) view in a browser, 3) view in info program, 4) view man page
in woman, 5) view the man page in koqueror 6) view the man page in man.

Only in one case among many would the user expect to see man, therefore
a colorized `git grep` is more surprising.

> > 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 the LESS variable to render bold, underlined, and standout
> > text with colors in the less pager.
> > 
> > Bold is rendered as red, underlined as blue, and standout (prompt and
> > highlighted search) as inverse magenta.
> > 
> > Obviously this only works when the less pager is used.
> > 
> > If the user has already set the LESS variable in his/her environment,
> > that is respected, and nothing changes.
> 
> However if they have specified the colors they would like by using the 
> LESS_TERMCAP_xx environment variables that the previous versions of this 
> patch used their choice is overridden by this new patch.

That is true. We could add a check for that:

  if (getenv("LESS_TERMCAP_md"))
          return;

However, it may not be necessary since many of the tips online set these
variables inside a function.

> I've got LESS_TERMCAP_xx set and running
> 	LESS='Dd+r$Du+b$Ds' man git add
> changes the output colors

You have them set in the environtment? Not inside a function like
man () { ... command man "$@" } ?

> > 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.
> > 
> > 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
> 
> s/git the one/git is not/

You mean s/it's not git/git is not/

Fine by me.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v4] help: colorize man pages

[Phillip Wood]

On 20/05/2021 14:58, Felipe Contreras wrote:
> Phillip Wood wrote:
>> On 20/05/2021 05:07, Felipe Contreras wrote:
>>> We already colorize tools traditionally not colorized by default, like
>>> diff and grep. Let's do the same for man.
>>
>> I think there is a distinction between 'diff' and 'grep' where we are
>> generating the content and help where we are running man
> 
> It makes a difference for git developers, not for the user.
> 
> The user doesn't care how the output of `git grep` was generated, all
> she sees is that it's different from `grep`. It's in fact more
> surprising than a difference in `git help` because it's even the same
> comand.
> 
> Maybe if the command was `git man` they would be equally surprising, but
> it's not, in fact, `git help` can be used to 1) output directly to the
> terminal 2) view in a browser, 3) view in info program, 4) view man page
> in woman, 5) view the man page in koqueror 6) view the man page in man.
> 
> Only in one case among many would the user expect to see man, therefore
> a colorized `git grep` is more surprising.

I'm not sure I follow that argument

>>> 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 the LESS variable to render bold, underlined, and standout
>>> text with colors in the less pager.
>>>
>>> Bold is rendered as red, underlined as blue, and standout (prompt and
>>> highlighted search) as inverse magenta.
>>>
>>> Obviously this only works when the less pager is used.
>>>
>>> If the user has already set the LESS variable in his/her environment,
>>> that is respected, and nothing changes.
>>
>> However if they have specified the colors they would like by using the
>> LESS_TERMCAP_xx environment variables that the previous versions of this
>> patch used their choice is overridden by this new patch.
> 
> That is true. We could add a check for that:
> 
>    if (getenv("LESS_TERMCAP_md"))
>            return;
> 
> However, it may not be necessary since many of the tips online set these
> variables inside a function.

The only person who has tested this patch has reported a problem with 
it, it seems unlikely that no other users will have similar issues. The 
Arch Linux wiki (which I think is probably where I got the idea to set 
LESS_TERMCAP_xx) has a section on less[1] suggesting that 
LESS_TERMCAP_xx is exported unconditionally in .bashrc, and a later on 
man suggesting setting them in a function.

Best Wishes

Phillip

[1]
https://wiki.archlinux.org/title/Color_output_in_console#less

>> I've got LESS_TERMCAP_xx set and running
>> 	LESS='Dd+r$Du+b$Ds' man git add
>> changes the output colors
> 
> You have them set in the environtment? Not inside a function like
> man () { ... command man "$@" } ?
> 
>>> 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.
>>>
>>> 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
>>
>> s/git the one/git is not/
> 
> You mean s/it's not git/git is not/
> 
> Fine by me.
> 
> Cheers.
> 

Re: [PATCH v4] help: colorize man pages

[Felipe Contreras]

Phillip Wood wrote:
> On 20/05/2021 14:58, Felipe Contreras wrote:
> > Phillip Wood wrote:
> >> On 20/05/2021 05:07, Felipe Contreras wrote:
> >>> We already colorize tools traditionally not colorized by default, like
> >>> diff and grep. Let's do the same for man.
> >>
> >> I think there is a distinction between 'diff' and 'grep' where we are
> >> generating the content and help where we are running man
> > 
> > It makes a difference for git developers, not for the user.
> > 
> > The user doesn't care how the output of `git grep` was generated, all
> > she sees is that it's different from `grep`. It's in fact more
> > surprising than a difference in `git help` because it's even the same
> > comand.
> > 
> > Maybe if the command was `git man` they would be equally surprising, but
> > it's not, in fact, `git help` can be used to 1) output directly to the
> > terminal 2) view in a browser, 3) view in info program, 4) view man page
> > in woman, 5) view the man page in koqueror 6) view the man page in man.
> > 
> > Only in one case among many would the user expect to see man, therefore
> > a colorized `git grep` is more surprising.
> 
> I'm not sure I follow that argument

Do this:

  git config --global help.format html
  git help git

Do you see a man page on less?
 
> >>> If the user has already set the LESS variable in his/her environment,
> >>> that is respected, and nothing changes.
> >>
> >> However if they have specified the colors they would like by using the
> >> LESS_TERMCAP_xx environment variables that the previous versions of this
> >> patch used their choice is overridden by this new patch.
> > 
> > That is true. We could add a check for that:
> > 
> >    if (getenv("LESS_TERMCAP_md"))
> >            return;
> > 
> > However, it may not be necessary since many of the tips online set these
> > variables inside a function.
> 
> The only person who has tested this patch has reported a problem with 
> it, it seems unlikely that no other users will have similar issues.

The check above will fix your problem, will it not?

-- 
Felipe Contreras

Re: [PATCH v4] help: colorize man pages

[Phillip Wood]

On 20/05/2021 16:59, Felipe Contreras wrote:
> Phillip Wood wrote:
>> On 20/05/2021 14:58, Felipe Contreras wrote:
>>> Phillip Wood wrote:
>>>> On 20/05/2021 05:07, Felipe Contreras wrote:
>>>>> [...]
>>>>> If the user has already set the LESS variable in his/her environment,
>>>>> that is respected, and nothing changes.
>>>>
>>>> However if they have specified the colors they would like by using the
>>>> LESS_TERMCAP_xx environment variables that the previous versions of this
>>>> patch used their choice is overridden by this new patch.
>>>
>>> That is true. We could add a check for that:
>>>
>>>     if (getenv("LESS_TERMCAP_md"))
>>>             return;
>>>
>>> However, it may not be necessary since many of the tips online set these
>>> variables inside a function.
>>
>> The only person who has tested this patch has reported a problem with
>> it, it seems unlikely that no other users will have similar issues.
> 
> The check above will fix your problem, will it not?

Yes it will if it is implemented which was not clear as your message 
suggested it may not be necessary. I think it would be safer to check 
LESS_TERMCAP_{md,us,so} and not set LESS if any of them are set as it is 
possible a user may only override some of them.

Best Wishes

Phillip

Re: [PATCH v4] help: colorize man pages

[Felipe Contreras]

Phillip Wood wrote:
> On 20/05/2021 16:59, Felipe Contreras wrote:
> > Phillip Wood wrote:
> >> On 20/05/2021 14:58, Felipe Contreras wrote:

> >>> That is true. We could add a check for that:
> >>>
> >>>     if (getenv("LESS_TERMCAP_md"))
> >>>             return;
> >>>
> >>> However, it may not be necessary since many of the tips online set these
> >>> variables inside a function.
> >>
> >> The only person who has tested this patch has reported a problem with
> >> it, it seems unlikely that no other users will have similar issues.
> > 
> > The check above will fix your problem, will it not?
> 
> Yes it will if it is implemented which was not clear as your message 
> suggested it may not be necessary.

It was a maybe.

> I think it would be safer to check LESS_TERMCAP_{md,us,so} and not set
> LESS if any of them are set as it is possible a user may only override
> some of them.

Sure, if we could set 6 variables before, we can check for 3 afterwards.

-- 
Felipe Contreras

Re: [PATCH v4] help: colorize man pages

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 20/05/2021 05:07, Felipe Contreras wrote:
>> We already colorize tools traditionally not colorized by default, like
>> diff and grep. Let's do the same for man.
>
> I think there is a distinction between 'diff' and 'grep' where we are
> generating the content and help where we are running man - I would 
> expect a man page to look the same whether it is displayed by 'man git
> foo' or 'git help foo'

... as long as the user chooses "man" backend, that is.  And I tend
to agree, but that is our expectation.

If we added this new mode of driving the same "man" but with
different environment variables exported to tweak how "less"
behaves, and taught it to builtin/help.c::exec_viewer() and
builtin/help.c::man_viewer_list, that might become more palatable in
the sense that we can view it as feeding the same manual page to
this another "man" that behaves differently from the plain "man",
just like we can feed it to "woman" or "konqueror" to get a different
view.  So those (like you and I) who expect a man page to look the
same in "man git foo" and "git help -m foo" can keep using our current
configuration, while those who want yet another variant of "man" output
in addition to the current "man", "woman", and "konqueror" can choose
it and get "colorized" output.

By the way, this new round mentions NO_COLOR, and while I think it
is good idea to teach git to honor it, I think it does it at a wrong
level.  Each ui driver that is optionally capable of coloring its
output shouldn't have to care, and the right level is either inside
want_color() or its helper function check_auto_color(), both in
color.c, to say "the user hasn't configured the output of this
subcommand for coloring, and by default we use color under certain
conditions (i.e. "auto"), but we decide not to use color because
NO_COLOR environment is set before even checking these "auto"
conditions.

Re: [PATCH v4] help: colorize man pages

[Jeff King]

On Fri, May 21, 2021 at 02:06:48PM +0900, Junio C Hamano wrote:

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > On 20/05/2021 05:07, Felipe Contreras wrote:
> >> We already colorize tools traditionally not colorized by default, like
> >> diff and grep. Let's do the same for man.
> >
> > I think there is a distinction between 'diff' and 'grep' where we are
> > generating the content and help where we are running man - I would 
> > expect a man page to look the same whether it is displayed by 'man git
> > foo' or 'git help foo'
> 
> ... as long as the user chooses "man" backend, that is.  And I tend
> to agree, but that is our expectation.
> 
> If we added this new mode of driving the same "man" but with
> different environment variables exported to tweak how "less"
> behaves, and taught it to builtin/help.c::exec_viewer() and
> builtin/help.c::man_viewer_list, that might become more palatable in
> the sense that we can view it as feeding the same manual page to
> this another "man" that behaves differently from the plain "man",
> just like we can feed it to "woman" or "konqueror" to get a different
> view.  So those (like you and I) who expect a man page to look the
> same in "man git foo" and "git help -m foo" can keep using our current
> configuration, while those who want yet another variant of "man" output
> in addition to the current "man", "woman", and "konqueror" can choose
> it and get "colorized" output.

I still don't understand what we gain by making this a Git feature, as
all of the changed behavior is totally within the program we are
calling. Imagine that konqueror (or an html viewer like firefox) had an
option to set its color scheme from the command line. Should we
introduce a new baked-in fancy-konqueror backend that is "run the tool
with a tweaked color scheme"?

Why would we do that versus saying: if you want to change the colors in
the tool that Git calls, then configure the tool?

If you like to see colors in manpages, why not configure "man" (either
by setting these environment variables all the time, or by triggering
them in MANPAGER)? And then Git doesn't have to care either way; it is
calling "man" which does what the user wants, colors or no. If you
really for some reason only want colorized man pages when called via
"git help", then why not set man.fancy.cmd to invoke your preferred
config?

If those configurations are awkward to trigger via man (e.g., putting
escapes into termcap variables), isn't that something that could be
improved in man? And then it would benefit everyone who uses man, not
just Git.

-Peff

Re: [PATCH v4] help: colorize man pages

[Felipe Contreras]

Jeff King wrote:
> On Fri, May 21, 2021 at 02:06:48PM +0900, Junio C Hamano wrote:
> 
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> > 
> > > On 20/05/2021 05:07, Felipe Contreras wrote:
> > >> We already colorize tools traditionally not colorized by default, like
> > >> diff and grep. Let's do the same for man.
> > >
> > > I think there is a distinction between 'diff' and 'grep' where we are
> > > generating the content and help where we are running man - I would 
> > > expect a man page to look the same whether it is displayed by 'man git
> > > foo' or 'git help foo'
> > 
> > ... as long as the user chooses "man" backend, that is.  And I tend
> > to agree, but that is our expectation.
> > 
> > If we added this new mode of driving the same "man" but with
> > different environment variables exported to tweak how "less"
> > behaves, and taught it to builtin/help.c::exec_viewer() and
> > builtin/help.c::man_viewer_list, that might become more palatable in
> > the sense that we can view it as feeding the same manual page to
> > this another "man" that behaves differently from the plain "man",
> > just like we can feed it to "woman" or "konqueror" to get a different
> > view.  So those (like you and I) who expect a man page to look the
> > same in "man git foo" and "git help -m foo" can keep using our current
> > configuration, while those who want yet another variant of "man" output
> > in addition to the current "man", "woman", and "konqueror" can choose
> > it and get "colorized" output.
> 
> I still don't understand what we gain by making this a Git feature,

What do we gain by making `git diff` output color?

> Why would we do that versus saying: if you want to change the colors in
> the tool that Git calls, then configure the tool?

Once again... How?

> If you like to see colors in manpages, why not configure "man" (either
> by setting these environment variables all the time, or by triggering
> them in MANPAGER)?

Let me try that...

  MANPAGER="less -Dd+r -Du+b -Ds+m" git help git

It doesn't work.

> If those configurations are awkward to trigger via man (e.g., putting
> escapes into termcap variables), isn't that something that could be
> improved in man? And then it would benefit everyone who uses man, not
> just Git.

Sure. In the meantime let's make `git help` output with color just like
`git diff`.

Cheers.

(and good luck convincing a GNU project of anything)

-- 
Felipe Contreras

Re: [PATCH v4] help: colorize man pages

[Jeff King]

On Fri, May 21, 2021 at 01:01:35PM -0500, Felipe Contreras wrote:

> > I still don't understand what we gain by making this a Git feature,
> 
> What do we gain by making `git diff` output color?

Huh? Git is outputting the diff. Who else would output the color?

> > Why would we do that versus saying: if you want to change the colors in
> > the tool that Git calls, then configure the tool?
> 
> Once again... How?

By exporting the environment variables that ask it to do so, just like
you showed already?

> > If you like to see colors in manpages, why not configure "man" (either
> > by setting these environment variables all the time, or by triggering
> > them in MANPAGER)?
> 
> Let me try that...
> 
>   MANPAGER="less -Dd+r -Du+b -Ds+m" git help git
> 
> It doesn't work.

  ESC=$(printf '\33')
  export MANCOLORS="LESS_TERMCAP_md=$ESC[31m LESS_TERMCAP_me=$ESC[0m"
  export MANPAGER='sh -c "eval $MANCOLORS less"'
  man ls
  git help git

At least on Linux, $MANPAGER is some weird limbo that is not run with
the shell, but not just a simple command. Hence the extra layer of "sh".

If I were actually planning to use this myself, I'd probably put it in a
"manpager" script in my $PATH and just do MANPAGER=manpager.

-Peff

Re: [PATCH v4] help: colorize man pages

[Felipe Contreras]

Jeff King wrote:
> On Fri, May 21, 2021 at 01:01:35PM -0500, Felipe Contreras wrote:
> 
> > > I still don't understand what we gain by making this a Git feature,
> > 
> > What do we gain by making `git diff` output color?
> 
> Huh? Git is outputting the diff. Who else would output the color?

Do you think our users know or care which binary has the final
connection to the tty?

Many probably think git is sending the output to `diff --color -u`, and
it doesn't matter at all.

> > > Why would we do that versus saying: if you want to change the colors in
> > > the tool that Git calls, then configure the tool?
> > 
> > Once again... How?
> 
> By exporting the environment variables that ask it to do so, just like
> you showed already?

Exporting MANPAGER is not enough. That would only work on systems that
have SGR disabled.

The user would have to in addition export GROFF_NO_SGR=1, but that would
disble groff color for everything, which may not be what the user wants.

There is no MANGROFFNOSGR.

> > > If you like to see colors in manpages, why not configure "man" (either
> > > by setting these environment variables all the time, or by triggering
> > > them in MANPAGER)?
> > 
> > Let me try that...
> > 
> >   MANPAGER="less -Dd+r -Du+b -Ds+m" git help git
> > 
> > It doesn't work.
> 
>   ESC=$(printf '\33')
>   export MANCOLORS="LESS_TERMCAP_md=$ESC[31m LESS_TERMCAP_me=$ESC[0m"
>   export MANPAGER='sh -c "eval $MANCOLORS less"'
>   man ls
>   git help git

That still doesn't work here.

https://snipboard.io/GmhRtU.jpg

I see the default docbook colos generated by groff, but not the ones you
specified (both on `man` and `git help`).

I need to do this as well:

  export GROFF_NO_SGR=1

Your system probably has groff's SGR disabled in /usr/share/groff/site-tmac/man.local

It's not that simple.

There is in fact a way to configure man to do what we want here but if
*nobody* knows what that way is, then does it really matter?

Cheers.

-- 
Felipe Contreras

Re: [PATCH v4] help: colorize man pages

[Jeff King]

On Fri, May 21, 2021 at 04:40:30PM -0500, Felipe Contreras wrote:

> Jeff King wrote:
> > On Fri, May 21, 2021 at 01:01:35PM -0500, Felipe Contreras wrote:
> > 
> > > > I still don't understand what we gain by making this a Git feature,
> > > 
> > > What do we gain by making `git diff` output color?
> > 
> > Huh? Git is outputting the diff. Who else would output the color?
> 
> Do you think our users know or care which binary has the final
> connection to the tty?

Yes. If we are telling them that "git help git" is using "man", which we
do, then I think they should expect it to behave like "man".

Moreover, I think that if they like colorized manpages, they'd probably
want them when running "man" themselves.

-Peff

Re: [PATCH v4] help: colorize man pages

[Philip Oakley]

On 22/05/2021 10:55, Jeff King wrote:
> On Fri, May 21, 2021 at 04:40:30PM -0500, Felipe Contreras wrote:
>
>> Jeff King wrote:
>>> On Fri, May 21, 2021 at 01:01:35PM -0500, Felipe Contreras wrote:
>>>
>>>>> I still don't understand what we gain by making this a Git feature,
>>>> What do we gain by making `git diff` output color?
>>> Huh? Git is outputting the diff. Who else would output the color?
>> Do you think our users know or care which binary has the final
>> connection to the tty?
> Yes. If we are telling them that "git help git" is using "man", which we
> do, then I think they should expect it to behave like "man".
>
> Moreover, I think that if they like colorized manpages, they'd probably
> want them when running "man" themselves.
>
> -Peff
And we have the whole Git for Windows community who don't have `man`
anyway...
It's a bit of a conundrum, especially when considering all the
'terminals' Windows folk maybe using.

Re: [PATCH v4] help: colorize man pages

[Felipe Contreras]

Philip Oakley wrote:
> And we have the whole Git for Windows community who don't have `man`
> anyway...

If they don't have man, then they won't be affected in any way.

-- 
Felipe Contreras

Re: [PATCH v4] help: colorize man pages

[Felipe Contreras]

Jeff King wrote:
> On Fri, May 21, 2021 at 04:40:30PM -0500, Felipe Contreras wrote:
> 
> > Jeff King wrote:
> > > On Fri, May 21, 2021 at 01:01:35PM -0500, Felipe Contreras wrote:
> > > 
> > > > > I still don't understand what we gain by making this a Git feature,
> > > > 
> > > > What do we gain by making `git diff` output color?
> > > 
> > > Huh? Git is outputting the diff. Who else would output the color?
> > 
> > Do you think our users know or care which binary has the final
> > connection to the tty?
> 
> Yes. If we are telling them that "git help git" is using "man", which we
> do, then I think they should expect it to behave like "man".

But we are not telling them.

Software is not in the business of explaining users exactly what it is
doing. Software is in the business of being useful to users, and in
order to do that it must remain as silent as possible while achieving
what the user potentially wants.

Unless we throw an advice("this command runs man"), then we are not
telling them.

If a dilligent user does `git help help` they might learn about this
fact, but we didn't tell them, they found out.

> Moreover, I think that if they like colorized manpages, they'd probably
> want them when running "man" themselves.

This doesn't matter.

The user might have "configured" man like this:

  man() {
      LESS_TERMCAP_md=$'\e[01;31m' \
      LESS_TERMCAP_me=$'\e[0m' \
      LESS_TERMCAP_so=$'\e[01;44;33m' \
      LESS_TERMCAP_se=$'\e[0m' \
      LESS_TERMCAP_us=$'\e[01;32m' \
      LESS_TERMCAP_ue=$'\e[0m' \
      command man "$@"
  }

Git isn't going utilize that.

Arch Linux recommends the above, and so does many online resources.

So even if it's the case what you said, that they want colorized man
pages, *and* they have man configured, that doesn't matter.

In addition, not everyone is a Linux guru. Some might want colorized man
pages, but not know how to get them.

I myself only learned it was possible to configure that about a year ago
when reading Arch Linux's installation guide. Luckily I clicked "Color
output in console", even though I thought I already had most console
software configured.

I have 20 years of experience using Linux. Some people have less.

You presume too much of our users.

And you still haven't explained how they can properly configure
colorized man pages for both man and git, in a way that works in all
distributions.

[1] https://wiki.archlinux.org/title/Color_output_in_console

-- 
Felipe Contreras

Re: [PATCH v4] help: colorize man pages

[Felipe Contreras]

Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > On 20/05/2021 05:07, Felipe Contreras wrote:
> >> We already colorize tools traditionally not colorized by default, like
> >> diff and grep. Let's do the same for man.
> >
> > I think there is a distinction between 'diff' and 'grep' where we are
> > generating the content and help where we are running man - I would 
> > expect a man page to look the same whether it is displayed by 'man git
> > foo' or 'git help foo'
> 
> ... as long as the user chooses "man" backend, that is.  And I tend
> to agree, but that is our expectation.
> 
> If we added this new mode of driving the same "man" but with
> different environment variables exported to tweak how "less"
> behaves, and taught it to builtin/help.c::exec_viewer() and
> builtin/help.c::man_viewer_list, that might become more palatable in
> the sense that we can view it as feeding the same manual page to
> this another "man" that behaves differently from the plain "man",
> just like we can feed it to "woman" or "konqueror" to get a different
> view.  So those (like you and I) who expect a man page to look the
> same in "man git foo" and "git help -m foo" can keep using our current
> configuration, while those who want yet another variant of "man" output
> in addition to the current "man", "woman", and "konqueror" can choose
> it and get "colorized" output.

So... "mancolor"?

> By the way, this new round mentions NO_COLOR, and while I think it
> is good idea to teach git to honor it, I think it does it at a wrong
> level.

Other people have already mentioend the FAQ [1]:

  It is reasonable to configure certain software such as a text editor
  to use color or other ANSI attributes sparingly (such as the reverse
  attribute for a status bar) while still desiring that other software
  not add color unless configured to.

At whatever level it's chosen it shouldn't blatantly disable all color.

> Each ui driver that is optionally capable of coloring its
> output shouldn't have to care,

But they do have to care. The purpose of NO_COLOR is not to disable all
color, but to disable annoying color.

https://no-color.org/

-- 
Felipe Contreras

Re: [PATCH v4] help: colorize man pages

[Leah Neukirchen]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> So, in order for this change to have any effect:
>
>  1. The user must use less
>  2. Not have the LESS variable set
>  3. Have color.ui enabled
>  4. Not have color.pager disabled
>  5. Not have color.man disabled
>  6. Not have NO_COLOR set
>  7. Not have git with stdout directed to a file

I can't review the code thoroughly right now, but if it works as
described here, +1 from me.

-- 
Leah Neukirchen  <leah@vuxu.org>  https://leahneukirchen.org