remote, #525: FIX BUG push-cmd misses error messages

+ Bug discovered after enabling TC in prev commit and rework of fetch.
+ remote_tc: unitestize assertions.
+ util: DEL unused `_mktemp()`.

5 files changed, 91 insertions, 83 deletions

diff --git a/git/cmd.py b/git/cmd.py
index ebf2bd75..e3efb25c 100644
--- a/git/cmd.py
+++ b/git/cmd.py
@@ -247,7 +247,7 @@ class Git(LazyMixin):
         def __getattr__(self, attr):
             return getattr(self.proc, attr)
 
-        def wait(self, stderr=b''):
+        def wait(self, stderr=b''):  # TODO: Bad choice to mimic `proc.wait()` but with different args.
             """Wait for the process and return its status code.
 
             :param stderr: Previously read value of stderr, in case stderr is already closed.
diff --git a/git/remote.py b/git/remote.py
index 2b924aaf..71585a41 100644
--- a/git/remote.py
+++ b/git/remote.py
@@ -27,7 +27,6 @@ from git.util import (
 )
 from git.util import (
     join_path,
-    finalize_process
 )
 from git.cmd import handle_process_output, Git
 from gitdb.util import join
@@ -681,16 +680,19 @@ class Remote(LazyMixin, Iterable):
             try:
                 output.append(PushInfo._from_line(self, line))
             except ValueError:
-                # if an error happens, additional info is given which we cannot parse
+                # If an error happens, additional info is given which we parse below.
                 pass
-            # END exception handling
-        # END for each line
 
+        handle_process_output(proc, stdout_handler, progress_handler, finalizer=None, decode_streams=False)
+        stderr_text = progress.error_lines and '\n'.join(progress.error_lines) or ''
         try:
-            handle_process_output(proc, stdout_handler, progress_handler, finalize_process, decode_streams=False)
+            proc.wait(stderr=stderr_text)
         except Exception:
-            if len(output) == 0:
+            if not output:
                 raise
+            elif stderr_text:
+                log.warning("Error lines received while fetching: %s", stderr_text)
+
         return output
 
     def _assert_refspec(self):
diff --git a/git/test/lib/helper.py b/git/test/lib/helper.py
index 80d1ae7a..c5a003ea 100644
--- a/git/test/lib/helper.py
+++ b/git/test/lib/helper.py
@@ -13,8 +13,9 @@ import tempfile
 import textwrap
 import time
 from unittest import TestCase
+import unittest
 
-from git.compat import string_types, is_win
+from git.compat import string_types, is_win, PY3
 from git.util import rmtree
 
 import os.path as osp
@@ -68,18 +69,6 @@ class StringProcessAdapter(object):
 #{ Decorators
 
 
-def _mktemp(*args):
-    """Wrapper around default tempfile.mktemp to fix an osx issue
-    :note: the OSX special case was removed as it was unclear why that was needed in the first place. It seems
-    to be just fine without it. However, if we leave this special case, and if TMPDIR is set to something custom,
-    prefixing /private/ will lead to incorrect paths on OSX."""
-    tdir = tempfile.mktemp(*args)
-    # See :note: above to learn why this is comented out.
-    # if is_darwin:
-    #     tdir = '/private' + tdir
-    return tdir
-
-
 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"""
@@ -129,7 +118,7 @@ def with_rw_repo(working_tree_ref, bare=False):
             if bare:
                 prefix = ''
             # END handle prefix
-            repo_dir = _mktemp("%sbare_%s" % (prefix, func.__name__))
+            repo_dir = tempfile.mktemp("%sbare_%s" % (prefix, func.__name__))
             rw_repo = self.rorepo.clone(repo_dir, shared=True, bare=bare, n=True)
 
             rw_repo.head.commit = rw_repo.commit(working_tree_ref)
@@ -222,8 +211,8 @@ def with_rw_and_rw_remote_repo(working_tree_ref):
 
         @wraps(func)
         def remote_repo_creator(self):
-            remote_repo_dir = _mktemp("remote_repo_%s" % func.__name__)
-            repo_dir = _mktemp("remote_clone_non_bare_repo")
+            remote_repo_dir = tempfile.mktemp("remote_repo_%s" % func.__name__)
+            repo_dir = tempfile.mktemp("remote_clone_non_bare_repo")
 
             rw_remote_repo = self.rorepo.clone(remote_repo_dir, shared=True, bare=True)
             # recursive alternates info ?
