D7517: filemerge: byteify the open() mode

[mharbison72 (Matt Harbison)]

mharbison72 added a comment.
mharbison72 abandoned this revision.


  The py3 breakage fixed by 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7517: filemerge: byteify the open() mode

[marmoute (Pierre-Yves David)]

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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7517: filemerge: byteify the open() mode

[mharbison72 (Matt Harbison)]

mharbison72 added a comment.


  In 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7517: filemerge: byteify the open() mode

[marmoute (Pierre-Yves David)]

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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7517: filemerge: byteify the open() mode

[mharbison72 (Matt Harbison)]

mharbison72 added a comment.


  In 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7517: filemerge: byteify the open() mode

[durin42 (Augie Fackler)]

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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7517: filemerge: byteify the open() mode

[mharbison72 (Matt Harbison)]

mharbison72 added a comment.


  In 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 
, 
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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7517: filemerge: byteify the open() mode

[dlax (Denis Laxalde)]

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 
, 
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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7517: filemerge: byteify the open() mode

[mharbison72 (Matt Harbison)]

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

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")



To: mharbison72, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel