Patchwork [3,of,4,py3] config: open config files as binary explicitly all the time

login
register
mail settings
Submitter Augie Fackler
Date Nov. 9, 2016, 4:23 p.m.
Message ID <5f29fffcc722d8993fda.1478708620@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/17416/
State Accepted
Headers show

Comments

Augie Fackler - Nov. 9, 2016, 4:23 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1476019794 14400
#      Sun Oct 09 09:29:54 2016 -0400
# Node ID 5f29fffcc722d8993fda4155884c1aae4469d2f3
# Parent  576e4c82886c7e5cf59f7b95b38e0f3c29932581
config: open config files as binary explicitly all the time

We had been getting lucky that on posix-like systems the default for
files in Python 2 is binary IO, but now we're explicitly using binary
IO all the time.
Augie Fackler - Nov. 9, 2016, 4:29 p.m.
> On Nov 9, 2016, at 11:23, Augie Fackler <raf@durin42.com> wrote:
> 
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1476019794 14400
> #      Sun Oct 09 09:29:54 2016 -0400
> # Node ID 5f29fffcc722d8993fda4155884c1aae4469d2f3
> # Parent  576e4c82886c7e5cf59f7b95b38e0f3c29932581
> config: open config files as binary explicitly all the time
> 
> We had been getting lucky that on posix-like systems the default for
> files in Python 2 is binary IO, but now we're explicitly using binary
> IO all the time.
> 
> diff --git a/mercurial/config.py b/mercurial/config.py
> --- a/mercurial/config.py
> +++ b/mercurial/config.py
> @@ -169,5 +169,5 @@ class config(object):
> 
>     def read(self, path, fp=None, sections=None, remap=None):
>         if not fp:
> -            fp = util.posixfile(path)
> +            fp = util.posixfile(path, r'rb')

I just realized I missed some feedback on this: this mode should be b'' rather than r'' since this is a util.posixfile, right?

>         self.parse(path, fp.read(), sections, remap, self.read)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Nov. 10, 2016, 1:25 p.m.
On Wed, 9 Nov 2016 11:29:26 -0500, Augie Fackler wrote:
> > On Nov 9, 2016, at 11:23, Augie Fackler <raf@durin42.com> wrote:
> > # HG changeset patch
> > # User Augie Fackler <augie@google.com>
> > # Date 1476019794 14400
> > #      Sun Oct 09 09:29:54 2016 -0400
> > # Node ID 5f29fffcc722d8993fda4155884c1aae4469d2f3
> > # Parent  576e4c82886c7e5cf59f7b95b38e0f3c29932581
> > config: open config files as binary explicitly all the time
> > 
> > We had been getting lucky that on posix-like systems the default for
> > files in Python 2 is binary IO, but now we're explicitly using binary
> > IO all the time.
> > 
> > diff --git a/mercurial/config.py b/mercurial/config.py
> > --- a/mercurial/config.py
> > +++ b/mercurial/config.py
> > @@ -169,5 +169,5 @@ class config(object):
> > 
> >     def read(self, path, fp=None, sections=None, remap=None):
> >         if not fp:
> > -            fp = util.posixfile(path)
> > +            fp = util.posixfile(path, r'rb')
> 
> I just realized I missed some feedback on this: this mode should be b'' rather than r'' since this is a util.posixfile, right?

Yes. And we can wrap posix.posixfile() to add 'b' automatically. Windows'
posixfile() is our code, which won't become a text IO on Python 3.

Furthermore, we could force pycompat.open() to open files in binary mode,
but I'm not sure if we have no open() calls which expect CR+LF translation.
Augie Fackler - Nov. 10, 2016, 10:05 p.m.
On Thu, Nov 10, 2016 at 10:25:16PM +0900, Yuya Nishihara wrote:
> On Wed, 9 Nov 2016 11:29:26 -0500, Augie Fackler wrote:
> > > On Nov 9, 2016, at 11:23, Augie Fackler <raf@durin42.com> wrote:
> > > # HG changeset patch
> > > # User Augie Fackler <augie@google.com>
> > > # Date 1476019794 14400
> > > #      Sun Oct 09 09:29:54 2016 -0400
> > > # Node ID 5f29fffcc722d8993fda4155884c1aae4469d2f3
> > > # Parent  576e4c82886c7e5cf59f7b95b38e0f3c29932581
> > > config: open config files as binary explicitly all the time
> > >
> > > We had been getting lucky that on posix-like systems the default for
> > > files in Python 2 is binary IO, but now we're explicitly using binary
> > > IO all the time.
> > >
> > > diff --git a/mercurial/config.py b/mercurial/config.py
> > > --- a/mercurial/config.py
> > > +++ b/mercurial/config.py
> > > @@ -169,5 +169,5 @@ class config(object):
> > >
> > >     def read(self, path, fp=None, sections=None, remap=None):
> > >         if not fp:
> > > -            fp = util.posixfile(path)
> > > +            fp = util.posixfile(path, r'rb')
> >
> > I just realized I missed some feedback on this: this mode should be b'' rather than r'' since this is a util.posixfile, right?
>
> Yes. And we can wrap posix.posixfile() to add 'b' automatically. Windows'
> posixfile() is our code, which won't become a text IO on Python 3.

Hm. It's actually going to make this layer of the code easier if I
allow unicode mode arguments into util.posixfile and then make
windows.posixfile turn it back into bytes if needed. I'll roll that variant.

>
> Furthermore, we could force pycompat.open() to open files in binary mode,
> but I'm not sure if we have no open() calls which expect CR+LF translation.

Patch

diff --git a/mercurial/config.py b/mercurial/config.py
--- a/mercurial/config.py
+++ b/mercurial/config.py
@@ -169,5 +169,5 @@  class config(object):
 
     def read(self, path, fp=None, sections=None, remap=None):
         if not fp:
-            fp = util.posixfile(path)
+            fp = util.posixfile(path, r'rb')
         self.parse(path, fp.read(), sections, remap, self.read)