@@ -340,6 +329,9 @@ class TestBase(TestCase):
       of the project history ( to assure tests don't fail for others ).
     """
 
+    if not PY3:
+        assertRaisesRegex = unittest.TestCase.assertRaisesRegexp
+
     def _small_repo_url(self):
         """:return" a path to a small, clonable repository"""
         from git.cmd import Git
diff --git a/git/test/test_remote.py b/git/test/test_remote.py
index 7203bee4..8b50ea35 100644
--- a/git/test/test_remote.py
+++ b/git/test/test_remote.py
@@ -31,7 +31,7 @@ from git.test.lib import (
     GIT_DAEMON_PORT,
     assert_raises
 )
-from git.util import IterableList, rmtree, HIDE_WINDOWS_FREEZE_ERRORS, HIDE_WINDOWS_KNOWN_ERRORS
+from git.util import IterableList, rmtree, HIDE_WINDOWS_FREEZE_ERRORS
 import os.path as osp
 
 
@@ -90,7 +90,7 @@ class TestRemoteProgress(RemoteProgress):
             return
 
         # sometimes objects are not compressed which is okay
-        assert len(self._seen_ops) in (2, 3)
+        assert len(self._seen_ops) in (2, 3), len(self._seen_ops)
         assert self._stages_per_op
 
         # must have seen all stages
@@ -114,40 +114,42 @@ class TestRemote(TestBase):
 
     def _do_test_fetch_result(self, results, remote):
         # self._print_fetchhead(remote.repo)
-        assert len(results) > 0 and isinstance(results[0], FetchInfo)
+        self.assertGreater(len(results), 0)
+        self.assertIsInstance(results[0], FetchInfo)
         for info in results:
-            assert isinstance(info.note, string_types)
+            self.assertIsInstance(info.note, string_types)
             if isinstance(info.ref, Reference):
-                assert info.flags != 0
+                self.assertTrue(info.flags)
             # END reference type flags handling
-            assert isinstance(info.ref, (SymbolicReference, Reference))
+            self.assertIsInstance(info.ref, (SymbolicReference, Reference))
             if info.flags & (info.FORCED_UPDATE | info.FAST_FORWARD):
-                assert isinstance(info.old_commit, Commit)
+                self.assertIsInstance(info.old_commit, Commit)
             else:
-                assert info.old_commit is None
+                self.assertIsNone(info.old_commit)
             # END forced update checking
         # END for each info
 
     def _do_test_push_result(self, results, remote):
-        assert len(results) > 0 and isinstance(results[0], PushInfo)
+        self.assertGreater(len(results), 0)
+        self.assertIsInstance(results[0], PushInfo)
         for info in results:
-            assert info.flags
-            assert isinstance(info.summary, string_types)
+            self.assertTrue(info.flags)
+            self.assertIsInstance(info.summary, string_types)
             if info.old_commit is not None:
-                assert isinstance(info.old_commit, Commit)
+                self.assertIsInstance(info.old_commit, Commit)
             if info.flags & info.ERROR:
                 has_one = False
                 for bitflag in (info.REJECTED, info.REMOTE_REJECTED, info.REMOTE_FAILURE):
                     has_one |= bool(info.flags & bitflag)
                 # END for each bitflag
-                assert has_one
+                self.assertTrue(has_one)
             else:
                 # there must be a remote commit
                 if info.flags & info.DELETED == 0:
-                    assert isinstance(info.local_ref, Reference)
+                    self.assertIsInstance(info.local_ref, Reference)
                 else:
-                    assert info.local_ref is None
-                assert type(info.remote_ref) in (TagReference, RemoteReference)
+                    self.assertIsNone(info.local_ref)
+                self.assertIn(type(info.remote_ref), (TagReference, RemoteReference))
             # END error checking
         # END for each info
 
@@ -187,7 +189,7 @@ class TestRemote(TestBase):
         res = fetch_and_test(remote)
         # all up to date
         for info in res:
-            assert info.flags & info.HEAD_UPTODATE
+            self.assertTrue(info.flags & info.HEAD_UPTODATE)
 
         # rewind remote head to trigger rejection
         # index must be false as remote is a bare repo
@@ -197,18 +199,19 @@ class TestRemote(TestBase):
         res = fetch_and_test(remote)
         mkey = "%s/%s" % (remote, 'master')
         master_info = res[mkey]
-        assert master_info.flags & FetchInfo.FORCED_UPDATE and master_info.note is not None
+        self.assertTrue(master_info.flags & FetchInfo.FORCED_UPDATE)
+        self.assertIsNotNone(master_info.note)
 
         # normal fast forward - set head back to previous one
         rhead.commit = remote_commit
         res = fetch_and_test(remote)
-        assert res[mkey].flags & FetchInfo.FAST_FORWARD
+        self.assertTrue(res[mkey].flags & FetchInfo.FAST_FORWARD)
 
         # new remote branch
         new_remote_branch = Head.create(remote_repo, "new_branch")
         res = fetch_and_test(remote)
         new_branch_info = get_info(res, remote, new_remote_branch)
-        assert new_branch_info.flags & FetchInfo.NEW_HEAD
+        self.assertTrue(new_branch_info.flags & FetchInfo.NEW_HEAD)
 
         # remote branch rename ( causes creation of a new one locally )
         new_remote_branch.rename("other_branch_name")
@@ -248,14 +251,14 @@ class TestRemote(TestBase):
         tinfo = res[str(rtag)]
         self.assertIsInstance(tinfo.ref, TagReference)
         self.assertEqual(tinfo.ref.commit, rtag.commit)
-        assert tinfo.flags & tinfo.NEW_TAG
+        self.assertTrue(tinfo.flags & tinfo.NEW_TAG)
 
         # adjust tag commit
         Reference.set_object(rtag, rhead.commit.parents[0].parents[0])
         res = fetch_and_test(remote, tags=True)
         tinfo = res[str(rtag)]
         self.assertEqual(tinfo.commit, rtag.commit)
-        assert tinfo.flags & tinfo.TAG_UPDATE
+        self.assertTrue(tinfo.flags & tinfo.TAG_UPDATE)
 
         # delete remote tag - local one will stay
         TagReference.delete(remote_repo, rtag)
@@ -317,21 +320,21 @@ class TestRemote(TestBase):
         self._commit_random_file(rw_repo)
         progress = TestRemoteProgress()
         res = remote.push(lhead.reference, progress)
-        assert isinstance(res, IterableList)
+        self.assertIsInstance(res, IterableList)
         self._do_test_push_result(res, remote)
         progress.make_assertion()
 
         # rejected - undo last commit
         lhead.reset("HEAD~1")
         res = remote.push(lhead.reference)
-        assert res[0].flags & PushInfo.ERROR
-        assert res[0].flags & PushInfo.REJECTED
+        self.assertTrue(res[0].flags & PushInfo.ERROR)
+        self.assertTrue(res[0].flags & PushInfo.REJECTED)
         self._do_test_push_result(res, remote)
 
         # force rejected pull
         res = remote.push('+%s' % lhead.reference)
         self.assertEqual(res[0].flags & PushInfo.ERROR, 0)
-        assert res[0].flags & PushInfo.FORCED_UPDATE
+        self.assertTrue(res[0].flags & PushInfo.FORCED_UPDATE)
         self._do_test_push_result(res, remote)
 
         # invalid refspec
@@ -343,7 +346,7 @@ class TestRemote(TestBase):
         new_tag = TagReference.create(rw_repo, to_be_updated)  # @UnusedVariable
         other_tag = TagReference.create(rw_repo, "my_obj_tag.2.1aRV", message="my message")
         res = remote.push(progress=progress, tags=True)
-        assert res[-1].flags & PushInfo.NEW_TAG
+        self.assertTrue(res[-1].flags & PushInfo.NEW_TAG)
         progress.make_assertion()
         self._do_test_push_result(res, remote)
 
@@ -352,7 +355,8 @@ class TestRemote(TestBase):
         new_tag = TagReference.create(rw_repo, to_be_updated, ref='HEAD~1', force=True)
         res = remote.push(tags=True)
         self._do_test_push_result(res, remote)
-        assert res[-1].flags & PushInfo.REJECTED and res[-1].flags & PushInfo.ERROR
+        self.assertTrue(res[-1].flags & PushInfo.REJECTED)
+        self.assertTrue(res[-1].flags & PushInfo.ERROR)
 
         # push force this tag
         res = remote.push("+%s" % new_tag.path)
@@ -362,7 +366,7 @@ class TestRemote(TestBase):
         # delete tag - have to do it using refspec
         res = remote.push(":%s" % new_tag.path)
         self._do_test_push_result(res, remote)
-        assert res[0].flags & PushInfo.DELETED
+        self.assertTrue(res[0].flags & PushInfo.DELETED)
         # Currently progress is not properly transferred, especially not using
         # the git daemon
         # progress.assert_received_message()
@@ -371,8 +375,8 @@ class TestRemote(TestBase):
         new_head = Head.create(rw_repo, "my_new_branch")
         progress = TestRemoteProgress()
         res = remote.push(new_head, progress)
-        assert len(res) > 0
-        assert res[0].flags & PushInfo.NEW_HEAD
+        self.assertGreater(len(res), 0)
+        self.assertTrue(res[0].flags & PushInfo.NEW_HEAD)
         progress.make_assertion()
         self._do_test_push_result(res, remote)
 
@@ -380,7 +384,7 @@ class TestRemote(TestBase):
         res = remote.push(":%s" % new_head.path)
         self._do_test_push_result(res, remote)
         Head.delete(rw_repo, new_head)
-        assert res[-1].flags & PushInfo.DELETED
+        self.assertTrue(res[-1].flags & PushInfo.DELETED)
 
         # --all
         res = remote.push(all=True)
@@ -393,7 +397,7 @@ class TestRemote(TestBase):
         TagReference.delete(rw_repo, new_tag, other_tag)
         remote.push(":%s" % other_tag.path)
 
-    @skipIf(HIDE_WINDOWS_KNOWN_ERRORS, "FIXME: Freezes!")
+    @skipIf(HIDE_WINDOWS_FREEZE_ERRORS, "FIXME: Freezes!")
     @with_rw_and_rw_remote_repo('0.1.6')
     def test_base(self, rw_repo, remote_repo):
         num_remotes = 0
@@ -402,17 +406,16 @@ class TestRemote(TestBase):
 
         for remote in rw_repo.remotes:
             num_remotes += 1
-            assert remote == remote
-            assert str(remote) != repr(remote)
+            self.assertEqual(remote, remote)
+            self.assertNotEqual(str(remote), repr(remote))
             remote_set.add(remote)
             remote_set.add(remote)  # should already exist
-
             # REFS
             refs = remote.refs
-            assert refs
+            self.assertTrue(refs)
             for ref in refs:
-                assert ref.remote_name == remote.name
-                assert ref.remote_head
+                self.assertEqual(ref.remote_name, remote.name)
+                self.assertTrue(ref.remote_head)
             # END for each ref
 
             # OPTIONS
@@ -439,11 +442,11 @@ class TestRemote(TestBase):
             # RENAME
             other_name = "totally_other_name"
             prev_name = remote.name
-            assert remote.rename(other_name) == remote
-            assert prev_name != remote.name
+            self.assertEqual(remote.rename(other_name), remote)
+            self.assertNotEqual(prev_name, remote.name)
             # multiple times
             for _ in range(2):
-                assert remote.rename(prev_name).name == prev_name
+                self.assertEqual(remote.rename(prev_name).name, prev_name)
             # END for each rename ( back to prev_name )
 
             # PUSH/PULL TESTING
@@ -460,9 +463,9 @@ class TestRemote(TestBase):
             remote.update()
         # END for each remote
 
-        assert ran_fetch_test
-        assert num_remotes
-        assert num_remotes == len(remote_set)
+        self.assertTrue(ran_fetch_test)
+        self.assertTrue(num_remotes)
+        self.assertEqual(num_remotes, len(remote_set))
 
         origin = rw_repo.remote('origin')
         assert origin == rw_repo.remotes.origin
@@ -482,8 +485,8 @@ class TestRemote(TestBase):
                 num_deleted += 1
             # end
         # end for each branch
-        assert num_deleted > 0
-        assert len(rw_repo.remotes.origin.fetch(prune=True)) == 1, "deleted everything but master"
+        self.assertGreater(num_deleted, 0)
+        self.assertEqual(len(rw_repo.remotes.origin.fetch(prune=True)), 1, "deleted everything but master")
 
     @with_rw_repo('HEAD', bare=True)
     def test_creation_and_removal(self, bare_rw_repo):
@@ -491,14 +494,14 @@ class TestRemote(TestBase):
         arg_list = (new_name, "git@server:hello.git")
         remote = Remote.create(bare_rw_repo, *arg_list)
         self.assertEqual(remote.name, "test_new_one")
-        assert remote in bare_rw_repo.remotes
-        assert remote.exists()
+        self.assertIn(remote, bare_rw_repo.remotes)
+        self.assertTrue(remote.exists())
 
         # create same one again
         self.failUnlessRaises(GitCommandError, Remote.create, bare_rw_repo, *arg_list)
 
         Remote.remove(bare_rw_repo, new_name)
-        assert remote.exists()      # We still have a cache that doesn't know we were deleted by name
+        self.assertTrue(remote.exists())      # We still have a cache that doesn't know we were deleted by name
         remote._clear_cache()
         assert not remote.exists()  # Cache should be renewed now. This is an issue ...
 
@@ -534,16 +537,16 @@ class TestRemote(TestBase):
                                   remote_info_line_fmt % "subdir/tagname",
                                   fetch_info_line_fmt % 'tag')
 
