Patchwork [1,of,4] config: don't read the same config file twice

login
register
mail settings
Submitter Mads Kiilerich
Date Sept. 25, 2014, 1:33 a.m.
Message ID <e04d746df4b9a7c5ef32.1411608782@localhost.localdomain>
Download mbox | patch
Permalink /patch/5969/
State Accepted
Commit 23c995ed466b5a5a8b486c3abdf523891ff42fbe
Headers show

Comments

Mads Kiilerich - Sept. 25, 2014, 1:33 a.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1409859395 -7200
#      Thu Sep 04 21:36:35 2014 +0200
# Node ID e04d746df4b9a7c5ef32b0bf70284e3dd8402791
# Parent  fa3181323c0aa9b2d2c2f81a7d68d57bd3a7a515
config: don't read the same config file twice

In some cases some config files would be read twice and shown twice in
showconfig --debug.
Pierre-Yves David - Sept. 25, 2014, 2:48 a.m.
On 09/24/2014 06:33 PM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1409859395 -7200
> #      Thu Sep 04 21:36:35 2014 +0200
> # Node ID e04d746df4b9a7c5ef32b0bf70284e3dd8402791
> # Parent  fa3181323c0aa9b2d2c2f81a7d68d57bd3a7a515
> config: don't read the same config file twice
>
> In some cases some config files would be read twice and shown twice in
> showconfig --debug.

Can you details how the two changes fixes this bug? I'm curious.

>
> diff --git a/mercurial/scmposix.py b/mercurial/scmposix.py
> --- a/mercurial/scmposix.py
> +++ b/mercurial/scmposix.py
> @@ -21,7 +21,8 @@ def systemrcpath():
>       # old mod_python does not set sys.argv
>       if len(getattr(sys, 'argv', [])) > 0:
>           p = os.path.dirname(os.path.dirname(sys.argv[0]))
> -        path.extend(_rcfiles(os.path.join(p, root)))
> +        if p != '/':
> +            path.extend(_rcfiles(os.path.join(p, root)))
>       path.extend(_rcfiles('/' + root))
>       return path
>
> diff --git a/mercurial/scmwindows.py b/mercurial/scmwindows.py
> --- a/mercurial/scmwindows.py
> +++ b/mercurial/scmwindows.py
> @@ -40,7 +40,7 @@ def userrcpath():
>       path = [os.path.join(home, 'mercurial.ini'),
>               os.path.join(home, '.hgrc')]
>       userprofile = os.environ.get('USERPROFILE')
> -    if userprofile:
> +    if userprofile and userprofile != home:
>           path.append(os.path.join(userprofile, 'mercurial.ini'))
>           path.append(os.path.join(userprofile, '.hgrc'))
>       return path
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Mads Kiilerich - Sept. 25, 2014, 3:58 p.m.
On 09/25/2014 04:48 AM, Pierre-Yves David wrote:
>
>
> On 09/24/2014 06:33 PM, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1409859395 -7200
>> #      Thu Sep 04 21:36:35 2014 +0200
>> # Node ID e04d746df4b9a7c5ef32b0bf70284e3dd8402791
>> # Parent  fa3181323c0aa9b2d2c2f81a7d68d57bd3a7a515
>> config: don't read the same config file twice
>>
>> In some cases some config files would be read twice and shown twice in
>> showconfig --debug.
>
> Can you details how the two changes fixes this bug? I'm curious.
>
>>
>> diff --git a/mercurial/scmposix.py b/mercurial/scmposix.py
>> --- a/mercurial/scmposix.py
>> +++ b/mercurial/scmposix.py
>> @@ -21,7 +21,8 @@ def systemrcpath():
>>       # old mod_python does not set sys.argv
>>       if len(getattr(sys, 'argv', [])) > 0:
>>           p = os.path.dirname(os.path.dirname(sys.argv[0]))
>> -        path.extend(_rcfiles(os.path.join(p, root)))
>> +        if p != '/':
>> +            path.extend(_rcfiles(os.path.join(p, root)))
>>       path.extend(_rcfiles('/' + root))

The same paths were added twice if p == '/'.

>>       return path
>>
>> diff --git a/mercurial/scmwindows.py b/mercurial/scmwindows.py
>> --- a/mercurial/scmwindows.py
>> +++ b/mercurial/scmwindows.py
>> @@ -40,7 +40,7 @@ def userrcpath():
>>       path = [os.path.join(home, 'mercurial.ini'),
>>               os.path.join(home, '.hgrc')]
>>       userprofile = os.environ.get('USERPROFILE')
>> -    if userprofile:
>> +    if userprofile and userprofile != home:
>>           path.append(os.path.join(userprofile, 'mercurial.ini'))
>>           path.append(os.path.join(userprofile, '.hgrc'))

The same paths were added twice if home == userprofile.

>>       return path

/Mads
Pierre-Yves David - Sept. 26, 2014, 1:25 a.m.
On 09/25/2014 08:58 AM, Mads Kiilerich wrote:
> On 09/25/2014 04:48 AM, Pierre-Yves David wrote:
>>
>>
>> On 09/24/2014 06:33 PM, Mads Kiilerich wrote:
>>> # HG changeset patch
>>> # User Mads Kiilerich <madski@unity3d.com>
>>> # Date 1409859395 -7200
>>> #      Thu Sep 04 21:36:35 2014 +0200
>>> # Node ID e04d746df4b9a7c5ef32b0bf70284e3dd8402791
>>> # Parent  fa3181323c0aa9b2d2c2f81a7d68d57bd3a7a515
>>> config: don't read the same config file twice
>>>
>>> In some cases some config files would be read twice and shown twice in
>>> showconfig --debug.
>>
>> Can you details how the two changes fixes this bug? I'm curious.
>>
>>>
>>> diff --git a/mercurial/scmposix.py b/mercurial/scmposix.py
>>> --- a/mercurial/scmposix.py
>>> +++ b/mercurial/scmposix.py
>>> @@ -21,7 +21,8 @@ def systemrcpath():
>>>       # old mod_python does not set sys.argv
>>>       if len(getattr(sys, 'argv', [])) > 0:
>>>           p = os.path.dirname(os.path.dirname(sys.argv[0]))
>>> -        path.extend(_rcfiles(os.path.join(p, root)))
>>> +        if p != '/':
>>> +            path.extend(_rcfiles(os.path.join(p, root)))
>>>       path.extend(_rcfiles('/' + root))
>
> The same paths were added twice if p == '/'.

Well, I already assumed that your changed this in relation with the 
patch description. But this does not explain how this double adding was 
happening.
Mads Kiilerich - Sept. 26, 2014, 5:11 p.m.
On 09/26/2014 03:25 AM, Pierre-Yves David wrote:
>
>
> On 09/25/2014 08:58 AM, Mads Kiilerich wrote:
>> On 09/25/2014 04:48 AM, Pierre-Yves David wrote:
>>>
>>>
>>> On 09/24/2014 06:33 PM, Mads Kiilerich wrote:
>>>> # HG changeset patch
>>>> # User Mads Kiilerich <madski@unity3d.com>
>>>> # Date 1409859395 -7200
>>>> #      Thu Sep 04 21:36:35 2014 +0200
>>>> # Node ID e04d746df4b9a7c5ef32b0bf70284e3dd8402791
>>>> # Parent  fa3181323c0aa9b2d2c2f81a7d68d57bd3a7a515
>>>> config: don't read the same config file twice
>>>>
>>>> In some cases some config files would be read twice and shown twice in
>>>> showconfig --debug.
>>>
>>> Can you details how the two changes fixes this bug? I'm curious.
>>>
>>>>
>>>> diff --git a/mercurial/scmposix.py b/mercurial/scmposix.py
>>>> --- a/mercurial/scmposix.py
>>>> +++ b/mercurial/scmposix.py
>>>> @@ -21,7 +21,8 @@ def systemrcpath():
>>>>       # old mod_python does not set sys.argv
>>>>       if len(getattr(sys, 'argv', [])) > 0:
>>>>           p = os.path.dirname(os.path.dirname(sys.argv[0]))
>>>> -        path.extend(_rcfiles(os.path.join(p, root)))
>>>> +        if p != '/':
>>>> +            path.extend(_rcfiles(os.path.join(p, root)))
>>>>       path.extend(_rcfiles('/' + root))
>>
>> The same paths were added twice if p == '/'.
>
> Well, I already assumed that your changed this in relation with the 
> patch description. But this does not explain how this double adding 
> was happening.
>

well ... if os.path.dirname(os.path.dirname(sys.argv[0])) == '/' ... 
which I guess can happen if hg is installed in /X/hg . As the confusing 
documentation shows, we have several different rules for finding config 
files. It is thus fair enough that they sometimes will give the same 
result - in the same place in the priority list. No matter what cases 
that is, there is no need to read the same twice.

In the other case it can happen if   os.path.expanduser('~') == 
os.environ.get('USERPROFILE')   - I have no idea when that can happen, 
but it sounds plausible that it can happen, and if it happens then it 
totally makes sense to only read the file once.

