From patchwork Sun Jan  3 00:45:32 2016
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: [4,of,5,V2] scmutil: use context managers for file handles
From: Gregory Szorc <gregory.szorc@gmail.com>
X-Patchwork-Id: 12498
Message-Id: <438dce208879a651879d.1451781932@ubuntu-vm-main>
To: mercurial-devel@selenic.com
Date: Sat, 02 Jan 2016 16:45:32 -0800

# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1451776787 28800
#      Sat Jan 02 15:19:47 2016 -0800
# Node ID 438dce208879a651879d123d646c6782e3936f42
# Parent  bb932201352783ce4863e49f2c5ed43734505d34
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.

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: