Patchwork [stable] color: tweak behavior of ui.color config knob

login
register
mail settings
Submitter Augie Fackler
Date May 2, 2017, 1:13 a.m.
Message ID <9524b0b5eedb877c4dc2.1493687592@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/20346/
State Rejected
Headers show

Comments

Augie Fackler - May 2, 2017, 1:13 a.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1493678508 14400
#      Mon May 01 18:41:48 2017 -0400
# Branch stable
# Node ID 9524b0b5eedb877c4dc2d1afefa423ea9149e874
# Parent  7c76f3923b6a4fd6a35a9b498be15b9c549955eb
color: tweak behavior of ui.color config knob

marmoute sent some other patches today that tried to bring
pager.enabled into conformity with ui.color, and while reviewing those
patches I tripped on some unfortunate usability warts on the behavior
of the (new in 4.2) ui.color config knob. Specifically, ui.color=yes
actually means "always show color", not "use color automatically when
it appears sensible". This feels problematic because if an
administrator has disabled color, and a user wants to turn it on, the
obvious setting (color=on) is wrong (because it breaks their output
when redirected to a file.) This patch changes ui.color to only move
the default value of --color from "never" to "auto", which matches the
relationship of pager.enabed and --pager.

That is, before this patch:

+--------------------+--------------------+--------------------+
|                    | not a tty          | a tty              |
|                    | --color not set    | --color not set    |
|                    |                    |                    |
+--------------------+--------------------+--------------------+
| [ui]               |                    |                    |
| color (not set)    | no color           | no color           |
|                    |                    |                    |
+--------------------+--------------------+--------------------+
| [ui]               |                    |                    |
| color = auto       | no color           | color              |
|                    |                    |                    |
+--------------------+--------------------+--------------------+
| [ui]               |                    |                    |
| color = yes        | *color*            | color              |
|                    |                    |                    |
+--------------------+--------------------+--------------------+
| [ui]               |                    |                    |
| color = no         | no color           | no color           |
|                    |                    |                    |
+--------------------+--------------------+--------------------+
(if --color is specified, it always clobbers the setting in [ui])

and after this patch:

+--------------------+--------------------+--------------------+
|                    | not a tty          | a tty              |
|                    | --color not set    | --color not set    |
|                    |                    |                    |
+--------------------+--------------------+--------------------+
| [ui]               |                    |                    |
| color (not set)    | no color           | no color           |
|                    |                    |                    |
+--------------------+--------------------+--------------------+
| [ui]               |                    |                    |
| color = auto       | no color           | color              |
|                    |                    |                    |
+--------------------+--------------------+--------------------+
| [ui]               |                    |                    |
| color = yes        | *no color*         | color              |
|                    |                    |                    |
+--------------------+--------------------+--------------------+
| [ui]               |                    |                    |
| color = no         | no color           | no color           |
|                    |                    |                    |
+--------------------+--------------------+--------------------+
(if --color is specified, it always clobbers the setting in [ui])

Users that want to force colors without specifying --color on the
command line can use the ui.formatted config knob, which had to be
enabled in a handful of tests for this patch.

In the name of consistency we've also discussed renaming pager.enabled
to ui.pager. This feels like the wrong move, and I think Jun Wu's
observation on IRC feels like the right justification: "ui.pager as a
bool is confusing because we have pager.pager and ui.editor" - that
is, if we have ui.editor and that's an executable to use as the
editor, it feels like ui.pager should be the executable to use as the
pager, not the knob to enable/disable pager.

