Patchwork [3,of,9] py3: replace os.environ with encoding.environ (part 3 of 5)

login
register
mail settings
Submitter Pulkit Goyal
Date Dec. 20, 2016, 2:03 p.m.
Message ID <9cea8eb14d9e7fbf9be2.1482242629@pulkit-goyal.Home>
Download mbox | patch
Permalink /patch/17975/
State Accepted
Headers show

Comments

Pulkit Goyal - Dec. 20, 2016, 2:03 p.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1482006276 -19800
#      Sun Dec 18 01:54:36 2016 +0530
# Node ID 9cea8eb14d9e7fbf9be2fa6a67c94e6832d57c11
# Parent  c1a8b0e2a088a8cd365f7aa6a3f5d609fc57a92f
py3: replace os.environ with encoding.environ (part 3 of 5)
Yuya Nishihara - Dec. 21, 2016, 3:11 p.m.
On Tue, 20 Dec 2016 19:33:49 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1482006276 -19800
> #      Sun Dec 18 01:54:36 2016 +0530
> # Node ID 9cea8eb14d9e7fbf9be2fa6a67c94e6832d57c11
> # Parent  c1a8b0e2a088a8cd365f7aa6a3f5d609fc57a92f
> py3: replace os.environ with encoding.environ (part 3 of 5)

> --- a/mercurial/url.py	Sun Dec 18 01:46:39 2016 +0530
> +++ b/mercurial/url.py	Sun Dec 18 01:54:36 2016 +0530
> @@ -15,6 +15,7 @@
>  
>  from .i18n import _
>  from . import (
> +    encoding,
>      error,
>      httpconnection as httpconnectionmod,
>      keepalive,
> @@ -118,8 +119,8 @@
>          if ui.config("http_proxy", "host"):
>              for env in ["HTTP_PROXY", "http_proxy", "no_proxy"]:
>                  try:
> -                    if env in os.environ:
> -                        del os.environ[env]
> +                    if env in encoding.environ:
> +                        del encoding.environ[env]
>                  except OSError:
>                      pass

Here we have to pass new environ dict to urllib2 over the global os.environ
dict. So we'll need to revisit this part later since encoding.environ can be
a read-only copy of os.environ on Python 3.
Pulkit Goyal - Dec. 21, 2016, 4:55 p.m.
>>          if ui.config("http_proxy", "host"):
>>              for env in ["HTTP_PROXY", "http_proxy", "no_proxy"]:
>>                  try:
>> -                    if env in os.environ:
>> -                        del os.environ[env]
>> +                    if env in encoding.environ:
>> +                        del encoding.environ[env]
>>                  except OSError:
>>                      pass
>
> Here we have to pass new environ dict to urllib2 over the global os.environ
> dict. So we'll need to revisit this part later since encoding.environ can be
> a read-only copy of os.environ on Python 3.

We have two possible options,

1.  for env in [u"HTTP_PROXY", u"http_proxy", u"no_proxy"]:
                 try:
                    if env in os.environ:
                        del os.environ[env]

2. for env in ["HTTP_PROXY", "http_proxy", "no_proxy"]:
                 try:
                    if py3:
                        if env in os.environb:
                            del os.environb[env]
                    else:
                        if env in os.environ:
                            del os.environ[env]

IIUC encoding.environ everytimes goes into encoding.py, check those
conditions and returns a dictionary by reading os.environ or
os.environb. Or is it like every time we are reading(getting by
calling encoding.environ) the same dictionary.

If first argument is correct than any of these will work because
os.environ and os.environb are synchronised in python 3.
Yuya Nishihara - Dec. 22, 2016, 1:13 p.m.
On Wed, 21 Dec 2016 22:25:25 +0530, Pulkit Goyal wrote:
> >>          if ui.config("http_proxy", "host"):
> >>              for env in ["HTTP_PROXY", "http_proxy", "no_proxy"]:
> >>                  try:
> >> -                    if env in os.environ:
> >> -                        del os.environ[env]
> >> +                    if env in encoding.environ:
> >> +                        del encoding.environ[env]
> >>                  except OSError:
> >>                      pass
> >
> > Here we have to pass new environ dict to urllib2 over the global os.environ
> > dict. So we'll need to revisit this part later since encoding.environ can be
> > a read-only copy of os.environ on Python 3.
> 
> We have two possible options,
> 
> 1.  for env in [u"HTTP_PROXY", u"http_proxy", u"no_proxy"]:
>                  try:
>                     if env in os.environ:
>                         del os.environ[env]
> 
> 2. for env in ["HTTP_PROXY", "http_proxy", "no_proxy"]:
>                  try:
>                     if py3:
>                         if env in os.environb:
>                             del os.environb[env]
>                     else:
>                         if env in os.environ:
>                             del os.environ[env]
> 
> IIUC encoding.environ everytimes goes into encoding.py, check those
> conditions and returns a dictionary by reading os.environ or
> os.environb. Or is it like every time we are reading(getting by
> calling encoding.environ) the same dictionary.
> 
> If first argument is correct than any of these will work because
> os.environ and os.environb are synchronised in python 3.

Perhaps a better option is to prevent the use of http_proxy in some way other
than modifying the environ dict. Updating the environ here is theoretically
wrong since ui may have per-repository values.

https://docs.python.org/2/library/urllib2.html#urllib2.ProxyHandler
Pulkit Goyal - Dec. 23, 2016, 6:11 p.m.
I am now confused as why we have this block as it must not be there.
Digging into the ProxyHandler class, it takes proxy values from the
environment if we pass proxies as None. And in url.py, its sure that
we don't pass proxies as None.
Look for bug 2451, whose resolution resulted into the current state.
https://bz.mercurial-scm.org/show_bug.cgi?id=2451.
Comment 6 explains my feelings. :)

