[PATCH] atomictempfile: make close() consistent with other file-like objects
Greg Ward
greg-hg at gerg.ca
Wed Aug 24 19:21:33 CDT 2011
# HG changeset patch
# User Greg Ward <greg at gerg.ca>
# Date 1314230781 14400
# Node ID 21ed0d84367a43dde677cff6f2391e7ea2e9bb48
# Parent ef43610a4cce7b8810af95fab1591adf821c3669
atomictempfile: make close() consistent with other file-like objects.
The usual contract is that close() makes your writes permanent, so
atomictempfile's use of close() to *discard* writes (and rename() to
keep them) is rather unexpected. Thus, change it so close() makes
things permanent and add a new discard() method to throw them away.
discard() is only used internally, in __del__(), to ensure that writes
are discarded when an atomictempfile object goes out of scope.
I audited mercurial.*, hgext.*, and ~80 third-party extensions, and
found no one using the existing semantics of close() to discard
writes, so this should be safe.
Up for discussion: I also left in a rename() method that issues a
deprecation warning and calls close(). This makes the transition a
little easier at the cost of crufting up the code. Opinions?
diff --git a/hgext/mq.py b/hgext/mq.py
--- a/hgext/mq.py
+++ b/hgext/mq.py
@@ -1492,7 +1492,7 @@
n = repo.commit(message, user, ph.date, match=match,
force=True)
# only write patch after a successful commit
- patchf.rename()
+ patchf.close()
self.applied.append(statusentry(n, patchfn))
except:
ctx = repo[cparents[0]]
diff --git a/mercurial/archival.py b/mercurial/archival.py
--- a/mercurial/archival.py
+++ b/mercurial/archival.py
@@ -195,7 +195,7 @@
return
f = self.opener(name, "w", atomictemp=True)
f.write(data)
- f.rename()
+ f.close()
destfile = os.path.join(self.basedir, name)
os.chmod(destfile, mode)
diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -90,7 +90,7 @@
file = repo.opener('bookmarks', 'w', atomictemp=True)
for refspec, node in refs.iteritems():
file.write("%s %s\n" % (hex(node), encoding.fromlocal(refspec)))
- file.rename()
+ file.close()
# touch 00changelog.i so hgweb reloads bookmarks (no lock needed)
try:
@@ -121,7 +121,7 @@
try:
file = repo.opener('bookmarks.current', 'w', atomictemp=True)
file.write(encoding.fromlocal(mark))
- file.rename()
+ file.close()
finally:
wlock.release()
repo._bookmarkcurrent = mark
diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -453,7 +453,7 @@
write(e)
write(f)
st.write(cs.getvalue())
- st.rename()
+ st.close()
self._lastnormaltime = None
self._dirty = self._dirtypl = False
diff --git a/mercurial/hbisect.py b/mercurial/hbisect.py
--- a/mercurial/hbisect.py
+++ b/mercurial/hbisect.py
@@ -150,7 +150,7 @@
for kind in state:
for node in state[kind]:
f.write("%s %s\n" % (kind, hex(node)))
- f.rename()
+ f.close()
finally:
wlock.release()
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -521,7 +521,7 @@
for label, nodes in branches.iteritems():
for node in nodes:
f.write("%s %s\n" % (hex(node), encoding.fromlocal(label)))
- f.rename()
+ f.close()
except (IOError, OSError):
pass
diff --git a/mercurial/revlog.py b/mercurial/revlog.py
--- a/mercurial/revlog.py
+++ b/mercurial/revlog.py
@@ -946,9 +946,9 @@
e = self._io.packentry(self.index[i], self.node, self.version, i)
fp.write(e)
- # if we don't call rename, the temp file will never replace the
+ # if we don't call close, the temp file will never replace the
# real index
- fp.rename()
+ fp.close()
tr.replace(self.indexfile, trindex * self._io.size)
self._chunkclear()
diff --git a/mercurial/simplemerge.py b/mercurial/simplemerge.py
--- a/mercurial/simplemerge.py
+++ b/mercurial/simplemerge.py
@@ -445,7 +445,7 @@
out.write(line)
if not opts.get('print'):
- out.rename()
+ out.close()
if m3.conflicts:
if not opts.get('quiet'):
diff --git a/mercurial/store.py b/mercurial/store.py
--- a/mercurial/store.py
+++ b/mercurial/store.py
@@ -345,7 +345,7 @@
fp = self.opener('fncache', mode='wb', atomictemp=True)
for p in self.entries:
fp.write(encodedir(p) + '\n')
- fp.rename()
+ fp.close()
self._dirty = False
def add(self, fn):
diff --git a/mercurial/tags.py b/mercurial/tags.py
--- a/mercurial/tags.py
+++ b/mercurial/tags.py
@@ -287,6 +287,6 @@
cachefile.write("%s %s\n" % (hex(node), name))
try:
- cachefile.rename()
+ cachefile.close()
except (OSError, IOError):
pass
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -71,6 +71,8 @@
unlinkpath = platform.unlinkpath
username = platform.username
+import warnings
+
# Python compatibility
def sha1(s):
@@ -745,11 +747,10 @@
'''writeable file object that atomically updates a file
All writes will go to a temporary copy of the original file. Call
- rename() when you are done writing, and atomictempfile will rename
- the temporary copy to the original name, making the changes visible.
-
- Unlike other file-like objects, close() discards your writes by
- simply deleting the temporary file.
+ close() when you are done writing, and atomictempfile will rename
+ the temporary copy to the original name, making the changes
+ visible. If the object is destroyed without being closed, all your
+ writes are discarded.
'''
def __init__(self, name, mode='w+b', createmode=None):
self.__name = name # permanent name
@@ -761,12 +762,17 @@
self.write = self._fp.write
self.fileno = self._fp.fileno
- def rename(self):
+ def close(self):
if not self._fp.closed:
self._fp.close()
rename(self._tempname, localpath(self.__name))
- def close(self):
+ def rename(self):
+ warnings.warn('atomictempfile.rename() replaced by close()',
+ DeprecationWarning, 2)
+ self.close()
+
+ def discard(self):
if not self._fp.closed:
try:
os.unlink(self._tempname)
@@ -776,7 +782,7 @@
def __del__(self):
if safehasattr(self, '_fp'): # constructor actually did something
- self.close()
+ self.discard()
def makedirs(name, mode=None):
"""recursive directory creation with parent mode inheritance"""
diff --git a/tests/test-atomictempfile.py b/tests/test-atomictempfile.py
--- a/tests/test-atomictempfile.py
+++ b/tests/test-atomictempfile.py
@@ -12,22 +12,21 @@
assert basename in glob.glob('.foo-*')
file.write('argh\n')
- file.rename()
+ file.close()
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():
+# discard() removes the temp file without making the write permanent
+def test2_discard():
if os.path.exists('foo'):
os.remove('foo')
file = atomictempfile('foo')
(dir, basename) = os.path.split(file._tempname)
file.write('yo\n')
- file.close()
+ file.discard()
assert not os.path.isfile('foo')
assert basename not in os.listdir('.')
@@ -45,5 +44,5 @@
if __name__ == '__main__':
test1_simple()
- test2_close()
+ test2_discard()
test3_oops()
More information about the Mercurial-devel
mailing list