Since this is a behavior change in a flag that I'm proposing for
stable unacceptably late in a cycle, I went ahead and made this change
in the simplest possible way. We should probably plan to do some
further cleanups in the future to make this a little less cluttered.
Yuya Nishihara - May 2, 2017, 2:44 a.m.
On Mon, 01 May 2017 21:13:12 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1493678508 14400
> #      Mon May 01 18:41:48 2017 -0400
> # Branch stable
> # Node ID 9524b0b5eedb877c4dc2d1afefa423ea9149e874
> # Parent  7c76f3923b6a4fd6a35a9b498be15b9c549955eb
> color: tweak behavior of ui.color config knob
> 
> marmoute sent some other patches today that tried to bring
> pager.enabled into conformity with ui.color, and while reviewing those
> patches I tripped on some unfortunate usability warts on the behavior
> of the (new in 4.2) ui.color config knob. Specifically, ui.color=yes
> actually means "always show color", not "use color automatically when
> it appears sensible". This feels problematic because if an
> administrator has disabled color, and a user wants to turn it on, the
> obvious setting (color=on) is wrong (because it breaks their output
> when redirected to a file.) This patch changes ui.color to only move
> the default value of --color from "never" to "auto", which matches the
> relationship of pager.enabed and --pager.

I'm 0 on this since ui.color sounds okay not being a simple boolean.

> --- a/mercurial/color.py
> +++ b/mercurial/color.py
> @@ -185,10 +185,17 @@ def setup(ui):
>  def _modesetup(ui):
>      if ui.plain():
>          return None
> +    # ui.color only controls the default of the --color flag: true
> +    # means to automatically use color when sensible, and false means
> +    # to never use color.
> +    wantcolor = ui.configbool('ui', 'color', _enabledbydefault)

ui.color=auto becomes invalid so we'll have to update the doc.
Pierre-Yves David - May 2, 2017, 11:26 a.m.
On 05/02/2017 04:44 AM, Yuya Nishihara wrote:
> On Mon, 01 May 2017 21:13:12 -0400, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1493678508 14400
>> #      Mon May 01 18:41:48 2017 -0400
>> # Branch stable
>> # Node ID 9524b0b5eedb877c4dc2d1afefa423ea9149e874
>> # Parent  7c76f3923b6a4fd6a35a9b498be15b9c549955eb
>> color: tweak behavior of ui.color config knob
>>
>> Marmoute sent some other patches today that tried to bring
>> pager.enabled into conformity with ui.color, and while reviewing those
>> patches I tripped on some unfortunate usability warts on the behavior
>> of the (new in 4.2) ui.color config knob. Specifically, ui.color=yes
>> actually means "always show color", not "use color automatically when
>> it appears sensible". This feels problematic because if an
>> administrator has disabled color, and a user wants to turn it on, the
>> obvious setting (color=on) is wrong (because it breaks their output
>> when redirected to a file.) This patch changes ui.color to only move
>> the default value of --color from "never" to "auto", which matches the
>> relationship of pager.enabed and --pager.
>
> I'm 0 on this since ui.color sounds okay not being a simple boolean.

I'm -0 on the change, The scenario Augie describe in practice is valid, 
but I'm not sure it will be a real issue in practice (since all doc 
mention always, auto and never). I would rather wait and see if we get 
actual feedback about this before making the code more complex.

In addition, the current path has quirk, so I would rather delay the 
discussion about this for 4.2.1. Changing the handling of some config 
value should be acceptable (while renaming important variable is more 
annoying).

  * `ui.color=always` will not result in color being always on. That 
seems unfortunate and confusing. (keeping 'auto' as a valid value might 
be valuable too).

  * the introduction and use of the 'ui.color-flag' option seems 
strange. I would rather have that held by an attribute on the `ui` 
object or have the config option be explicitly internal (not in the 'ui' 
section).

* As pointed by Yuya, the documentation needs to be updated.

