This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6555

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

Authored by durin42 on Jun 20 2019, 2:29 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Jun 20 2019, 2:29 PM
mharbison72 added a subscriber: mharbison72.Jun 20 2019, 9:17 PM

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

durin42 abandoned this revision.Jul 8 2019, 1:15 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/vfs.py (50 lines)

Diff 15618

mercurial/vfs.py

with backgroundfilecloser(ui, expectedcount=expectedcount) as bfc: with backgroundfilecloser(ui, expectedcount=expectedcount) as bfc:
try: try:
vfs._backgroundfilecloser = bfc vfs._backgroundfilecloser = bfc
yield bfc yield bfc
finally: finally:
vfs._backgroundfilecloser = None 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): class vfs(abstractvfs):
'''Operate files relative to a base directory '''Operate files relative to a base directory
This class is used to hide the details of COW semantics and This class is used to hide the details of COW semantics and
remote file access from higher level code. remote file access from higher level code.
'cacheaudited' should be enabled only if (a) vfs object is short-lived, or 'cacheaudited' should be enabled only if (a) vfs object is short-lived, or
(b) the base directory is managed by hg and considered sort-of append-only. (b) the base directory is managed by hg and considered sort-of append-only.
if (backgroundclose and if (backgroundclose and
isinstance(threading.currentThread(), threading._MainThread)): isinstance(threading.currentThread(), threading._MainThread)):
if not self._backgroundfilecloser: if not self._backgroundfilecloser:
raise error.Abort(_('backgroundclose can only be used when a ' raise error.Abort(_('backgroundclose can only be used when a '
'backgroundclosing context manager is active') 'backgroundclosing context manager is active')
) )
fp = delayclosedfile(fp, self._backgroundfilecloser) fp = delayclosedfile(fp, self._backgroundfilecloser)
if mode in ('a', 'ab'):
return _appendfileproxy(fp)
return fp return fp
def symlink(self, src, dst): def symlink(self, src, dst):
self.audit(dst) self.audit(dst)
linkname = self.join(dst) linkname = self.join(dst)
util.tryunlink(linkname) util.tryunlink(linkname)
util.makedirs(os.path.dirname(linkname), self.createmode) util.makedirs(os.path.dirname(linkname), self.createmode)