diff --git a/rhodecode/api/views/pull_request_api.py b/rhodecode/api/views/pull_request_api.py --- a/rhodecode/api/views/pull_request_api.py +++ b/rhodecode/api/views/pull_request_api.py @@ -313,7 +313,7 @@ def merge_pull_request( # In previous versions the merge response directly contained the merge # commit id. It is now contained in the merge reference object. To be # backwards compatible we have to extract it again. - merge_response = merge_response._asdict() + merge_response = merge_response.asdict() merge_response['merge_commit_id'] = merge_response['merge_ref'].commit_id return merge_response diff --git a/rhodecode/apps/repository/tests/test_repo_pullrequests.py b/rhodecode/apps/repository/tests/test_repo_pullrequests.py --- a/rhodecode/apps/repository/tests/test_repo_pullrequests.py +++ b/rhodecode/apps/repository/tests/test_repo_pullrequests.py @@ -32,7 +32,6 @@ from rhodecode.model.pull_request import from rhodecode.model.user import UserModel from rhodecode.tests import ( assert_session_flash, TEST_USER_ADMIN_LOGIN, TEST_USER_REGULAR_LOGIN) -from rhodecode.tests.utils import AssertResponse def route_path(name, params=None, **kwargs): @@ -233,8 +232,7 @@ class TestPullrequestsView(object): route_path('pullrequest_update', repo_name=pull_request.target_repo.repo_name, pull_request_id=pull_request_id), - params={'update_commits': 'true', - 'csrf_token': csrf_token}) + params={'update_commits': 'true', 'csrf_token': csrf_token}) expected_msg = str(PullRequestModel.UPDATE_STATUS_MESSAGES[ UpdateFailureReason.MISSING_SOURCE_REF]) @@ -244,7 +242,8 @@ class TestPullrequestsView(object): from rhodecode.lib.vcs.backends.base import MergeFailureReason pull_request = pr_util.create_pull_request( approved=True, mergeable=True) - pull_request.target_ref = 'branch:invalid-branch:invalid-commit-id' + unicode_reference = u'branch:invalid-branch:invalid-commit-id' + pull_request.target_ref = unicode_reference Session().add(pull_request) Session().commit() @@ -255,12 +254,12 @@ class TestPullrequestsView(object): pull_request_id=pull_request_id) response = self.app.get(pull_request_url) - - assertr = AssertResponse(response) - expected_msg = PullRequestModel.MERGE_STATUS_MESSAGES[ - MergeFailureReason.MISSING_TARGET_REF] - assertr.element_contains( - 'span[data-role="merge-message"]', str(expected_msg)) + target_ref_id = 'invalid-branch' + merge_resp = MergeResponse( + True, True, '', MergeFailureReason.MISSING_TARGET_REF, + metadata={'target_ref': PullRequest.unicode_to_reference(unicode_reference)}) + response.assert_response().element_contains( + 'span[data-role="merge-message"]', merge_resp.merge_status_message) def test_comment_and_close_pull_request_custom_message_approved( self, pr_util, csrf_token, xhr_header): @@ -272,8 +271,8 @@ class TestPullrequestsView(object): self.app.post( route_path('pullrequest_comment_create', - repo_name=pull_request.target_repo.scm_instance().name, - pull_request_id=pull_request_id), + repo_name=pull_request.target_repo.scm_instance().name, + pull_request_id=pull_request_id), params={ 'close_pull_request': '1', 'text': 'Closing a PR', @@ -608,8 +607,7 @@ class TestPullrequestsView(object): response = self.app.post( route_path('pullrequest_merge', - repo_name=repo_name, - pull_request_id=pull_request_id), + repo_name=repo_name, pull_request_id=pull_request_id), params={'csrf_token': csrf_token}).follow() assert response.status_int == 200 @@ -624,10 +622,13 @@ class TestPullrequestsView(object): pull_request_id = pull_request.pull_request_id repo_name = pull_request.target_repo.scm_instance().name + merge_resp = MergeResponse(True, False, 'STUB_COMMIT_ID', + MergeFailureReason.PUSH_FAILED, + metadata={'target': 'shadow repo', + 'merge_commit': 'xxx'}) model_patcher = mock.patch.multiple( PullRequestModel, - merge_repo=mock.Mock(return_value=MergeResponse( - True, False, 'STUB_COMMIT_ID', MergeFailureReason.PUSH_FAILED)), + merge_repo=mock.Mock(return_value=merge_resp), merge_status=mock.Mock(return_value=(True, 'WRONG_MESSAGE'))) with model_patcher: @@ -637,8 +638,10 @@ class TestPullrequestsView(object): pull_request_id=pull_request_id), params={'csrf_token': csrf_token}, status=302) - assert_session_flash(response, PullRequestModel.MERGE_STATUS_MESSAGES[ - MergeFailureReason.PUSH_FAILED]) + merge_resp = MergeResponse(True, True, '', MergeFailureReason.PUSH_FAILED, + metadata={'target': 'shadow repo', + 'merge_commit': 'xxx'}) + assert_session_flash(response, merge_resp.merge_status_message) def test_update_source_revision(self, backend, csrf_token): commits = [ @@ -909,11 +912,11 @@ class TestPullrequestsView(object): pull_request_id=pull_request.pull_request_id)) assert response.status_int == 200 - assert_response = AssertResponse(response) - assert_response.element_contains( + + response.assert_response().element_contains( '#changeset_compare_view_content .alert strong', 'Missing commits') - assert_response.element_contains( + response.assert_response().element_contains( '#changeset_compare_view_content .alert', 'This pull request cannot be displayed, because one or more' ' commits no longer exist in the source repository.') @@ -941,15 +944,15 @@ class TestPullrequestsView(object): pull_request_id=pull_request.pull_request_id)) assert response.status_int == 200 - assert_response = AssertResponse(response) - assert_response.element_contains( + + response.assert_response().element_contains( '#changeset_compare_view_content .alert strong', 'Missing commits') - assert_response.element_contains( + response.assert_response().element_contains( '#changeset_compare_view_content .alert', 'This pull request cannot be displayed, because one or more' ' commits no longer exist in the source repository.') - assert_response.element_contains( + response.assert_response().element_contains( '#update_commits', 'Update commits') @@ -987,8 +990,7 @@ class TestPullrequestsView(object): pull_request_id=pull_request.pull_request_id)) assert response.status_int == 200 - assert_response = AssertResponse(response) - assert_response.element_contains( + response.assert_response().element_contains( '#changeset_compare_view_content .alert strong', 'Missing commits') @@ -1004,12 +1006,11 @@ class TestPullrequestsView(object): repo_name=pull_request.target_repo.scm_instance().name, pull_request_id=pull_request.pull_request_id)) assert response.status_int == 200 - assert_response = AssertResponse(response) - origin = assert_response.get_element('.pr-origininfo .tag') + origin = response.assert_response().get_element('.pr-origininfo .tag') origin_children = origin.getchildren() assert len(origin_children) == 1 - target = assert_response.get_element('.pr-targetinfo .tag') + target = response.assert_response().get_element('.pr-targetinfo .tag') target_children = target.getchildren() assert len(target_children) == 1 @@ -1038,13 +1039,12 @@ class TestPullrequestsView(object): repo_name=pull_request.target_repo.scm_instance().name, pull_request_id=pull_request.pull_request_id)) assert response.status_int == 200 - assert_response = AssertResponse(response) - origin = assert_response.get_element('.pr-origininfo .tag') + origin = response.assert_response().get_element('.pr-origininfo .tag') assert origin.text.strip() == 'bookmark: origin' assert origin.getchildren() == [] - target = assert_response.get_element('.pr-targetinfo .tag') + target = response.assert_response().get_element('.pr-targetinfo .tag') assert target.text.strip() == 'bookmark: target' assert target.getchildren() == [] @@ -1060,13 +1060,12 @@ class TestPullrequestsView(object): repo_name=pull_request.target_repo.scm_instance().name, pull_request_id=pull_request.pull_request_id)) assert response.status_int == 200 - assert_response = AssertResponse(response) - origin = assert_response.get_element('.pr-origininfo .tag') + origin = response.assert_response().get_element('.pr-origininfo .tag') assert origin.text.strip() == 'tag: origin' assert origin.getchildren() == [] - target = assert_response.get_element('.pr-targetinfo .tag') + target = response.assert_response().get_element('.pr-targetinfo .tag') assert target.text.strip() == 'tag: target' assert target.getchildren() == [] @@ -1090,12 +1089,13 @@ class TestPullrequestsView(object): repo_name=target_repo.name, pull_request_id=pr_id)) - assertr = AssertResponse(response) if mergeable: - assertr.element_value_contains('input.pr-mergeinfo', shadow_url) - assertr.element_value_contains('input.pr-mergeinfo ', 'pr-merge') + response.assert_response().element_value_contains( + 'input.pr-mergeinfo', shadow_url) + response.assert_response().element_value_contains( + 'input.pr-mergeinfo ', 'pr-merge') else: - assertr.no_element_exists('.pr-mergeinfo') + response.assert_response().no_element_exists('.pr-mergeinfo') @pytest.mark.usefixtures('app') diff --git a/rhodecode/apps/repository/views/repo_pull_requests.py b/rhodecode/apps/repository/views/repo_pull_requests.py --- a/rhodecode/apps/repository/views/repo_pull_requests.py +++ b/rhodecode/apps/repository/views/repo_pull_requests.py @@ -1181,10 +1181,8 @@ class RepoPullRequestsView(RepoAppView, h.flash(msg, category='success') else: log.debug( - "The merge was not successful. Merge response: %s", - merge_resp) - msg = PullRequestModel().merge_status_message( - merge_resp.failure_reason) + "The merge was not successful. Merge response: %s", merge_resp) + msg = merge_resp.merge_status_message h.flash(msg, category='error') def _update_reviewers(self, pull_request, review_members, reviewer_rules): diff --git a/rhodecode/lib/vcs/backends/base.py b/rhodecode/lib/vcs/backends/base.py --- a/rhodecode/lib/vcs/backends/base.py +++ b/rhodecode/lib/vcs/backends/base.py @@ -21,20 +21,20 @@ """ Base module for all VCS systems """ - -import collections +import os +import re +import time +import shutil import datetime import fnmatch import itertools import logging -import os -import re -import time +import collections import warnings -import shutil from zope.cachedescriptors.property import Lazy as LazyProperty +from rhodecode.translation import lazy_ugettext from rhodecode.lib.utils2 import safe_str, safe_unicode from rhodecode.lib.vcs import connection from rhodecode.lib.vcs.utils import author_name, author_email @@ -54,9 +54,6 @@ FILEMODE_DEFAULT = 0o100644 FILEMODE_EXECUTABLE = 0o100755 Reference = collections.namedtuple('Reference', ('type', 'name', 'commit_id')) -MergeResponse = collections.namedtuple( - 'MergeResponse', - ('possible', 'executed', 'merge_ref', 'failure_reason')) class MergeFailureReason(object): @@ -142,6 +139,92 @@ class UpdateFailureReason(object): MISSING_SOURCE_REF = 5 +class MergeResponse(object): + + # uses .format(**metadata) for variables + MERGE_STATUS_MESSAGES = { + MergeFailureReason.NONE: lazy_ugettext( + u'This pull request can be automatically merged.'), + MergeFailureReason.UNKNOWN: lazy_ugettext( + u'This pull request cannot be merged because of an unhandled exception. ' + u'{exception}'), + MergeFailureReason.MERGE_FAILED: lazy_ugettext( + u'This pull request cannot be merged because of merge conflicts.'), + MergeFailureReason.PUSH_FAILED: lazy_ugettext( + u'This pull request could not be merged because push to ' + u'target:`{target}@{merge_commit}` failed.'), + MergeFailureReason.TARGET_IS_NOT_HEAD: lazy_ugettext( + u'This pull request cannot be merged because the target ' + u'`{target_ref.name}` is not a head.'), + MergeFailureReason.HG_SOURCE_HAS_MORE_BRANCHES: lazy_ugettext( + u'This pull request cannot be merged because the source contains ' + u'more branches than the target.'), + MergeFailureReason.HG_TARGET_HAS_MULTIPLE_HEADS: lazy_ugettext( + u'This pull request cannot be merged because the target ' + u'has multiple heads: `{heads}`.'), + MergeFailureReason.TARGET_IS_LOCKED: lazy_ugettext( + u'This pull request cannot be merged because the target repository is ' + u'locked by {locked_by}.'), + + MergeFailureReason.MISSING_TARGET_REF: lazy_ugettext( + u'This pull request cannot be merged because the target ' + u'reference `{target_ref.name}` is missing.'), + MergeFailureReason.MISSING_SOURCE_REF: lazy_ugettext( + u'This pull request cannot be merged because the source ' + u'reference `{source_ref.name}` is missing.'), + MergeFailureReason.SUBREPO_MERGE_FAILED: lazy_ugettext( + u'This pull request cannot be merged because of conflicts related ' + u'to sub repositories.'), + + # Deprecations + MergeFailureReason._DEPRECATED_MISSING_COMMIT: lazy_ugettext( + u'This pull request cannot be merged because the target or the ' + u'source reference is missing.'), + + } + + def __init__(self, possible, executed, merge_ref, failure_reason, metadata=None): + self.possible = possible + self.executed = executed + self.merge_ref = merge_ref + self.failure_reason = failure_reason + self.metadata = metadata or {} + + def __repr__(self): + return ''.format(self.label, self.failure_reason) + + def __eq__(self, other): + same_instance = isinstance(other, self.__class__) + return same_instance \ + and self.possible == other.possible \ + and self.executed == other.executed \ + and self.failure_reason == other.failure_reason + + @property + def label(self): + label_dict = dict((v, k) for k, v in MergeFailureReason.__dict__.items() if + not k.startswith('_')) + return label_dict.get(self.failure_reason) + + @property + def merge_status_message(self): + """ + Return a human friendly error message for the given merge status code. + """ + msg = safe_unicode(self.MERGE_STATUS_MESSAGES[self.failure_reason]) + try: + return msg.format(**self.metadata) + except Exception: + log.exception('Failed to format %s message', self) + return msg + + def asdict(self): + data = {} + for k in ['possible', 'executed', 'merge_ref', 'failure_reason']: + data[k] = getattr(self, k) + return data + + class BaseRepository(object): """ Base Repository for final backends @@ -501,12 +584,11 @@ class BaseRepository(object): repo_id, workspace_id, target_ref, source_repo, source_ref, message, user_name, user_email, dry_run=dry_run, use_rebase=use_rebase, close_branch=close_branch) - except RepositoryError: - log.exception( - 'Unexpected failure when running merge, dry-run=%s', - dry_run) + except RepositoryError as exc: + log.exception('Unexpected failure when running merge, dry-run=%s', dry_run) return MergeResponse( - False, False, None, MergeFailureReason.UNKNOWN) + False, False, None, MergeFailureReason.UNKNOWN, + metadata={'exception': str(exc)}) def _merge_repo(self, repo_id, workspace_id, target_ref, source_repo, source_ref, merge_message, diff --git a/rhodecode/lib/vcs/backends/git/repository.py b/rhodecode/lib/vcs/backends/git/repository.py --- a/rhodecode/lib/vcs/backends/git/repository.py +++ b/rhodecode/lib/vcs/backends/git/repository.py @@ -911,11 +911,15 @@ class GitRepository(BaseRepository): source_repo, source_ref, merge_message, merger_name, merger_email, dry_run=False, use_rebase=False, close_branch=False): + + log.debug('Executing merge_repo with %s strategy, dry_run mode:%s', + 'rebase' if use_rebase else 'merge', dry_run) if target_ref.commit_id != self.branches[target_ref.name]: log.warning('Target ref %s commit mismatch %s vs %s', target_ref, target_ref.commit_id, self.branches[target_ref.name]) return MergeResponse( - False, False, None, MergeFailureReason.TARGET_IS_NOT_HEAD) + False, False, None, MergeFailureReason.TARGET_IS_NOT_HEAD, + metadata={'target_ref': target_ref}) shadow_repository_path = self._maybe_prepare_merge_workspace( repo_id, workspace_id, target_ref, source_ref) @@ -943,7 +947,8 @@ class GitRepository(BaseRepository): target_ref, target_ref.commit_id, shadow_repo.branches[target_ref.name]) return MergeResponse( - False, False, None, MergeFailureReason.TARGET_IS_NOT_HEAD) + False, False, None, MergeFailureReason.TARGET_IS_NOT_HEAD, + metadata={'target_ref': target_ref}) # calculate new branch pr_branch = shadow_repo._get_new_pr_branch( @@ -954,12 +959,15 @@ class GitRepository(BaseRepository): try: shadow_repo._local_fetch(source_repo.path, source_ref.name) except RepositoryError: - log.exception('Failure when doing local fetch on git shadow repo') + log.exception('Failure when doing local fetch on ' + 'shadow repo: %s', shadow_repo) return MergeResponse( - False, False, None, MergeFailureReason.MISSING_SOURCE_REF) + False, False, None, MergeFailureReason.MISSING_SOURCE_REF, + metadata={'source_ref': source_ref}) merge_ref = None merge_failure_reason = MergeFailureReason.NONE + metadata = {} try: shadow_repo._local_merge(merge_message, merger_name, merger_email, [source_ref.commit_id]) @@ -988,12 +996,15 @@ class GitRepository(BaseRepository): merge_succeeded = True except RepositoryError: log.exception( - 'Failure when doing local push on git shadow repo') + 'Failure when doing local push from the shadow ' + 'repository to the target repository at %s.', self.path) merge_succeeded = False merge_failure_reason = MergeFailureReason.PUSH_FAILED + metadata['target'] = 'git shadow repo' + metadata['merge_commit'] = pr_branch else: merge_succeeded = False return MergeResponse( - merge_possible, merge_succeeded, merge_ref, - merge_failure_reason) + merge_possible, merge_succeeded, merge_ref, merge_failure_reason, + metadata=metadata) diff --git a/rhodecode/lib/vcs/backends/hg/repository.py b/rhodecode/lib/vcs/backends/hg/repository.py --- a/rhodecode/lib/vcs/backends/hg/repository.py +++ b/rhodecode/lib/vcs/backends/hg/repository.py @@ -610,7 +610,7 @@ class MercurialRepository(BaseRepository Returns the commit id of the merge and a boolean indicating if the commit needs to be pushed. """ - self._update(target_ref.commit_id) + self._update(target_ref.commit_id, clean=True) ancestor = self._ancestor(target_ref.commit_id, source_ref.commit_id) is_the_same_branch = self._is_the_same_branch(target_ref, source_ref) @@ -631,7 +631,7 @@ class MercurialRepository(BaseRepository self._remote.rebase( source=source_ref.commit_id, dest=target_ref.commit_id) self._remote.invalidate_vcs_cache() - self._update(bookmark_name) + self._update(bookmark_name, clean=True) return self._identify(), True except RepositoryError: # The rebase-abort may raise another exception which 'hides' @@ -710,18 +710,21 @@ class MercurialRepository(BaseRepository 'rebase' if use_rebase else 'merge', dry_run) if target_ref.commit_id not in self._heads(): return MergeResponse( - False, False, None, MergeFailureReason.TARGET_IS_NOT_HEAD) + False, False, None, MergeFailureReason.TARGET_IS_NOT_HEAD, + metadata={'target_ref': target_ref}) try: - if (target_ref.type == 'branch' and - len(self._heads(target_ref.name)) != 1): + if target_ref.type == 'branch' and len(self._heads(target_ref.name)) != 1: + heads = ','.join(self._heads(target_ref.name)) return MergeResponse( False, False, None, - MergeFailureReason.HG_TARGET_HAS_MULTIPLE_HEADS) + MergeFailureReason.HG_TARGET_HAS_MULTIPLE_HEADS, + metadata={'heads': heads}) except CommitDoesNotExistError: log.exception('Failure when looking up branch heads on hg target') return MergeResponse( - False, False, None, MergeFailureReason.MISSING_TARGET_REF) + False, False, None, MergeFailureReason.MISSING_TARGET_REF, + metadata={'target_ref': target_ref}) shadow_repository_path = self._maybe_prepare_merge_workspace( repo_id, workspace_id, target_ref, source_ref) @@ -730,6 +733,7 @@ class MercurialRepository(BaseRepository log.debug('Pulling in target reference %s', target_ref) self._validate_pull_reference(target_ref) shadow_repo._local_pull(self.path, target_ref) + try: log.debug('Pulling in source reference %s', source_ref) source_repo._validate_pull_reference(source_ref) @@ -737,12 +741,14 @@ class MercurialRepository(BaseRepository except CommitDoesNotExistError: log.exception('Failure when doing local pull on hg shadow repo') return MergeResponse( - False, False, None, MergeFailureReason.MISSING_SOURCE_REF) + False, False, None, MergeFailureReason.MISSING_SOURCE_REF, + metadata={'source_ref': source_ref}) merge_ref = None merge_commit_id = None close_commit_id = None merge_failure_reason = MergeFailureReason.NONE + metadata = {} # enforce that close branch should be used only in case we source from # an actual Branch @@ -758,8 +764,8 @@ class MercurialRepository(BaseRepository target_ref, merger_name, merger_email, source_ref) merge_possible = True except RepositoryError: - log.exception( - 'Failure when doing close branch on hg shadow repo') + log.exception('Failure when doing close branch on ' + 'shadow repo: %s', shadow_repo) merge_possible = False merge_failure_reason = MergeFailureReason.MERGE_FAILED else: @@ -824,19 +830,21 @@ class MercurialRepository(BaseRepository except RepositoryError: log.exception( 'Failure when doing local push from the shadow ' - 'repository to the target repository.') + 'repository to the target repository at %s.', self.path) merge_succeeded = False merge_failure_reason = MergeFailureReason.PUSH_FAILED + metadata['target'] = 'hg shadow repo' + metadata['merge_commit'] = merge_commit_id else: merge_succeeded = True else: merge_succeeded = False return MergeResponse( - merge_possible, merge_succeeded, merge_ref, merge_failure_reason) + merge_possible, merge_succeeded, merge_ref, merge_failure_reason, + metadata=metadata) - def _get_shadow_instance( - self, shadow_repository_path, enable_hooks=False): + def _get_shadow_instance(self, shadow_repository_path, enable_hooks=False): config = self.config.copy() if not enable_hooks: config.clear_section('hooks') diff --git a/rhodecode/model/pull_request.py b/rhodecode/model/pull_request.py --- a/rhodecode/model/pull_request.py +++ b/rhodecode/model/pull_request.py @@ -33,7 +33,7 @@ import collections from pyramid.threadlocal import get_current_request from rhodecode import events -from rhodecode.translation import lazy_ugettext#, _ +from rhodecode.translation import lazy_ugettext from rhodecode.lib import helpers as h, hooks_utils, diffs from rhodecode.lib import audit_logger from rhodecode.lib.compat import OrderedDict @@ -75,43 +75,6 @@ class PullRequestModel(BaseModel): DIFF_CONTEXT = diffs.DEFAULT_CONTEXT - MERGE_STATUS_MESSAGES = { - MergeFailureReason.NONE: lazy_ugettext( - 'This pull request can be automatically merged.'), - MergeFailureReason.UNKNOWN: lazy_ugettext( - 'This pull request cannot be merged because of an unhandled' - ' exception.'), - MergeFailureReason.MERGE_FAILED: lazy_ugettext( - 'This pull request cannot be merged because of merge conflicts.'), - MergeFailureReason.PUSH_FAILED: lazy_ugettext( - 'This pull request could not be merged because push to target' - ' failed.'), - MergeFailureReason.TARGET_IS_NOT_HEAD: lazy_ugettext( - 'This pull request cannot be merged because the target is not a' - ' head.'), - MergeFailureReason.HG_SOURCE_HAS_MORE_BRANCHES: lazy_ugettext( - 'This pull request cannot be merged because the source contains' - ' more branches than the target.'), - MergeFailureReason.HG_TARGET_HAS_MULTIPLE_HEADS: lazy_ugettext( - 'This pull request cannot be merged because the target has' - ' multiple heads.'), - MergeFailureReason.TARGET_IS_LOCKED: lazy_ugettext( - 'This pull request cannot be merged because the target repository' - ' is locked.'), - MergeFailureReason._DEPRECATED_MISSING_COMMIT: lazy_ugettext( - 'This pull request cannot be merged because the target or the ' - 'source reference is missing.'), - MergeFailureReason.MISSING_TARGET_REF: lazy_ugettext( - 'This pull request cannot be merged because the target ' - 'reference is missing.'), - MergeFailureReason.MISSING_SOURCE_REF: lazy_ugettext( - 'This pull request cannot be merged because the source ' - 'reference is missing.'), - MergeFailureReason.SUBREPO_MERGE_FAILED: lazy_ugettext( - 'This pull request cannot be merged because of conflicts related ' - 'to sub repositories.'), - } - UPDATE_STATUS_MESSAGES = { UpdateFailureReason.NONE: lazy_ugettext( 'Pull request update successful.'), @@ -593,8 +556,7 @@ class PullRequestModel(BaseModel): extras['user_agent'] = 'internal-merge' merge_state = self._merge_pull_request(pull_request, user, extras) if merge_state.executed: - log.debug( - "Merge was successful, updating the pull request comments.") + log.debug("Merge was successful, updating the pull request comments.") self._comment_and_close_pr(pull_request, user, merge_state) self._log_audit_action( @@ -1254,8 +1216,7 @@ class PullRequestModel(BaseModel): pull_request, force_shadow_repo_refresh=force_shadow_repo_refresh) log.debug("Merge response: %s", resp) - status = resp.possible, self.merge_status_message( - resp.failure_reason) + status = resp.possible, resp.merge_status_message except NotImplementedError: status = False, _('Pull request merging is not supported.') @@ -1297,21 +1258,23 @@ class PullRequestModel(BaseModel): "Trying out if the pull request %s can be merged. Force_refresh=%s", pull_request.pull_request_id, force_shadow_repo_refresh) target_vcs = pull_request.target_repo.scm_instance() - # Refresh the target reference. try: target_ref = self._refresh_reference( pull_request.target_ref_parts, target_vcs) except CommitDoesNotExistError: merge_state = MergeResponse( - False, False, None, MergeFailureReason.MISSING_TARGET_REF) + False, False, None, MergeFailureReason.MISSING_TARGET_REF, + metadata={'target_ref': pull_request.target_ref_parts}) return merge_state target_locked = pull_request.target_repo.locked if target_locked and target_locked[0]: - log.debug("The target repository is locked.") + locked_by = 'user:{}'.format(target_locked[0]) + log.debug("The target repository is locked by %s.", locked_by) merge_state = MergeResponse( - False, False, None, MergeFailureReason.TARGET_IS_LOCKED) + False, False, None, MergeFailureReason.TARGET_IS_LOCKED, + metadata={'locked_by': locked_by}) elif force_shadow_repo_refresh or self._needs_merge_state_refresh( pull_request, target_ref): log.debug("Refreshing the merge status of the repository.") @@ -1369,12 +1332,6 @@ class PullRequestModel(BaseModel): workspace_id = 'pr-%s' % pull_request.pull_request_id return workspace_id - def merge_status_message(self, status_code): - """ - Return a human friendly error message for the given merge status code. - """ - return self.MERGE_STATUS_MESSAGES[status_code] - def generate_repo_data(self, repo, commit_id=None, branch=None, bookmark=None, translator=None): from rhodecode.model.repo import RepoModel diff --git a/rhodecode/tests/models/test_pullrequest.py b/rhodecode/tests/models/test_pullrequest.py --- a/rhodecode/tests/models/test_pullrequest.py +++ b/rhodecode/tests/models/test_pullrequest.py @@ -50,9 +50,11 @@ class TestPullRequestModel(object): A pull request combined with multiples patches. """ BackendClass = get_backend(backend.alias) + merge_resp = MergeResponse( + False, False, None, MergeFailureReason.UNKNOWN, + metadata={'exception': 'MockError'}) self.merge_patcher = mock.patch.object( - BackendClass, 'merge', return_value=MergeResponse( - False, False, None, MergeFailureReason.UNKNOWN)) + BackendClass, 'merge', return_value=merge_resp) self.workspace_remove_patcher = mock.patch.object( BackendClass, 'cleanup_merge_workspace') @@ -162,7 +164,7 @@ class TestPullRequestModel(object): status, msg = PullRequestModel().merge_status(pull_request) assert status is True - assert msg.eval() == 'This pull request can be automatically merged.' + assert msg == 'This pull request can be automatically merged.' self.merge_mock.assert_called_with( self.repo_id, self.workspace_id, pull_request.target_ref_parts, @@ -177,7 +179,7 @@ class TestPullRequestModel(object): self.merge_mock.reset_mock() status, msg = PullRequestModel().merge_status(pull_request) assert status is True - assert msg.eval() == 'This pull request can be automatically merged.' + assert msg == 'This pull request can be automatically merged.' assert self.merge_mock.called is False def test_merge_status_known_failure(self, pull_request): @@ -190,9 +192,7 @@ class TestPullRequestModel(object): status, msg = PullRequestModel().merge_status(pull_request) assert status is False - assert ( - msg.eval() == - 'This pull request cannot be merged because of merge conflicts.') + assert msg == 'This pull request cannot be merged because of merge conflicts.' self.merge_mock.assert_called_with( self.repo_id, self.workspace_id, pull_request.target_ref_parts, @@ -208,14 +208,13 @@ class TestPullRequestModel(object): self.merge_mock.reset_mock() status, msg = PullRequestModel().merge_status(pull_request) assert status is False - assert ( - msg.eval() == - 'This pull request cannot be merged because of merge conflicts.') + assert msg == 'This pull request cannot be merged because of merge conflicts.' assert self.merge_mock.called is False def test_merge_status_unknown_failure(self, pull_request): self.merge_mock.return_value = MergeResponse( - False, False, None, MergeFailureReason.UNKNOWN) + False, False, None, MergeFailureReason.UNKNOWN, + metadata={'exception': 'MockError'}) assert pull_request._last_merge_source_rev is None assert pull_request._last_merge_target_rev is None @@ -223,9 +222,9 @@ class TestPullRequestModel(object): status, msg = PullRequestModel().merge_status(pull_request) assert status is False - assert msg.eval() == ( - 'This pull request cannot be merged because of an unhandled' - ' exception.') + assert msg == ( + 'This pull request cannot be merged because of an unhandled exception. ' + 'MockError') self.merge_mock.assert_called_with( self.repo_id, self.workspace_id, pull_request.target_ref_parts, @@ -240,18 +239,18 @@ class TestPullRequestModel(object): self.merge_mock.reset_mock() status, msg = PullRequestModel().merge_status(pull_request) assert status is False - assert msg.eval() == ( - 'This pull request cannot be merged because of an unhandled' - ' exception.') + assert msg == ( + 'This pull request cannot be merged because of an unhandled exception. ' + 'MockError') assert self.merge_mock.called is True def test_merge_status_when_target_is_locked(self, pull_request): pull_request.target_repo.locked = [1, u'12345.50', 'lock_web'] status, msg = PullRequestModel().merge_status(pull_request) assert status is False - assert msg.eval() == ( - 'This pull request cannot be merged because the target repository' - ' is locked.') + assert msg == ( + 'This pull request cannot be merged because the target repository ' + 'is locked by user:1.') def test_merge_status_requirements_check_target(self, pull_request): @@ -460,6 +459,46 @@ def test_outdated_comments( outdated_comment_mock.assert_called_with(pull_request) +@pytest.mark.parametrize('mr_type, expected_msg', [ + (MergeFailureReason.NONE, + 'This pull request can be automatically merged.'), + (MergeFailureReason.UNKNOWN, + 'This pull request cannot be merged because of an unhandled exception. CRASH'), + (MergeFailureReason.MERGE_FAILED, + 'This pull request cannot be merged because of merge conflicts.'), + (MergeFailureReason.PUSH_FAILED, + 'This pull request could not be merged because push to target:`some-repo@merge_commit` failed.'), + (MergeFailureReason.TARGET_IS_NOT_HEAD, + 'This pull request cannot be merged because the target `ref_name` is not a head.'), + (MergeFailureReason.HG_SOURCE_HAS_MORE_BRANCHES, + 'This pull request cannot be merged because the source contains more branches than the target.'), + (MergeFailureReason.HG_TARGET_HAS_MULTIPLE_HEADS, + 'This pull request cannot be merged because the target has multiple heads: `a,b,c`.'), + (MergeFailureReason.TARGET_IS_LOCKED, + 'This pull request cannot be merged because the target repository is locked by user:123.'), + (MergeFailureReason.MISSING_TARGET_REF, + 'This pull request cannot be merged because the target reference `ref_name` is missing.'), + (MergeFailureReason.MISSING_SOURCE_REF, + 'This pull request cannot be merged because the source reference `ref_name` is missing.'), + (MergeFailureReason.SUBREPO_MERGE_FAILED, + 'This pull request cannot be merged because of conflicts related to sub repositories.'), + +]) +def test_merge_response_message(mr_type, expected_msg): + merge_ref = Reference('type', 'ref_name', '6126b7bfcc82ad2d3deaee22af926b082ce54cc6') + metadata = { + 'exception': "CRASH", + 'target': 'some-repo', + 'merge_commit': 'merge_commit', + 'target_ref': merge_ref, + 'source_ref': merge_ref, + 'heads': ','.join(['a', 'b', 'c']), + 'locked_by': 'user:123'} + + merge_response = MergeResponse(True, True, merge_ref, mr_type, metadata=metadata) + assert merge_response.merge_status_message == expected_msg + + @pytest.fixture def merge_extras(user_regular): """ diff --git a/rhodecode/tests/plugin.py b/rhodecode/tests/plugin.py --- a/rhodecode/tests/plugin.py +++ b/rhodecode/tests/plugin.py @@ -661,8 +661,7 @@ class Backend(object): def _next_repo_name(self): return u"%s_%s" % ( - self.invalid_repo_name.sub(u'_', self._test_name), - len(self._cleanup_repos)) + self.invalid_repo_name.sub(u'_', self._test_name), len(self._cleanup_repos)) def ensure_file(self, filename, content='Test content\n'): assert self._cleanup_repos, "Avoid writing into vcs_test repos" diff --git a/rhodecode/tests/vcs/test_hg.py b/rhodecode/tests/vcs/test_hg.py --- a/rhodecode/tests/vcs/test_hg.py +++ b/rhodecode/tests/vcs/test_hg.py @@ -681,9 +681,11 @@ TODO: To be written... workspace_id = 'test-merge' assert len(target_repo._heads(branch='default')) == 2 + heads = target_repo._heads(branch='default') expected_merge_response = MergeResponse( False, False, None, - MergeFailureReason.HG_TARGET_HAS_MULTIPLE_HEADS) + MergeFailureReason.HG_TARGET_HAS_MULTIPLE_HEADS, + metadata={'heads': heads}) repo_id = repo_id_generator(target_repo.path) merge_response = target_repo.merge( repo_id, workspace_id, target_ref, source_repo, source_ref, diff --git a/rhodecode/tests/vcs/test_repository.py b/rhodecode/tests/vcs/test_repository.py --- a/rhodecode/tests/vcs/test_repository.py +++ b/rhodecode/tests/vcs/test_repository.py @@ -284,11 +284,9 @@ class TestRepositoryMerge(object): self.source_commit = self.source_repo.get_commit() # This only works for Git and Mercurial default_branch = self.target_repo.DEFAULT_BRANCH_NAME - self.target_ref = Reference( - 'branch', default_branch, self.target_commit.raw_id) - self.source_ref = Reference( - 'branch', default_branch, self.source_commit.raw_id) - self.workspace_id = 'test-merge' + self.target_ref = Reference('branch', default_branch, self.target_commit.raw_id) + self.source_ref = Reference('branch', default_branch, self.source_commit.raw_id) + self.workspace_id = 'test-merge-{}'.format(vcsbackend.alias) self.repo_id = repo_id_generator(self.target_repo.path) def prepare_for_conflict(self, vcsbackend): @@ -300,11 +298,9 @@ class TestRepositoryMerge(object): self.source_commit = self.source_repo.get_commit() # This only works for Git and Mercurial default_branch = self.target_repo.DEFAULT_BRANCH_NAME - self.target_ref = Reference( - 'branch', default_branch, self.target_commit.raw_id) - self.source_ref = Reference( - 'branch', default_branch, self.source_commit.raw_id) - self.workspace_id = 'test-merge' + self.target_ref = Reference('branch', default_branch, self.target_commit.raw_id) + self.source_ref = Reference('branch', default_branch, self.source_commit.raw_id) + self.workspace_id = 'test-merge-{}'.format(vcsbackend.alias) self.repo_id = repo_id_generator(self.target_repo.path) def test_merge_success(self, vcsbackend): @@ -367,10 +363,11 @@ class TestRepositoryMerge(object): # Multiple merges may differ in their commit id. Therefore we set the # commit id to `None` before comparing the merge responses. - merge_response = merge_response._replace( - merge_ref=merge_response.merge_ref._replace(commit_id=None)) - merge_response_update = merge_response_update._replace( - merge_ref=merge_response_update.merge_ref._replace(commit_id=None)) + new_merge_ref = merge_response.merge_ref._replace(commit_id=None) + merge_response.merge_ref = new_merge_ref + + new_update_merge_ref = merge_response_update.merge_ref._replace(commit_id=None) + merge_response_update.merge_ref = new_update_merge_ref assert merge_response == merge_response_update assert merge_response.possible is True @@ -381,6 +378,7 @@ class TestRepositoryMerge(object): @pytest.mark.parametrize('dry_run', [True, False]) def test_merge_conflict(self, vcsbackend, dry_run): self.prepare_for_conflict(vcsbackend) + expected_merge_response = MergeResponse( False, False, None, MergeFailureReason.MERGE_FAILED) @@ -399,12 +397,11 @@ class TestRepositoryMerge(object): def test_merge_target_is_not_head(self, vcsbackend): self.prepare_for_success(vcsbackend) - expected_merge_response = MergeResponse( - False, False, None, MergeFailureReason.TARGET_IS_NOT_HEAD) - target_ref = Reference( self.target_ref.type, self.target_ref.name, '0' * 40) - + expected_merge_response = MergeResponse( + False, False, None, MergeFailureReason.TARGET_IS_NOT_HEAD, + metadata={'target_ref': target_ref}) merge_response = self.target_repo.merge( self.repo_id, self.workspace_id, target_ref, self.source_repo, self.source_ref, dry_run=True) @@ -413,11 +410,12 @@ class TestRepositoryMerge(object): def test_merge_missing_source_reference(self, vcsbackend): self.prepare_for_success(vcsbackend) - expected_merge_response = MergeResponse( - False, False, None, MergeFailureReason.MISSING_SOURCE_REF) source_ref = Reference( self.source_ref.type, 'not_existing', self.source_ref.commit_id) + expected_merge_response = MergeResponse( + False, False, None, MergeFailureReason.MISSING_SOURCE_REF, + metadata={'source_ref': source_ref}) merge_response = self.target_repo.merge( self.repo_id, self.workspace_id, self.target_ref, @@ -429,7 +427,8 @@ class TestRepositoryMerge(object): def test_merge_raises_exception(self, vcsbackend): self.prepare_for_success(vcsbackend) expected_merge_response = MergeResponse( - False, False, None, MergeFailureReason.UNKNOWN) + False, False, None, MergeFailureReason.UNKNOWN, + metadata={'exception': 'ErrorForTest'}) with mock.patch.object(self.target_repo, '_merge_repo', side_effect=RepositoryError()):