Patchwork [07,of,10,V5] rcutil: let environ override system configs (BC)

login
register
mail settings
Submitter Jun Wu
Date March 27, 2017, 6:02 a.m.
Message ID <38572bb2cffd815526a7.1490594526@localhost.localdomain>
Download mbox | patch
Permalink /patch/19748/
State Accepted
Headers show

Comments

Jun Wu - March 27, 2017, 6:02 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1490589217 25200
#      Sun Mar 26 21:33:37 2017 -0700
# Node ID 38572bb2cffd815526a727bc6f3aacdca2902f4f
# Parent  9b0aa30bf151b6c0e999b017fd328e29440bd447
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 38572bb2cffd
rcutil: let environ override system configs (BC)

This is BC because system configs won't be able to override $EDITOR, $PAGER.
The new behavior is arguably more rational.
Ryan McElroy - March 28, 2017, 8:47 a.m.
On 3/27/17 7:02 AM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1490589217 25200
> #      Sun Mar 26 21:33:37 2017 -0700
> # Node ID 38572bb2cffd815526a727bc6f3aacdca2902f4f
> # Parent  9b0aa30bf151b6c0e999b017fd328e29440bd447
> rcutil: let environ override system configs (BC)

Small nits inline. Could easily be addressed by a follow-up.

