Win, #519: FIX with_rw_directory() to remove read-only dirs

+ Stop using gitdb's respective helper.
+ Fix files chmod(555) which CANNOT DELETE on Windows (but do on Linux).

14 files changed, 69 insertions, 49 deletions

diff --git a/git/cmd.py b/git/cmd.py
index 69844366..fb94c200 100644
--- a/git/cmd.py
+++ b/git/cmd.py
@@ -1013,6 +1013,10 @@ class Git(LazyMixin):
         Currently persistent commands will be interrupted.
 
         :return: self"""
+        for cmd in (self.cat_file_all, self.cat_file_header):
+            if cmd:
+                cmd.__del__()
+
         self.cat_file_all = None
         self.cat_file_header = None
         return self
diff --git a/git/test/lib/helper.py b/git/test/lib/helper.py
index 7f4e81e0..6d840027 100644
--- a/git/test/lib/helper.py
+++ b/git/test/lib/helper.py
@@ -4,15 +4,16 @@
 # This module is part of GitPython and is released under
 # the BSD License: http://www.opensource.org/licenses/bsd-license.php
 from __future__ import print_function
+
 import os
 from unittest import TestCase
 import time
 import tempfile
-import shutil
 import io
 import logging
 
 from git import Repo, Remote, GitCommandError, Git
+from git.util import rmtree
 from git.compat import string_types, is_win
 import textwrap
 
@@ -23,7 +24,8 @@ GIT_DAEMON_PORT = os.environ.get("GIT_PYTHON_TEST_GIT_DAEMON_PORT", "9418")
 
 __all__ = (
     'fixture_path', 'fixture', 'absolute_project_path', 'StringProcessAdapter',
-    'with_rw_repo', 'with_rw_and_rw_remote_repo', 'TestBase', 'TestCase', 'GIT_REPO', 'GIT_DAEMON_PORT'
+    'with_rw_directory', 'with_rw_repo', 'with_rw_and_rw_remote_repo', 'TestBase', 'TestCase',
+    'GIT_REPO', 'GIT_DAEMON_PORT'
 )
 
 log = logging.getLogger('git.util')
@@ -79,16 +81,31 @@ def _mktemp(*args):
     return tdir
 
 
-def _rmtree_onerror(osremove, fullpath, exec_info):
-    """
-    Handle the case on windows that read-only files cannot be deleted by
-    os.remove by setting it to mode 777, then retry deletion.
-    """
-    if is_win or osremove is not os.remove:
-        raise
+def with_rw_directory(func):
+    """Create a temporary directory which can be written to, remove it if the
+    test succeeds, but leave it otherwise to aid additional debugging"""
 
-    os.chmod(fullpath, 0o777)
-    os.remove(fullpath)
+    def wrapper(self):
+        path = tempfile.mktemp(prefix=func.__name__)
+        os.mkdir(path)
+        keep = False
+        try:
+            try:
+                return func(self, path)
+            except Exception:
+                log.info.write("Test %s.%s failed, output is at %r\n",
+                               type(self).__name__, func.__name__, path)
+                keep = True
+                raise
+        finally:
+            # Need to collect here to be sure all handles have been closed. It appears
+            # a windows-only issue. In fact things should be deleted, as well as
+            # memory maps closed, once objects go out of scope. For some reason
+            # though this is not the case here unless we collect explicitly.
+            import gc
+            gc.collect()
+            if not keep:
+                rmtree(path)
 
 
 def with_rw_repo(working_tree_ref, bare=False):
@@ -129,8 +146,11 @@ def with_rw_repo(working_tree_ref, bare=False):
             finally:
                 os.chdir(prev_cwd)
                 rw_repo.git.clear_cache()
+                rw_repo = None
+                import gc
+                gc.collect()
                 if repo_dir is not None:
-                    shutil.rmtree(repo_dir, onerror=_rmtree_onerror)
+                    rmtree(repo_dir)
                 # END rm test repo if possible
             # END cleanup
         # END rw repo creator
@@ -279,14 +299,15 @@ def with_rw_and_rw_remote_repo(working_tree_ref):
                 if gd is not None:
                     gd.proc.kill()
 
-                import gc
-                gc.collect()
                 rw_repo.git.clear_cache()
                 rw_remote_repo.git.clear_cache()
+                rw_repo = rw_remote_repo = None
+                import gc
+                gc.collect()
                 if repo_dir:
-                    shutil.rmtree(repo_dir, onerror=_rmtree_onerror)
+                    rmtree(repo_dir)
                 if remote_repo_dir:
-                    shutil.rmtree(remote_repo_dir, onerror=_rmtree_onerror)
+                    rmtree(remote_repo_dir)
 
                 if gd is not None:
                     gd.proc.wait()
diff --git a/git/test/performance/lib.py b/git/test/performance/lib.py
index bb3f7a99..eebbfd76 100644
--- a/git/test/performance/lib.py
+++ b/git/test/performance/lib.py
@@ -4,7 +4,6 @@ from git.test.lib import (
     TestBase
 )
 from gitdb.test.lib import skip_on_travis_ci
-import shutil
 import tempfile
 import logging
 
@@ -16,6 +15,7 @@ from git.db import (
 from git import (
     Repo
 )
+from git.util import rmtree
 
 #{ Invvariants
 k_env_git_repo = "GIT_PYTHON_TEST_GIT_REPO_BASE"
@@ -86,7 +86,7 @@ class TestBigRepoRW(TestBigRepoR):
     def tearDown(self):
         super(TestBigRepoRW, self).tearDown()
         if self.gitrwrepo is not None:
-            shutil.rmtree(self.gitrwrepo.working_dir)
+            rmtree(self.gitrwrepo.working_dir)
             self.gitrwrepo.git.clear_cache()
         self.gitrwrepo = None
         self.puregitrwrepo.git.clear_cache()
diff --git a/git/test/test_commit.py b/git/test/test_commit.py
index 2f5270d4..33f8081c 100644
--- a/git/test/test_commit.py
+++ b/git/test/test_commit.py
@@ -19,7 +19,7 @@ from git import (
     Actor,
 )
 from gitdb import IStream
-from gitdb.test.lib import with_rw_directory
+from git.test.lib import with_rw_directory
 from git.compat import (
     string_types,
     text_type
diff --git a/git/test/test_config.py b/git/test/test_config.py
index c0889c1a..d47349fa 100644
--- a/git/test/test_config.py
+++ b/git/test/test_config.py
@@ -9,7 +9,7 @@ from git.test.lib import (
     fixture_path,
     assert_equal,
 )
-from gitdb.test.lib import with_rw_directory
+from git.test.lib import with_rw_directory
 from git import (
     GitConfigParser
 )
diff --git a/git/test/test_diff.py b/git/test/test_diff.py
index cab72d2a..57c6bc79 100644
--- a/git/test/test_diff.py
+++ b/git/test/test_diff.py
@@ -15,7 +15,7 @@ from git.test.lib import (
 
 )
 
-from gitdb.test.lib import with_rw_directory
+from git.test.lib import with_rw_directory
 
 from git import (
     Repo,
diff --git a/git/test/test_docs.py b/git/test/test_docs.py
index 85c647dd..a6e92543 100644
--- a/git/test/test_docs.py
+++ b/git/test/test_docs.py
@@ -7,7 +7,7 @@
 import os
 
 from git.test.lib import TestBase
-from gitdb.test.lib import with_rw_directory
+from git.test.lib.helper import with_rw_directory
 
 
 class Tutorials(TestBase):
@@ -210,7 +210,7 @@ class Tutorials(TestBase):
         master = head.reference     # retrieve the reference the head points to
         master.commit               # from here you use it as any other reference
         # ![3-test_references_and_objects]
-
+#
         # [4-test_references_and_objects]
         log = master.log()
         log[0]                      # first (i.e. oldest) reflog entry
@@ -448,6 +448,8 @@ class Tutorials(TestBase):
         git.for_each_ref()                              # '-' becomes '_' when calling it
         # ![31-test_references_and_objects]
 
+        repo.git.clear_cache()
+
     def test_submodules(self):
         # [1-test_submodules]
         repo = self.rorepo
diff --git a/git/test/test_git.py b/git/test/test_git.py
index 36bbbb10..a676d7f7 100644
--- a/git/test/test_git.py
+++ b/git/test/test_git.py
@@ -24,7 +24,7 @@ from git import (
     Repo,
     cmd
 )
-from gitdb.test.lib import with_rw_directory
+from git.test.lib import with_rw_directory
 
 from git.compat import PY3, is_darwin
 
@@ -174,7 +174,7 @@ class TestGit(TestBase):
 
     def test_env_vars_passed_to_git(self):
         editor = 'non_existant_editor'
-        with mock.patch.dict('os.environ', {'GIT_EDITOR': editor}):
+        with mock.patch.dict('os.environ', {'GIT_EDITOR': editor}):  # @UndefinedVariable
             self.assertEqual(self.git.var("GIT_EDITOR"), editor)
 
     @with_rw_directory
@@ -203,7 +203,7 @@ class TestGit(TestBase):
         stream.write("#!/usr/bin/env sh\n" +
                      "echo FOO\n")
         stream.close()
-        os.chmod(path, 0o555)
+        os.chmod(path, 0o777)
 
         rw_repo = Repo.init(os.path.join(rw_dir, 'repo'))
         remote = rw_repo.create_remote('ssh-origin', "ssh://git@server/foo")
@@ -220,9 +220,6 @@ class TestGit(TestBase):
                         self.assertEqual(err.status, 128)
                     else:
                         self.assertIn('FOO', str(err))
-                # end
-            # end
-        # end if select.poll exists
 
     def test_handle_process_output(self):
         from git.cmd import handle_process_output
diff --git a/git/test/test_index.py b/git/test/test_index.py
index 2a8df798..0e2bc98c 100644
--- a/git/test/test_index.py
+++ b/git/test/test_index.py
@@ -11,7 +11,7 @@ from git.test.lib import (
     fixture,
     with_rw_repo
 )
-from git.util import Actor
+from git.util import Actor, rmtree
 from git.exc import (
     HookExecutionError,
     InvalidGitRepositoryError
@@ -32,7 +32,6 @@ from gitdb.util import hex_to_bin
 import os
 import sys
 import tempfile
-import shutil
 from stat import (
     S_ISLNK,
     ST_MODE
@@ -46,7 +45,7 @@ from git.index.typ import (
     IndexEntry
 )
 from git.index.fun import hook_path
-from gitdb.test.lib import with_rw_directory
+from git.test.lib import with_rw_directory
 
 
 class TestIndex(TestBase):
@@ -387,7 +386,7 @@ class TestIndex(TestBase):
         assert not open(test_file, 'rb').read().endswith(append_data)
 
         # checkout directory
-        shutil.rmtree(os.path.join(rw_repo.working_tree_dir, "lib"))
+        rmtree(os.path.join(rw_repo.working_tree_dir, "lib"))
         rval = index.checkout('lib')
         assert len(list(rval)) > 1
 
@@ -719,7 +718,7 @@ class TestIndex(TestBase):
             with open(hp, "wt") as fp:
                 fp.write("#!/usr/bin/env sh\necho stdout; echo stderr 1>&2; exit 1")
             # end
-            os.chmod(hp, 0o544)
+            os.chmod(hp, 0o744)
             try:
                 index.commit("This should fail")
             except HookExecutionError as err:
diff --git a/git/test/test_reflog.py b/git/test/test_reflog.py
index 3571e083..dffedf3b 100644
--- a/git/test/test_reflog.py
+++ b/git/test/test_reflog.py
@@ -7,11 +7,10 @@ from git.refs import (
     RefLogEntry,
     RefLog
 )
-from git.util import Actor
+from git.util import Actor, rmtree
 from gitdb.util import hex_to_bin
 
 import tempfile
-import shutil
 import os
 
 
@@ -104,4 +103,4 @@ class TestRefLog(TestBase):
         # END for each reflog
 
         # finally remove our temporary data
-        shutil.rmtree(tdir)
+        rmtree(tdir)
diff --git a/git/test/test_remote.py b/git/test/test_remote.py
index 2716d5b9..05de4ae2 100644
--- a/git/test/test_remote.py
+++ b/git/test/test_remote.py
@@ -25,10 +25,9 @@ from git import (
     Remote,
     GitCommandError
 )
-from git.util import IterableList
+from git.util import IterableList, rmtree
 from git.compat import string_types
 import tempfile
-import shutil
 import os
 import random
 
@@ -285,7 +284,7 @@ class TestRemote(TestBase):
             # and only provides progress information to ttys
             res = fetch_and_test(other_origin)
         finally:
-            shutil.rmtree(other_repo_dir)
+            rmtree(other_repo_dir)
         # END test and cleanup
 
     def _assert_push_and_pull(self, remote, rw_repo, remote_repo):
diff --git a/git/test/test_repo.py b/git/test/test_repo.py
index b516402a..3e030a05 100644
--- a/git/test/test_repo.py
+++ b/git/test/test_repo.py
@@ -34,18 +34,17 @@ from git import (
     GitCommandError
 )
 from git.repo.fun import touch
-from git.util import join_path_native
+from git.util import join_path_native, rmtree
 from git.exc import (
     BadObject,
 )
 from gitdb.util import bin_to_hex
 from git.compat import string_types
-from gitdb.test.lib import with_rw_directory
+from git.test.lib import with_rw_directory
 
 import os
 import sys
 import tempfile
-import shutil
 import itertools
 from io import BytesIO
 
@@ -200,7 +199,7 @@ class TestRepo(TestBase):
                 self._assert_empty_repo(rc)
 
                 try:
-                    shutil.rmtree(clone_path)
+                    rmtree(clone_path)
                 except OSError:
                     # when relative paths are used, the clone may actually be inside
                     # of the parent directory
@@ -211,9 +210,9 @@ class TestRepo(TestBase):
                 rc = Repo.clone_from(r.git_dir, clone_path)
                 self._assert_empty_repo(rc)
 
-                shutil.rmtree(git_dir_abs)
+                rmtree(git_dir_abs)
                 try:
-                    shutil.rmtree(clone_path)
+                    rmtree(clone_path)
                 except OSError:
                     # when relative paths are used, the clone may actually be inside
                     # of the parent directory
@@ -231,7 +230,7 @@ class TestRepo(TestBase):
             self._assert_empty_repo(r)
         finally:
             try:
-                shutil.rmtree(del_dir_abs)
+                rmtree(del_dir_abs)
             except OSError:
                 pass
             os.chdir(prev_cwd)
diff --git a/git/test/test_submodule.py b/git/test/test_submodule.py
index 9307bab2..dcfe9216 100644
--- a/git/test/test_submodule.py
+++ b/git/test/test_submodule.py
@@ -9,7 +9,7 @@ from git.test.lib import (
     TestBase,
     with_rw_repo
 )
-from gitdb.test.lib import with_rw_directory
+from git.test.lib import with_rw_directory
 from git.exc import (
     InvalidGitRepositoryError,
     RepositoryDirtyError
diff --git a/git/util.py b/git/util.py
index f931abe2..eb5a6ac1 100644
--- a/git/util.py
+++ b/git/util.py
@@ -68,7 +68,7 @@ def rmtree(path):
             os.chmod(path, stat.S_IWUSR)
             func(path)
         else:
-            raise
+            raise FileExistsError("Cannot delete '%s'", path)
     # END end onerror
     return shutil.rmtree(path, False, onerror)