Patchwork [01,of,10,RFC,v2] ui: refactoring handling of trusted, user and overlay

login
register
mail settings
Submitter David Soria Parra
Date March 12, 2017, 10:40 p.m.
Message ID <99514a82d5b23c75bd6d.1489358424@davidsp-mbp.dhcp.thefacebook.com>
Download mbox | patch
Permalink /patch/19245/
State Changes Requested
Headers show

Comments

David Soria Parra - March 12, 2017, 10:40 p.m.
# HG changeset patch
# User David Soria Parra <davidsp@fb.com>
# Date 1489349204 25200
#      Sun Mar 12 13:06:44 2017 -0700
# Node ID 99514a82d5b23c75bd6da38e522acfd14a618c14
# Parent  1c3352d7eaf24533ad52d4b8a024211e9189fb0b
ui: refactoring handling of trusted, user and overlay

We are using obscure names such as _ocfg for overlay-config in the UI. This is
sometimes confusing and not very flexible. We are moving this into a dictionary
now that has a specific ordering in which we would apply multiple layers of
configuration. At the moment this is not needed as we always pick either
user-config or trusted-config and overlay it, but it gets us a good machinery to
add a defaults layer for ui.compat.
Jun Wu - March 15, 2017, 5:27 a.m.
The direction looks fine.

I'm especially interested in the performance and correctness of the "config
merging" part (not in this patch), I'll comment in that patch.

It seems it does not conflict with the immutable config objects I had
before. Maybe we want the immutable thing eventually. But that could be done
later. Or I could mix them with ui.compat if you think we should go
immutable directly.

Excerpts from David Soria Parra's message of 2017-03-12 15:40:24 -0700:

[...]

> +    def cfg(self):
> +        # Ordered in ascneding order of preference.
> +        return util.sortdict(
> +            [('user', config.config()),
> +            ('trusted', config.config()),
> +            ('overlay', config.config())])

Maybe make this a private static method. People may expect ui.cfg() to
return the "current" config, instead of new ones.

      @staticmethod
      def _emptyconfig():
          return ....

> +
>      def fixconfig(self, root=None, section=None):
>          if section in (None, 'paths'):
>              # expand vars and ~
>              # translate paths relative to root (or home) into absolute paths
>              root = root or pycompat.getcwd()
> -            for c in self._tcfg, self._ucfg, self._ocfg:
> +            for c in self._cfg.values():
>                  for n, p in c.items('paths'):
>                      # Ignore sub-options.
>                      if ':' in n:
> @@ -345,21 +348,22 @@
>              self._trustgroups.update(self.configlist('trusted', 'groups'))
>  
>      def backupconfig(self, section, item):
> -        return (self._ocfg.backup(section, item),
> -                self._tcfg.backup(section, item),
> -                self._ucfg.backup(section, item),)
> +        return {k: cfg.backup(section, item) for k, cfg in self._cfg.items()}

I guess we still prefer iteritems to reduce overheads on Python 2.
There is an AST transformer rewriting "iteritems" to "items" for Python 3.

[...]
Jun Wu - March 17, 2017, 3:54 p.m.
Another issue with the "compat" layer outside "ucfg" or "tcfg" is that it
cannot handle the case where ucfg or tcfg have different ui.compat setting.

I just discovered this when writing the immutable stuff. Hopefully I can
send a V1 today.