>
> This is BC because system configs won't be able to override $EDITOR, $PAGER.
> The new behavior is arguably more rational.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -1807,4 +1807,6 @@ def config(ui, repo, *values, **opts):
>           if t == 'path':
>               ui.debug('read config from: %s\n' % f)
> +        elif t == 'items':
> +            pass
>           else:
>               raise error.ProgrammingError('unknown rctype: %s' % t)
> diff --git a/mercurial/rcutil.py b/mercurial/rcutil.py
> --- a/mercurial/rcutil.py
> +++ b/mercurial/rcutil.py
> @@ -77,8 +77,12 @@ def rccomponents():
>       name, value, source) that should fill the config directly.
>       '''
> +    envrc = ('items', envrcitems())
> +
>       global _rccomponents
>       if _rccomponents is None:
>           if 'HGRCPATH' in encoding.environ:
> -            _rccomponents = []
> +            # assume HGRCPATH is all about user configs so environments can be
> +            # overridden.
> +            _rccomponents = [envrc]
>               for p in encoding.environ['HGRCPATH'].split(pycompat.ospathsep):
>                   if not p:
> @@ -86,5 +90,8 @@ def rccomponents():
>                   _rccomponents.extend(('path', p) for p in _expandrcpath(p))
>           else:
> -            paths = defaultrcpath() + systemrcpath() + userrcpath()
> +            paths = defaultrcpath() + systemrcpath()
>               _rccomponents = [('path', os.path.normpath(p)) for p in paths]
> +            _rccomponents.append(envrc)
> +            paths = userrcpath()
> +            _rccomponents.extend(('path', os.path.normpath(p)) for p in paths)

This line is essentially repeated from above... I think it could be 
factored out in a future clean-up.

>       return _rccomponents
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -212,8 +212,18 @@ class ui(object):
>           """Create a ui and load global and user configs"""
>           u = cls()
> -        # we always trust global config files
> +        # we always trust global config files and environment variables
>           for t, f in rcutil.rccomponents():
>               if t == 'path':
>                   u.readconfig(f, trust=True)
> +            elif t == 'items':
> +                sections = set()
> +                for section, name, value, source in f:
> +                    # do not set u._ocfg

This comment doesn't explain why, so it's not very helpful.

> +                    # XXX clean this up once immutable config object is a thing
> +                    u._tcfg.set(section, name, value, source)
> +                    u._ucfg.set(section, name, value, source)
> +                    sections.add(section)
> +                for section in sections:
> +                    u.fixconfig(section=section)
>               else:
>                   raise error.ProgrammingError('unknown rctype: %s' % t)
> diff --git a/tests/test-config-env.py b/tests/test-config-env.py
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-config-env.py
> @@ -0,0 +1,48 @@
> +# Test the config layer generated by environment variables
> +
> +from __future__ import absolute_import, print_function
> +
> +import os
> +
> +from mercurial import (
> +    encoding,
> +    rcutil,
> +    ui as uimod,
> +)
> +
> +testtmp = encoding.environ['TESTTMP']
> +
> +# prepare hgrc files
> +def join(name):
> +    return os.path.join(testtmp, name)
> +
> +with open(join('sysrc'), 'w') as f:
> +    f.write('[ui]\neditor=e0\n[pager]\npager=p0\n')
> +
> +with open(join('userrc'), 'w') as f:
> +    f.write('[ui]\neditor=e1')
> +
> +# replace rcpath functions so they point to the files above
> +def systemrcpath():
> +    return [join('sysrc')]
> +
> +def userrcpath():
> +    return [join('userrc')]
> +
> +rcutil.systemrcpath = systemrcpath
> +rcutil.userrcpath = userrcpath
> +os.path.isdir = lambda x: False # hack: do not load default.d/*.rc
> +
> +# utility to print configs
> +def printconfigs(env):
> +    encoding.environ = env
> +    rcutil._rccomponents = None # reset cache
> +    ui = uimod.ui.load()
> +    for section, name, value in ui.walkconfig():
> +        source = ui.configsource(section, name)
> +        print('%s.%s=%s # %s' % (section, name, value, source))
> +    print('')
> +
> +# environment variable overrides
> +printconfigs({})
> +printconfigs({'EDITOR': 'e2', 'PAGER': 'p2'})
> diff --git a/tests/test-config-env.py.out b/tests/test-config-env.py.out
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-config-env.py.out
> @@ -0,0 +1,6 @@
> +pager.pager=p0 # $TESTTMP/sysrc:4
> +ui.editor=e1 # $TESTTMP/userrc:2
> +
> +pager.pager=p2 # $PAGER
> +ui.editor=e1 # $TESTTMP/userrc:2
> +
> diff --git a/tests/test-config.t b/tests/test-config.t
> --- a/tests/test-config.t
> +++ b/tests/test-config.t
> @@ -165,2 +165,16 @@ edit failure
>     abort: edit failed: false exited with status 1
>     [255]
> +
> +config affected by environment variables
> +
> +  $ EDITOR=e1 VISUAL=e2 hg config --debug | grep 'ui\.editor'
> +  $VISUAL: ui.editor=e2
> +
> +  $ VISUAL=e2 hg config --debug --config ui.editor=e3 | grep 'ui\.editor'
> +  --config: ui.editor=e3
> +
> +  $ PAGER=p1 hg config --debug | grep 'pager\.pager'
> +  $PAGER: pager.pager=p1
> +
> +  $ PAGER=p1 hg config --debug --config pager.pager=p2 | grep 'pager\.pager'
> +  --config: pager.pager=p2
>
Yuya Nishihara - March 28, 2017, 12:53 p.m.
On Tue, 28 Mar 2017 09:47:18 +0100, Ryan McElroy wrote:
> On 3/27/17 7:02 AM, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1490589217 25200
> > #      Sun Mar 26 21:33:37 2017 -0700
> > # Node ID 38572bb2cffd815526a727bc6f3aacdca2902f4f
> > # Parent  9b0aa30bf151b6c0e999b017fd328e29440bd447
> > rcutil: let environ override system configs (BC)

> > --- a/mercurial/rcutil.py
> > +++ b/mercurial/rcutil.py
> > @@ -77,8 +77,12 @@ def rccomponents():
> >       name, value, source) that should fill the config directly.
> >       '''
> > +    envrc = ('items', envrcitems())
> > +
> >       global _rccomponents
> >       if _rccomponents is None:
> >           if 'HGRCPATH' in encoding.environ:
> > -            _rccomponents = []
> > +            # assume HGRCPATH is all about user configs so environments can be
> > +            # overridden.
> > +            _rccomponents = [envrc]
> >               for p in encoding.environ['HGRCPATH'].split(pycompat.ospathsep):
> >                   if not p:
> > @@ -86,5 +90,8 @@ def rccomponents():
> >                   _rccomponents.extend(('path', p) for p in _expandrcpath(p))
> >           else:
> > -            paths = defaultrcpath() + systemrcpath() + userrcpath()
> > +            paths = defaultrcpath() + systemrcpath()
> >               _rccomponents = [('path', os.path.normpath(p)) for p in paths]
> > +            _rccomponents.append(envrc)
> > +            paths = userrcpath()
> > +            _rccomponents.extend(('path', os.path.normpath(p)) for p in paths)
> 
> This line is essentially repeated from above... I think it could be 
> factored out in a future clean-up.

