Patchwork D2297: py3: backout 7c54917b31f6 to make sure second argument of open() is str

login
register
mail settings
Submitter phabricator
Date Feb. 17, 2018, 10:03 a.m.
Message ID <differential-rev-PHID-DREV-cjqqjv4mwz2hlclf5lqk-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/28020/
State Superseded
Headers show

Comments

phabricator - Feb. 17, 2018, 10:03 a.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This backouts changeset https://phab.mercurial-scm.org/rHG7c54917b31f6200449bb7afd253ca0561288456a in which code to prevent adding b'' in
  front of second argument of open() was deleted in favour of pycompat.open().
  Looking back, that's looks like a wrong decision as replacing open() with
  pycompat.open() is not a good idea.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2297

AFFECTED FILES
  mercurial/__init__.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - Feb. 17, 2018, 10:16 a.m.
Can you elaborate why you think it wasn't a good idea to replace open()?

IMHO, it's as hackish as replacing '' with b''.
phabricator - Feb. 17, 2018, 10:16 a.m.
yuja added a comment.


  Can you elaborate why you think it wasn't a good idea to replace open()?
  
  IMHO, it's as hackish as replacing '' with b''.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2297

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - Feb. 22, 2018, 3:06 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D2297#37960, @yuja wrote:
  
  > Can you elaborate why you think it wasn't a good idea to replace open()?
  >
  > IMHO, it's as hackish as replacing '' with b''.
  
  
  Added couple of points.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2297

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel
Yuya Nishihara - Feb. 22, 2018, 3:49 p.m.
"There are around 100 occurences of open() call, it's better if we can
prevent replacing all these calls
"In future, when we will dropping all the compatibility code, we don't have
to take care of changing pycompat.open back to open

Yeah, that's the point why we have `from pycompat import open` to compensate
the mess introduced by our code transformer. We want to keep 'rb' without b''.
We could do that by the transformer, but I think it's better to keep the
transformer less complicated.

FWIW, we'll need to port many of these 100 open() calls to vfs. I have vague
memory that Python 3.5 or 3.6 introduced another mess on Windows I/O around
ANSI vs Unicode.
phabricator - Feb. 22, 2018, 3:50 p.m.
yuja added a comment.


  "There are around 100 occurences of open() call, it's better if we can
  prevent replacing all these calls
  "In future, when we will dropping all the compatibility code, we don't have
  to take care of changing pycompat.open back to open
  
  Yeah, that's the point why we have `from pycompat import open` to compensate
  the mess introduced by our code transformer. We want to keep 'rb' without b''.
  We could do that by the transformer, but I think it's better to keep the
  transformer less complicated.
  
  FWIW, we'll need to port many of these 100 open() calls to vfs. I have vague
  memory that Python 3.5 or 3.6 introduced another mess on Windows I/O around
  ANSI vs Unicode.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2297

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - June 14, 2018, 8:03 p.m.
pulkit abandoned this revision.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D2297#39249, @yuja wrote:
  
  > "There are around 100 occurences of open() call, it's better if we can
  >  prevent replacing all these calls
  >  "In future, when we will dropping all the compatibility code, we don't have
  >  to take care of changing pycompat.open back to open
  >
  > Yeah, that's the point why we have `from pycompat import open` to compensate
  >  the mess introduced by our code transformer. We want to keep 'rb' without b''.
  >  We could do that by the transformer, but I think it's better to keep the
  >  transformer less complicated.
  
  
  I agree with you that we should keep transformer less complicated as much possible. Abandoning this revision.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2297

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/mercurial/__init__.py b/mercurial/__init__.py
--- a/mercurial/__init__.py
+++ b/mercurial/__init__.py
@@ -207,6 +207,13 @@ 
                         if argidx is not None:
                             _ensureunicode(argidx)
 
+                # Bare open call (not an attribute on something else), the
+                # second argument (mode) must be a string, not bytes
+                elif fn == 'open' and not _isop(i - 1, '.'):
+                    arg1idx = _findargnofcall(1)
+                    if arg1idx is not None:
+                        _ensureunicode(arg1idx)
+
                 # It changes iteritems/values to items/values as they are not
                 # present in Python 3 world.
                 elif fn in ('iteritems', 'itervalues'):
@@ -220,7 +227,7 @@ 
     # ``replacetoken`` or any mechanism that changes semantics of module
     # loading is changed. Otherwise cached bytecode may get loaded without
     # the new transformation mechanisms applied.
-    BYTECODEHEADER = b'HG\x00\x0a'
+    BYTECODEHEADER = b'HG\x00\x0b'
 
     class hgloader(importlib.machinery.SourceFileLoader):
         """Custom module loader that transforms source code.