On Thu, Dec 22, 2016 at 6:43 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Wed, 21 Dec 2016 22:25:25 +0530, Pulkit Goyal wrote:
>> >>          if ui.config("http_proxy", "host"):
>> >>              for env in ["HTTP_PROXY", "http_proxy", "no_proxy"]:
>> >>                  try:
>> >> -                    if env in os.environ:
>> >> -                        del os.environ[env]
>> >> +                    if env in encoding.environ:
>> >> +                        del encoding.environ[env]
>> >>                  except OSError:
>> >>                      pass
>> >
>> > Here we have to pass new environ dict to urllib2 over the global os.environ
>> > dict. So we'll need to revisit this part later since encoding.environ can be
>> > a read-only copy of os.environ on Python 3.
>>
>> We have two possible options,
>>
>> 1.  for env in [u"HTTP_PROXY", u"http_proxy", u"no_proxy"]:
>>                  try:
>>                     if env in os.environ:
>>                         del os.environ[env]
>>
>> 2. for env in ["HTTP_PROXY", "http_proxy", "no_proxy"]:
>>                  try:
>>                     if py3:
>>                         if env in os.environb:
>>                             del os.environb[env]
>>                     else:
>>                         if env in os.environ:
>>                             del os.environ[env]
>>
>> IIUC encoding.environ everytimes goes into encoding.py, check those
>> conditions and returns a dictionary by reading os.environ or
>> os.environb. Or is it like every time we are reading(getting by
>> calling encoding.environ) the same dictionary.
>>
>> If first argument is correct than any of these will work because
>> os.environ and os.environb are synchronised in python 3.
>
> Perhaps a better option is to prevent the use of http_proxy in some way other
> than modifying the environ dict. Updating the environ here is theoretically
> wrong since ui may have per-repository values.
>
> https://docs.python.org/2/library/urllib2.html#urllib2.ProxyHandler
Yuya Nishihara - Dec. 25, 2016, 7:55 a.m.
On Fri, 23 Dec 2016 23:41:53 +0530, Pulkit Goyal wrote:
> I am now confused as why we have this block as it must not be there.
> Digging into the ProxyHandler class, it takes proxy values from the
> environment if we pass proxies as None. And in url.py, its sure that
> we don't pass proxies as None.
> Look for bug 2451, whose resolution resulted into the current state.
> https://bz.mercurial-scm.org/show_bug.cgi?id=2451.
> Comment 6 explains my feelings. :)

Yep. Appears that 95a53961d7a6 removed the necessity of the environ hack.

https://www.mercurial-scm.org/repo/hg/rev/95a53961d7a6

Patch

