Win, hook, #519: Consume Hook Popen-proc out of GIL

+ HookException thrown on Popen, and were missed on Windows.
+ No SHELL on Popen??
+ Minor fixes:
  + Try harder to delete trees - no remorses.
  + Simplify exception reprs.
  + Unittest-ize test_index assertions.

5 files changed, 124 insertions, 108 deletions

diff --git a/git/compat.py b/git/compat.py
index cbfb5785..d6be6ede 100644
--- a/git/compat.py
+++ b/git/compat.py
@@ -62,7 +62,8 @@ def safe_decode(s):
         return s
     elif isinstance(s, bytes):
         return s.decode(defenc, 'replace')
-    raise TypeError('Expected bytes or text, but got %r' % (s,))
+    elif s is not None:
+        raise TypeError('Expected bytes or text, but got %r' % (s,))
 
 
 def with_metaclass(meta, *bases):
diff --git a/git/exc.py b/git/exc.py
index 3a93c447..37712d11 100644
--- a/git/exc.py
+++ b/git/exc.py
@@ -37,13 +37,9 @@ class GitCommandError(UnicodeMixin, Exception):
         self.command = command
 
     def __unicode__(self):
-        ret = u"'%s' returned with exit code %s" % \
-              (u' '.join(safe_decode(i) for i in self.command), self.status)
-        if self.stderr:
-            ret += u"\nstderr: '%s'" % safe_decode(self.stderr)
-        if self.stdout:
-            ret += u"\nstdout: '%s'" % safe_decode(self.stdout)
-        return ret
+        cmdline = u' '.join(safe_decode(i) for i in self.command)
+        return (u"'%s' returned with exit code %s\n  stdout: '%s'\n  stderr: '%s'"
+                % (cmdline, self.status, safe_decode(self.stdout), safe_decode(self.stderr)))
 
 
 class CheckoutError(Exception):