Excerpts from David Soria Parra's message of 2017-03-12 15:40:24 -0700:
> # HG changeset patch
> # User David Soria Parra <davidsp@fb.com>
> # Date 1489349204 25200
> #      Sun Mar 12 13:06:44 2017 -0700
> # Node ID 99514a82d5b23c75bd6da38e522acfd14a618c14
> # Parent  1c3352d7eaf24533ad52d4b8a024211e9189fb0b
> ui: refactoring handling of trusted, user and overlay
> 
> We are using obscure names such as _ocfg for overlay-config in the UI. This is
> sometimes confusing and not very flexible. We are moving this into a dictionary
> now that has a specific ordering in which we would apply multiple layers of
> configuration. At the moment this is not needed as we always pick either
> user-config or trusted-config and overlay it, but it gets us a good machinery to
> add a defaults layer for ui.compat.
> 
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -146,9 +146,7 @@
>          self._bufferapplylabels = None
>          self.quiet = self.verbose = self.debugflag = self.tracebackflag = False
>          self._reportuntrusted = True
> -        self._ocfg = config.config() # overlay
> -        self._tcfg = config.config() # trusted
> -        self._ucfg = config.config() # untrusted
> +        self._cfg = self.cfg()
>          self._trustusers = set()
>          self._trustgroups = set()
>          self.callhooks = True
> @@ -167,10 +165,6 @@
>              self.fin = src.fin
>              self.pageractive = src.pageractive
>              self._disablepager = src._disablepager
> -
> -            self._tcfg = src._tcfg.copy()
> -            self._ucfg = src._ucfg.copy()
> -            self._ocfg = src._ocfg.copy()
>              self._trustusers = src._trustusers.copy()
>              self._trustgroups = src._trustgroups.copy()
>              self.environ = src.environ
> @@ -179,6 +173,8 @@
>              self._colormode = src._colormode
>              self._terminfoparams = src._terminfoparams.copy()
>              self._styles = src._styles.copy()
> +            for k in self._cfg.keys():
> +                self._cfg[k] = src._cfg[k].copy()
>  
>              self.fixconfig()
>  
> @@ -296,21 +292,28 @@
>                  del cfg['templatealias'][k]
>  
>          if trusted:
> -            self._tcfg.update(cfg)
> -            self._tcfg.update(self._ocfg)
> -        self._ucfg.update(cfg)
> -        self._ucfg.update(self._ocfg)
> +            self._cfg['trusted'].update(cfg)
> +            self._cfg['trusted'].update(self._cfg['overlay'])
> +        self._cfg['user'].update(cfg)
> +        self._cfg['user'].update(self._cfg['overlay'])
>  
>          if root is None:
>              root = os.path.expanduser('~')
>          self.fixconfig(root=root)
>  
> +    def cfg(self):
> +        # Ordered in ascneding order of preference.
> +        return util.sortdict(
> +            [('user', config.config()),
> +            ('trusted', config.config()),
> +            ('overlay', config.config())])
> +
>      def fixconfig(self, root=None, section=None):
>          if section in (None, 'paths'):
>              # expand vars and ~
>              # translate paths relative to root (or home) into absolute paths
>              root = root or pycompat.getcwd()
> -            for c in self._tcfg, self._ucfg, self._ocfg:
> +            for c in self._cfg.values():
>                  for n, p in c.items('paths'):
>                      # Ignore sub-options.
>                      if ':' in n:
> @@ -345,21 +348,22 @@
>              self._trustgroups.update(self.configlist('trusted', 'groups'))
>  
>      def backupconfig(self, section, item):
> -        return (self._ocfg.backup(section, item),
> -                self._tcfg.backup(section, item),
> -                self._ucfg.backup(section, item),)
> +        return {k: cfg.backup(section, item) for k, cfg in self._cfg.items()}
> +
>      def restoreconfig(self, data):
> -        self._ocfg.restore(data[0])
> -        self._tcfg.restore(data[1])
> -        self._ucfg.restore(data[2])
> +        for k, d in data.items():
> +            self._cfg[k].restore(d)
>  
>      def setconfig(self, section, name, value, source=''):
> -        for cfg in (self._ocfg, self._tcfg, self._ucfg):
> +        for cfg in self._cfg.values():
>              cfg.set(section, name, value, source)
>          self.fixconfig(section=section)
>  
>      def _data(self, untrusted):
> -        return untrusted and self._ucfg or self._tcfg
> +        if untrusted:
> +            return self._cfg['user']
> +        else:
> +            return self._cfg['trusted']
>  
>      def configsource(self, section, name, untrusted=False):
>          return self._data(untrusted).source(section, name)
> @@ -380,7 +384,7 @@
>  
>          if self.debugflag and not untrusted and self._reportuntrusted:
>              for n in alternates:
> -                uvalue = self._ucfg.get(section, n)
> +                uvalue = self._cfg['user'].get(section, n)
>                  if uvalue is not None and uvalue != value:
>                      self.debug("ignoring untrusted configuration option "
>                                 "%s.%s = %s\n" % (section, n, uvalue))
> @@ -399,7 +403,7 @@
>          data = self._data(untrusted)
>          main = data.get(section, name, default)
>          if self.debugflag and not untrusted and self._reportuntrusted:
> -            uvalue = self._ucfg.get(section, name)
> +            uvalue = self._cfg['user'].get(section, name)
>              if uvalue is not None and uvalue != main:
>                  self.debug('ignoring untrusted configuration option '
>                             '%s.%s = %s\n' % (section, name, uvalue))
> @@ -412,7 +416,7 @@
>  
>          if self.debugflag and not untrusted and self._reportuntrusted:
>              for k, v in sub.items():
> -                uvalue = self._ucfg.get(section, '%s:%s' % (name, k))
> +                uvalue = self._cfg['user'].get(section, '%s:%s' % (name, k))
>                  if uvalue is not None and uvalue != v:
>                      self.debug('ignoring untrusted configuration option '
>                                 '%s:%s.%s = %s\n' % (section, name, k, uvalue))
> @@ -658,8 +662,8 @@
>                      newitems[k] = v
>              items = newitems.items()
>          if self.debugflag and not untrusted and self._reportuntrusted:
> -            for k, v in self._ucfg.items(section):
> -                if self._tcfg.get(section, k) != v:
> +            for k, v in self._cfg['user'].items(section):
> +                if self._cfg['trusted'].get(section, k) != v:
>                      self.debug("ignoring untrusted configuration option "
>                                 "%s.%s = %s\n" % (section, k, v))
>          return items

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -146,9 +146,7 @@ 
         self._bufferapplylabels = None
         self.quiet = self.verbose = self.debugflag = self.tracebackflag = False
         self._reportuntrusted = True
