[PATCH] Use try/finally to close files on error

[Matt Mackall]

On Thu, 2011-11-03 at 16:17 +0100, Victor Stinner wrote:
> Le Mercredi 2 Novembre 2011 16:54:45 vous avez écrit :
> > We're aware of that. But until you point to -actual- bugs that this
> > fixes, I consider this bloat.
> 
> The patch fixes 4 functions by closing explicitly files:
> 
> "Close the file in ignore.ignore(), revlog.revlog._loadchunk(),
> ui.ui.readconfig()."
> 
> I consider these changes as bugfixes because it is important to close files to 
> ensure that data is written on disk (even if a os.fsync() is even better ;-)).

What? Those three functions are READING data?

Even for a write, ensuring a prompt close() after a write() error
accomplishes nothing.

What I want to see is an explanation that includes "actual undesirable
behavior that is fixed by using try/finally".

You are correct that those three functions are missing an explicit close
and we should add those. But I see no advantage to adding try/finally
everywhere. I've queued the following:

# HG changeset patch
# User Matt Mackall <mpm at selenic.com>
# Date 1320336777 18000
# Node ID 3aa9801214e41df5446d0d03f5a73a7ddf0408db
# Parent  1f677c7e494df609c8d5be5b22f62dde0fb24ff4
misc: adding missing file close() calls

Spotted by Victor Stinner <victor.stinner at haypocalc.com>

diff -r 1f677c7e494d -r 3aa9801214e4 mercurial/ignore.py
--- a/mercurial/ignore.py	Thu Oct 20 00:37:34 2011 +0200
+++ b/mercurial/ignore.py	Thu Nov 03 11:12:57 2011 -0500
@@ -78,6 +78,7 @@
             pats[f] = []
             fp = open(f)
             pats[f], warnings = ignorepats(fp)
+            fp.close()
             for warning in warnings:
                 warn("%s: %s\n" % (f, warning))
         except IOError, inst:
diff -r 1f677c7e494d -r 3aa9801214e4 mercurial/revlog.py
--- a/mercurial/revlog.py	Thu Oct 20 00:37:34 2011 +0200
+++ b/mercurial/revlog.py	Thu Nov 03 11:12:57 2011 -0500
@@ -800,6 +800,7 @@
         readahead = max(65536, length)
         df.seek(offset)
         d = df.read(readahead)
+        df.close()
         self._addchunk(offset, d)
         if readahead > length:
             return d[:length]
diff -r 1f677c7e494d -r 3aa9801214e4 mercurial/ui.py
--- a/mercurial/ui.py	Thu Oct 20 00:37:34 2011 +0200
+++ b/mercurial/ui.py	Thu Nov 03 11:12:57 2011 -0500
@@ -79,6 +79,7 @@
 
         try:
             cfg.read(filename, fp, sections=sections, remap=remap)
+            fp.close()
         except error.ConfigError, inst:
             if trusted:
                 raise


-- 
Mathematics is the supreme nostalgia of our time.