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

[Pierre-Yves David]

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

-- 
Pierre-Yves David