Patchwork [2,of,7] color: initialize color for local peer ui

login
register
mail settings
Submitter Pierre-Yves David
Date Feb. 27, 2017, 2:59 p.m.
Message ID <c3224694bdae9cdb7530.1488207598@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/18802/
State Superseded
Headers show

Comments

Pierre-Yves David - Feb. 27, 2017, 2:59 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1488044041 -3600
#      Sat Feb 25 18:34:01 2017 +0100
# Node ID c3224694bdae9cdb7530f952e2c767e419b7f280
# Parent  92526381242cd381375a465d5a800446916d2d7b
# EXP-Topic color
color: initialize color for local peer ui

The local peer is creating its own local ui and goes through full
initialization (reading config, setting up extensions, etc). We must setup the
color for this one too.

This was overlooked when the rest of the initialization changed but did not
had impact yet because all setup is still global. We fix it before it is too
late.
via Mercurial-devel - Feb. 28, 2017, 6:57 a.m.
On Mon, Feb 27, 2017 at 6:59 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1488044041 -3600
> #      Sat Feb 25 18:34:01 2017 +0100
> # Node ID c3224694bdae9cdb7530f952e2c767e419b7f280
> # Parent  92526381242cd381375a465d5a800446916d2d7b
> # EXP-Topic color
> color: initialize color for local peer ui
>
> The local peer

"local peer" or "localrepository"? The patch seems to be in "class
localrepository".

> is creating its own local ui and goes through full
> initialization (reading config, setting up extensions, etc). We must setup the
> color for this one too.
>
> This was overlooked when the rest of the initialization changed but did not
> had impact yet because all setup is still global. We fix it before it is too
> late.
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -28,6 +28,7 @@ from . import (
>      bundle2,
>      changegroup,
>      changelog,
> +    color,
>      context,
>      dirstate,
>      dirstateguard,
> @@ -285,6 +286,7 @@ class localrepository(object):
>                      setupfunc(self.ui, self.supported)
>          else:
>              self.supported = self._basesupported
> +        color.setup(self.ui)
>
>          # Add compression engines.
>          for name in util.compengines:
> 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
> @@ -296,6 +296,7 @@ test unknown color
>    $ hg --config color.status.modified=periwinkle status
>    ignoring unknown color/effect 'periwinkle' (configured in color.status.modified)
>    ignoring unknown color/effect 'periwinkle' (configured in color.status.modified)
> +  ignoring unknown color/effect 'periwinkle' (configured in color.status.modified)
>    M modified
>    \x1b[0;32;1mA \x1b[0m\x1b[0;32;1madded\x1b[0m (esc)
>    \x1b[0;32;1mA \x1b[0m\x1b[0;32;1mcopied\x1b[0m (esc)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Feb. 28, 2017, 10:24 a.m.
On 02/28/2017 07:57 AM, Martin von Zweigbergk wrote:
> On Mon, Feb 27, 2017 at 6:59 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1488044041 -3600
>> #      Sat Feb 25 18:34:01 2017 +0100
>> # Node ID c3224694bdae9cdb7530f952e2c767e419b7f280
>> # Parent  92526381242cd381375a465d5a800446916d2d7b
>> # EXP-Topic color
>> color: initialize color for local peer ui
>>
>> The local peer
>
> "local peer" or "localrepository"? The patch seems to be in "class
> localrepository".

hum, good catch. Things seems clowner than I expected. It looks like we 
don't use the "lui" (local ui, goes through uisetup) to create the 
repository but the "baseui" (does not goes through uisetup).

Let me grab a shovel and go bad into that code.

>> is creating its own local ui and goes through full
>> initialization (reading config, setting up extensions, etc). We must setup the
>> color for this one too.
>>
>> This was overlooked when the rest of the initialization changed but did not
>> had impact yet because all setup is still global. We fix it before it is too
>> late.
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -28,6 +28,7 @@ from . import (
>>      bundle2,
>>      changegroup,
>>      changelog,
>> +    color,
>>      context,
>>      dirstate,
>>      dirstateguard,
>> @@ -285,6 +286,7 @@ class localrepository(object):
>>                      setupfunc(self.ui, self.supported)
>>          else:
>>              self.supported = self._basesupported
>> +        color.setup(self.ui)
>>
>>          # Add compression engines.
>>          for name in util.compengines:
>> 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
>> @@ -296,6 +296,7 @@ test unknown color
>>    $ hg --config color.status.modified=periwinkle status
>>    ignoring unknown color/effect 'periwinkle' (configured in color.status.modified)
>>    ignoring unknown color/effect 'periwinkle' (configured in color.status.modified)
>> +  ignoring unknown color/effect 'periwinkle' (configured in color.status.modified)
>>    M modified
>>    \x1b[0;32;1mA \x1b[0m\x1b[0;32;1madded\x1b[0m (esc)
>>    \x1b[0;32;1mA \x1b[0m\x1b[0;32;1mcopied\x1b[0m (esc)
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Feb. 28, 2017, 10:35 a.m.
On 02/28/2017 11:24 AM, Pierre-Yves David wrote:
>
>
> On 02/28/2017 07:57 AM, Martin von Zweigbergk wrote:
>> On Mon, Feb 27, 2017 at 6:59 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>> # Date 1488044041 -3600
>>> #      Sat Feb 25 18:34:01 2017 +0100
>>> # Node ID c3224694bdae9cdb7530f952e2c767e419b7f280
>>> # Parent  92526381242cd381375a465d5a800446916d2d7b
>>> # EXP-Topic color
>>> color: initialize color for local peer ui
>>>
>>> The local peer
>>
>> "local peer" or "localrepository"? The patch seems to be in "class
>> localrepository".
>
> hum, good catch. Things seems clowner than I expected. It looks like we
> don't use the "lui" (local ui, goes through uisetup) to create the
> repository but the "baseui" (does not goes through uisetup).
>
> Let me grab a shovel and go bad into that code.

Okay so in short "ui initialisation business is not simple". That 
description should be:

color: initialize color for the localrepo ui

The 'ui' object dedicated to a 'localrepo' is independant from the one 
available in dispatch (and 'uisetup'). In addition, it is created from 
the 'baseui' (for good reason apparently). For this reason, we need to 
run the color setup on it after the local repository config is read.
Yuya Nishihara - Feb. 28, 2017, 2:29 p.m.
On Tue, 28 Feb 2017 11:35:03 +0100, Pierre-Yves David wrote:
> On 02/28/2017 11:24 AM, Pierre-Yves David wrote:
> > On 02/28/2017 07:57 AM, Martin von Zweigbergk wrote:
> >> On Mon, Feb 27, 2017 at 6:59 AM, Pierre-Yves David
> >> <pierre-yves.david@ens-lyon.org> wrote:
> >>> # HG changeset patch
> >>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> >>> # Date 1488044041 -3600
> >>> #      Sat Feb 25 18:34:01 2017 +0100
> >>> # Node ID c3224694bdae9cdb7530f952e2c767e419b7f280
> >>> # Parent  92526381242cd381375a465d5a800446916d2d7b
> >>> # EXP-Topic color
> >>> color: initialize color for local peer ui
> >>>
> >>> The local peer
> >>
> >> "local peer" or "localrepository"? The patch seems to be in "class
> >> localrepository".
> >
> > hum, good catch. Things seems clowner than I expected. It looks like we
> > don't use the "lui" (local ui, goes through uisetup) to create the
> > repository but the "baseui" (does not goes through uisetup).
> >
> > Let me grab a shovel and go bad into that code.
> 
> Okay so in short "ui initialisation business is not simple". That 
> description should be:
> 
> color: initialize color for the localrepo ui
> 
> The 'ui' object dedicated to a 'localrepo' is independant from the one 
> available in dispatch (and 'uisetup'). In addition, it is created from 
> the 'baseui' (for good reason apparently). For this reason, we need to 
> run the color setup on it after the local repository config is read.

Maybe we can move color.setup() to ui.fixconfig(), where ui attributes are
updated reflecting to config changes.
Pierre-Yves David - Feb. 28, 2017, 3:44 p.m.
On 02/28/2017 03:29 PM, Yuya Nishihara wrote:
> On Tue, 28 Feb 2017 11:35:03 +0100, Pierre-Yves David wrote:
>> On 02/28/2017 11:24 AM, Pierre-Yves David wrote:
>>> On 02/28/2017 07:57 AM, Martin von Zweigbergk wrote:
>>>> On Mon, Feb 27, 2017 at 6:59 AM, Pierre-Yves David
>>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>> # HG changeset patch
>>>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>>>> # Date 1488044041 -3600
>>>>> #      Sat Feb 25 18:34:01 2017 +0100
>>>>> # Node ID c3224694bdae9cdb7530f952e2c767e419b7f280
>>>>> # Parent  92526381242cd381375a465d5a800446916d2d7b
>>>>> # EXP-Topic color
>>>>> color: initialize color for local peer ui
>>>>>
>>>>> The local peer
>>>>
>>>> "local peer" or "localrepository"? The patch seems to be in "class
>>>> localrepository".
>>>
>>> hum, good catch. Things seems clowner than I expected. It looks like we
>>> don't use the "lui" (local ui, goes through uisetup) to create the
>>> repository but the "baseui" (does not goes through uisetup).
>>>
>>> Let me grab a shovel and go bad into that code.
>>
>> Okay so in short "ui initialisation business is not simple". That
>> description should be:
>>
>> color: initialize color for the localrepo ui
>>
>> The 'ui' object dedicated to a 'localrepo' is independant from the one
>> available in dispatch (and 'uisetup'). In addition, it is created from
>> the 'baseui' (for good reason apparently). For this reason, we need to
>> run the color setup on it after the local repository config is read.
>
> Maybe we can move color.setup() to ui.fixconfig(), where ui attributes are
> updated reflecting to config changes.

I considered it but it did not seemed appropriate. Fixconfig is run 
quite often and the setup function is erasing most internal state 
related to color. I was not confident make that change. I'm not saying 
it is entirely impossible but that requires a careful review of the 
setup process and other fixconfig related thing. So I would rather not 
go this route for now. In particular, running during 'fixconfig' would 
mean running it 'too early' before the extensions are loaded (they might 
change the default style.

Cheers,
Yuya Nishihara - March 1, 2017, 1:03 p.m.
On Tue, 28 Feb 2017 16:44:36 +0100, Pierre-Yves David wrote:
> On 02/28/2017 03:29 PM, Yuya Nishihara wrote:
> > On Tue, 28 Feb 2017 11:35:03 +0100, Pierre-Yves David wrote:
> >> On 02/28/2017 11:24 AM, Pierre-Yves David wrote:
> >>> On 02/28/2017 07:57 AM, Martin von Zweigbergk wrote:
> >>>> On Mon, Feb 27, 2017 at 6:59 AM, Pierre-Yves David
> >>>> <pierre-yves.david@ens-lyon.org> wrote:
> >>>>> # HG changeset patch
> >>>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> >>>>> # Date 1488044041 -3600
> >>>>> #      Sat Feb 25 18:34:01 2017 +0100
> >>>>> # Node ID c3224694bdae9cdb7530f952e2c767e419b7f280
> >>>>> # Parent  92526381242cd381375a465d5a800446916d2d7b
> >>>>> # EXP-Topic color
> >>>>> color: initialize color for local peer ui
> >>>>>
> >>>>> The local peer
> >>>>
> >>>> "local peer" or "localrepository"? The patch seems to be in "class
> >>>> localrepository".
> >>>
> >>> hum, good catch. Things seems clowner than I expected. It looks like we
> >>> don't use the "lui" (local ui, goes through uisetup) to create the
> >>> repository but the "baseui" (does not goes through uisetup).
> >>>
> >>> Let me grab a shovel and go bad into that code.
> >>
> >> Okay so in short "ui initialisation business is not simple". That
> >> description should be:
> >>
> >> color: initialize color for the localrepo ui
> >>
> >> The 'ui' object dedicated to a 'localrepo' is independant from the one
> >> available in dispatch (and 'uisetup'). In addition, it is created from
> >> the 'baseui' (for good reason apparently). For this reason, we need to
> >> run the color setup on it after the local repository config is read.
> >
> > Maybe we can move color.setup() to ui.fixconfig(), where ui attributes are
> > updated reflecting to config changes.
> 
> I considered it but it did not seemed appropriate. Fixconfig is run 
> quite often and the setup function is erasing most internal state 
> related to color. I was not confident make that change. I'm not saying 
> it is entirely impossible but that requires a careful review of the 
> setup process and other fixconfig related thing. So I would rather not 
> go this route for now. In particular, running during 'fixconfig' would 
> mean running it 'too early' before the extensions are loaded (they might 
> change the default style.

Good point and agreed.

It would be nice if we can get rid of the color dependency from localrepo
in future.
Pierre-Yves David - March 5, 2017, 1:38 p.m.
On 03/01/2017 02:03 PM, Yuya Nishihara wrote:
> On Tue, 28 Feb 2017 16:44:36 +0100, Pierre-Yves David wrote:
>> On 02/28/2017 03:29 PM, Yuya Nishihara wrote:
>>> On Tue, 28 Feb 2017 11:35:03 +0100, Pierre-Yves David wrote:
>>>> On 02/28/2017 11:24 AM, Pierre-Yves David wrote:
>>>>> On 02/28/2017 07:57 AM, Martin von Zweigbergk wrote:
>>>>>> On Mon, Feb 27, 2017 at 6:59 AM, Pierre-Yves David
>>>>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>>>> # HG changeset patch
>>>>>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>>>>>> # Date 1488044041 -3600
>>>>>>> #      Sat Feb 25 18:34:01 2017 +0100
>>>>>>> # Node ID c3224694bdae9cdb7530f952e2c767e419b7f280
>>>>>>> # Parent  92526381242cd381375a465d5a800446916d2d7b
>>>>>>> # EXP-Topic color
>>>>>>> color: initialize color for local peer ui
>>>>>>>
>>>>>>> The local peer
>>>>>>
>>>>>> "local peer" or "localrepository"? The patch seems to be in "class
>>>>>> localrepository".
>>>>>
>>>>> hum, good catch. Things seems clowner than I expected. It looks like we
>>>>> don't use the "lui" (local ui, goes through uisetup) to create the
>>>>> repository but the "baseui" (does not goes through uisetup).
>>>>>
>>>>> Let me grab a shovel and go bad into that code.
>>>>
>>>> Okay so in short "ui initialisation business is not simple". That
>>>> description should be:
>>>>
>>>> color: initialize color for the localrepo ui
>>>>
>>>> The 'ui' object dedicated to a 'localrepo' is independant from the one
>>>> available in dispatch (and 'uisetup'). In addition, it is created from
>>>> the 'baseui' (for good reason apparently). For this reason, we need to
>>>> run the color setup on it after the local repository config is read.
>>>
>>> Maybe we can move color.setup() to ui.fixconfig(), where ui attributes are
>>> updated reflecting to config changes.
>>
>> I considered it but it did not seemed appropriate. Fixconfig is run
>> quite often and the setup function is erasing most internal state
>> related to color. I was not confident make that change. I'm not saying
>> it is entirely impossible but that requires a careful review of the
>> setup process and other fixconfig related thing. So I would rather not
>> go this route for now. In particular, running during 'fixconfig' would
>> mean running it 'too early' before the extensions are loaded (they might
>> change the default style.
>
> Good point and agreed.
>
> It would be nice if we can get rid of the color dependency from localrepo
> in future.

There is two majors tasks in the way of having the color initialization 
more contained to the 'ui' class itself.

First, extensions can define a 'colortable' that augment the 
'_defaultstyles' mapping. This creates an order dependency between the 
initialization of extensions and the setup of color on the 'ui' object. 
That dependency makes things harder since we have to be careful to not 
initialize color too early.
One easy way to remove this dependency is to split the style config in 
two layers. One "default" layer that would be "immutable" from a "
ui" point of view (and updated by the extensions). And one extra layer 
at the ui level for change made through the configuration. When a style 
is requested, the local override would be searched before querying the 
global one. That was, update made by extensions to the global 
'_defaultstyle' could happen -after- the color is initialized on a 'ui' 
object.

Second, the current initialization of the color mode is meant as a 
one-time run and tend to modify, update and clear various mapping. It 
should me possible to update it to be a simple dispatch of value in a 
way that allow to setup the mode multiple time over the life of an 'ui' 
object. That should not be too hard but someone has to look into it.

I'm not planning to do that clean up for now since I've enough on my 
plate already. But I hope this message will hope someone clearing this 
in the future (maybe a me further in the future).

Cheers,

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -28,6 +28,7 @@  from . import (
     bundle2,
     changegroup,
     changelog,
+    color,
     context,
     dirstate,
     dirstateguard,
@@ -285,6 +286,7 @@  class localrepository(object):
                     setupfunc(self.ui, self.supported)
         else:
             self.supported = self._basesupported
+        color.setup(self.ui)
 
         # Add compression engines.
         for name in util.compengines:
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
@@ -296,6 +296,7 @@  test unknown color
   $ hg --config color.status.modified=periwinkle status
   ignoring unknown color/effect 'periwinkle' (configured in color.status.modified)
   ignoring unknown color/effect 'periwinkle' (configured in color.status.modified)
+  ignoring unknown color/effect 'periwinkle' (configured in color.status.modified)
   M modified
   \x1b[0;32;1mA \x1b[0m\x1b[0;32;1madded\x1b[0m (esc)
   \x1b[0;32;1mA \x1b[0m\x1b[0;32;1mcopied\x1b[0m (esc)