cfg_TCs, gitpython-developers#519: FIX config resource leaks

+ Modify lock/read-config-file code to ansure files closed
+ Use `with GitConfigarser()` more systematically in TCs.
+ Clear any locks left hanging from pev Tcs

Author: ankostis

git/config.py

@@ -388,23 +388,18 @@ def read(self):
         while files_to_read:
             file_path = files_to_read.pop(0)
             fp = file_path
-            close_fp = False
+            file_ok = False
 
-            # assume a path if it is not a file-object
-            if not hasattr(fp, "seek"):
+            if hasattr(fp, "seek"):
+                self._read(fp, fp.name)
+            else:
+                # assume a path if it is not a file-object
                 try:
-                    fp = open(file_path, 'rb')
-                    close_fp = True
+                    with open(file_path, 'rb') as fp:
+                        file_ok = True
+                        self._read(fp, fp.name)
                 except IOError:
                     continue
-            # END fp handling
-
-            try:
-                self._read(fp, fp.name)
-            finally:
-                if close_fp:
-                    fp.close()
-            # END read-handling
 
             # Read includes and append those that we didn't handle yet
             # We expect all paths to be normalized and absolute (and will assure that is the case)
@@ -413,7 +408,7 @@ def read(self):
                     if include_path.startswith('~'):
                         include_path = os.path.expanduser(include_path)
                     if not os.path.isabs(include_path):
-                        if not close_fp:
+                        if not file_ok:
                             continue
                         # end ignore relative paths if we don't know the configuration file path
                         assert os.path.isabs(file_path), "Need absolute paths to be sure our cycle checks will work"
@@ -477,34 +472,25 @@ def write(self):
         # end
 
         fp = self._file_or_files
-        close_fp = False
 
         # we have a physical file on disk, so get a lock
-        if isinstance(fp, string_types + (FileType, )):
+        is_file_lock = isinstance(fp, string_types + (FileType, ))
+        if is_file_lock:
             self._lock._obtain_lock()
-        # END get lock for physical files
-
-        if not hasattr(fp, "seek"):
-            fp = open(self._file_or_files, "wb")
-            close_fp = True
-        else:
-            fp.seek(0)
-            # make sure we do not overwrite into an existing file
-            if hasattr(fp, 'truncate'):
-                fp.truncate()
-            # END
-        # END handle stream or file
-
-        # WRITE DATA
         try:
-            self._write(fp)
+            if not hasattr(fp, "seek"):
+                with open(self._file_or_files, "wb") as fp:
+                    self._write(fp)
+            else:
+                fp.seek(0)
+                # make sure we do not overwrite into an existing file
+                if hasattr(fp, 'truncate'):
+                    fp.truncate()
+                self._write(fp)
         finally:
-            if close_fp:
-                fp.close()
-        # END data writing
-
-        # we do not release the lock - it will be done automatically once the
-        # instance vanishes
+            # we release the lock - it will not vanish automatically in PY3.5+
+            if is_file_lock:
+                self._lock._release_lock()
 
     def _assure_writable(self, method_name):
         if self.read_only:

git/test/test_config.py

@@ -4,80 +4,98 @@
 # This module is part of GitPython and is released under
 # the BSD License: http://www.opensource.org/licenses/bsd-license.php
 
-from git.test.lib import (
-    TestCase,
-    fixture_path,
-    assert_equal,
-)
-from git.test.lib import with_rw_directory
+import glob
+import io
+import os
+
 from git import (
     GitConfigParser
 )
 from git.compat import (
     string_types,
-)
-import io
-import os
+    is_win,)
 from git.config import cp
+from git.test.lib import (
+    TestCase,
+    fixture_path,
+)
+from git.test.lib import with_rw_directory
+
+import os.path as osp
+
+
+_tc_lock_fpaths = osp.join(osp.dirname(__file__), 'fixtures/*.lock')
+
+
+def _rm_lock_files():
+    for lfp in glob.glob(_tc_lock_fpaths):
+        if is_win and osp.isfile(lfp):
+            os.chmod(lfp, 0o777)
+        os.remove(lfp)
 
 
 class TestBase(TestCase):
+    def setUp(self):
+        _rm_lock_files()
+
+    def tearDown(self):
+        for lfp in glob.glob(_tc_lock_fpaths):
+            if osp.isfile(lfp):
+                raise AssertionError('Previous TC left hanging git-lock file: %s', lfp)
 
     def _to_memcache(self, file_path):
-        fp = open(file_path, "rb")
-        sio = io.BytesIO(fp.read())
+        with open(file_path, "rb") as fp:
+            sio = io.BytesIO(fp.read())
         sio.name = file_path
         return sio
 
     def test_read_write(self):
         # writer must create the exact same file as the one read before
         for filename in ("git_config", "git_config_global"):
             file_obj = self._to_memcache(fixture_path(filename))
-            w_config = GitConfigParser(file_obj, read_only=False)
-            w_config.read()                 # enforce reading
-            assert w_config._sections
-            w_config.write()                # enforce writing
-
-            # we stripped lines when reading, so the results differ
-            assert file_obj.getvalue()
-            self.assertEqual(file_obj.getvalue(), self._to_memcache(fixture_path(filename)).getvalue())
-
-            # creating an additional config writer must fail due to exclusive access
-            self.failUnlessRaises(IOError, GitConfigParser, file_obj, read_only=False)
-
-            # should still have a lock and be able to make changes
-            assert w_config._lock._has_lock()
-
-            # changes should be written right away
-            sname = "my_section"
-            oname = "mykey"
-            val = "myvalue"
-            w_config.add_section(sname)
-            assert w_config.has_section(sname)
-            w_config.set(sname, oname, val)
-            assert w_config.has_option(sname, oname)
-            assert w_config.get(sname, oname) == val
-
-            sname_new = "new_section"
-            oname_new = "new_key"
-            ival = 10
-            w_config.set_value(sname_new, oname_new, ival)
-            assert w_config.get_value(sname_new, oname_new) == ival
-
-            file_obj.seek(0)
-            r_config = GitConfigParser(file_obj, read_only=True)
-            assert r_config.has_section(sname)
-            assert r_config.has_option(sname, oname)
-            assert r_config.get(sname, oname) == val
-            w_config.release()
+            with GitConfigParser(file_obj, read_only=False) as w_config:
+                w_config.read()                 # enforce reading
+                assert w_config._sections
+                w_config.write()                # enforce writing
+
+                # we stripped lines when reading, so the results differ
+                assert file_obj.getvalue()
+                self.assertEqual(file_obj.getvalue(), self._to_memcache(fixture_path(filename)).getvalue())
+
+                # creating an additional config writer must fail due to exclusive access
+                self.failUnlessRaises(IOError, GitConfigParser, file_obj, read_only=False)
+
+                # should still have a lock and be able to make changes
+                assert w_config._lock._has_lock()
+
+                # changes should be written right away
+                sname = "my_section"
+                oname = "mykey"
+                val = "myvalue"
+                w_config.add_section(sname)
+                assert w_config.has_section(sname)
+                w_config.set(sname, oname, val)
+                assert w_config.has_option(sname, oname)
+                assert w_config.get(sname, oname) == val
+
+                sname_new = "new_section"
+                oname_new = "new_key"
+                ival = 10
+                w_config.set_value(sname_new, oname_new, ival)
+                assert w_config.get_value(sname_new, oname_new) == ival
+
+                file_obj.seek(0)
+                r_config = GitConfigParser(file_obj, read_only=True)
+                assert r_config.has_section(sname)
+                assert r_config.has_option(sname, oname)
+                assert r_config.get(sname, oname) == val
         # END for each filename
 
     @with_rw_directory
     def test_lock_reentry(self, rw_dir):
         fpl = os.path.join(rw_dir, 'l')
-        gcp = GitConfigParser(fpl, read_only=False)
-        with gcp as cw:
-            cw.set_value('include', 'some_value', 'a')
+        with GitConfigParser(fpl, read_only=False) as gcp:
+            gcp.set_value('include', 'some_value', 'a')
         # entering again locks the file again...
         with gcp as cw:
             cw.set_value('include', 'some_other_value', 'b')
@@ -91,21 +109,21 @@ def test_lock_reentry(self, rw_dir):
 
     def test_multi_line_config(self):
         file_obj = self._to_memcache(fixture_path("git_config_with_comments"))
