[PATCH 07 of 11] revlog: fix many file handle opens to explicitly use bytes mode

Gregory Szorc gregory.szorc at gmail.com
Wed Mar 29 22:24:11 EDT 2017


On Mon, Mar 27, 2017 at 6:54 AM, Yuya Nishihara <yuya at tcha.org> wrote:

> On Sun, 26 Mar 2017 18:36:41 -0400, Augie Fackler wrote:
> > # HG changeset patch
> > # User Augie Fackler <raf at durin42.com>
> > # Date 1490567324 14400
> > #      Sun Mar 26 18:28:44 2017 -0400
> > # Node ID e7fe3ab60132647a9c3b86b2960b385e61b9dcf0
> > # Parent  48144fe2d912b7d9fc300955d0c881aceead6930
> > revlog: fix many file handle opens to explicitly use bytes mode
> >
> > This broke on Python 3, but we've probably been getting lucky that
> > this hasn't caused EOL corruption on Windows or something for
> > years. :(
> >
> > diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> > --- a/mercurial/revlog.py
> > +++ b/mercurial/revlog.py
> > @@ -1467,8 +1467,8 @@ class revlog(object):
> >
> >          dfh = None
> >          if not self._inline:
> > -            dfh = self.opener(self.datafile, "a+")
> > -        ifh = self.opener(self.indexfile, "a+",
> checkambig=self._checkambig)
> > +            dfh = self.opener(self.datafile, "a+b")
> > +        ifh = self.opener(self.indexfile, "a+b",
> checkambig=self._checkambig)
>
> 'b' shouldn't be needed since vfs adds 'b' unless text=True is explicitly
> specified.
>

That "text" argument always seemed weird to me because all it does is
reinvent the wheel for the "mode" argument to open(). I'd prefer we
deprecate the argument and use explicit mode values so the I/O code more
closely aligns with what Python does. Principal of least surprise.

That being said, it is a big BC and I'm not sure we can change it easily.

I do like Augie's patch to add "b" because it results in explicit code,
even if it is redundant. Although, I can see how some call sites specifying
"b" and others not with both having the same behavior (without "text=True")
could be confusing. That's a nice corner we painted ourselves into :/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170329/8415d232/attachment.html>


More information about the Mercurial-devel mailing list