repo-TCs, #519: FIX config resource leaks

+ Modify lock/read-config-file code to ensure files closed.
+ Use `with GitConfigarser()` more systematically in TCs.
+ Clear any locks left hanging from prev Tcs.
+ Util: mark lock-files as SHORT_LIVED; save some SSDs...

5 files changed, 58 insertions, 52 deletions

diff --git a/git/repo/base.py b/git/repo/base.py
index 2a56eaed..9cc70571 100644
--- a/git/repo/base.py
+++ b/git/repo/base.py
@@ -210,11 +210,13 @@ class Repo(object):
     # Description property
     def _get_description(self):
         filename = join(self.git_dir, 'description')
-        return open(filename, 'rb').read().rstrip().decode(defenc)
+        with open(filename, 'rb') as fp:
+            return fp.read().rstrip().decode(defenc)
 
     def _set_description(self, descr):
         filename = join(self.git_dir, 'description')
-        open(filename, 'wb').write((descr + '\n').encode(defenc))
+        with open(filename, 'wb') as fp:
+            fp.write((descr + '\n').encode(defenc))
 
     description = property(_get_description, _set_description,
                            doc="the project's description")
@@ -548,11 +550,8 @@ class Repo(object):
         alternates_path = join(self.git_dir, 'objects', 'info', 'alternates')
 
         if os.path.exists(alternates_path):
-            try:
-                f = open(alternates_path, 'rb')
+            with open(alternates_path, 'rb') as f:
                 alts = f.read().decode(defenc)
-            finally:
-                f.close()
             return alts.strip().splitlines()
         else:
             return list()
@@ -573,13 +572,8 @@ class Repo(object):
             if isfile(alternates_path):
                 os.remove(alternates_path)
         else:
-            try:
-                f = open(alternates_path, 'wb')
+            with open(alternates_path, 'wb') as f:
                 f.write("\n".join(alts).encode(defenc))
-            finally:
-                f.close()
-            # END file handling
-        # END alts handling
 
     alternates = property(_get_alternates, _set_alternates,
                           doc="Retrieve a list of alternates paths or set a list paths to be used as alternates")