Positive ending note: the fact `ui.formatted` allows to force color "on" 
in all cases lift most of my concern about having a more boolean config. 
(but I'm not convinced yet it is worth the extra complexity)

>> --- a/mercurial/color.py
>> +++ b/mercurial/color.py
>> @@ -185,10 +185,17 @@ def setup(ui):
>>  def _modesetup(ui):
>>      if ui.plain():
>>          return None
>> +    # ui.color only controls the default of the --color flag: true
>> +    # means to automatically use color when sensible, and false means
>> +    # to never use color.
>> +    wantcolor = ui.configbool('ui', 'color', _enabledbydefault)
>
> ui.color=auto becomes invalid so we'll have to update the doc.
Augie Fackler - May 2, 2017, 2:05 p.m.
> On May 2, 2017, at 07:26, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 05/02/2017 04:44 AM, Yuya Nishihara wrote:
>> On Mon, 01 May 2017 21:13:12 -0400, Augie Fackler wrote:
>>> # HG changeset patch
>>> # User Augie Fackler <augie@google.com>
>>> # Date 1493678508 14400
>>> #      Mon May 01 18:41:48 2017 -0400
>>> # Branch stable
>>> # Node ID 9524b0b5eedb877c4dc2d1afefa423ea9149e874
>>> # Parent  7c76f3923b6a4fd6a35a9b498be15b9c549955eb
>>> color: tweak behavior of ui.color config knob
>>> 
>>> Marmoute sent some other patches today that tried to bring
>>> pager.enabled into conformity with ui.color, and while reviewing those
>>> patches I tripped on some unfortunate usability warts on the behavior
>>> of the (new in 4.2) ui.color config knob. Specifically, ui.color=yes
>>> actually means "always show color", not "use color automatically when
>>> it appears sensible". This feels problematic because if an
>>> administrator has disabled color, and a user wants to turn it on, the
>>> obvious setting (color=on) is wrong (because it breaks their output
>>> when redirected to a file.) This patch changes ui.color to only move
>>> the default value of --color from "never" to "auto", which matches the
>>> relationship of pager.enabed and --pager.
>> 
>> I'm 0 on this since ui.color sounds okay not being a simple boolean.
> 
> I'm -0 on the change, The scenario Augie describe in practice is valid, but I'm not sure it will be a real issue in practice (since all doc mention always, auto and never). I would rather wait and see if we get actual feedback about this before making the code more complex.

The wait-and-see approach implies treating this as a bug retroactively, but you're arguing that the existing behavior represents a feature. I'm having trouble reconciling those two positions. I'd much rather fix it now to be a pure boolean, since it removes a potentially surprising (and therefore frustrating) behavior.

> In addition, the current path has quirk, so I would rather delay the discussion about this for 4.2.1. Changing the handling of some config value should be acceptable (while renaming important variable is more annoying).
> 
> * `ui.color=always` will not result in color being always on. That seems unfortunate and confusing. (keeping 'auto' as a valid value might be valuable too).

`always` is an alias of `true` in our boolean parser. Pretty sure that's my fault, but nothing to be done about it now.

> * the introduction and use of the 'ui.color-flag' option seems strange. I would rather have that held by an attribute on the `ui` object or have the config option be explicitly internal (not in the 'ui' section).

As I said in the patch, this is the absolute smallest patch I could come up with in the name of fitting something on stable on unacceptably short notice.

> * As pointed by Yuya, the documentation needs to be updated.

Yes. If I get more feedback on the direction we should go I'll send a v2 with updated documentation.

> 
> Positive ending note: the fact `ui.formatted` allows to force color "on" in all cases lift most of my concern about having a more boolean config. (but I'm not convinced yet it is worth the extra complexity)
> 
>>> --- a/mercurial/color.py
>>> +++ b/mercurial/color.py
>>> @@ -185,10 +185,17 @@ def setup(ui):
>>> def _modesetup(ui):
>>>     if ui.plain():
>>>         return None
>>> +    # ui.color only controls the default of the --color flag: true
>>> +    # means to automatically use color when sensible, and false means
>>> +    # to never use color.
>>> +    wantcolor = ui.configbool('ui', 'color', _enabledbydefault)
>> 
>> ui.color=auto becomes invalid so we'll have to update the doc.
> 
> -- 
> Pierre-Yves David
Pierre-Yves David - May 2, 2017, 2:55 p.m.
On 05/02/2017 04:05 PM, Augie Fackler wrote:
>
>> On May 2, 2017, at 07:26, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>>
>> On 05/02/2017 04:44 AM, Yuya Nishihara wrote:
>>> On Mon, 01 May 2017 21:13:12 -0400, Augie Fackler wrote:
>>>> # HG changeset patch
>>>> # User Augie Fackler <augie@google.com>
>>>> # Date 1493678508 14400
>>>> #      Mon May 01 18:41:48 2017 -0400
>>>> # Branch stable
>>>> # Node ID 9524b0b5eedb877c4dc2d1afefa423ea9149e874
>>>> # Parent  7c76f3923b6a4fd6a35a9b498be15b9c549955eb
>>>> color: tweak behavior of ui.color config knob
>>>>
>>>> Marmoute sent some other patches today that tried to bring
>>>> pager.enabled into conformity with ui.color, and while reviewing those
>>>> patches I tripped on some unfortunate usability warts on the behavior
>>>> of the (new in 4.2) ui.color config knob. Specifically, ui.color=yes
>>>> actually means "always show color", not "use color automatically when
>>>> it appears sensible". This feels problematic because if an
>>>> administrator has disabled color, and a user wants to turn it on, the
>>>> obvious setting (color=on) is wrong (because it breaks their output
>>>> when redirected to a file.) This patch changes ui.color to only move
>>>> the default value of --color from "never" to "auto", which matches the
>>>> relationship of pager.enabed and --pager.
>>>
>>> I'm 0 on this since ui.color sounds okay not being a simple boolean.
>>
>> I'm -0 on the change, The scenario Augie describe in practice is valid, but I'm not sure it will be a real issue in practice (since all doc mention always, auto and never). I would rather wait and see if we get actual feedback about this before making the code more complex.
>
> The wait-and-see approach implies treating this as a bug retroactively, but you're arguing that the existing behavior represents a feature. I'm having trouble reconciling those two positions. I'd much rather fix it now to be a pure boolean, since it removes a potentially surprising (and therefore frustrating) behavior.

Let me try to clarify. We are talking about possible surprising corner 
cases here.

The current implementation "suffer" from one surprise:

  1) ui.color=True is actually "always color"

The proposed patch "suffer" from two surprises:

  2) ui.color=always is actually "--color=auto" not "--color=always"
  3) '--color=auto works', but 'ui.color=auto' doesn not (= off)

