Patchwork [3,of,4] scmutil: use context managers for file handles

login
register
mail settings
Submitter Gregory Szorc
Date Jan. 2, 2016, 11:38 p.m.
Message ID <7626e49bc8d37409c6ef.1451777927@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/12493/
State Superseded
Headers show

Comments

Gregory Szorc - Jan. 2, 2016, 11:38 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1451776787 28800
#      Sat Jan 02 15:19:47 2016 -0800
# Node ID 7626e49bc8d37409c6ef173eff3ddc1dc925413d
# Parent  55459e98812bcec222fabe63bb60b5e5c922d3c1
scmutil: use context managers for file handles

Now that we dropped support for Python 2.4, we are able to use context
managers. Let's replace the try..finally pattern in scmutil.py with
context managers, which close files automatically when the context
manager is exited.

There should be no change in behavior with this patch.

Why convert to context managers if nothing is broken? I'm working on
closing file handles in background threads to improve performance on
Windows. As part of this, I realized there could be some future issues
if the background file closing code isn't designed with context
managers in mind. So, I'd like to switch some code to context managers
so I can design an API that works with context managers.
Adrian Buehlmann - Jan. 3, 2016, 12:27 a.m.
On 2016-01-03 00:38, Gregory Szorc wrote:
> scmutil: use context managers for file handles

Does this work on Windows for the pure case (that is, using posixfile
from line 189 in pure/osutil.py)?
Gregory Szorc - Jan. 3, 2016, 12:38 a.m.
On Sat, Jan 2, 2016 at 4:27 PM, Adrian Buehlmann <adrian@cadifra.com> wrote:

> On 2016-01-03 00:38, Gregory Szorc wrote:
> > scmutil: use context managers for file handles
>
> Does this work on Windows for the pure case (that is, using posixfile
> from line 189 in pure/osutil.py)?
>

Nope :(

I should have tested this on Windows before I patchbombed...

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -260,49 +260,34 @@  class abstractvfs(object):
         Newly created directories are marked as "not to be indexed by
         the content indexing service", if ``notindexed`` is specified
         for "write" mode access.
         '''
         self.open = self.__call__
         return self.__call__(path, mode, text, atomictemp, notindexed)
 
     def read(self, path):
-        fp = self(path, 'rb')
-        try:
+        with self(path, 'rb') as fp:
             return fp.read()
-        finally:
-            fp.close()
 
     def readlines(self, path, mode='rb'):
-        fp = self(path, mode=mode)
-        try:
+        with self(path, mode=mode) as fp:
             return fp.readlines()
-        finally:
-            fp.close()
 
     def write(self, path, data):
-        fp = self(path, 'wb')
-        try:
+        with self(path, 'wb') as fp:
             return fp.write(data)
-        finally:
-            fp.close()
 
     def writelines(self, path, data, mode='wb', notindexed=False):
-        fp = self(path, mode=mode, notindexed=notindexed)
-        try:
+        with self(path, mode=mode, notindexed=notindexed) as fp:
             return fp.writelines(data)
-        finally:
-            fp.close()
 
     def append(self, path, data):
-        fp = self(path, 'ab')
-        try:
+        with self(path, 'ab') as fp:
             return fp.write(data)
-        finally:
-            fp.close()
 
     def basename(self, path):
         """return base element of a path (as os.path.basename would do)
 
         This exists to allow handling of strange encoding if needed."""
         return os.path.basename(path)
 
     def chmod(self, path, mode):
@@ -521,21 +506,20 @@  class vfs(abstractvfs):
                     return util.atomictempfile(f, mode, self.createmode)
                 try:
                     if 'w' in mode:
                         util.unlink(f)
                         nlink = 0
                     else:
                         # nlinks() may behave differently for files on Windows
                         # shares if the file is open.
-                        fd = util.posixfile(f)
-                        nlink = util.nlinks(f)
-                        if nlink < 1:
-                            nlink = 2 # force mktempcopy (issue1922)
-                        fd.close()
+                        with util.posixfile(f):
+                            nlink = util.nlinks(f)
+                            if nlink < 1:
+                                nlink = 2 # force mktempcopy (issue1922)
                 except (OSError, IOError) as e:
                     if e.errno != errno.ENOENT:
                         raise
                     nlink = 0
                     util.ensuredirs(dirname, self.createmode, notindexed)
                 if nlink > 0:
                     if self._trustnlink is None:
                         self._trustnlink = nlink > 1 or util.checknlink(f)
@@ -1022,20 +1006,19 @@  def readrequires(opener, supported):
         raise error.RequirementError(
             _("repository requires features unknown to this Mercurial: %s")
             % " ".join(missings),
             hint=_("see https://mercurial-scm.org/wiki/MissingRequirement"
                    " for more information"))
     return requirements
 
 def writerequires(opener, requirements):
-    reqfile = opener("requires", "w")
-    for r in sorted(requirements):
-        reqfile.write("%s\n" % r)
-    reqfile.close()
+    with opener('requires', 'w') as fp:
+        for r in sorted(requirements):
+            fp.write("%s\n" % r)
 
 class filecachesubentry(object):
     def __init__(self, path, stat):
         self.path = path
         self.cachestat = None
         self._cacheable = None
 
         if stat: