Patchwork D7517: filemerge: byteify the open() mode

login
register
mail settings
Submitter phabricator
Date Nov. 24, 2019, 5:45 a.m.
Message ID <differential-rev-PHID-DREV-axqff6jtesjptxgctnem-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43509/
State Superseded
Headers show

Comments

phabricator - Nov. 24, 2019, 5:45 a.m.
mharbison72 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This is actually `pycompat.open()`, so it need bytes.  It regressed recently on
  default.
  
  VSCode flagged some invalid mode to open() the other day, but I don't remember
  where.  That's what got me searching in this area.  I'm almost certain that it
  was the other way (i.e. saying open doesn't take bytes), but I can't find that
  now.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/filemerge.py

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 24, 2019, 11:51 a.m.
dlax added a comment.


  > This is actually pycompat.open(), so it need bytes.
  
  I don't understand why this is needed. The default value for "mode" as bytes comes from a407f9009392 <https://phab.mercurial-scm.org/rHGa407f900939257680a59c5279e481cf92605b98d>, but I don't understand the rationale.
  Shouldn't we instead change all calls to `pycompat.open()` to use a native str for `mode`? (and drop `sysstr(mode)` in `pycompat.open()`).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7517/new/

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

To: mharbison72, #hg-reviewers
Cc: dlax, mercurial-devel
phabricator - Nov. 24, 2019, 2:08 p.m.
mharbison72 added a comment.


  In D7517#110547 <https://phab.mercurial-scm.org/D7517#110547>, @dlax wrote:
  
  >> This is actually pycompat.open(), so it need bytes.
  >
  > I don't understand why this is needed. The default value for "mode" as bytes comes from a407f9009392 <https://phab.mercurial-scm.org/rHGa407f900939257680a59c5279e481cf92605b98d>, but I don't understand the rationale.
  
  The reason for explicitly marking the mode there was that `sysstr(mode)` was already in place.  So that didn't change anything- `mode` was already required to be bytes.
  
  > Shouldn't we instead change all calls to `pycompat.open()` to use a native str for `mode`? (and drop `sysstr(mode)` in `pycompat.open()`).
  
  I suspect the byte `mode` was done to be consistent with the file name being bytes?  I agree with the first point though- I’d much rather this be an explicit call to `pycompat.open()` so that it’s clear that it isn’t the builtin function.  I noticed that *attr() builtins are similarly replaced, but I didn’t look inside to see what they were about. (I’m assuming they’re also about bytes -> str.)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7517/new/

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

To: mharbison72, #hg-reviewers
Cc: dlax, mercurial-devel
phabricator - Dec. 11, 2019, 5:33 p.m.
durin42 added a comment.


  I'm a little fuzzy on this: should I see some test failures? or...?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7517/new/

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

To: mharbison72, #hg-reviewers
Cc: durin42, dlax, mercurial-devel
phabricator - Dec. 11, 2019, 10:58 p.m.
mharbison72 added a comment.


  In D7517#111885 <https://phab.mercurial-scm.org/D7517#111885>, @durin42 wrote:
  
  > I'm a little fuzzy on this: should I see some test failures? or...?
  
  It looks like not.  I glossed over the fact that `pycompat.sysstr()` will simply return `str` if given one, before trying to decode it.  So it will take either `bytes` or `str`.
  
  I looks like the VSCode complaint about passing bytes as the mode is only flagged when used in a context manager (several lines below), even though clicking through to the definition brings it to pycompat.  I don't remember if this particular thing was flagged by static analysis, or it just caught my eye because 99% of the modes in other uses (outside tests/, contrib/ and setup.py) are bytes.  But the latest version of PyCharm isn't complaining, nor is pytype.  So I can abandon it if you'd prefer.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7517/new/

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

To: mharbison72, #hg-reviewers
Cc: durin42, dlax, mercurial-devel
phabricator - Jan. 23, 2020, 4:47 p.m.
marmoute added a comment.


  This have been around for a while. What should we do with it?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7517/new/

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

To: mharbison72, #hg-reviewers
Cc: marmoute, durin42, dlax, mercurial-devel
phabricator - Jan. 23, 2020, 5:30 p.m.
mharbison72 added a comment.


  In D7517#117274 <https://phab.mercurial-scm.org/D7517#117274>, @marmoute wrote:
  
  > This have been around for a while. What should we do with it?
  
  Since it's not a bug fix, I guess I can abandon it.  I left it open as a reminder that there are inconsistencies with with how strings are passed to builtins, and am wondering if there's any interest in using str instead of bytes for things like this since it seems to occasionally confuse static analyzers.  (Yes, I know this change goes the opposite way.)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7517/new/

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

To: mharbison72, #hg-reviewers
Cc: marmoute, durin42, dlax, mercurial-devel
phabricator - Feb. 5, 2020, 11:26 p.m.
marmoute added a comment.
marmoute accepted this revision.


  I am leaning toward taking it for consistency (as @mharbison72 said) however, I also seems fine to simply abandon it.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7517/new/

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

To: mharbison72, #hg-reviewers, marmoute
Cc: marmoute, durin42, dlax, mercurial-devel
phabricator - Feb. 9, 2020, 4:49 a.m.
mharbison72 added a comment.
mharbison72 abandoned this revision.


  The py3 breakage fixed by D8099 <https://phab.mercurial-scm.org/D8099> convinced me that this is going in the wrong direction.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7517/new/

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

To: mharbison72, #hg-reviewers, marmoute
Cc: marmoute, durin42, dlax, mercurial-devel

Patch

diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -934,7 +934,7 @@ 
             name = os.path.join(tmproot, pre)
             if ext:
                 name += ext
-            f = open(name, "wb")
+            f = open(name, b"wb")
         else:
             fd, name = pycompat.mkstemp(prefix=pre + b'.', suffix=ext)
             f = os.fdopen(fd, "wb")