Patchwork [2,of,6] largefiles: use context for file closing

login
register
mail settings
Submitter Mads Kiilerich
Date Oct. 7, 2016, 11:26 p.m.
Message ID <87cea1040403001e660d.1475882766@localhost.localdomain>
Download mbox | patch
Permalink /patch/16902/
State Accepted
Headers show

Comments

Mads Kiilerich - Oct. 7, 2016, 11:26 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1475881181 -7200
#      Sat Oct 08 00:59:41 2016 +0200
# Node ID 87cea1040403001e660dd1c6b2e2d069d8a51d2e
# Parent  96315a5833ed015acb7bd8f6d7f1e38db6fa9c50
largefiles: use context for file closing

Make the code slightly smaller and safer (and more deeply indented).

Patch

diff --git a/hgext/largefiles/basestore.py b/hgext/largefiles/basestore.py
--- a/hgext/largefiles/basestore.py
+++ b/hgext/largefiles/basestore.py
@@ -91,15 +91,13 @@  class basestore(object):
         storefilename = lfutil.storepath(self.repo, hash)
 
         tmpname = storefilename + '.tmp'
-        tmpfile = util.atomictempfile(tmpname,
-                                      createmode=self.repo.store.createmode)
-
-        try:
-            gothash = self._getfile(tmpfile, filename, hash)
-        except StoreError as err:
-            self.ui.warn(err.longmessage())
-            gothash = ""
-        tmpfile.close()
+        with util.atomictempfile(tmpname,
+                createmode=self.repo.store.createmode) as tmpfile:
+            try:
+                gothash = self._getfile(tmpfile, filename, hash)
+            except StoreError as err:
+                self.ui.warn(err.longmessage())
+                gothash = ""
 
         if gothash != hash:
             if gothash != "":
diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
--- a/hgext/largefiles/lfutil.py
+++ b/hgext/largefiles/lfutil.py
@@ -54,10 +54,10 @@  def link(src, dest):
         util.oslink(src, dest)
     except OSError:
         # if hardlinks fail, fallback on atomic copy
-        dst = util.atomictempfile(dest)
-        for chunk in util.filechunkiter(open(src, 'rb')):
-            dst.write(chunk)
-        dst.close()
+        with open(src, 'rb') as srcf:
+            with util.atomictempfile(dest) as dstf:
+                for chunk in util.filechunkiter(srcf):
+                    dstf.write(chunk)
         os.chmod(dest, os.stat(src).st_mode)
 
 def usercachepath(ui, hash):