It is currently unclear to me trading (2) and (3) in to solve (1) is 
better. It is also not clear to me the corner case matter at all. So 
given the current doubt I advocate to keep it simple (similar 
implementation for --color and ui.color).

We are past the freeze date and this is a minor corner case with a 
possible easy way forward for 4.2.1. I think we should drop the matter 
for 4.2 and quietly discuss it after the release.

If we decide the issue is serious enough, we can probably build a 
solution that fix (1) without breaking (2) and (3) (accept "auto", 
"always" is special) early in the 4.2.1 cycle.

Changing the handling of that value between 4.2 and 4.2.1 is not ideal. 
But not a big deal either as only on special case (color explicitly set 
to "yes" + not a tty) will be affected (and that case would likely be 
broken anyway). The values most important to user (never, off, false, 0) 
will not changes.

In addition, there are very low risk of having a stack overflow question 
with a problematic question in this area created between 4.2 and 4.2.1.

Is my position clearer to  you now ?

>> In addition, the current path has quirk, so I would rather delay the discussion about this for 4.2.1. Changing the handling of some config value should be acceptable (while renaming important variable is more annoying).
>>
>> * `ui.color=always` will not result in color being always on. That seems unfortunate and confusing. (keeping 'auto' as a valid value might be valuable too).
>
> `always` is an alias of `true` in our boolean parser. Pretty sure that's my fault, but nothing to be done about it now.
>
>> * the introduction and use of the 'ui.color-flag' option seems strange. I would rather have that held by an attribute on the `ui` object or have the config option be explicitly internal (not in the 'ui' section).
>
> As I said in the patch, this is the absolute smallest patch I could come up with in the name of fitting something on stable on unacceptably short notice.

