mirror of
https://github.com/facebook/sapling.git
synced 2024-10-07 15:27:13 +03:00
d95f0484f4
The problem is that a programmer using atomictempfile directly can make an innocent everyday mistake -- not enough args to the constructor -- which escalates badly. You would expect a simple TypeError crash in that case, but you actually get an infinite recursion that is surprisingly difficult to kill: it happens between __del__() and __getattr__(), and Python does not handle infinite recursion from __del__() well. The fix is to not implement __getattr__(), but instead assign instance attributes for the methods we wish to delegate to the builtin file type: write() and fileno(). I've audited mercurial.* and hgext.* and found no users of atomictempfile using methods other than write() and rename(). I audited third-party extensions and found one (snap) passing an atomictempfile to util.fstat(), so I also threw in fileno(). The last time I submitted a similar patch, Matt proposed that we make atomictempfile a subclass of file instead of wrapping it. Rejected on grounds of unnecessary complexity: for one thing, it would make the Windows implementation of posixfile quite a bit more complex. It would have to become a subclass of file rather than a simple function -- but since it's written in C, this is non-obvious and non-trivial. Furthermore, there's nothing wrong with wrapping objects and delegating methods: it's a well-established pattern that works just fine in many cases. Subclassing is not the answer to all of life's problems.
50 lines
1.2 KiB
Python
50 lines
1.2 KiB
Python
import os
|
|
import glob
|
|
from mercurial.util import atomictempfile
|
|
|
|
# basic usage
|
|
def test1_simple():
|
|
if os.path.exists('foo'):
|
|
os.remove('foo')
|
|
file = atomictempfile('foo')
|
|
(dir, basename) = os.path.split(file._tempname)
|
|
assert not os.path.isfile('foo')
|
|
assert basename in glob.glob('.foo-*')
|
|
|
|
file.write('argh\n')
|
|
file.rename()
|
|
|
|
assert os.path.isfile('foo')
|
|
assert basename not in glob.glob('.foo-*')
|
|
print 'OK'
|
|
|
|
# close() removes the temp file but does not make the write
|
|
# permanent -- essentially discards your work (WTF?!)
|
|
def test2_close():
|
|
if os.path.exists('foo'):
|
|
os.remove('foo')
|
|
file = atomictempfile('foo')
|
|
(dir, basename) = os.path.split(file._tempname)
|
|
|
|
file.write('yo\n')
|
|
file.close()
|
|
|
|
assert not os.path.isfile('foo')
|
|
assert basename not in os.listdir('.')
|
|
print 'OK'
|
|
|
|
# if a programmer screws up and passes bad args to atomictempfile, they
|
|
# get a plain ordinary TypeError, not infinite recursion
|
|
def test3_oops():
|
|
try:
|
|
file = atomictempfile()
|
|
except TypeError:
|
|
print "OK"
|
|
else:
|
|
print "expected TypeError"
|
|
|
|
if __name__ == '__main__':
|
|
test1_simple()
|
|
test2_close()
|
|
test3_oops()
|