Patchwork [evolve-ext] evolve: do not set experimental.evolution in uisetup

login
register
mail settings
Submitter Jun Wu
Date April 3, 2016, 12:02 a.m.
Message ID <eb063ecaef4c50fb50f6.1459641778@x1c>
Download mbox | patch
Permalink /patch/14266/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Jun Wu - April 3, 2016, 12:02 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1459641729 -3600
#      Sun Apr 03 01:02:09 2016 +0100
# Node ID eb063ecaef4c50fb50f6f21dd04b087cf1ac35b4
# Parent  45b2b59ce5d3a8641814107c7fc0808a361a8f30
evolve: do not set experimental.evolution in uisetup

Before this patch, evolve will set experimental.evolution=all if it's empty.
This does not work well with chg, since chg only runs uisetup once and will
copy configs modified by extensions when loading new ui unconditionally.

In this case, if the user starts a chgserver with empty experimental.evolution,
evolve will set it to "all" and the user will lose changes to set it to other
value.

This patch addressed the issue by replacing ui.setconfig with a default value
at reading time (ui.configlist).
Pierre-Yves David - April 3, 2016, 8:01 p.m.
On 04/02/2016 05:02 PM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1459641729 -3600
> #      Sun Apr 03 01:02:09 2016 +0100
> # Node ID eb063ecaef4c50fb50f6f21dd04b087cf1ac35b4
> # Parent  45b2b59ce5d3a8641814107c7fc0808a361a8f30
> evolve: do not set experimental.evolution in uisetup
>
> Before this patch, evolve will set experimental.evolution=all if it's empty.
> This does not work well with chg, since chg only runs uisetup once and will
> copy configs modified by extensions when loading new ui unconditionally.
>
> In this case, if the user starts a chgserver with empty experimental.evolution,
> evolve will set it to "all" and the user will lose changes to set it to other
> value.
>
> This patch addressed the issue by replacing ui.setconfig with a default value
> at reading time (ui.configlist).

I don't think this will work. There is third party code (especially… 
mercurial core) that rely on the extension to set this when enabled. And 
I assume this would break it all.

I this time for a field test of the extensibility of chg config dispatch?

Cheers,
Jun Wu - April 3, 2016, 10:49 p.m.
On 04/03/2016 09:01 PM, Pierre-Yves David wrote:
> I don't think this will work. There is third party code (especially…
> mercurial core) that rely on the extension to set this when enabled. And I
> assume this would break it all.

Thanks. I didn't realize obsolete.py is reading it.

How about doing these instead:
- evolve.py: remove setconfig
- evolve.py: set obsolete._enabled = True

> I this time for a field test of the extensibility of chg config dispatch?

Not yet.

APIs to mark config items as sensitive are planned as we discussed in IRC,
and that will happen in extensions.py.

For chg, at present it's impossible to handle "if getconfig(x): setconfig(x)"
correctly because the information of the roriginal config value will be lost.
My ui refactoring plain will help solve the issue but that won't happen
soon.
Jun Wu - April 6, 2016, 5:27 p.m.
On 04/03/2016 11:49 PM, Jun Wu wrote:
> On 04/03/2016 09:01 PM, Pierre-Yves David wrote:
>> I don't think this will work. There is third party code (especially…
>> mercurial core) that rely on the extension to set this when enabled. And I
>> assume this would break it all.
>
> Thanks. I didn't realize obsolete.py is reading it.
>
> How about doing these instead:
> - evolve.py: remove setconfig
> - evolve.py: set obsolete._enabled = True

Gentle ping. If we want to prioritize evolve chg tests fixes, we need to have
a way to get rid of setconfig. I think it's okay to do the above. Opinions?
Pierre-Yves David - April 6, 2016, 5:58 p.m.
On 04/06/2016 10:27 AM, Jun Wu wrote:
> On 04/03/2016 11:49 PM, Jun Wu wrote:
>> On 04/03/2016 09:01 PM, Pierre-Yves David wrote:
>>> I don't think this will work. There is third party code (especially…
>>> mercurial core) that rely on the extension to set this when enabled. 
>>> And I
>>> assume this would break it all.
>>
>> Thanks. I didn't realize obsolete.py is reading it.
>>
>> How about doing these instead:
>> - evolve.py: remove setconfig
>> - evolve.py: set obsolete._enabled = True
>
> Gentle ping. If we want to prioritize evolve chg tests fixes, we need 
> to have
> a way to get rid of setconfig. I think it's okay to do the above. 
> Opinions?

Unfortunately is a bit too short for me to understand what you are 
actually proposing here. Can you elaborate?

On the mercurial.obsolete._enabled front. Durham did a significant 
amount of work years ago to be able to get ride of it (this created the 
config option and the finer grained selection). However the final bits 
actually removing the boolean apparently never made it. So don't 
remember all the details (and don't have too much setup to do archeology 
right now) but moving more dependency back on obsolete._enabled seems a 
move in the wrong direction (but some archeology is needed and maybe 
finishing killing obsolete._enabled if appropriate)