If we stay on this track for 4.2, can you move 'color-flag' to the 
'_internal' section of something similar?

Cheers,
Pierre-Yves David - May 2, 2017, 4:22 p.m.
On 05/02/2017 06:09 PM, Kevin Bullock wrote:
>> On May 2, 2017, at 09:55, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>> Let me try to clarify. We are talking about possible surprising corner cases here.
>>
>> The current implementation "suffer" from one surprise:
>>
>> 1) ui.color=True is actually "always color"
>
> To me, this is a pretty bad situation. In 4.1 and earlier, turning on color by enabling the color extension and then cat'ing output to a file does the right thing (strips colors). Now if I enable color by setting ui.color=True, I get horrible escape codes in my output.

To put things into perspective, users won't have to enable color since 
color is on by default. So I see the risk of confusion as pretty low.
They only have to -re-enable- it if the system why config turned it off 
globally (something I expect to be very rare).

Patch

diff --git a/mercurial/color.py b/mercurial/color.py
--- a/mercurial/color.py
+++ b/mercurial/color.py
@@ -185,10 +185,17 @@  def setup(ui):
 def _modesetup(ui):
     if ui.plain():
         return None
+    # ui.color only controls the default of the --color flag: true
+    # means to automatically use color when sensible, and false means
+    # to never use color.
+    wantcolor = ui.configbool('ui', 'color', _enabledbydefault)
     default = 'never'
-    if _enabledbydefault:
+    if wantcolor:
         default = 'auto'
-    config = ui.config('ui', 'color', default)
+    # internal config: ui.color-flag
+    # Used for carrying --color state on ui objects.
+    config = ui.config('ui', 'color-flag', default)
+
     if config == 'debug':
         return 'debug'
 
diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -846,7 +846,7 @@  def _dispatch(req):
         coloropt = options['color']
         for ui_ in uis:
             if coloropt:
-                ui_.setconfig('ui', 'color', coloropt, '--color')
+                ui_.setconfig('ui', 'color-flag', coloropt, '--color')
             color.setup(ui_)
 
         if options['version']:
diff --git a/tests/test-diff-color.t b/tests/test-diff-color.t
--- a/tests/test-diff-color.t
+++ b/tests/test-diff-color.t
@@ -3,6 +3,7 @@  Setup
   $ cat <<EOF >> $HGRCPATH
   > [ui]
   > color = always
+  > formatted = yes
   > [color]
   > mode = ansi
   > EOF
diff --git a/tests/test-pager-legacy.t b/tests/test-pager-legacy.t
--- a/tests/test-pager-legacy.t
+++ b/tests/test-pager-legacy.t
@@ -161,6 +161,7 @@  even though stdout is no longer a tty.
   $ cat >> $HGRCPATH <<EOF
   > [ui]
   > color = yes
+  > formatted = yes
   > [color]
   > mode = ansi
   > EOF
diff --git a/tests/test-pager.t b/tests/test-pager.t
--- a/tests/test-pager.t
+++ b/tests/test-pager.t
@@ -142,6 +142,7 @@  even though stdout is no longer a tty.
   $ cat >> $HGRCPATH <<EOF
   > [ui]
   > color = yes
+  > formatted = yes
   > [color]
   > mode = ansi
   > EOF
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
@@ -1,6 +1,7 @@ 
   $ cat <<EOF >> $HGRCPATH
   > [ui]
   > color = always
+  > formatted = yes
   > [color]
   > mode = ansi
   > EOF