changeset 15057:774da7121fc9

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.
author Greg Ward <greg@gerg.ca>
date Thu, 25 Aug 2011 20:21:04 -0400
parents 8413916df816
children 81f33be0ea79
files hgext/mq.py mercurial/archival.py mercurial/bookmarks.py mercurial/dirstate.py mercurial/hbisect.py mercurial/localrepo.py mercurial/revlog.py mercurial/simplemerge.py mercurial/store.py mercurial/tags.py mercurial/util.py tests/test-atomictempfile.py tests/test-filecache.py
diffstat 13 files changed, 25 insertions(+), 27 deletions(-) [+]
line wrap: on
line diff
--- a/hgext/mq.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/hgext/mq.py	Thu Aug 25 20:21:04 2011 -0400
@@ -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]]
--- a/mercurial/archival.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/mercurial/archival.py	Thu Aug 25 20:21:04 2011 -0400
@@ -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)
 
--- a/mercurial/bookmarks.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/mercurial/bookmarks.py	Thu Aug 25 20:21:04 2011 -0400
@@ -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
--- a/mercurial/dirstate.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/mercurial/dirstate.py	Thu Aug 25 20:21:04 2011 -0400
@@ -453,7 +453,7 @@
             write(e)
             write(f)
         st.write(cs.getvalue())
-        st.rename()
+        st.close()
         self._lastnormaltime = None
         self._dirty = self._dirtypl = False
 
--- a/mercurial/hbisect.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/mercurial/hbisect.py	Thu Aug 25 20:21:04 2011 -0400
@@ -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()
 
--- a/mercurial/localrepo.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/mercurial/localrepo.py	Thu Aug 25 20:21:04 2011 -0400
@@ -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
 
--- a/mercurial/revlog.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/mercurial/revlog.py	Thu Aug 25 20:21:04 2011 -0400
@@ -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()
--- a/mercurial/simplemerge.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/mercurial/simplemerge.py	Thu Aug 25 20:21:04 2011 -0400
@@ -445,7 +445,7 @@
         out.write(line)
 
     if not opts.get('print'):
-        out.rename()
+        out.close()
 
     if m3.conflicts:
         if not opts.get('quiet'):
--- a/mercurial/store.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/mercurial/store.py	Thu Aug 25 20:21:04 2011 -0400
@@ -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):
--- a/mercurial/tags.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/mercurial/tags.py	Thu Aug 25 20:21:04 2011 -0400
@@ -287,6 +287,6 @@
         cachefile.write("%s %s\n" % (hex(node), name))
 
     try:
-        cachefile.rename()
+        cachefile.close()
     except (OSError, IOError):
         pass
--- a/mercurial/util.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/mercurial/util.py	Thu Aug 25 20:21:04 2011 -0400
@@ -745,11 +745,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 +760,12 @@
         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 discard(self):
         if not self._fp.closed:
             try:
                 os.unlink(self._tempname)
@@ -776,7 +775,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"""
--- a/tests/test-atomictempfile.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/tests/test-atomictempfile.py	Thu Aug 25 20:21:04 2011 -0400
@@ -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()
--- a/tests/test-filecache.py	Wed Aug 24 05:42:41 2011 -0400
+++ b/tests/test-filecache.py	Thu Aug 25 20:21:04 2011 -0400
@@ -59,7 +59,7 @@
     # because of inode change
     f = scmutil.opener('.')('x', 'w', atomictemp=True)
     f.write('b')
-    f.rename()
+    f.close()
 
     repo.invalidate()
     repo.cached