-        config = GitConfigParser(file_obj, read_only=False)
-        ev = "ruby -e '\n"
-        ev += "		system %(git), %(merge-file), %(--marker-size=%L), %(%A), %(%O), %(%B)\n"
-        ev += "		b = File.read(%(%A))\n"
-        ev += "		b.sub!(/^<+ .*\\nActiveRecord::Schema\\.define.:version => (\\d+). do\\n=+\\nActiveRecord::Schema\\."
-        ev += "define.:version => (\\d+). do\\n>+ .*/) do\n"
-        ev += "		  %(ActiveRecord::Schema.define(:version => #{[$1, $2].max}) do)\n"
-        ev += "		end\n"
-        ev += "		File.open(%(%A), %(w)) {|f| f.write(b)}\n"
-        ev += "		exit 1 if b.include?(%(<)*%L)'"
-        assert_equal(config.get('merge "railsschema"', 'driver'), ev)
-        assert_equal(config.get('alias', 'lg'),
-                     "log --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr)%Creset'"
-                     " --abbrev-commit --date=relative")
-        assert len(config.sections()) == 23
+        with GitConfigParser(file_obj, read_only=False) as config:
+            ev = "ruby -e '\n"
+            ev += "		system %(git), %(merge-file), %(--marker-size=%L), %(%A), %(%O), %(%B)\n"
+            ev += "		b = File.read(%(%A))\n"
+            ev += "		b.sub!(/^<+ .*\\nActiveRecord::Schema\\.define.:version => (\\d+). do\\n=+\\nActiveRecord::Schema\\."  # noqa E501
+            ev += "define.:version => (\\d+). do\\n>+ .*/) do\n"
+            ev += "		  %(ActiveRecord::Schema.define(:version => #{[$1, $2].max}) do)\n"
+            ev += "		end\n"
+            ev += "		File.open(%(%A), %(w)) {|f| f.write(b)}\n"
+            ev += "		exit 1 if b.include?(%(<)*%L)'"
+            self.assertEqual(config.get('merge "railsschema"', 'driver'), ev)
+            self.assertEqual(config.get('alias', 'lg'),
+                             "log --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr)%Creset'"
+                             " --abbrev-commit --date=relative")
+            self.assertEqual(len(config.sections()), 23)
 
     def test_base(self):
         path_repo = fixture_path("git_config")
@@ -202,22 +220,19 @@ def check_test_value(cr, value):
 
     def test_rename(self):
         file_obj = self._to_memcache(fixture_path('git_config'))
-        cw = GitConfigParser(file_obj, read_only=False, merge_includes=False)
-
-        self.failUnlessRaises(ValueError, cw.rename_section, "doesntexist", "foo")
-        self.failUnlessRaises(ValueError, cw.rename_section, "core", "include")
+        with GitConfigParser(file_obj, read_only=False, merge_includes=False) as cw:
+            self.failUnlessRaises(ValueError, cw.rename_section, "doesntexist", "foo")
+            self.failUnlessRaises(ValueError, cw.rename_section, "core", "include")
 
-        nn = "bee"
-        assert cw.rename_section('core', nn) is cw
-        assert not cw.has_section('core')
-        assert len(cw.items(nn)) == 4
-        cw.release()
+            nn = "bee"
+            assert cw.rename_section('core', nn) is cw
+            assert not cw.has_section('core')
+            assert len(cw.items(nn)) == 4
 
     def test_complex_aliases(self):
         file_obj = self._to_memcache(fixture_path('.gitconfig'))
-        w_config = GitConfigParser(file_obj, read_only=False)
-        self.assertEqual(w_config.get('alias', 'rbi'), '"!g() { git rebase -i origin/${1:-master} ; } ; g"')
-        w_config.release()
+        with GitConfigParser(file_obj, read_only=False) as w_config:
+            self.assertEqual(w_config.get('alias', 'rbi'), '"!g() { git rebase -i origin/${1:-master} ; } ; g"')
         self.assertEqual(file_obj.getvalue(), self._to_memcache(fixture_path('.gitconfig')).getvalue())
 
     def test_empty_config_value(self):