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

login
register
mail settings
Submitter Gregory Szorc
Date Jan. 3, 2016, 12:45 a.m.
Message ID <438dce208879a651879d.1451781932@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/12498/
State Accepted
Delegated to: Matt Mackall
Headers show

Comments

Gregory Szorc - Jan. 3, 2016, 12:45 a.m.
# 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.
Adrian Buehlmann - Jan. 5, 2016, 5:53 p.m.
On 2016-01-03 01:45, Gregory Szorc wrote:
> @@ -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)

Churning this esoteric code area just in order to do this rather
cosmetic change feels a bit pointless to me, but - at least - it looks
correct to me (but I won't test it).

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: