Patchwork [STABLE] posix: insert seek between reads and writes on Solaris (issue4943)

login
register
mail settings
Submitter Adrian Buehlmann
Date Nov. 16, 2015, 8:29 a.m.
Message ID <564993FD.9010107@cadifra.com>
Download mbox | patch
Permalink /patch/11410/
State Not Applicable
Headers show

Comments

Adrian Buehlmann - Nov. 16, 2015, 8:29 a.m.
On 2015-11-16 00:34, Adrian Buehlmann wrote:
> On 2015-11-15 23:21, Gregory Szorc wrote:
>> Yes, we copied a large block of source code. This is not ideal.
>> However, posix.py and windows.py are very low level modules. There
>> is no obvious existing module we could add the function to without
>> introducing an import cycle. A new module will likely need to be
>> invented. I'm not comfortable doing that on the stable branch.
>> Once this patch is merged into @, we should clean up the duplicated
>> code.
> 
> Perhaps you could move mixedfilemodewrapper to util.py and do the
> wrapping there for the Solaris and Windows cases.
> 
> Likely not appropriate for stable indeed...

I was thinking of something like this (completely untested):
Gregory Szorc - Nov. 16, 2015, 7:41 p.m.
> On Nov 16, 2015, at 00:29, Adrian Buehlmann <adrian@cadifra.com> wrote:
> 
>> On 2015-11-16 00:34, Adrian Buehlmann wrote:
>>> On 2015-11-15 23:21, Gregory Szorc wrote:
>>> Yes, we copied a large block of source code. This is not ideal.
>>> However, posix.py and windows.py are very low level modules. There
>>> is no obvious existing module we could add the function to without
>>> introducing an import cycle. A new module will likely need to be
>>> invented. I'm not comfortable doing that on the stable branch.
>>> Once this patch is merged into @, we should clean up the duplicated
>>> code.
>> 
>> Perhaps you could move mixedfilemodewrapper to util.py and do the
>> wrapping there for the Solaris and Windows cases.
>> 
>> Likely not appropriate for stable indeed...
> 
> I was thinking of something like this (completely untested):
> 
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -24,6 +24,7 @@
> import gc
> import bz2
> import zlib
> +import platform as pyplatform
> 
> if os.name == 'nt':
>     import windows as platform
> @@ -91,6 +92,78 @@
> def safehasattr(thing, attr):
>     return getattr(thing, attr, _notset) is not _notset
> 
> +class mixedfilemodewrapper(object):
> +    """Wraps a file handle when it is opened in read/write mode.
> +
> +    This class wraps posixfile instances when the file is opened in read/write
> +    mode and automatically adds checks or inserts appropriate file positioning
> +    calls when necessary.
> +    """
> +    OPNONE = 0
> +    OPREAD = 1
> +    OPWRITE = 2
> +
> +    def __init__(self, fp):
> +        object.__setattr__(self, '_fp', fp)
> +        object.__setattr__(self, '_lastop', 0)
> +
> +    def __getattr__(self, name):
> +        return getattr(self._fp, name)
> +
> +    def __setattr__(self, name, value):
> +        return self._fp.__setattr__(name, value)
> +
> +    def _noopseek(self):
> +        self._fp.seek(0, os.SEEK_CUR)
> +
> +    def seek(self, *args, **kwargs):
> +        object.__setattr__(self, '_lastop', self.OPNONE)
> +        return self._fp.seek(*args, **kwargs)
> +
> +    def write(self, d):
> +        if self._lastop == self.OPREAD:
> +            self._noopseek()
> +
> +        object.__setattr__(self, '_lastop', self.OPWRITE)
> +        return self._fp.write(d)
> +
> +    def writelines(self, *args, **kwargs):
> +        if self._lastop == self.OPREAD:
> +            self._noopeseek()
> +
> +        object.__setattr__(self, '_lastop', self.OPWRITE)
> +        return self._fp.writelines(*args, **kwargs)
> +
> +    def read(self, *args, **kwargs):
> +        if self._lastop == self.OPWRITE:
> +            self._noopseek()
> +
> +        object.__setattr__(self, '_lastop', self.OPREAD)
> +        return self._fp.read(*args, **kwargs)
> +
> +    def readline(self, *args, **kwargs):
> +        if self._lastop == self.OPWRITE:
> +            self._noopseek()
> +
> +        object.__setattr__(self, '_lastop', self.OPREAD)
> +        return self._fp.readline(*args, **kwargs)
> +
> +    def readlines(self, *args, **kwargs):
> +        if self._lastop == self.OPWRITE:
> +            self._noopseek()
> +
> +        object.__setattr__(self, '_lastop', self.OPREAD)
> +        return self._fp.readlines(*args, **kwargs)
> +
> +def _wrappedposixfile(name, mode='r', buffering=-1):
> +    fp = platform.posixfile(name, mode, buffering)
> +    if '+' in mode:
> +        return mixedfilemodewrapper(fp)
> +    return fp
> +
> +if os.name == 'nt' or pyplatform.system() == 'SunOS'
> +    posixfile = _wrappedposixfile
> +
> from hashlib import md5, sha1
> 
> DIGESTS = {
> diff --git a/mercurial/windows.py b/mercurial/windows.py
> --- a/mercurial/windows.py
> +++ b/mercurial/windows.py
> @@ -27,73 +27,6 @@
> 
> umask = 0o022
> 
> -class mixedfilemodewrapper(object):
> -    """Wraps a file handle when it is opened in read/write mode.
> -
> -    fopen() and fdopen() on Windows have a specific-to-Windows requirement
> -    that files opened with mode r+, w+, or a+ make a call to a file positioning
> -    function when switching between reads and writes. Without this extra call,
> -    Python will raise a not very intuitive "IOError: [Errno 0] Error."
> -
> -    This class wraps posixfile instances when the file is opened in read/write
> -    mode and automatically adds checks or inserts appropriate file positioning
> -    calls when necessary.
> -    """
> -    OPNONE = 0
> -    OPREAD = 1
> -    OPWRITE = 2
> -
> -    def __init__(self, fp):
> -        object.__setattr__(self, '_fp', fp)
> -        object.__setattr__(self, '_lastop', 0)
> -
> -    def __getattr__(self, name):
> -        return getattr(self._fp, name)
> -
> -    def __setattr__(self, name, value):
> -        return self._fp.__setattr__(name, value)
> -
> -    def _noopseek(self):
> -        self._fp.seek(0, os.SEEK_CUR)
> -
> -    def seek(self, *args, **kwargs):
> -        object.__setattr__(self, '_lastop', self.OPNONE)
> -        return self._fp.seek(*args, **kwargs)
> -
> -    def write(self, d):
> -        if self._lastop == self.OPREAD:
> -            self._noopseek()
> -
> -        object.__setattr__(self, '_lastop', self.OPWRITE)
> -        return self._fp.write(d)
> -
> -    def writelines(self, *args, **kwargs):
> -        if self._lastop == self.OPREAD:
> -            self._noopeseek()
> -
> -        object.__setattr__(self, '_lastop', self.OPWRITE)
> -        return self._fp.writelines(*args, **kwargs)
> -
> -    def read(self, *args, **kwargs):
> -        if self._lastop == self.OPWRITE:
> -            self._noopseek()
> -
> -        object.__setattr__(self, '_lastop', self.OPREAD)
> -        return self._fp.read(*args, **kwargs)
> -
> -    def readline(self, *args, **kwargs):
> -        if self._lastop == self.OPWRITE:
> -            self._noopseek()
> -
> -        object.__setattr__(self, '_lastop', self.OPREAD)
> -        return self._fp.readline(*args, **kwargs)
> -
> -    def readlines(self, *args, **kwargs):
> -        if self._lastop == self.OPWRITE:
> -            self._noopseek()
> -
> -        object.__setattr__(self, '_lastop', self.OPREAD)
> -        return self._fp.readlines(*args, **kwargs)
> 
> def posixfile(name, mode='r', buffering=-1):
>     '''Open a file with even more POSIX-like semantics'''
> @@ -105,9 +38,6 @@
>         if 'a' in mode:
>             fp.seek(0, os.SEEK_END)
> 
> -        if '+' in mode:
> -            return mixedfilemodewrapper(fp)
> -
>         return fp
>     except WindowsError as err:
>         # convert to a friendlier exception


This should work! I'm battling a cold and completely forgot util is the only module that imports windows and posix.

I'm still not sure what's appropriate for stable.

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -24,6 +24,7 @@ 
 import gc
 import bz2
 import zlib
+import platform as pyplatform
 
 if os.name == 'nt':
     import windows as platform
@@ -91,6 +92,78 @@ 
 def safehasattr(thing, attr):
     return getattr(thing, attr, _notset) is not _notset
 
+class mixedfilemodewrapper(object):
+    """Wraps a file handle when it is opened in read/write mode.
+
+    This class wraps posixfile instances when the file is opened in read/write
+    mode and automatically adds checks or inserts appropriate file positioning
+    calls when necessary.
+    """
+    OPNONE = 0
+    OPREAD = 1
+    OPWRITE = 2
+
+    def __init__(self, fp):
+        object.__setattr__(self, '_fp', fp)
+        object.__setattr__(self, '_lastop', 0)
+
+    def __getattr__(self, name):
+        return getattr(self._fp, name)
+
+    def __setattr__(self, name, value):
+        return self._fp.__setattr__(name, value)
+
+    def _noopseek(self):
+        self._fp.seek(0, os.SEEK_CUR)
+
+    def seek(self, *args, **kwargs):
+        object.__setattr__(self, '_lastop', self.OPNONE)
+        return self._fp.seek(*args, **kwargs)
+
+    def write(self, d):
+        if self._lastop == self.OPREAD:
+            self._noopseek()
+
+        object.__setattr__(self, '_lastop', self.OPWRITE)
+        return self._fp.write(d)
+
+    def writelines(self, *args, **kwargs):
+        if self._lastop == self.OPREAD:
+            self._noopeseek()
+
+        object.__setattr__(self, '_lastop', self.OPWRITE)
+        return self._fp.writelines(*args, **kwargs)
+
+    def read(self, *args, **kwargs):
+        if self._lastop == self.OPWRITE:
+            self._noopseek()
+
+        object.__setattr__(self, '_lastop', self.OPREAD)
+        return self._fp.read(*args, **kwargs)
+
+    def readline(self, *args, **kwargs):
+        if self._lastop == self.OPWRITE:
+            self._noopseek()
+
+        object.__setattr__(self, '_lastop', self.OPREAD)
+        return self._fp.readline(*args, **kwargs)
+
+    def readlines(self, *args, **kwargs):
+        if self._lastop == self.OPWRITE:
+            self._noopseek()
+
+        object.__setattr__(self, '_lastop', self.OPREAD)
+        return self._fp.readlines(*args, **kwargs)
+
+def _wrappedposixfile(name, mode='r', buffering=-1):
+    fp = platform.posixfile(name, mode, buffering)
+    if '+' in mode:
+        return mixedfilemodewrapper(fp)
+    return fp
+
+if os.name == 'nt' or pyplatform.system() == 'SunOS'
+    posixfile = _wrappedposixfile
+
 from hashlib import md5, sha1
 
 DIGESTS = {
diff --git a/mercurial/windows.py b/mercurial/windows.py
--- a/mercurial/windows.py
+++ b/mercurial/windows.py
@@ -27,73 +27,6 @@ 
 
 umask = 0o022
 
-class mixedfilemodewrapper(object):
-    """Wraps a file handle when it is opened in read/write mode.
-
-    fopen() and fdopen() on Windows have a specific-to-Windows requirement
-    that files opened with mode r+, w+, or a+ make a call to a file positioning
-    function when switching between reads and writes. Without this extra call,
-    Python will raise a not very intuitive "IOError: [Errno 0] Error."
-
-    This class wraps posixfile instances when the file is opened in read/write
-    mode and automatically adds checks or inserts appropriate file positioning
-    calls when necessary.
-    """
-    OPNONE = 0
-    OPREAD = 1
-    OPWRITE = 2
-
-    def __init__(self, fp):
-        object.__setattr__(self, '_fp', fp)
-        object.__setattr__(self, '_lastop', 0)
-
-    def __getattr__(self, name):
-        return getattr(self._fp, name)
-
-    def __setattr__(self, name, value):
-        return self._fp.__setattr__(name, value)
-
-    def _noopseek(self):
-        self._fp.seek(0, os.SEEK_CUR)
-
-    def seek(self, *args, **kwargs):
-        object.__setattr__(self, '_lastop', self.OPNONE)
-        return self._fp.seek(*args, **kwargs)
-
-    def write(self, d):
-        if self._lastop == self.OPREAD:
-            self._noopseek()
-
-        object.__setattr__(self, '_lastop', self.OPWRITE)
-        return self._fp.write(d)
-
-    def writelines(self, *args, **kwargs):
-        if self._lastop == self.OPREAD:
-            self._noopeseek()
-
-        object.__setattr__(self, '_lastop', self.OPWRITE)
-        return self._fp.writelines(*args, **kwargs)
-
-    def read(self, *args, **kwargs):
-        if self._lastop == self.OPWRITE:
-            self._noopseek()
-
-        object.__setattr__(self, '_lastop', self.OPREAD)
-        return self._fp.read(*args, **kwargs)
-
-    def readline(self, *args, **kwargs):
-        if self._lastop == self.OPWRITE:
-            self._noopseek()
-
-        object.__setattr__(self, '_lastop', self.OPREAD)
-        return self._fp.readline(*args, **kwargs)
-
-    def readlines(self, *args, **kwargs):
-        if self._lastop == self.OPWRITE:
-            self._noopseek()
-
-        object.__setattr__(self, '_lastop', self.OPREAD)
-        return self._fp.readlines(*args, **kwargs)
 
 def posixfile(name, mode='r', buffering=-1):
     '''Open a file with even more POSIX-like semantics'''
@@ -105,9 +38,6 @@ 
         if 'a' in mode:
             fp.seek(0, os.SEEK_END)
 
-        if '+' in mode:
-            return mixedfilemodewrapper(fp)
-
         return fp
     except WindowsError as err:
         # convert to a friendlier exception