Patchwork ui: don't read the same config file twice

login
register
mail settings
Submitter David Soria Parra
Date March 14, 2017, 2:47 a.m.
Message ID <7e2222599b4f534e27a0.1489459667@davidsp-mbp-2.local>
Download mbox | patch
Permalink /patch/19306/
State Changes Requested
Headers show

Comments

David Soria Parra - March 14, 2017, 2:47 a.m.
# HG changeset patch
# User David Soria Parra <davidsp@fb.com>
# Date 1489459557 25200
#      Mon Mar 13 19:45:57 2017 -0700
# Node ID 7e2222599b4f534e27a0462bb1263208cc1c8d71
# Parent  3d3109339b57341b333c1112beb41dd281fa944a
ui: don't read the same config file twice

During dispatch and localrepo setup we might read the same file twice if
the repository we are loading is the currnet working directory/.hg which is the
default case. We now keep a set of filenames we read around and abort
if we read the config already. It would be nice to do this in a decorator,
but I have not found a reliable, simple way to hash arguments.
Jun Wu - March 14, 2017, 2:56 a.m.
This is a BC because loading order matters:

  # a.rc
  %include b.rc
  [ui]
  editor = foo
  %include b.rc

  # b.rc
  [ui]
  editor = bar

Since people should be able to arrange config files in a way that no files
are included twice. I prefer not being too smart in hg.

Excerpts from David Soria Parra's message of 2017-03-13 19:47:47 -0700:
> # HG changeset patch
> # User David Soria Parra <davidsp@fb.com>
> # Date 1489459557 25200
> #      Mon Mar 13 19:45:57 2017 -0700
> # Node ID 7e2222599b4f534e27a0462bb1263208cc1c8d71
> # Parent  3d3109339b57341b333c1112beb41dd281fa944a
> ui: don't read the same config file twice
> 
> During dispatch and localrepo setup we might read the same file twice if
> the repository we are loading is the currnet working directory/.hg which is the
> default case. We now keep a set of filenames we read around and abort
> if we read the config already. It would be nice to do this in a decorator,
> but I have not found a reliable, simple way to hash arguments.
> 
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -160,6 +160,7 @@
>          self._colormode = None
>          self._terminfoparams = {}
>          self._styles = {}
> +        self._readfilenames = set()
>  
>          if src:
>              self.fout = src.fout
> @@ -258,6 +259,9 @@
>  
>      def readconfig(self, filename, root=None, trust=False,
>                     sections=None, remap=None):
> +        if filename in self._readfilenames:
> +            return
> +
>          try:
>              fp = open(filename, u'rb')
>          except IOError:
> @@ -304,6 +308,7 @@
>          if root is None:
>              root = os.path.expanduser('~')
>          self.fixconfig(root=root)
> +        self._readfilenames.add(filename)
>  
>      def fixconfig(self, root=None, section=None):
>          if section in (None, 'paths'):
Ryan McElroy - March 14, 2017, 3:20 a.m.
On 3/13/17 7:56 PM, Jun Wu wrote:
> This is a BC because loading order matters:
>
>    # a.rc
>    %include b.rc
>    [ui]
>    editor = foo
>    %include b.rc
>
>    # b.rc
>    [ui]
>    editor = bar
>
> Since people should be able to arrange config files in a way that no files
> are included twice. I prefer not being too smart in hg.

I agree with Jun that we need preserve last-loaded-wins semantics. We 
could still build single-loading if we read config files once, kept them 
in separate dicts in an ordered set, and just reordered to the end of a 
list when re-read, and once all configs are read, then combine the dicts 
in list order, but that sounds a bit complex.

I'd still be supportive of that direction if there's a perf win though.

>
> Excerpts from David Soria Parra's message of 2017-03-13 19:47:47 -0700:
>> # HG changeset patch
>> # User David Soria Parra <davidsp@fb.com>
>> # Date 1489459557 25200
>> #      Mon Mar 13 19:45:57 2017 -0700
>> # Node ID 7e2222599b4f534e27a0462bb1263208cc1c8d71
>> # Parent  3d3109339b57341b333c1112beb41dd281fa944a
>> ui: don't read the same config file twice
>>
>> During dispatch and localrepo setup we might read the same file twice if
>> the repository we are loading is the currnet working directory/.hg which is the

Typo: current is misspelled

>> default case. We now keep a set of filenames we read around and abort

We don't abort, we return early. Abort would imply raising an 
error.Abort (the way I inrepretted it anyway).

>> if we read the config already. It would be nice to do this in a decorator,
>> but I have not found a reliable, simple way to hash arguments.
>>
>> diff --git a/mercurial/ui.py b/mercurial/ui.py
>> --- a/mercurial/ui.py
>> +++ b/mercurial/ui.py
>> @@ -160,6 +160,7 @@
>>           self._colormode = None
>>           self._terminfoparams = {}
>>           self._styles = {}
>> +        self._readfilenames = set()
>>   
>>           if src:
>>               self.fout = src.fout
>> @@ -258,6 +259,9 @@
>>   
>>       def readconfig(self, filename, root=None, trust=False,
>>                      sections=None, remap=None):
>> +        if filename in self._readfilenames:
>> +            return
>> +
>>           try:
>>               fp = open(filename, u'rb')
>>           except IOError:
>> @@ -304,6 +308,7 @@
>>           if root is None:
>>               root = os.path.expanduser('~')
>>           self.fixconfig(root=root)
>> +        self._readfilenames.add(filename)
>>   
>>       def fixconfig(self, root=None, section=None):
>>           if section in (None, 'paths'):
David Soria Parra - March 14, 2017, 4:22 a.m.
On Mon, Mar 13, 2017 at 07:56:09PM -0700, Jun Wu wrote:
> This is a BC because loading order matters:

good catch. let's drop it until i find a better solution.

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -160,6 +160,7 @@ 
         self._colormode = None
         self._terminfoparams = {}
         self._styles = {}
+        self._readfilenames = set()
 
         if src:
             self.fout = src.fout
@@ -258,6 +259,9 @@ 
 
     def readconfig(self, filename, root=None, trust=False,
                    sections=None, remap=None):
+        if filename in self._readfilenames:
+            return
+
         try:
             fp = open(filename, u'rb')
         except IOError:
@@ -304,6 +308,7 @@ 
         if root is None:
             root = os.path.expanduser('~')
         self.fixconfig(root=root)
+        self._readfilenames.add(filename)
 
     def fixconfig(self, root=None, section=None):
         if section in (None, 'paths'):