And we'll need to avoid caching of envrcitems because of chg, could be fixed
by followup.
Jun Wu - March 28, 2017, 2:37 p.m.
Excerpts from Ryan McElroy's message of 2017-03-28 09:47:18 +0100:
> On 3/27/17 7:02 AM, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1490589217 25200
> > #      Sun Mar 26 21:33:37 2017 -0700
> > # Node ID 38572bb2cffd815526a727bc6f3aacdca2902f4f
> > # Parent  9b0aa30bf151b6c0e999b017fd328e29440bd447
> > rcutil: let environ override system configs (BC)
> 
> Small nits inline. Could easily be addressed by a follow-up.
> 
> >
> > This is BC because system configs won't be able to override $EDITOR, $PAGER.
> > The new behavior is arguably more rational.
> >
> > diff --git a/mercurial/commands.py b/mercurial/commands.py
> > --- a/mercurial/commands.py
> > +++ b/mercurial/commands.py
> > @@ -1807,4 +1807,6 @@ def config(ui, repo, *values, **opts):
> >           if t == 'path':
> >               ui.debug('read config from: %s\n' % f)
> > +        elif t == 'items':
> > +            pass
> >           else:
> >               raise error.ProgrammingError('unknown rctype: %s' % t)
> > diff --git a/mercurial/rcutil.py b/mercurial/rcutil.py
> > --- a/mercurial/rcutil.py
> > +++ b/mercurial/rcutil.py
> > @@ -77,8 +77,12 @@ def rccomponents():
> >       name, value, source) that should fill the config directly.
> >       '''
> > +    envrc = ('items', envrcitems())
> > +
> >       global _rccomponents
> >       if _rccomponents is None:
> >           if 'HGRCPATH' in encoding.environ:
> > -            _rccomponents = []
> > +            # assume HGRCPATH is all about user configs so environments can be
> > +            # overridden.
> > +            _rccomponents = [envrc]
> >               for p in encoding.environ['HGRCPATH'].split(pycompat.ospathsep):
> >                   if not p:
> > @@ -86,5 +90,8 @@ def rccomponents():
> >                   _rccomponents.extend(('path', p) for p in _expandrcpath(p))
> >           else:
> > -            paths = defaultrcpath() + systemrcpath() + userrcpath()
> > +            paths = defaultrcpath() + systemrcpath()
> >               _rccomponents = [('path', os.path.normpath(p)) for p in paths]
> > +            _rccomponents.append(envrc)
> > +            paths = userrcpath()
> > +            _rccomponents.extend(('path', os.path.normpath(p)) for p in paths)
> 
> This line is essentially repeated from above... I think it could be 
> factored out in a future clean-up.

I didn't do that because that makes the code at least 2 lines longer.

> 
> >       return _rccomponents
> > diff --git a/mercurial/ui.py b/mercurial/ui.py
> > --- a/mercurial/ui.py
> > +++ b/mercurial/ui.py
> > @@ -212,8 +212,18 @@ class ui(object):
> >           """Create a ui and load global and user configs"""
> >           u = cls()
> > -        # we always trust global config files
> > +        # we always trust global config files and environment variables
> >           for t, f in rcutil.rccomponents():
> >               if t == 'path':
> >                   u.readconfig(f, trust=True)
> > +            elif t == 'items':
> > +                sections = set()
> > +                for section, name, value, source in f:
> > +                    # do not set u._ocfg
> 
> This comment doesn't explain why, so it's not very helpful.

It will disappear with immutable config.

> 
> > +                    # XXX clean this up once immutable config object is a thing
> > +                    u._tcfg.set(section, name, value, source)
> > +                    u._ucfg.set(section, name, value, source)
> > +                    sections.add(section)
> > +                for section in sections:
> > +                    u.fixconfig(section=section)
> >               else:
> >                   raise error.ProgrammingError('unknown rctype: %s' % t)
> > diff --git a/tests/test-config-env.py b/tests/test-config-env.py
> > new file mode 100644
> > --- /dev/null
> > +++ b/tests/test-config-env.py
> > @@ -0,0 +1,48 @@
> > +# Test the config layer generated by environment variables
> > +
> > +from __future__ import absolute_import, print_function
> > +
> > +import os
> > +
> > +from mercurial import (
> > +    encoding,
> > +    rcutil,
> > +    ui as uimod,
> > +)
> > +
> > +testtmp = encoding.environ['TESTTMP']
> > +
> > +# prepare hgrc files
> > +def join(name):
> > +    return os.path.join(testtmp, name)
> > +
> > +with open(join('sysrc'), 'w') as f:
> > +    f.write('[ui]\neditor=e0\n[pager]\npager=p0\n')
> > +
> > +with open(join('userrc'), 'w') as f:
> > +    f.write('[ui]\neditor=e1')
> > +
> > +# replace rcpath functions so they point to the files above
> > +def systemrcpath():
> > +    return [join('sysrc')]
> > +
> > +def userrcpath():
> > +    return [join('userrc')]
> > +
> > +rcutil.systemrcpath = systemrcpath
> > +rcutil.userrcpath = userrcpath
> > +os.path.isdir = lambda x: False # hack: do not load default.d/*.rc
> > +
> > +# utility to print configs
> > +def printconfigs(env):
> > +    encoding.environ = env
> > +    rcutil._rccomponents = None # reset cache
> > +    ui = uimod.ui.load()
> > +    for section, name, value in ui.walkconfig():
> > +        source = ui.configsource(section, name)
> > +        print('%s.%s=%s # %s' % (section, name, value, source))
> > +    print('')
> > +
> > +# environment variable overrides
> > +printconfigs({})
> > +printconfigs({'EDITOR': 'e2', 'PAGER': 'p2'})
> > diff --git a/tests/test-config-env.py.out b/tests/test-config-env.py.out
> > new file mode 100644
> > --- /dev/null
> > +++ b/tests/test-config-env.py.out
> > @@ -0,0 +1,6 @@
> > +pager.pager=p0 # $TESTTMP/sysrc:4
> > +ui.editor=e1 # $TESTTMP/userrc:2
> > +
> > +pager.pager=p2 # $PAGER
> > +ui.editor=e1 # $TESTTMP/userrc:2
> > +
> > diff --git a/tests/test-config.t b/tests/test-config.t
> > --- a/tests/test-config.t
> > +++ b/tests/test-config.t
> > @@ -165,2 +165,16 @@ edit failure
> >     abort: edit failed: false exited with status 1
> >     [255]
> > +
> > +config affected by environment variables
> > +
> > +  $ EDITOR=e1 VISUAL=e2 hg config --debug | grep 'ui\.editor'
> > +  $VISUAL: ui.editor=e2
> > +
> > +  $ VISUAL=e2 hg config --debug --config ui.editor=e3 | grep 'ui\.editor'
> > +  --config: ui.editor=e3
> > +
> > +  $ PAGER=p1 hg config --debug | grep 'pager\.pager'
> > +  $PAGER: pager.pager=p1
> > +
> > +  $ PAGER=p1 hg config --debug --config pager.pager=p2 | grep 'pager\.pager'
> > +  --config: pager.pager=p2
> >
Jun Wu - March 28, 2017, 2:59 p.m.
Excerpts from Yuya Nishihara's message of 2017-03-28 21:53:31 +0900:
> And we'll need to avoid caching of envrcitems because of chg, could be fixed
> by followup.

Good catch. I think that also applies to config files - listing a directory
cannot be cached either. So it seems the bug has been there since the
beginning.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1807,4 +1807,6 @@  def config(ui, repo, *values, **opts):
         if t == 'path':
             ui.debug('read config from: %s\n' % f)
+        elif t == 'items':
+            pass
         else:
             raise error.ProgrammingError('unknown rctype: %s' % t)
diff --git a/mercurial/rcutil.py b/mercurial/rcutil.py
--- a/mercurial/rcutil.py
+++ b/mercurial/rcutil.py
@@ -77,8 +77,12 @@  def rccomponents():
     name, value, source) that should fill the config directly.
     '''
