| 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
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):
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?
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?
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):