Patchwork [7,of,7] py3: use encoding.environ in ui.py

login
register
mail settings
Submitter Pulkit Goyal
Date Nov. 2, 2016, 10:15 p.m.
Message ID <9bdf6ee77b17dc2a1ce2.1478124921@pulkit-goyal>
Download mbox | patch
Permalink /patch/17297/
State Not Applicable
Headers show

Comments

Pulkit Goyal - Nov. 2, 2016, 10:15 p.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1478122977 -19800
#      Thu Nov 03 03:12:57 2016 +0530
# Node ID 9bdf6ee77b17dc2a1ce29fb347e90bd8dd17eaed
# Parent  8cd0e855ebb448a4ec1e835b346a920a1cb835cb
py3: use encoding.environ in ui.py

Using source transformer we add b'' everywhere. So there are no chances
that those bytes string will work with os.environ on Py3 as that returns a
dict of unicodes. We are relying on the errors, even though no error is raised
even in future, these pieces of codes will tend to do wrong things.
if statements can result in wrong boolean and certain errors can be raised
while using this piece of code. Let's not wait for them to happen, fix what is
wrong. If this patch goes in, I will try to do it for all the cases.
Leaving it as it is buggy.
Pulkit Goyal - Nov. 5, 2016, 12:32 a.m.
On Thu, Nov 3, 2016 at 3:45 AM, Pulkit Goyal <7895pulkit@gmail.com> wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1478122977 -19800
> #      Thu Nov 03 03:12:57 2016 +0530
> # Node ID 9bdf6ee77b17dc2a1ce29fb347e90bd8dd17eaed
> # Parent  8cd0e855ebb448a4ec1e835b346a920a1cb835cb
> py3: use encoding.environ in ui.py
>
> Using source transformer we add b'' everywhere. So there are no chances
> that those bytes string will work with os.environ on Py3 as that returns a
> dict of unicodes. We are relying on the errors, even though no error is raised
> even in future, these pieces of codes will tend to do wrong things.
> if statements can result in wrong boolean and certain errors can be raised
> while using this piece of code. Let's not wait for them to happen, fix what is
> wrong. If this patch goes in, I will try to do it for all the cases.
Yuya any comments on this. I remember asking this from you in the
sprint and your answer was no. But I think we have to change all
occurences.
> Leaving it as it is buggy.
>
> diff -r 8cd0e855ebb4 -r 9bdf6ee77b17 mercurial/ui.py
> --- a/mercurial/ui.py   Thu Nov 03 02:53:45 2016 +0530
> +++ b/mercurial/ui.py   Thu Nov 03 03:12:57 2016 +0530
> @@ -22,6 +22,7 @@
>
>  from . import (
>      config,
> +    encoding,
>      error,
>      formatter,
>      progress,
> @@ -574,9 +575,11 @@
>          - False if HGPLAIN is not set, or feature is in HGPLAINEXCEPT
>          - True otherwise
>          '''
> -        if 'HGPLAIN' not in os.environ and 'HGPLAINEXCEPT' not in os.environ:
> +        if ('HGPLAIN' not in encoding.environ and
> +                'HGPLAINEXCEPT' not in encoding.environ):
>              return False
> -        exceptions = os.environ.get('HGPLAINEXCEPT', '').strip().split(',')
> +        exceptions = encoding.environ.get('HGPLAINEXCEPT',
> +                '').strip().split(',')
>          if feature and exceptions:
>              return feature not in exceptions
>          return True
> @@ -589,13 +592,13 @@
>          If not found and ui.askusername is True, ask the user, else use
>          ($LOGNAME or $USER or $LNAME or $USERNAME) + "@full.hostname".
>          """
> -        user = os.environ.get("HGUSER")
> +        user = encoding.environ.get("HGUSER")
>          if user is None:
>              user = self.config("ui", ["username", "user"])
>              if user is not None:
>                  user = os.path.expandvars(user)
>          if user is None:
> -            user = os.environ.get("EMAIL")
> +            user = encoding.environ.get("EMAIL")
>          if user is None and self.configbool("ui", "askusername"):
>              user = self.prompt(_("enter a commit username:"), default=None)
>          if user is None and not self.interactive():
> @@ -819,9 +822,9 @@
>      def termwidth(self):
>          '''how wide is the terminal in columns?
>          '''
> -        if 'COLUMNS' in os.environ:
> +        if 'COLUMNS' in encoding.environ:
>              try:
> -                return int(os.environ['COLUMNS'])
> +                return int(encoding.environ['COLUMNS'])
>              except ValueError:
>                  pass
>          return util.termwidth()
> @@ -1080,10 +1083,10 @@
>              editor = 'E'
>          else:
>              editor = 'vi'
> -        return (os.environ.get("HGEDITOR") or
> +        return (encoding.environ.get("HGEDITOR") or
>                  self.config("ui", "editor") or
> -                os.environ.get("VISUAL") or
> -                os.environ.get("EDITOR", editor))
> +                encoding.environ.get("VISUAL") or
> +                encoding.environ.get("EDITOR", editor))
>
>      @util.propertycache
>      def _progbar(self):
Yuya Nishihara - Nov. 5, 2016, 1:35 a.m.
On Sat, 5 Nov 2016 06:02:52 +0530, Pulkit Goyal wrote:
> On Thu, Nov 3, 2016 at 3:45 AM, Pulkit Goyal <7895pulkit@gmail.com> wrote:
> > # HG changeset patch
> > # User Pulkit Goyal <7895pulkit@gmail.com>
> > # Date 1478122977 -19800
> > #      Thu Nov 03 03:12:57 2016 +0530
> > # Node ID 9bdf6ee77b17dc2a1ce29fb347e90bd8dd17eaed
> > # Parent  8cd0e855ebb448a4ec1e835b346a920a1cb835cb
> > py3: use encoding.environ in ui.py
> >
> > Using source transformer we add b'' everywhere. So there are no chances
> > that those bytes string will work with os.environ on Py3 as that returns a
> > dict of unicodes. We are relying on the errors, even though no error is raised
> > even in future, these pieces of codes will tend to do wrong things.
> > if statements can result in wrong boolean and certain errors can be raised
> > while using this piece of code. Let's not wait for them to happen, fix what is
> > wrong. If this patch goes in, I will try to do it for all the cases.
> Yuya any comments on this. I remember asking this from you in the
> sprint and your answer was no. But I think we have to change all
> occurences.

I've queued this as s/os.environ/encoding.environ/ seems the way to go. (We
could use ui.environ consistently, but that's another issue.)

Did I say something different?
Pulkit Goyal - Nov. 30, 2016, 6:16 p.m.
Can you elaborate on how we can use ui.environ because I will like to
change all os.environ things because they are source of errors on
Python 3.

On Sat, Nov 5, 2016 at 7:05 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Sat, 5 Nov 2016 06:02:52 +0530, Pulkit Goyal wrote:
>> On Thu, Nov 3, 2016 at 3:45 AM, Pulkit Goyal <7895pulkit@gmail.com> wrote:
>> > # HG changeset patch
>> > # User Pulkit Goyal <7895pulkit@gmail.com>
>> > # Date 1478122977 -19800
>> > #      Thu Nov 03 03:12:57 2016 +0530
>> > # Node ID 9bdf6ee77b17dc2a1ce29fb347e90bd8dd17eaed
>> > # Parent  8cd0e855ebb448a4ec1e835b346a920a1cb835cb
>> > py3: use encoding.environ in ui.py
>> >
>> > Using source transformer we add b'' everywhere. So there are no chances
>> > that those bytes string will work with os.environ on Py3 as that returns a
>> > dict of unicodes. We are relying on the errors, even though no error is raised
>> > even in future, these pieces of codes will tend to do wrong things.
>> > if statements can result in wrong boolean and certain errors can be raised
>> > while using this piece of code. Let's not wait for them to happen, fix what is
>> > wrong. If this patch goes in, I will try to do it for all the cases.
>> Yuya any comments on this. I remember asking this from you in the
>> sprint and your answer was no. But I think we have to change all
>> occurences.
>
> I've queued this as s/os.environ/encoding.environ/ seems the way to go. (We
> could use ui.environ consistently, but that's another issue.)
>
> Did I say something different?
Yuya Nishihara - Dec. 1, 2016, 2:20 p.m.
On Wed, 30 Nov 2016 23:46:37 +0530, Pulkit Goyal wrote:
> Can you elaborate on how we can use ui.environ because I will like to
> change all os.environ things because they are source of errors on
> Python 3.

I had fuzzy feeling that ui.environ could be a source of environ dict. It was
introduced by 38170eeed18c, but appears not (or barely?) used. OTOH, doing that
might cause httpoxy vulnerability.

So, I have no concrete idea how we should handle ui.environ.

Patch

diff -r 8cd0e855ebb4 -r 9bdf6ee77b17 mercurial/ui.py
--- a/mercurial/ui.py	Thu Nov 03 02:53:45 2016 +0530
+++ b/mercurial/ui.py	Thu Nov 03 03:12:57 2016 +0530
@@ -22,6 +22,7 @@ 
 
 from . import (
     config,
+    encoding,
     error,
     formatter,
     progress,
@@ -574,9 +575,11 @@ 
         - False if HGPLAIN is not set, or feature is in HGPLAINEXCEPT
         - True otherwise
         '''
-        if 'HGPLAIN' not in os.environ and 'HGPLAINEXCEPT' not in os.environ:
+        if ('HGPLAIN' not in encoding.environ and
+                'HGPLAINEXCEPT' not in encoding.environ):
             return False
-        exceptions = os.environ.get('HGPLAINEXCEPT', '').strip().split(',')
+        exceptions = encoding.environ.get('HGPLAINEXCEPT',
+                '').strip().split(',')
         if feature and exceptions:
             return feature not in exceptions
         return True
@@ -589,13 +592,13 @@ 
         If not found and ui.askusername is True, ask the user, else use
         ($LOGNAME or $USER or $LNAME or $USERNAME) + "@full.hostname".
         """
-        user = os.environ.get("HGUSER")
+        user = encoding.environ.get("HGUSER")
         if user is None:
             user = self.config("ui", ["username", "user"])
             if user is not None:
                 user = os.path.expandvars(user)
         if user is None:
-            user = os.environ.get("EMAIL")
+            user = encoding.environ.get("EMAIL")
         if user is None and self.configbool("ui", "askusername"):
             user = self.prompt(_("enter a commit username:"), default=None)
         if user is None and not self.interactive():
@@ -819,9 +822,9 @@ 
     def termwidth(self):
         '''how wide is the terminal in columns?
         '''
-        if 'COLUMNS' in os.environ:
+        if 'COLUMNS' in encoding.environ:
             try:
-                return int(os.environ['COLUMNS'])
+                return int(encoding.environ['COLUMNS'])
             except ValueError:
                 pass
         return util.termwidth()
@@ -1080,10 +1083,10 @@ 
             editor = 'E'
         else:
             editor = 'vi'
-        return (os.environ.get("HGEDITOR") or
+        return (encoding.environ.get("HGEDITOR") or
                 self.config("ui", "editor") or
-                os.environ.get("VISUAL") or
-                os.environ.get("EDITOR", editor))
+                encoding.environ.get("VISUAL") or
+                encoding.environ.get("EDITOR", editor))
 
     @util.propertycache
     def _progbar(self):