+    envrc = ('items', envrcitems())
+
     global _rccomponents
     if _rccomponents is None:
         if 'HGRCPATH' in encoding.environ:
-            _rccomponents = []
+            # assume HGRCPATH is all about user configs so environments can be
+            # overridden.
+            _rccomponents = [envrc]
             for p in encoding.environ['HGRCPATH'].split(pycompat.ospathsep):
                 if not p:
@@ -86,5 +90,8 @@  def rccomponents():
                 _rccomponents.extend(('path', p) for p in _expandrcpath(p))
         else:
-            paths = defaultrcpath() + systemrcpath() + userrcpath()
+            paths = defaultrcpath() + systemrcpath()
             _rccomponents = [('path', os.path.normpath(p)) for p in paths]
+            _rccomponents.append(envrc)
+            paths = userrcpath()
+            _rccomponents.extend(('path', os.path.normpath(p)) for p in paths)
     return _rccomponents
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -212,8 +212,18 @@  class ui(object):
         """Create a ui and load global and user configs"""
         u = cls()
-        # we always trust global config files
+        # we always trust global config files and environment variables
         for t, f in rcutil.rccomponents():
             if t == 'path':
                 u.readconfig(f, trust=True)
+            elif t == 'items':
+                sections = set()
+                for section, name, value, source in f:
+                    # do not set u._ocfg
+                    # XXX clean this up once immutable config object is a thing
+                    u._tcfg.set(section, name, value, source)
+                    u._ucfg.set(section, name, value, source)
+                    sections.add(section)
+                for section in sections:
+                    u.fixconfig(section=section)
             else:
                 raise error.ProgrammingError('unknown rctype: %s' % t)
