Patchwork D6555: vfs: require use of .seek() or .write() before .tell() on append-mode files

login
register
mail settings
Submitter phabricator
Date June 20, 2019, 6:29 p.m.
Message ID <differential-rev-PHID-DREV-vf2asfaz5px6zhssxqhn-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/40624/
State Superseded
Headers show

Comments

phabricator - June 20, 2019, 6:29 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This prevents the bug in the previous change, but avoids an extraneous
  seek() call in the common case when it's not required. My preference
  was to ban .seek() and .tell() entirely on append-mode files since
  they're potentially misleading, but our codebase doesn't make that
  easy. This is better than nothing.
  
  See the previous change for a detailed explanation of the bug we've
  observed in the wild.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/vfs.py

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - June 21, 2019, 1:17 a.m.
mharbison72 added a comment.


  I worked around the same bug in Windows in platform.posixfile [1].  Should this be done in the posix layer (which is currently only an alias to `open()`)?  It looks like there are uses of posixfile outside of vfs.
  
  [1] https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/windows.py#l161

REPOSITORY
  rHG Mercurial

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

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

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

Patch

diff --git a/mercurial/vfs.py b/mercurial/vfs.py
--- a/mercurial/vfs.py
+++ b/mercurial/vfs.py
@@ -303,6 +303,53 @@ 
             finally:
                 vfs._backgroundfilecloser = None
 
+class _appendfileproxy(object):
+    """Proxy type to prevent .tell() before .seek() or .write() append files.
+
+    Files opened for append aren't required to have a meaningful fpos
+    after open, so we require a write or explicit seek before allowing
+    .tell() on those files.
+    """
+    def __init__(self, fp):
+        object.__setattr__(self, r'_fp', fp)
+        object.__setattr__(self, r'_tellok', False)
+
+    def tell(self):
+        if not self._tellok:
+            raise error.ProgrammingError(
+                'tell() on append-mode file requires write() or seek() first')
+        return self._fp.tell()
+
+    def seek(self, *args, **kwargs):
+        object.__setattr__(self, r'_tellok', True)
+        return self._fp.seek(*args, **kwargs)
+
+    def write(self, *args, **kwargs):
+        object.__setattr__(self, r'_tellok', True)
+        return self._fp.write(*args, **kwargs)
+
+    def __repr__(self):
+        return r'<_appendfileproxy of %r>' % self._fp
+
+    # __magic__ methods get looked up on the type, not the instance,
+    # so we have to proxy __enter__ and __exit__ by hand.
+    def __enter__(self):
+        self._fp.__enter__()
+        return self
+
+    def __exit__(self, *args, **kwargs):
+        self._fp.__exit__(*args, **kwargs)
+
+    # proxy all other methods
+    def __getattr__(self, attr):
+        return getattr(self._fp, attr)
+
+    def __setattr__(self, attr, value):
+        return setattr(self._fp, attr, value)
+
+    def __delattr__(self, attr):
+        return delattr(self._fp, attr)
+
 class vfs(abstractvfs):
     '''Operate files relative to a base directory
 
@@ -441,7 +488,8 @@ 
                                   )
 
             fp = delayclosedfile(fp, self._backgroundfilecloser)
-
+        if mode in ('a', 'ab'):
+            return _appendfileproxy(fp)
         return fp
 
     def symlink(self, src, dst):