diff --git a/git/repo/fun.py b/git/repo/fun.py
index 6b06663a..0483eaa9 100644
--- a/git/repo/fun.py
+++ b/git/repo/fun.py
@@ -25,8 +25,8 @@ __all__ = ('rev_parse', 'is_git_dir', 'touch', 'find_git_dir', 'name_to_object',
 
 
 def touch(filename):
-    fp = open(filename, "ab")
-    fp.close()
+    with open(filename, "ab"):
+        pass
     return filename
 
 
diff --git a/git/test/test_config.py b/git/test/test_config.py
index bd2bad0a..154aaa24 100644
--- a/git/test/test_config.py
+++ b/git/test/test_config.py
@@ -11,8 +11,7 @@ import os
 from git import (
     GitConfigParser
 )
-from git.compat import (
-    string_types)
+from git.compat import string_types
 from git.config import cp
 from git.test.lib import (
     TestCase,
diff --git a/git/test/test_repo.py b/git/test/test_repo.py
index 3e030a05..e2c18d3f 100644
--- a/git/test/test_repo.py
+++ b/git/test/test_repo.py
@@ -4,18 +4,14 @@
 #
 # This module is part of GitPython and is released under
 # the BSD License: http://www.opensource.org/licenses/bsd-license.php
+import glob
+from io import BytesIO
+import itertools
+import os
 import pickle
+import sys
+import tempfile
 
-from git.test.lib import (
-    patch,
-    TestBase,
-    with_rw_repo,
-    fixture,
-    assert_false,
-    assert_equal,
-    assert_true,
-    raises
-)
 from git import (
     InvalidGitRepositoryError,
     Repo,
@@ -33,23 +29,28 @@ from git import (
     BadName,
     GitCommandError
 )
-from git.repo.fun import touch
-from git.util import join_path_native, rmtree
+from git.compat import string_types
 from git.exc import (
     BadObject,
 )
-from gitdb.util import bin_to_hex
-from git.compat import string_types
+from git.repo.fun import touch
+from git.test.lib import (
+    patch,
+    TestBase,
+    with_rw_repo,
+    fixture,
+    assert_false,
+    assert_equal,
+    assert_true,
+    raises
+)
 from git.test.lib import with_rw_directory
-
-import os
-import sys
-import tempfile
-import itertools
-from io import BytesIO
-
+from git.util import join_path_native, rmtree, rmfile
+from gitdb.util import bin_to_hex
 from nose import SkipTest
 
+import os.path as osp
+
 
 def iter_flatten(lol):
     for items in lol:
@@ -61,9 +62,23 @@ def flatten(lol):
     return list(iter_flatten(lol))
 
 
+_tc_lock_fpaths = osp.join(osp.dirname(__file__), '../../.git/*.lock')
+
+
+def _rm_lock_files():
+    for lfp in glob.glob(_tc_lock_fpaths):
+        rmfile(lfp)
+
+
 class TestRepo(TestBase):
 
+    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)
         import gc
         gc.collect()
 
@@ -309,10 +324,9 @@ class TestRepo(TestBase):
 
     def test_archive(self):
         tmpfile = tempfile.mktemp(suffix='archive-test')
-        stream = open(tmpfile, 'wb')
-        self.rorepo.archive(stream, '0.1.6', path='doc')
-        assert stream.tell()
-        stream.close()
+        with open(tmpfile, 'wb') as stream:
+            self.rorepo.archive(stream, '0.1.6', path='doc')
+            assert stream.tell()
         os.remove(tmpfile)
 
     @patch.object(Git, '_call_process')
@@ -401,9 +415,8 @@ class TestRepo(TestBase):
 
             num_recently_untracked = 0
             for fpath in files:
-                fd = open(fpath, "wb")
-                fd.close()
-            # END for each filename
+                with open(fpath, "wb"):
+                    pass
             untracked_files = rwrepo.untracked_files
             num_recently_untracked = len(untracked_files)
 
@@ -426,19 +439,16 @@ class TestRepo(TestBase):
     def test_config_writer(self):
         for config_level in self.rorepo.config_level:
             try:
-                writer = self.rorepo.config_writer(config_level)
-                assert not writer.read_only
-                writer.release()
+                with self.rorepo.config_writer(config_level) as writer:
+                    self.assertFalse(writer.read_only)
             except IOError:
                 # its okay not to get a writer for some configuration files if we
                 # have no permissions
                 pass
-        # END for each config level
 
     def test_config_level_paths(self):
         for config_level in self.rorepo.config_level:
             assert self.rorepo._get_config_path(config_level)
-        # end for each config level
 
     def test_creation_deletion(self):
         # just a very quick test to assure it generally works. There are
@@ -448,8 +458,8 @@ class TestRepo(TestBase):
 
         tag = self.rorepo.create_tag("new_tag", "HEAD~2")
         self.rorepo.delete_tag(tag)
-        writer = self.rorepo.config_writer()
-        writer.release()
+        with self.rorepo.config_writer():
+            pass
         remote = self.rorepo.create_remote("new_remote", "git@server:repo.git")
         self.rorepo.delete_remote(remote)
 
diff --git a/git/util.py b/git/util.py
index 87ef38d3..a6c5a100 100644
--- a/git/util.py
+++ b/git/util.py
@@ -574,7 +574,10 @@ class LockFile(object):
                           (self._file_path, lock_file))
 
         try:
-            fd = os.open(lock_file, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0)
+            flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL
+            if is_win:
+                flags |= getattr(os, 'O_SHORT_LIVED')
+            fd = os.open(lock_file, flags, 0)
             os.close(fd)
         except OSError as e:
             raise IOError(str(e))