-        assert isinstance(fi.ref, TagReference)
-        assert fi.ref.path.startswith('refs/tags')
+        self.assertIsInstance(fi.ref, TagReference)
+        assert fi.ref.path.startswith('refs/tags'), fi.ref.path
 
         # it could be in a remote direcftory though
         fi = FetchInfo._from_line(self.rorepo,
                                   remote_info_line_fmt % "remotename/tags/tagname",
                                   fetch_info_line_fmt % 'tag')
 
-        assert isinstance(fi.ref, TagReference)
-        assert fi.ref.path.startswith('refs/remotes/')
+        self.assertIsInstance(fi.ref, TagReference)
+        assert fi.ref.path.startswith('refs/remotes/'), fi.ref.path
 
         # it can also be anywhere !
         tag_path = "refs/something/remotename/tags/tagname"
@@ -551,7 +554,7 @@ class TestRemote(TestBase):
                                   remote_info_line_fmt % tag_path,
                                   fetch_info_line_fmt % 'tag')
 
-        assert isinstance(fi.ref, TagReference)
+        self.assertIsInstance(fi.ref, TagReference)
         self.assertEqual(fi.ref.path, tag_path)
 
         # branches default to refs/remotes
@@ -559,7 +562,7 @@ class TestRemote(TestBase):
                                   remote_info_line_fmt % "remotename/branch",
                                   fetch_info_line_fmt % 'branch')
 