@@ -264,11 +264,11 @@  def copytostoreabsolute(repo, file, hash
         link(usercachepath(repo.ui, hash), storepath(repo, hash))
     else:
         util.makedirs(os.path.dirname(storepath(repo, hash)))
-        dst = util.atomictempfile(storepath(repo, hash),
-                                  createmode=repo.store.createmode)
-        for chunk in util.filechunkiter(open(file, 'rb')):
-            dst.write(chunk)
-        dst.close()
+        with open(file, 'rb') as srcf:
+            with util.atomictempfile(storepath(repo, hash),
+                                     createmode=repo.store.createmode) as dstf:
+                for chunk in util.filechunkiter(srcf):
+                    dstf.write(chunk)
         linktousercache(repo, hash)
 
 def linktousercache(repo, hash):
@@ -370,10 +370,9 @@  def hashfile(file):
     if not os.path.exists(file):
         return ''
     hasher = hashlib.sha1('')
-    fd = open(file, 'rb')
-    for data in util.filechunkiter(fd, 128 * 1024):
-        hasher.update(data)
-    fd.close()
+    with open(file, 'rb') as fd:
+        for data in util.filechunkiter(fd, 128 * 1024):
+            hasher.update(data)
     return hasher.hexdigest()
 
 def getexecutable(filename):
diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -883,11 +883,8 @@  def hgclone(orig, ui, opts, *args, **kwa
 
         # If largefiles is required for this repo, permanently enable it locally
         if 'largefiles' in repo.requirements:
-            fp = repo.vfs('hgrc', 'a', text=True)
-            try:
+            with repo.vfs('hgrc', 'a', text=True) as fp:
                 fp.write('\n[extensions]\nlargefiles=\n')
-            finally:
-                fp.close()
 
         # Caching is implicitly limited to 'rev' option, since the dest repo was
         # truncated at that point.  The user may expect a download count with
@@ -1339,30 +1336,28 @@  def overridecat(orig, ui, repo, file1, *
     m.visitdir = lfvisitdirfn
 
     for f in ctx.walk(m):
-        fp = cmdutil.makefileobj(repo, opts.get('output'), ctx.node(),
-                                 pathname=f)
-        lf = lfutil.splitstandin(f)
-        if lf is None or origmatchfn(f):
-            # duplicating unreachable code from commands.cat
-            data = ctx[f].data()
-            if opts.get('decode'):
-                data = repo.wwritedata(f, data)
-            fp.write(data)
-        else:
-            hash = lfutil.readstandin(repo, lf, ctx.rev())
-            if not lfutil.inusercache(repo.ui, hash):
-                store = storefactory.openstore(repo)
-                success, missing = store.get([(lf, hash)])
-                if len(success) != 1:
-                    raise error.Abort(
-                        _('largefile %s is not in cache and could not be '
-                          'downloaded')  % lf)
-            path = lfutil.usercachepath(repo.ui, hash)
-            fpin = open(path, "rb")
-            for chunk in util.filechunkiter(fpin, 128 * 1024):
-                fp.write(chunk)
-            fpin.close()
-        fp.close()
+        with cmdutil.makefileobj(repo, opts.get('output'), ctx.node(),
+                                 pathname=f) as fp:
+            lf = lfutil.splitstandin(f)
+            if lf is None or origmatchfn(f):
+                # duplicating unreachable code from commands.cat
+                data = ctx[f].data()
+                if opts.get('decode'):
+                    data = repo.wwritedata(f, data)
+                fp.write(data)
+            else:
+                hash = lfutil.readstandin(repo, lf, ctx.rev())
+                if not lfutil.inusercache(repo.ui, hash):
+                    store = storefactory.openstore(repo)
+                    success, missing = store.get([(lf, hash)])
+                    if len(success) != 1:
+                        raise error.Abort(
+                            _('largefile %s is not in cache and could not be '
+                              'downloaded')  % lf)
+                path = lfutil.usercachepath(repo.ui, hash)
+                with open(path, "rb") as fpin:
+                    for chunk in util.filechunkiter(fpin, 128 * 1024):
+                        fp.write(chunk)
         err = 0
     return err
 
diff --git a/hgext/largefiles/remotestore.py b/hgext/largefiles/remotestore.py
--- a/hgext/largefiles/remotestore.py
+++ b/hgext/largefiles/remotestore.py
@@ -45,17 +45,13 @@  class remotestore(basestore.basestore):
 
     def sendfile(self, filename, hash):
         self.ui.debug('remotestore: sendfile(%s, %s)\n' % (filename, hash))
-        fd = None
         try:
-            fd = lfutil.httpsendfile(self.ui, filename)
-            return self._put(hash, fd)
+            with lfutil.httpsendfile(self.ui, filename) as fd:
+                return self._put(hash, fd)
         except IOError as e:
             raise error.Abort(
                 _('remotestore: could not open file %s: %s')
                 % (filename, str(e)))
-        finally:
-            if fd:
-                fd.close()
 
     def _getfile(self, tmpfile, filename, hash):
         try:
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -499,6 +499,12 @@  class _unclosablefile(object):
     def __getattr__(self, attr):
         return getattr(self._fp, attr)
 
+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_value, exc_tb):
+        pass
+
 def makefileobj(repo, pat, node=None, desc=None, total=None,
                 seqno=None, revwidth=None, mode='wb', modemap=None,
                 pathname=None):
diff --git a/mercurial/httpconnection.py b/mercurial/httpconnection.py
--- a/mercurial/httpconnection.py
+++ b/mercurial/httpconnection.py
@@ -58,6 +58,12 @@  class httpsendfile(object):
                          unit=_('kb'), total=self._total)
         return ret
 
+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        self.close()
+
 # moved here from url.py to avoid a cycle
 def readauthforuri(ui, uri, user):
     # Read configuration