Cheers,
Jun Wu - April 6, 2016, 7:10 p.m.
On 04/06/2016 06:58 PM, Pierre-Yves David wrote:
> On 04/06/2016 10:27 AM, Jun Wu wrote:
>> On 04/03/2016 11:49 PM, Jun Wu wrote:
>>> On 04/03/2016 09:01 PM, Pierre-Yves David wrote:
>>>> I don't think this will work. There is third party code (especially…
>>>> mercurial core) that rely on the extension to set this when enabled.
>>>> And I assume this would break it all.
>>>
>>> Thanks. I didn't realize obsolete.py is reading it.
>>>
>>> How about doing these instead: - evolve.py: remove setconfig -
>>> evolve.py: set obsolete._enabled = True
>>
>> Gentle ping. If we want to prioritize evolve chg tests fixes, we need to
>> have a way to get rid of setconfig. I think it's okay to do the above.
>> Opinions?
>
> Unfortunately is a bit too short for me to understand what you are actually
>  proposing here. Can you elaborate?

Okay. So first let's talk about why the following pattern has issues with chg:

   def uisetup(ui):
       if not ui.getconfig('x', 'y'):
           ui.setconfig('x', 'y', 'z')

Currently, when reloading config per request, chg will scan all the old config
items not read from files by looking at source and apply them as is. This means
the side effect of setconfig will be applied as is without the "if" condition
(also because uisetup is executed only once).

It does not work with whitelisting the x.y config because:
1) chgserver.uisetup runs after all other extensions (i.e. other extensions
have done setconfig()s already), so the confighash chgserver gets is the one
after setconfig()s
2) there is no way to get the original value overrided by setconfig().

That can also be solved by my big ui/config refactoring plan because it would
allow accessing the values being overrided and calculating confighash in a more
clean way. But as the refactoring is not trivial., it's unlikely to happen soon.

Back to evolve, the attempt here is to get rid of the pattern above. Using
ui.setconfig and ui.getconfig sounds like global variables to me so I think
it's okay to move back to obsolete._enabled temporarily. Alternatively it may
make sense to blacklist the test for chg temporarily. By temporarily I mean
until I get the ui/config refactoring done but I don't have an ETA for the
refactoring now.
Yuya Nishihara - April 7, 2016, 1:09 p.m.
On Wed, 6 Apr 2016 20:10:22 +0100, Jun Wu wrote:
> On 04/06/2016 06:58 PM, Pierre-Yves David wrote:
> > On 04/06/2016 10:27 AM, Jun Wu wrote:  
> >> On 04/03/2016 11:49 PM, Jun Wu wrote:  
> >>> On 04/03/2016 09:01 PM, Pierre-Yves David wrote:  
> >>>> I don't think this will work. There is third party code (especially…
> >>>> mercurial core) that rely on the extension to set this when enabled.
> >>>> And I assume this would break it all.  
> >>>
> >>> Thanks. I didn't realize obsolete.py is reading it.
> >>>
> >>> How about doing these instead: - evolve.py: remove setconfig -
> >>> evolve.py: set obsolete._enabled = True  
> >>
> >> Gentle ping. If we want to prioritize evolve chg tests fixes, we need to
> >> have a way to get rid of setconfig. I think it's okay to do the above.
> >> Opinions?  
> >
> > Unfortunately is a bit too short for me to understand what you are actually
> >  proposing here. Can you elaborate?  
> 
> Okay. So first let's talk about why the following pattern has issues with chg:
> 
>    def uisetup(ui):
>        if not ui.getconfig('x', 'y'):
>            ui.setconfig('x', 'y', 'z')
> 
> Currently, when reloading config per request, chg will scan all the old config
> items not read from files by looking at source and apply them as is. This means
> the side effect of setconfig will be applied as is without the "if" condition
> (also because uisetup is executed only once).
> 
> It does not work with whitelisting the x.y config because:
> 1) chgserver.uisetup runs after all other extensions (i.e. other extensions
> have done setconfig()s already), so the confighash chgserver gets is the one
> after setconfig()s
> 2) there is no way to get the original value overrided by setconfig().

FYI, ui.setconfig() in uisetup() is generally noop because the passed "ui" is
actually a "lui". Setting aliases and some hooks works just because they are
retrieved from lui.

https://selenic.com/repo/hg/file/3.7.3/mercurial/dispatch.py#l783

I expect your ui redesign will fix this issue, too.

Patch

diff --git a/hgext/evolve.py b/hgext/evolve.py
--- a/hgext/evolve.py
+++ b/hgext/evolve.py
@@ -390,15 +390,6 @@ 
 ### Option configuration                                          ###
 #####################################################################
 
-@eh.reposetup # must be the first of its kin.
-def _configureoptions(ui, repo):
-    # If no capabilities are specified, enable everything.
-    # This is so existing evolve users don't need to change their config.
-    evolveopts = ui.configlist('experimental', 'evolution')
-    if not evolveopts:
-        evolveopts = ['all']
-        ui.setconfig('experimental', 'evolution', evolveopts)
-
 @eh.uisetup
 def _configurecmdoptions(ui):
     # Unregister evolve commands if the command capability is not specified.
@@ -407,7 +398,7 @@ 
     # guarantee it happens after the above configuration, but before the
     # extsetup functions.
     evolvecommands = ui.configlist('experimental', 'evolutioncommands')
-    evolveopts = ui.configlist('experimental', 'evolution')
+    evolveopts = ui.configlist('experimental', 'evolution', ['all'])
     if evolveopts and (commandopt not in evolveopts and
                        'all' not in evolveopts):
         # We build whitelist containing the commands we want to enable