From 5e6827e98c2732863857c0887d5de4138a8ae48b Mon Sep 17 00:00:00 2001 From: Kostis Anagnostopoulos Date: Thu, 13 Oct 2016 00:42:04 +0200 Subject: [PATCH] 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()`. --- git/cmd.py | 2 +- git/remote.py | 14 +++-- git/test/lib/helper.py | 24 +++----- git/test/test_remote.py | 132 ++++++++++++++++++++++------------------ git/util.py | 2 +- 5 files changed, 91 insertions(+), 83 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index ebf2bd757..e3efb25c5 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -247,7 +247,7 @@ def __del__(self): 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 2b924aafd..71585a41b 100644 --- a/git/remote.py +++ b/git/remote.py @@ -27,7 +27,6 @@ ) 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 @@ def stdout_handler(line): 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 80d1ae7a2..c5a003ea1 100644 --- a/git/test/lib/helper.py +++ b/git/test/lib/helper.py @@ -13,8 +13,9 @@ 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 @@ def wait(self): #{ 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 repo_creator(self): 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 argument_passer(func): @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 7203bee45..8b50ea35c 100644 --- a/git/test/test_remote.py +++ b/git/test/test_remote.py @@ -31,7 +31,7 @@ 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 @@ def make_assertion(self): 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 @@ def _print_fetchhead(self, repo): 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 @@ def get_info(res, remote, name): 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 @@ def get_info(res, remote, name): 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 @@ def get_info(res, remote, name): 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 @@ def _assert_push_and_pull(self, remote, rw_repo, remote_repo): 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 @@ def _assert_push_and_pull(self, remote, rw_repo, remote_repo): 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 @@ def _assert_push_and_pull(self, remote, rw_repo, remote_repo): 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 @@ def _assert_push_and_pull(self, remote, rw_repo, remote_repo): # 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 @@ def _assert_push_and_pull(self, remote, rw_repo, remote_repo): 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 @@ def _assert_push_and_pull(self, remote, rw_repo, remote_repo): 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 @@ def _assert_push_and_pull(self, remote, rw_repo, remote_repo): 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 @@ def test_base(self, rw_repo, remote_repo): 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 @@ def test_base(self, rw_repo, remote_repo): # 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 @@ def test_base(self, rw_repo, remote_repo): 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 @@ def test_base(self, rw_repo, remote_repo): 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 @@ def test_creation_and_removal(self, bare_rw_repo): 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 @@ def test_fetch_info(self): 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 @@ def test_fetch_info(self): 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 @@ def test_fetch_info(self): 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 @@ def test_fetch_info(self): 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 @@ def test_uncommon_branch_names(self): # +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 @@ def test_multiple_urls(self, rw_repo): 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 7f18eed92..d00de1e4b 100644 --- a/git/util.py +++ b/git/util.py @@ -17,7 +17,7 @@ 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