@@ -80,19 +76,20 @@ class UnmergedEntriesError(CacheError):
     entries in the cache"""
 
 
-class HookExecutionError(Exception):
+class HookExecutionError(UnicodeMixin, Exception):
     """Thrown if a hook exits with a non-zero exit code. It provides access to the exit code and the string returned
     via standard output"""
 
-    def __init__(self, command, status, stdout, stderr):
+    def __init__(self, command, status, stdout=None, stderr=None):
         self.command = command
         self.status = status
         self.stdout = stdout
         self.stderr = stderr
 
-    def __str__(self):
-        return ("'%s' hook returned with exit code %i\nstdout: '%s'\nstderr: '%s'"
-                % (self.command, self.status, self.stdout, self.stderr))
+    def __unicode__(self):
+        cmdline = u' '.join(safe_decode(i) for i in self.command)
+        return (u"'%s' hook failed with %r\n  stdout: '%s'\n  stderr: '%s'"
+                % (cmdline, self.status, safe_decode(self.stdout), safe_decode(self.stderr)))
 
 
 class RepositoryDirtyError(Exception):
diff --git a/git/index/fun.py b/git/index/fun.py
index 80db46b1..0179625a 100644
--- a/git/index/fun.py
+++ b/git/index/fun.py
@@ -14,8 +14,8 @@ from io import BytesIO
 import os
 import subprocess
 
-from git.util import IndexFileSHA1Writer
-from git.cmd import PROC_CREATIONFLAGS
+from git.util import IndexFileSHA1Writer, finalize_process
+from git.cmd import PROC_CREATIONFLAGS, handle_process_output
 from git.exc import (
     UnmergedEntriesError,
     HookExecutionError
@@ -71,21 +71,26 @@ def run_commit_hook(name, index):
     env = os.environ.copy()
     env['GIT_INDEX_FILE'] = index.path
     env['GIT_EDITOR'] = ':'
-    cmd = subprocess.Popen(hp,
-                           env=env,
-                           stdout=subprocess.PIPE,
-                           stderr=subprocess.PIPE,
-                           cwd=index.repo.working_dir,
-                           close_fds=is_posix,
-                           creationflags=PROC_CREATIONFLAGS,)
-    stdout, stderr = cmd.communicate()
-    cmd.stdout.close()
-    cmd.stderr.close()
-
-    if cmd.returncode != 0:
-        stdout = force_text(stdout, defenc)
-        stderr = force_text(stderr, defenc)
-        raise HookExecutionError(hp, cmd.returncode, stdout, stderr)
+    try:
+        cmd = subprocess.Popen(hp,
+                               env=env,
+                               stdout=subprocess.PIPE,
+                               stderr=subprocess.PIPE,
+                               cwd=index.repo.working_dir,
+                               close_fds=is_posix,
+                               creationflags=PROC_CREATIONFLAGS,)
+    except Exception as ex:
+        raise HookExecutionError(hp, ex)
+    else:
+        stdout = []
+        stderr = []
+        handle_process_output(cmd, stdout.append, stderr.append, finalize_process)
+        stdout = ''.join(stdout)
+        stderr = ''.join(stderr)
+        if cmd.returncode != 0:
+            stdout = force_text(stdout, defenc)
+            stderr = force_text(stderr, defenc)
+            raise HookExecutionError(hp, cmd.returncode, stdout, stderr)
     # end handle return code
 
 
diff --git a/git/test/test_index.py b/git/test/test_index.py
index 0e2bc98c..c78890ae 100644
--- a/git/test/test_index.py
+++ b/git/test/test_index.py
@@ -55,9 +55,9 @@ class TestIndex(TestBase):
         self._reset_progress()
 
     def _assert_fprogress(self, entries):
-        assert len(entries) == len(self._fprogress_map)
+        self.assertEqual(len(entries), len(self._fprogress_map))
         for path, call_count in self._fprogress_map.items():
-            assert call_count == 2
+            self.assertEqual(call_count, 2)
         # END for each item in progress map
         self._reset_progress()
 
@@ -107,14 +107,14 @@ class TestIndex(TestBase):
 
         # test stage
         index_merge = IndexFile(self.rorepo, fixture_path("index_merge"))
-        assert len(index_merge.entries) == 106
+        self.assertEqual(len(index_merge.entries), 106)
         assert len(list(e for e in index_merge.entries.values() if e.stage != 0))
 
         # write the data - it must match the original
         tmpfile = tempfile.mktemp()
         index_merge.write(tmpfile)
         fp = open(tmpfile, 'rb')
-        assert fp.read() == fixture("index_merge")
+        self.assertEqual(fp.read(), fixture("index_merge"))
         fp.close()
         os.remove(tmpfile)
 
@@ -206,13 +206,13 @@ class TestIndex(TestBase):
             assert (blob.path, 0) in three_way_index.entries
             num_blobs += 1
         # END for each blob
-        assert num_blobs == len(three_way_index.entries)
+        self.assertEqual(num_blobs, len(three_way_index.entries))
 
     @with_rw_repo('0.1.6')
     def test_index_merge_tree(self, rw_repo):
         # A bit out of place, but we need a different repo for this:
-        assert self.rorepo != rw_repo and not (self.rorepo == rw_repo)
-        assert len(set((self.rorepo, self.rorepo, rw_repo, rw_repo))) == 2
+        self.assertNotEqual(self.rorepo, rw_repo)
+        self.assertEqual(len(set((self.rorepo, self.rorepo, rw_repo, rw_repo))), 2)
 
         # SINGLE TREE MERGE
         # current index is at the (virtual) cur_commit
@@ -225,7 +225,7 @@ class TestIndex(TestBase):
         assert manifest_entry.binsha != rw_repo.index.entries[manifest_key].binsha
 
         rw_repo.index.reset(rw_repo.head)
-        assert rw_repo.index.entries[manifest_key].binsha == manifest_entry.binsha
+        self.assertEqual(rw_repo.index.entries[manifest_key].binsha, manifest_entry.binsha)
 
         # FAKE MERGE
         #############
@@ -243,7 +243,7 @@ class TestIndex(TestBase):
         index = rw_repo.index
         index.entries[manifest_key] = IndexEntry.from_base(manifest_fake_entry)
         index.write()
-        assert rw_repo.index.entries[manifest_key].hexsha == Diff.NULL_HEX_SHA
+        self.assertEqual(rw_repo.index.entries[manifest_key].hexsha, Diff.NULL_HEX_SHA)
 
         # write an unchanged index ( just for the fun of it )
         rw_repo.index.write()
@@ -267,7 +267,8 @@ class TestIndex(TestBase):
         # now make a proper three way merge with unmerged entries
         unmerged_tree = IndexFile.from_tree(rw_repo, parent_commit, tree, next_commit)
         unmerged_blobs = unmerged_tree.unmerged_blobs()
-        assert len(unmerged_blobs) == 1 and list(unmerged_blobs.keys())[0] == manifest_key[0]
+        self.assertEqual(len(unmerged_blobs), 1)
+        self.assertEqual(list(unmerged_blobs.keys())[0], manifest_key[0])
 
     @with_rw_repo('0.1.6')
     def test_index_file_diffing(self, rw_repo):
@@ -289,11 +290,11 @@ class TestIndex(TestBase):
 
         # diff against same index is 0
         diff = index.diff()
-        assert len(diff) == 0
+        self.assertEqual(len(diff), 0)
 
         # against HEAD as string, must be the same as it matches index
         diff = index.diff('HEAD')
-        assert len(diff) == 0
+        self.assertEqual(len(diff), 0)
 
         # against previous head, there must be a difference
         diff = index.diff(cur_head_commit)
@@ -303,7 +304,7 @@ class TestIndex(TestBase):
         adiff = index.diff(str(cur_head_commit), R=True)
         odiff = index.diff(cur_head_commit, R=False)    # now its not reversed anymore
         assert adiff != odiff
-        assert odiff == diff                    # both unreversed diffs against HEAD
+        self.assertEqual(odiff, diff)                    # both unreversed diffs against HEAD
 
         # against working copy - its still at cur_commit
         wdiff = index.diff(None)
@@ -319,8 +320,8 @@ class TestIndex(TestBase):
         rev_head_parent = 'HEAD~1'
         assert index.reset(rev_head_parent) is index
 
-        assert cur_branch == rw_repo.active_branch
-        assert cur_commit == rw_repo.head.commit
+        self.assertEqual(cur_branch, rw_repo.active_branch)
+        self.assertEqual(cur_commit, rw_repo.head.commit)
 
         # there must be differences towards the working tree which is in the 'future'
         assert index.diff(None)
@@ -333,8 +334,8 @@ class TestIndex(TestBase):
         fp.close()
         index.reset(rev_head_parent, working_tree=True)
         assert not index.diff(None)
-        assert cur_branch == rw_repo.active_branch
-        assert cur_commit == rw_repo.head.commit
+        self.assertEqual(cur_branch, rw_repo.active_branch)
+        self.assertEqual(cur_commit, rw_repo.head.commit)
         fp = open(file_path, 'rb')
         try:
             assert fp.read() != new_data
@@ -358,7 +359,7 @@ class TestIndex(TestBase):
         # individual file
         os.remove(test_file)
         rval = index.checkout(test_file, fprogress=self._fprogress)
-        assert list(rval)[0] == 'CHANGES'
+        self.assertEqual(list(rval)[0], 'CHANGES')
         self._assert_fprogress([test_file])
         assert os.path.exists(test_file)
 
@@ -374,9 +375,11 @@ class TestIndex(TestBase):
         try:
             index.checkout(test_file)
         except CheckoutError as e:
-            assert len(e.failed_files) == 1 and e.failed_files[0] == os.path.basename(test_file)
-            assert (len(e.failed_files) == len(e.failed_reasons)) and isinstance(e.failed_reasons[0], string_types)
-            assert len(e.valid_files) == 0
+            self.assertEqual(len(e.failed_files), 1)
+            self.assertEqual(e.failed_files[0], os.path.basename(test_file))
+            self.assertEqual(len(e.failed_files), len(e.failed_reasons))
+            self.assertIsInstance(e.failed_reasons[0], string_types)
+            self.assertEqual(len(e.valid_files), 0)
             assert open(test_file, 'rb').read().endswith(append_data)
         else:
             raise AssertionError("Exception CheckoutError not thrown")
@@ -414,7 +417,7 @@ class TestIndex(TestBase):
         writer.set_value("user", "name", uname)
         writer.set_value("user", "email", umail)
         writer.release()
-        assert writer.get_value("user", "name") == uname
+        self.assertEqual(writer.get_value("user", "name"), uname)
 
         # remove all of the files, provide a wild mix of paths, BaseIndexEntries,
         # IndexEntries
@@ -437,21 +440,21 @@ class TestIndex(TestBase):
         # END mixed iterator
         deleted_files = index.remove(mixed_iterator(), working_tree=False)
         assert deleted_files
-        assert self._count_existing(rw_repo, deleted_files) == len(deleted_files)
-        assert len(index.entries) == 0
+        self.assertEqual(self._count_existing(rw_repo, deleted_files), len(deleted_files))
+        self.assertEqual(len(index.entries), 0)
 
         # reset the index to undo our changes
         index.reset()
-        assert len(index.entries) == num_entries
+        self.assertEqual(len(index.entries), num_entries)
 
         # remove with working copy
         deleted_files = index.remove(mixed_iterator(), working_tree=True)
         assert deleted_files
-        assert self._count_existing(rw_repo, deleted_files) == 0
+        self.assertEqual(self._count_existing(rw_repo, deleted_files), 0)
 
         # reset everything
         index.reset(working_tree=True)
-        assert self._count_existing(rw_repo, deleted_files) == len(deleted_files)
+        self.assertEqual(self._count_existing(rw_repo, deleted_files), len(deleted_files))
 
         # invalid type
         self.failUnlessRaises(TypeError, index.remove, [1])
@@ -468,14 +471,14 @@ class TestIndex(TestBase):
 
         new_commit = index.commit(commit_message, head=False)
         assert cur_commit != new_commit
-        assert new_commit.author.name == uname
-        assert new_commit.author.email == umail
-        assert new_commit.committer.name == uname
-        assert new_commit.committer.email == umail
-        assert new_commit.message == commit_message
-        assert new_commit.parents[0] == cur_commit
-        assert len(new_commit.parents) == 1
-        assert cur_head.commit == cur_commit
+        self.assertEqual(new_commit.author.name, uname)
+        self.assertEqual(new_commit.author.email, umail)
+        self.assertEqual(new_commit.committer.name, uname)
+        self.assertEqual(new_commit.committer.email, umail)
+        self.assertEqual(new_commit.message, commit_message)
+        self.assertEqual(new_commit.parents[0], cur_commit)
+        self.assertEqual(len(new_commit.parents), 1)
+        self.assertEqual(cur_head.commit, cur_commit)
 
         # commit with other actor
         cur_commit = cur_head.commit
@@ -484,15 +487,15 @@ class TestIndex(TestBase):
         my_committer = Actor(u"Committing Frèderic Çaufl€", "committer@example.com")
         commit_actor = index.commit(commit_message, author=my_author, committer=my_committer)
         assert cur_commit != commit_actor
-        assert commit_actor.author.name == u"Frèderic Çaufl€"
-        assert commit_actor.author.email == "author@example.com"
-        assert commit_actor.committer.name == u"Committing Frèderic Çaufl€"
-        assert commit_actor.committer.email == "committer@example.com"
-        assert commit_actor.message == commit_message
-        assert commit_actor.parents[0] == cur_commit
-        assert len(new_commit.parents) == 1
-        assert cur_head.commit == commit_actor
-        assert cur_head.log()[-1].actor == my_committer
+        self.assertEqual(commit_actor.author.name, u"Frèderic Çaufl€")
+        self.assertEqual(commit_actor.author.email, "author@example.com")
+        self.assertEqual(commit_actor.committer.name, u"Committing Frèderic Çaufl€")
+        self.assertEqual(commit_actor.committer.email, "committer@example.com")
+        self.assertEqual(commit_actor.message, commit_message)
+        self.assertEqual(commit_actor.parents[0], cur_commit)
+        self.assertEqual(len(new_commit.parents), 1)
+        self.assertEqual(cur_head.commit, commit_actor)
+        self.assertEqual(cur_head.log()[-1].actor, my_committer)
 
         # commit with author_date and commit_date
         cur_commit = cur_head.commit
@@ -501,25 +504,25 @@ class TestIndex(TestBase):
         new_commit = index.commit(commit_message, author_date="2006-04-07T22:13:13", commit_date="2005-04-07T22:13:13")
         assert cur_commit != new_commit
         print(new_commit.authored_date, new_commit.committed_date)
-        assert new_commit.message == commit_message
-        assert new_commit.authored_date == 1144447993
-        assert new_commit.committed_date == 1112911993
+        self.assertEqual(new_commit.message, commit_message)
+        self.assertEqual(new_commit.authored_date, 1144447993)
+        self.assertEqual(new_commit.committed_date, 1112911993)
 
         # same index, no parents
         commit_message = "index without parents"
         commit_no_parents = index.commit(commit_message, parent_commits=list(), head=True)
-        assert commit_no_parents.message == commit_message
-        assert len(commit_no_parents.parents) == 0
-        assert cur_head.commit == commit_no_parents
+        self.assertEqual(commit_no_parents.message, commit_message)
+        self.assertEqual(len(commit_no_parents.parents), 0)
+        self.assertEqual(cur_head.commit, commit_no_parents)
 
         # same index, multiple parents
         commit_message = "Index with multiple parents\n    commit with another line"
         commit_multi_parent = index.commit(commit_message, parent_commits=(commit_no_parents, new_commit))
-        assert commit_multi_parent.message == commit_message
-        assert len(commit_multi_parent.parents) == 2
-        assert commit_multi_parent.parents[0] == commit_no_parents
-        assert commit_multi_parent.parents[1] == new_commit
-        assert cur_head.commit == commit_multi_parent
+        self.assertEqual(commit_multi_parent.message, commit_message)
+        self.assertEqual(len(commit_multi_parent.parents), 2)
+        self.assertEqual(commit_multi_parent.parents[0], commit_no_parents)
+        self.assertEqual(commit_multi_parent.parents[1], new_commit)
+        self.assertEqual(cur_head.commit, commit_multi_parent)
 
         # re-add all files in lib
         # get the lib folder back on disk, but get an index without it
@@ -538,17 +541,17 @@ class TestIndex(TestBase):
         entries = index.reset(new_commit).add([os.path.join('lib', 'git', '*.py')], fprogress=self._fprogress_add)
         self._assert_entries(entries)
         self._assert_fprogress(entries)
-        assert len(entries) == 14
+        self.assertEqual(len(entries), 14)
 
         # same file
         entries = index.reset(new_commit).add(
             [os.path.join(rw_repo.working_tree_dir, 'lib', 'git', 'head.py')] * 2, fprogress=self._fprogress_add)
         self._assert_entries(entries)
-        assert entries[0].mode & 0o644 == 0o644
+        self.assertEqual(entries[0].mode & 0o644, 0o644)
         # would fail, test is too primitive to handle this case
         # self._assert_fprogress(entries)
         self._reset_progress()
-        assert len(entries) == 2
+        self.assertEqual(len(entries), 2)
 
         # missing path
         self.failUnlessRaises(OSError, index.reset(new_commit).add, ['doesnt/exist/must/raise'])
@@ -558,7 +561,8 @@ class TestIndex(TestBase):
         entries = index.reset(new_commit).add([old_blob], fprogress=self._fprogress_add)
         self._assert_entries(entries)
         self._assert_fprogress(entries)
-        assert index.entries[(old_blob.path, 0)].hexsha == old_blob.hexsha and len(entries) == 1
+        self.assertEqual(index.entries[(old_blob.path, 0)].hexsha, old_blob.hexsha)
+        self.assertEqual(len(entries), 1)
 
         # mode 0 not allowed
         null_hex_sha = Diff.NULL_HEX_SHA
@@ -573,7 +577,8 @@ class TestIndex(TestBase):
             [BaseIndexEntry((0o10644, null_bin_sha, 0, new_file_relapath))], fprogress=self._fprogress_add)
         self._assert_entries(entries)
         self._assert_fprogress(entries)
-        assert len(entries) == 1 and entries[0].hexsha != null_hex_sha
+        self.assertEqual(len(entries), 1)
+        self.assertNotEquals(entries[0].hexsha, null_hex_sha)
 
         # add symlink
         if not is_win:
@@ -585,11 +590,12 @@ class TestIndex(TestBase):
                 entries = index.reset(new_commit).add([link_file], fprogress=self._fprogress_add)
                 self._assert_entries(entries)
                 self._assert_fprogress(entries)
-                assert len(entries) == 1 and S_ISLNK(entries[0].mode)
-                assert S_ISLNK(index.entries[index.entry_key("my_real_symlink", 0)].mode)
+                self.assertEqual(len(entries), 1)
+                self.assertTrue(S_ISLNK(entries[0].mode))
+                self.assertTrue(S_ISLNK(index.entries[index.entry_key("my_real_symlink", 0)].mode))
 
                 # we expect only the target to be written
-                assert index.repo.odb.stream(entries[0].binsha).read().decode('ascii') == target
+                self.assertEqual(index.repo.odb.stream(entries[0].binsha).read().decode('ascii'), target)
 
                 os.remove(link_file)
             # end for each target
@@ -604,7 +610,8 @@ class TestIndex(TestBase):
         self._assert_entries(entries)
         self._assert_fprogress(entries)
         assert entries[0].hexsha != null_hex_sha
-        assert len(entries) == 1 and S_ISLNK(entries[0].mode)
+        self.assertEqual(len(entries), 1)
+        self.assertTrue(S_ISLNK(entries[0].mode))
 
         # assure this also works with an alternate method
         full_index_entry = IndexEntry.from_base(BaseIndexEntry((0o120000, entries[0].binsha, 0, entries[0].path)))
@@ -654,7 +661,7 @@ class TestIndex(TestBase):
         # files into directory - dry run
         paths = ['LICENSE', 'VERSION', 'doc']
         rval = index.move(paths, dry_run=True)
-        assert len(rval) == 2
+        self.assertEqual(len(rval), 2)
         assert os.path.exists(paths[0])
 
         # again, no dry run
@@ -722,11 +729,18 @@ class TestIndex(TestBase):
             try:
                 index.commit("This should fail")
             except HookExecutionError as err:
-                assert err.status == 1
-                assert err.command == hp
-                assert err.stdout == 'stdout\n'
-                assert err.stderr == 'stderr\n'
-                assert str(err)
+                if is_win:
+                    self.assertIsInstance(err.status, WindowsError)
+                    self.assertEqual(err.command, hp)
+                    self.assertIsNone(err.stdout)
+                    self.assertIsNone(err.stderr)
+                    assert str(err)
+                else:
+                    self.assertEqual(err.status, 1)
+                    self.assertEqual(err.command, hp)
+                    self.assertEqual(err.stdout, 'stdout\n')
+                    self.assertEqual(err.stderr, 'stderr\n')
+                    assert str(err)
             else:
                 raise AssertionError("Should have cought a HookExecutionError")
             # end exception handling
@@ -766,7 +780,7 @@ class TestIndex(TestBase):
             count += 1
             index = rw_repo.index.reset(commit)
             orig_tree = commit.tree
-            assert index.write_tree() == orig_tree
+            self.assertEqual(index.write_tree(), orig_tree)
         # END for each commit
 
     def test_index_new(self):
diff --git a/git/util.py b/git/util.py
index eb5a6ac1..9faa8eff 100644
--- a/git/util.py
+++ b/git/util.py
@@ -62,14 +62,12 @@ def rmtree(path):
 
     :note: we use shutil rmtree but adjust its behaviour to see whether files that
         couldn't be deleted are read-only. Windows will not remove them in that case"""
+
     def onerror(func, path, exc_info):
-        if not os.access(path, os.W_OK):
-            # Is the error an access error ?
-            os.chmod(path, stat.S_IWUSR)
-            func(path)
-        else:
-            raise FileExistsError("Cannot delete '%s'", path)
-    # END end onerror
+        # Is the error an access error ?
+        os.chmod(path, stat.S_IWUSR)
+        func(path)  # Will scream if still not possible to delete.
+
     return shutil.rmtree(path, False, onerror)
 
 
@@ -151,6 +149,7 @@ def get_user_id():
 
 def finalize_process(proc, **kwargs):
     """Wait for the process (clone, fetch, pull or push) and handle its errors accordingly"""
+    ## TODO: No close proc-streams??
     proc.wait(**kwargs)
 
 #} END utilities