diff --git a/tests/test-config-env.py b/tests/test-config-env.py
new file mode 100644
--- /dev/null
+++ b/tests/test-config-env.py
@@ -0,0 +1,48 @@ 
+# Test the config layer generated by environment variables
+
+from __future__ import absolute_import, print_function
+
+import os
+
+from mercurial import (
+    encoding,
+    rcutil,
+    ui as uimod,
+)
+
+testtmp = encoding.environ['TESTTMP']
+
+# prepare hgrc files
+def join(name):
+    return os.path.join(testtmp, name)
+
+with open(join('sysrc'), 'w') as f:
+    f.write('[ui]\neditor=e0\n[pager]\npager=p0\n')
+
+with open(join('userrc'), 'w') as f:
+    f.write('[ui]\neditor=e1')
+
+# replace rcpath functions so they point to the files above
+def systemrcpath():
+    return [join('sysrc')]
+
+def userrcpath():
+    return [join('userrc')]
+
+rcutil.systemrcpath = systemrcpath
+rcutil.userrcpath = userrcpath
+os.path.isdir = lambda x: False # hack: do not load default.d/*.rc
+
+# utility to print configs
+def printconfigs(env):
+    encoding.environ = env
+    rcutil._rccomponents = None # reset cache
+    ui = uimod.ui.load()
+    for section, name, value in ui.walkconfig():
+        source = ui.configsource(section, name)
+        print('%s.%s=%s # %s' % (section, name, value, source))
+    print('')
+
+# environment variable overrides
+printconfigs({})
+printconfigs({'EDITOR': 'e2', 'PAGER': 'p2'})
diff --git a/tests/test-config-env.py.out b/tests/test-config-env.py.out
new file mode 100644
--- /dev/null
+++ b/tests/test-config-env.py.out
@@ -0,0 +1,6 @@ 
+pager.pager=p0 # $TESTTMP/sysrc:4
+ui.editor=e1 # $TESTTMP/userrc:2
+
+pager.pager=p2 # $PAGER
+ui.editor=e1 # $TESTTMP/userrc:2
+
diff --git a/tests/test-config.t b/tests/test-config.t
--- a/tests/test-config.t
+++ b/tests/test-config.t
@@ -165,2 +165,16 @@  edit failure
   abort: edit failed: false exited with status 1
   [255]
+
+config affected by environment variables
+
+  $ EDITOR=e1 VISUAL=e2 hg config --debug | grep 'ui\.editor'
+  $VISUAL: ui.editor=e2
+
+  $ VISUAL=e2 hg config --debug --config ui.editor=e3 | grep 'ui\.editor'
+  --config: ui.editor=e3
+
+  $ PAGER=p1 hg config --debug | grep 'pager\.pager'
+  $PAGER: pager.pager=p1
+
+  $ PAGER=p1 hg config --debug --config pager.pager=p2 | grep 'pager\.pager'
+  --config: pager.pager=p2