/Mads
Pierre-Yves David - Sept. 29, 2014, 10:24 p.m.
On 09/24/2014 06:33 PM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1409859395 -7200
> #      Thu Sep 04 21:36:35 2014 +0200
> # Node ID e04d746df4b9a7c5ef32b0bf70284e3dd8402791
> # Parent  fa3181323c0aa9b2d2c2f81a7d68d57bd3a7a515
> config: don't read the same config file twice

Ok, I had a deeper look into this and this patch seems obviously 
correct. However, could we get a simpler and more robust version of this 
by checking is a path is already present before appending it.

How complicated would that be?

Sorry for the delay,
Mads Kiilerich - Sept. 29, 2014, 10:48 p.m.
On 09/30/2014 12:24 AM, Pierre-Yves David wrote:
>
>
> On 09/24/2014 06:33 PM, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1409859395 -7200
>> #      Thu Sep 04 21:36:35 2014 +0200
>> # Node ID e04d746df4b9a7c5ef32b0bf70284e3dd8402791
>> # Parent  fa3181323c0aa9b2d2c2f81a7d68d57bd3a7a515
>> config: don't read the same config file twice
>
> Ok, I had a deeper look into this and this patch seems obviously 
> correct. However, could we get a simpler and more robust version of 
> this by checking is a path is already present before appending it.
>
> How complicated would that be?

That would be complication level 7! ;-)

The patch I propose checks for the root cause of duplication and 
prevents adding it. I don't think it can be done any shorter or more 
spot-on.

It is very unlikely that same path is added twice for other reasons. 
Additional de-duplication would thus be redundant.

Removing duplicates after they have been added would not be complicated. 
But it would be significantly more complex than what I propose. It would 
also be more expensive (but with very low constants). I guess the 
simplest and most efficient solution would be a O(n*2) looping through 
the list. It would add complexity with no gain. I would rather not go there.

/Mads
Pierre-Yves David - Sept. 29, 2014, 10:52 p.m.
On 09/29/2014 03:48 PM, Mads Kiilerich wrote:
> On 09/30/2014 12:24 AM, Pierre-Yves David wrote:
>>
>>
>> On 09/24/2014 06:33 PM, Mads Kiilerich wrote:
>>> # HG changeset patch
>>> # User Mads Kiilerich <madski@unity3d.com>
>>> # Date 1409859395 -7200
>>> #      Thu Sep 04 21:36:35 2014 +0200
>>> # Node ID e04d746df4b9a7c5ef32b0bf70284e3dd8402791
>>> # Parent  fa3181323c0aa9b2d2c2f81a7d68d57bd3a7a515
>>> config: don't read the same config file twice
>>
>> Ok, I had a deeper look into this and this patch seems obviously
>> correct. However, could we get a simpler and more robust version of
>> this by checking is a path is already present before appending it.
>>
>> How complicated would that be?
>
> That would be complication level 7! ;-)
>
> The patch I propose checks for the root cause of duplication and
> prevents adding it. I don't think it can be done any shorter or more
> spot-on.
>
> It is very unlikely that same path is added twice for other reasons.
> Additional de-duplication would thus be redundant.
>
> Removing duplicates after they have been added would not be complicated.
> But it would be significantly more complex than what I propose. It would
> also be more expensive (but with very low constants). I guess the
> simplest and most efficient solution would be a O(n*2) looping through
> the list. It would add complexity with no gain. I would rather not go
> there.

Do we expect people to have path lenght were N² matters? Anyway I've 
queued this first one. As change are requested for 2 and 3. This 
conclude this round of review.

Patch

diff --git a/mercurial/scmposix.py b/mercurial/scmposix.py
--- a/mercurial/scmposix.py
+++ b/mercurial/scmposix.py
@@ -21,7 +21,8 @@  def systemrcpath():
     # old mod_python does not set sys.argv
     if len(getattr(sys, 'argv', [])) > 0:
         p = os.path.dirname(os.path.dirname(sys.argv[0]))
-        path.extend(_rcfiles(os.path.join(p, root)))
+        if p != '/':
+            path.extend(_rcfiles(os.path.join(p, root)))
     path.extend(_rcfiles('/' + root))
     return path
 
diff --git a/mercurial/scmwindows.py b/mercurial/scmwindows.py
--- a/mercurial/scmwindows.py
+++ b/mercurial/scmwindows.py
@@ -40,7 +40,7 @@  def userrcpath():
     path = [os.path.join(home, 'mercurial.ini'),
             os.path.join(home, '.hgrc')]
     userprofile = os.environ.get('USERPROFILE')
-    if userprofile:
+    if userprofile and userprofile != home:
         path.append(os.path.join(userprofile, 'mercurial.ini'))
         path.append(os.path.join(userprofile, '.hgrc'))
     return path