Patchwork D2079: color: honor NO_COLOR

login
register
mail settings
Submitter phabricator
Date Feb. 7, 2018, 5:17 p.m.
Message ID <differential-rev-PHID-DREV-yzmzwoyea5xfhov433br-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27432/
State New
Headers show

Comments

phabricator - Feb. 7, 2018, 5:17 p.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The http://no-color.org/ initiative is trying to get programs that
  emit color by default to honor a NO_COLOR environment variable to
  disable color.
  
  I think that's a good idea. So this commit implements support for
  NO_COLOR.
  
  I'm not sure if the precedence of settings is proper here. Right now,
  NO_COLOR overrides config settings set by hgrc or --config. But it
  doesn't override --color. I can see an argument for honoring
  --config as well. Same for hgrc (since color is enabled by default
  these days). But the existing logic/precedence is unclear to me. So
  I went with an easy implementation.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2079

AFFECTED FILES
  mercurial/color.py
  tests/test-status-color.t

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel, spectral
phabricator - Feb. 7, 2018, 8:41 p.m.
quark added a comment.


  You might want to let run-tests.py drop NO_COLOR for tests.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2079

To: indygreg, #hg-reviewers, lothiraldan
Cc: quark, lothiraldan, mercurial-devel
phabricator - Feb. 10, 2018, 12:23 p.m.
yuja added a comment.


  > I'm not sure if the precedence of settings is proper here.
  
  Perhaps it should be placed at the same level as `$EDITOR` (i.e. envrcitems)?
  
  And can you update help/color and environment?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2079

To: indygreg, #hg-reviewers, lothiraldan
Cc: yuja, quark, lothiraldan, mercurial-devel
phabricator - Feb. 10, 2018, 4:26 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D2079#35154, @yuja wrote:
  
  > > I'm not sure if the precedence of settings is proper here.
  >
  > Perhaps it should be placed at the same level as `$EDITOR` (i.e. envrcitems)?
  
  
  I haven't bothered checking what that means for precedence, but I have an argument for giving NO_COLOR higher precedence than at least the system config: we set the system config for all our users at Google and some may want to override with NO_COLOR. I know someone (Jun? Yuya?) did some work related to precedence of EDITOR a while ago, but I don't remember what the goal of that was.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2079

To: indygreg, #hg-reviewers, lothiraldan
Cc: martinvonz, yuja, quark, lothiraldan, mercurial-devel
phabricator - Feb. 10, 2018, 7:13 p.m.
quark added a subscriber: durin42.
quark added a comment.


  The goal was to allow users to override system default using environment variables. So a separate `[systemdefaults]` (proposed by @durin42) becomes unnecessary.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2079

To: indygreg, #hg-reviewers, lothiraldan
Cc: durin42, martinvonz, yuja, quark, lothiraldan, mercurial-devel
phabricator - Feb. 15, 2018, 12:22 p.m.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  (just clarify the current state of this patch)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2079

To: indygreg, #hg-reviewers, lothiraldan, yuja
Cc: durin42, martinvonz, yuja, quark, lothiraldan, mercurial-devel

Patch

diff --git a/tests/test-status-color.t b/tests/test-status-color.t
--- a/tests/test-status-color.t
+++ b/tests/test-status-color.t
@@ -46,6 +46,42 @@ 
   [status.unknown|? ][status.unknown|b/in_b] (glob)
   [status.unknown|? ][status.unknown|in_root]
 
+NO_COLOR disables color
+
+  $ NO_COLOR=1 hg status
+  ? a/1/in_a_1
+  ? a/in_a
+  ? b/1/in_b_1
+  ? b/2/in_b_2
+  ? b/in_b
+  ? in_root
+
+  $ NO_COLOR=0 hg status
+  ? a/1/in_a_1
+  ? a/in_a
+  ? b/1/in_b_1
+  ? b/2/in_b_2
+  ? b/in_b
+  ? in_root
+
+  $ NO_COLOR= hg status
+  ? a/1/in_a_1
+  ? a/in_a
+  ? b/1/in_b_1
+  ? b/2/in_b_2
+  ? b/in_b
+  ? in_root
+
+NO_COLOR is overridden by --color
+
+  $ NO_COLOR=1 hg --color=always status
+  \x1b[0;35;1;4m? \x1b[0m\x1b[0;35;1;4ma/1/in_a_1\x1b[0m (esc)
+  \x1b[0;35;1;4m? \x1b[0m\x1b[0;35;1;4ma/in_a\x1b[0m (esc)
+  \x1b[0;35;1;4m? \x1b[0m\x1b[0;35;1;4mb/1/in_b_1\x1b[0m (esc)
+  \x1b[0;35;1;4m? \x1b[0m\x1b[0;35;1;4mb/2/in_b_2\x1b[0m (esc)
+  \x1b[0;35;1;4m? \x1b[0m\x1b[0;35;1;4mb/in_b\x1b[0m (esc)
+  \x1b[0;35;1;4m? \x1b[0m\x1b[0;35;1;4min_root\x1b[0m (esc)
+
 hg status with template
   $ hg status -T "{label('red', path)}\n" --color=debug
   [red|a/1/in_a_1]
diff --git a/mercurial/color.py b/mercurial/color.py
--- a/mercurial/color.py
+++ b/mercurial/color.py
@@ -198,6 +198,13 @@ 
     if config == 'debug':
         return 'debug'
 
+    # The http://no-color.org/ initiative wants to standardize on an environment
+    # variable to disable color. The value of this variable doesn't matter.
+    if 'NO_COLOR' in encoding.environ:
+        # Allow --color CLI argument to override NO_COLOR
+        if ui.configsource('ui', 'color') != '--color':
+            return None
+
     auto = (config == 'auto')
     always = False
     if not auto and util.parsebool(config):