-        assert isinstance(fi.ref, RemoteReference)
+        self.assertIsInstance(fi.ref, RemoteReference)
         self.assertEqual(fi.ref.remote_name, 'remotename')
 
         # but you can force it anywhere, in which case we only have a references
@@ -567,7 +570,7 @@ class TestRemote(TestBase):
                                   remote_info_line_fmt % "refs/something/branch",
                                   fetch_info_line_fmt % 'branch')
 
-        assert type(fi.ref) is Reference
+        assert type(fi.ref) is Reference, type(fi.ref)
         self.assertEqual(fi.ref.path, "refs/something/branch")
 
     def test_uncommon_branch_names(self):
@@ -578,10 +581,10 @@ class TestRemote(TestBase):
         # +refs/pull/*:refs/heads/pull/*
         res = [FetchInfo._from_line('ShouldntMatterRepo', stderr, fetch_line)
                for stderr, fetch_line in zip(stderr_lines, fetch_lines)]
-        assert len(res)
+        self.assertGreater(len(res), 0)
         self.assertEqual(res[0].remote_ref_path, 'refs/pull/1/head')
         self.assertEqual(res[0].ref.path, 'refs/heads/pull/1/head')
-        assert isinstance(res[0].ref, Head)
+        self.assertIsInstance(res[0].ref, Head)
 
     @with_rw_repo('HEAD', bare=False)
     def test_multiple_urls(self, rw_repo):
@@ -626,3 +629,14 @@ class TestRemote(TestBase):
         self.assertEqual(list(remote.urls), [test3])
         # will raise fatal: Will not delete all non-push URLs
         assert_raises(GitCommandError, remote.delete_url, test3)
+
+    def test_fetch_error(self):
+        rem = self.rorepo.remote('origin')
+        with self.assertRaisesRegex(GitCommandError, "Couldn't find remote ref __BAD_REF__"):
+            rem.fetch('__BAD_REF__')
+
+    @with_rw_repo('0.1.6', bare=False)
+    def test_push_error(self, repo):
+        rem = repo.remote('origin')
+        with self.assertRaisesRegex(GitCommandError, "src refspec __BAD_REF__ does not match any"):
+            rem.push('__BAD_REF__')
diff --git a/git/util.py b/git/util.py
index 7f18eed9..d00de1e4 100644
--- a/git/util.py
+++ b/git/util.py
@@ -17,7 +17,7 @@ import time
 from functools import wraps
 
 from git.compat import is_win
-from gitdb.util import (# NOQA
+from gitdb.util import (# NOQA @IgnorePep8
     make_sha,
     LockedFD,               # @UnusedImport
     file_contents_ro,       # @UnusedImport