diff -r c1a8b0e2a088 -r 9cea8eb14d9e mercurial/filemerge.py
--- a/mercurial/filemerge.py	Sun Dec 18 01:46:39 2016 +0530
+++ b/mercurial/filemerge.py	Sun Dec 18 01:54:36 2016 +0530
@@ -16,6 +16,7 @@ 
 from .node import nullid, short
 
 from . import (
+    encoding,
     error,
     formatter,
     match,
@@ -165,7 +166,7 @@ 
                 return (force, force)
 
     # HGMERGE takes next precedence
-    hgmerge = os.environ.get("HGMERGE")
+    hgmerge = encoding.environ.get("HGMERGE")
     if hgmerge:
         if changedelete and not supportscd(hgmerge):
             return ":prompt", None
diff -r c1a8b0e2a088 -r 9cea8eb14d9e mercurial/hgweb/common.py
--- a/mercurial/hgweb/common.py	Sun Dec 18 01:46:39 2016 +0530
+++ b/mercurial/hgweb/common.py	Sun Dec 18 01:54:36 2016 +0530
@@ -13,6 +13,7 @@ 
 import os
 
 from .. import (
+    encoding,
     pycompat,
     util,
 )
@@ -191,7 +192,7 @@ 
     """
     return (config("web", "contact") or
             config("ui", "username") or
-            os.environ.get("EMAIL") or "")
+            encoding.environ.get("EMAIL") or "")
 
 def caching(web, req):
     tag = 'W/"%s"' % web.mtime
diff -r c1a8b0e2a088 -r 9cea8eb14d9e mercurial/hgweb/hgweb_mod.py
--- a/mercurial/hgweb/hgweb_mod.py	Sun Dec 18 01:46:39 2016 +0530
+++ b/mercurial/hgweb/hgweb_mod.py	Sun Dec 18 01:54:36 2016 +0530
@@ -286,7 +286,8 @@ 
         Modern servers should be using WSGI and should avoid this
         method, if possible.
         """
-        if not os.environ.get('GATEWAY_INTERFACE', '').startswith("CGI/1."):
+        if not encoding.environ.get('GATEWAY_INTERFACE',\
+                        '').startswith("CGI/1."):
             raise RuntimeError("This function is only intended to be "
                                "called while running as a CGI script.")
         wsgicgi.launch(self)
diff -r c1a8b0e2a088 -r 9cea8eb14d9e mercurial/hgweb/hgwebdir_mod.py
--- a/mercurial/hgweb/hgwebdir_mod.py	Sun Dec 18 01:46:39 2016 +0530
+++ b/mercurial/hgweb/hgwebdir_mod.py	Sun Dec 18 01:54:36 2016 +0530
@@ -186,7 +186,8 @@ 
         self.lastrefresh = time.time()
 
     def run(self):
-        if not os.environ.get('GATEWAY_INTERFACE', '').startswith("CGI/1."):
+        if not encoding.environ.get('GATEWAY_INTERFACE',\
+                        '').startswith("CGI/1."):
             raise RuntimeError("This function is only intended to be "
                                "called while running as a CGI script.")
         wsgicgi.launch(self)
diff -r c1a8b0e2a088 -r 9cea8eb14d9e mercurial/hgweb/wsgicgi.py
--- a/mercurial/hgweb/wsgicgi.py	Sun Dec 18 01:46:39 2016 +0530
+++ b/mercurial/hgweb/wsgicgi.py	Sun Dec 18 01:54:36 2016 +0530
@@ -10,9 +10,8 @@ 
 
 from __future__ import absolute_import
 
-import os
-
 from .. import (
+    encoding,
     util,
 )
 
@@ -24,7 +23,7 @@ 
     util.setbinary(util.stdin)
     util.setbinary(util.stdout)
 
-    environ = dict(os.environ.iteritems())
+    environ = dict(encoding.environ.iteritems())
     environ.setdefault('PATH_INFO', '')
     if environ.get('SERVER_SOFTWARE', '').startswith('Microsoft-IIS'):
         # IIS includes script_name in PATH_INFO
diff -r c1a8b0e2a088 -r 9cea8eb14d9e mercurial/url.py
--- a/mercurial/url.py	Sun Dec 18 01:46:39 2016 +0530
+++ b/mercurial/url.py	Sun Dec 18 01:54:36 2016 +0530
@@ -15,6 +15,7 @@ 
 
 from .i18n import _
 from . import (
+    encoding,
     error,
     httpconnection as httpconnectionmod,
     keepalive,
@@ -118,8 +119,8 @@ 
         if ui.config("http_proxy", "host"):
             for env in ["HTTP_PROXY", "http_proxy", "no_proxy"]:
                 try:
-                    if env in os.environ:
-                        del os.environ[env]
+                    if env in encoding.environ:
+                        del encoding.environ[env]
                 except OSError:
                     pass
 
diff -r c1a8b0e2a088 -r 9cea8eb14d9e mercurial/windows.py
--- a/mercurial/windows.py	Sun Dec 18 01:46:39 2016 +0530
+++ b/mercurial/windows.py	Sun Dec 18 01:54:36 2016 +0530
@@ -177,7 +177,7 @@ 
     try:
         return sys.getwindowsversion()[3] == 1
     except AttributeError:
-        return 'command' in os.environ.get('comspec', '')
+        return 'command' in encoding.environ.get('comspec', '')
 
 def openhardlinks():
     return not _is_win_9x()
@@ -303,7 +303,7 @@ 
     PATH isn't searched if command is an absolute or relative path.
     An extension from PATHEXT is found and added if not present.
     If command isn't found None is returned.'''
-    pathext = os.environ.get('PATHEXT', '.COM;.EXE;.BAT;.CMD')
+    pathext = encoding.environ.get('PATHEXT', '.COM;.EXE;.BAT;.CMD')
     pathexts = [ext for ext in pathext.lower().split(pycompat.ospathsep)]
     if os.path.splitext(command)[1].lower() in pathexts:
         pathexts = ['']
@@ -319,7 +319,7 @@ 
     if pycompat.ossep in command:
         return findexisting(command)
 
-    for path in os.environ.get('PATH', '').split(pycompat.ospathsep):
+    for path in encoding.environ.get('PATH', '').split(pycompat.ospathsep):
         executable = findexisting(os.path.join(path, command))
         if executable is not None:
             return executable