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
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. [...]
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