-        self._ocfg = config.config() # overlay
-        self._tcfg = config.config() # trusted
-        self._ucfg = config.config() # untrusted
+        self._cfg = self.cfg()
         self._trustusers = set()
         self._trustgroups = set()
         self.callhooks = True
@@ -167,10 +165,6 @@ 
             self.fin = src.fin
             self.pageractive = src.pageractive
             self._disablepager = src._disablepager
-
-            self._tcfg = src._tcfg.copy()
-            self._ucfg = src._ucfg.copy()
-            self._ocfg = src._ocfg.copy()
             self._trustusers = src._trustusers.copy()
             self._trustgroups = src._trustgroups.copy()
             self.environ = src.environ
@@ -179,6 +173,8 @@ 
             self._colormode = src._colormode
             self._terminfoparams = src._terminfoparams.copy()
             self._styles = src._styles.copy()
+            for k in self._cfg.keys():
+                self._cfg[k] = src._cfg[k].copy()
 
             self.fixconfig()
 
@@ -296,21 +292,28 @@ 
                 del cfg['templatealias'][k]
 
         if trusted:
-            self._tcfg.update(cfg)
-            self._tcfg.update(self._ocfg)
-        self._ucfg.update(cfg)
-        self._ucfg.update(self._ocfg)
+            self._cfg['trusted'].update(cfg)
+            self._cfg['trusted'].update(self._cfg['overlay'])
+        self._cfg['user'].update(cfg)
+        self._cfg['user'].update(self._cfg['overlay'])
 
         if root is None:
             root = os.path.expanduser('~')
         self.fixconfig(root=root)
 
+    def cfg(self):
+        # Ordered in ascneding order of preference.
+        return util.sortdict(
+            [('user', config.config()),
+            ('trusted', config.config()),
+            ('overlay', config.config())])
+
     def fixconfig(self, root=None, section=None):
         if section in (None, 'paths'):
             # expand vars and ~
             # translate paths relative to root (or home) into absolute paths
             root = root or pycompat.getcwd()
-            for c in self._tcfg, self._ucfg, self._ocfg:
+            for c in self._cfg.values():
                 for n, p in c.items('paths'):
                     # Ignore sub-options.
                     if ':' in n:
@@ -345,21 +348,22 @@ 
             self._trustgroups.update(self.configlist('trusted', 'groups'))
 
     def backupconfig(self, section, item):
-        return (self._ocfg.backup(section, item),
-                self._tcfg.backup(section, item),
-                self._ucfg.backup(section, item),)
+        return {k: cfg.backup(section, item) for k, cfg in self._cfg.items()}
+
     def restoreconfig(self, data):
-        self._ocfg.restore(data[0])
-        self._tcfg.restore(data[1])
-        self._ucfg.restore(data[2])
+        for k, d in data.items():
+            self._cfg[k].restore(d)
 
     def setconfig(self, section, name, value, source=''):
-        for cfg in (self._ocfg, self._tcfg, self._ucfg):
+        for cfg in self._cfg.values():
             cfg.set(section, name, value, source)
         self.fixconfig(section=section)
 
     def _data(self, untrusted):
-        return untrusted and self._ucfg or self._tcfg
+        if untrusted:
+            return self._cfg['user']
+        else:
+            return self._cfg['trusted']
 
     def configsource(self, section, name, untrusted=False):
         return self._data(untrusted).source(section, name)
@@ -380,7 +384,7 @@ 
 
         if self.debugflag and not untrusted and self._reportuntrusted:
             for n in alternates:
-                uvalue = self._ucfg.get(section, n)
+                uvalue = self._cfg['user'].get(section, n)
                 if uvalue is not None and uvalue != value:
                     self.debug("ignoring untrusted configuration option "
                                "%s.%s = %s\n" % (section, n, uvalue))
@@ -399,7 +403,7 @@ 
         data = self._data(untrusted)
         main = data.get(section, name, default)
         if self.debugflag and not untrusted and self._reportuntrusted:
-            uvalue = self._ucfg.get(section, name)
+            uvalue = self._cfg['user'].get(section, name)
             if uvalue is not None and uvalue != main:
                 self.debug('ignoring untrusted configuration option '
                            '%s.%s = %s\n' % (section, name, uvalue))
@@ -412,7 +416,7 @@ 
 
         if self.debugflag and not untrusted and self._reportuntrusted:
             for k, v in sub.items():
-                uvalue = self._ucfg.get(section, '%s:%s' % (name, k))
+                uvalue = self._cfg['user'].get(section, '%s:%s' % (name, k))
                 if uvalue is not None and uvalue != v:
                     self.debug('ignoring untrusted configuration option '
                                '%s:%s.%s = %s\n' % (section, name, k, uvalue))
@@ -658,8 +662,8 @@ 
                     newitems[k] = v
             items = newitems.items()
         if self.debugflag and not untrusted and self._reportuntrusted:
-            for k, v in self._ucfg.items(section):
-                if self._tcfg.get(section, k) != v:
+            for k, v in self._cfg['user'].items(section):
+                if self._cfg['trusted'].get(section, k) != v:
                     self.debug("ignoring untrusted configuration option "
                                "%s.%s = %s\n" % (section, k, v))
         return items