Re: [RFC PATCH] color: respect the $NO_COLOR convention

[Junio C Hamano]

"brian m. carlson"  writes:

> As a note, turning off color can improve accessibility for some people.
> I have a co-worker who has deuteranomaly and virtually all colored text
> at the terminal poses readability problems.  It would be beneficial if
> he could just set NO_COLOR=1 in his environment and have everything just
> work.
>
> For this reason, I'm in favor of taking this patch, assuming it comes
> with tests.

Oh, I agree 100% the world would be a better place if there already
is an established way to turn off all colors, instead of having to
run around and setting tool specific configuration like LS_COLORS
etc. for 42 different tools one uses during one's daily life.  I
just am not getting the feeling this no-color.org's effort is the
one.  We already have a way specific to our project already (i.e.
configuration variables), so if we adopt NO_COLOR but other people
do not universally support it (and they support something else),
we'd end up having to maintain yet another knob that only a handful
of projects understand forever, and that is where my reluctance
comes from.

Re: [RFC PATCH] color: respect the $NO_COLOR convention

[brian m. carlson]

On Thu, Mar 01, 2018 at 11:06:45AM -0800, Junio C Hamano wrote:
> Leah Neukirchen  writes:
> 
> > You are right in calling this out an emerging new thing, but the
> > second list of that page proves that it will be useful to settle on a
> > common configuration, and my hope is by getting a few popular projects
> > on board, others will soon follow.  It certainly is easy to implement,
> > and rather unintrusive.  Users which don't know about this feature are
> > completely unaffected.
> 
> There certainly is chicken-and-egg problem there.  Even though I
> personally prefer not to see overuse of colors, I am not sure if
> we the Git community as a whole would want to be involved until it
> gets mainstream.

As a note, turning off color can improve accessibility for some people.
I have a co-worker who has deuteranomaly and virtually all colored text
at the terminal poses readability problems.  It would be beneficial if
he could just set NO_COLOR=1 in his environment and have everything just
work.

For this reason, I'm in favor of taking this patch, assuming it comes
with tests.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [RFC PATCH] color: respect the $NO_COLOR convention

[Junio C Hamano]

Leah Neukirchen  writes:

> You are right in calling this out an emerging new thing, but the
> second list of that page proves that it will be useful to settle on a
> common configuration, and my hope is by getting a few popular projects
> on board, others will soon follow.  It certainly is easy to implement,
> and rather unintrusive.  Users which don't know about this feature are
> completely unaffected.

There certainly is chicken-and-egg problem there.  Even though I
personally prefer not to see overuse of colors, I am not sure if
we the Git community as a whole would want to be involved until it
gets mainstream.

>>> if (color_stdout_is_tty < 0)
>>> color_stdout_is_tty = isatty(1);
>>> if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
>>
>> According to no-color.org's FAQ #2, NO_COLOR should affect only the
>> "default" behaviour, and should stay back if there is an explicit
>> end-user configuration (or command line override).  And this helper
>> function is called only from want_color() when their is no such
>> higher precedence setting, which is in line with the recommendation.
>>
>> Which is good.
>
> Yes, I took care of that.  Should this also be tested?  It doesn't
> quite fit into the setting of t4026-color.sh I think.

It probably fits much better in t7006, I would suspect.  Earlier,
setting color.ui to auto meant the output is colored when run under
test_terminal, but with this new environment set, the output will
have to be bland.

Re: [RFC PATCH] color: respect the $NO_COLOR convention

[Leah Neukirchen]

Junio C Hamano  writes:

> Leah Neukirchen  writes:
>
>> NO_COLOR (http://no-color.org/) is a comprehensive approach to disable
>> colors by default for all tools:
>
> The list of software that supports that "convention" is, eh,
> respectable.  Is it really a "convention" yet, or yet another thing
> the user needs to worry about?

You are right in calling this out an emerging new thing, but the
second list of that page proves that it will be useful to settle on a
common configuration, and my hope is by getting a few popular projects
on board, others will soon follow.  It certainly is easy to implement,
and rather unintrusive.  Users which don't know about this feature are
completely unaffected.

>>  if (color_stdout_is_tty < 0)
>>  color_stdout_is_tty = isatty(1);
>>  if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
>
> According to no-color.org's FAQ #2, NO_COLOR should affect only the
> "default" behaviour, and should stay back if there is an explicit
> end-user configuration (or command line override).  And this helper
> function is called only from want_color() when their is no such
> higher precedence setting, which is in line with the recommendation.
>
> Which is good.

Yes, I took care of that.  Should this also be tested?  It doesn't
quite fit into the setting of t4026-color.sh I think.

Thanks,
-- 
Leah Neukirchen    http://leah.zone

Re: [RFC PATCH] color: respect the $NO_COLOR convention

[Junio C Hamano]

Leah Neukirchen  writes:

> NO_COLOR (http://no-color.org/) is a comprehensive approach to disable
> colors by default for all tools:

The list of software that supports that "convention" is, eh,
respectable.  Is it really a "convention" yet, or yet another thing
the user needs to worry about?

> diff --git a/color.c b/color.c
> index d48dd947c..59e9c2459 100644
> --- a/color.c
> +++ b/color.c
> @@ -326,6 +326,8 @@ int git_config_colorbool(const char *var, const char 
> *value)
>  
>  static int check_auto_color(void)
>  {
> + if (getenv("NO_COLOR"))
> + return 0;

Our convention often calls for CONFIG_VAR=false to mean "I do not
want to see what CONFIG_VAR wants to do done", i.e.

NO_COLOR=false git show

would show colored output if there is no other settings.  But this
code contradicts the convention, deliberately because that is what
no-color.org wants.  Makes me wonder if that convention is worth
following in the first place.

>   if (color_stdout_is_tty < 0)
>   color_stdout_is_tty = isatty(1);
>   if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {

According to no-color.org's FAQ #2, NO_COLOR should affect only the
"default" behaviour, and should stay back if there is an explicit
end-user configuration (or command line override).  And this helper
function is called only from want_color() when their is no such
higher precedence setting, which is in line with the recommendation.

Which is good.

[RFC PATCH] color: respect the $NO_COLOR convention

[Leah Neukirchen]

When the NO_COLOR environment variable is set to any value, default to
disabling color, i.e. resolve 'auto' to false.

NO_COLOR (http://no-color.org/) is a comprehensive approach to disable
colors by default for all tools:
> All command-line software which outputs text with ANSI color added
> should check for the presence of a NO_COLOR environment variable that,
> when present (regardless of its value), prevents the addition of ANSI
> color.

Signed-off-by: Leah Neukirchen 
---

This is a first stab at implementing NO_COLOR for git, effectively
making it then behave like before colors were enabled by default.

I feel this should be documented somewhere, but I'm not sure where the
best place is.  Perhaps in config.ui, or the Git environment variables
(but they all start with GIT_).

 color.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/color.c b/color.c
index d48dd947c..59e9c2459 100644
--- a/color.c
+++ b/color.c
@@ -326,6 +326,8 @@ int git_config_colorbool(const char *var, const char *value)
 
 static int check_auto_color(void)
 {
+   if (getenv("NO_COLOR"))
+   return 0;
if (color_stdout_is_tty < 0)
color_stdout_is_tty = isatty(1);
if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
-- 
2.16.2