# HG changeset patch # User Marcin Kuzminski # Date 2020-04-08 09:17:10 # Node ID 04e45b92100b6d8d63a66c7df0e94077fab90d74 # Parent f479d4dc6149cb97a9198848e02aab61ad274878 pull-requests: fixed case for GIT repositories when a merge check failed due to merge conflicts the pull request wrongly reported missing commits. - we're now searching for dangling commits in a repo that has them and cannot see them because of failed merge checks. - pull-request: will save metadata during merge simulation so merge conflicts are permanently stored, and showed to all users. diff --git a/rhodecode/__init__.py b/rhodecode/__init__.py --- a/rhodecode/__init__.py +++ b/rhodecode/__init__.py @@ -45,7 +45,7 @@ PYRAMID_SETTINGS = {} EXTENSIONS = {} __version__ = ('.'.join((str(each) for each in VERSION[:3]))) -__dbversion__ = 103 # defines current db version for migrations +__dbversion__ = 104 # defines current db version for migrations __platform__ = platform.system() __license__ = 'AGPLv3, and Commercial License' __author__ = 'RhodeCode GmbH' 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 @@ -629,7 +629,7 @@ class TestPullrequestsView(object): model_patcher = mock.patch.multiple( PullRequestModel, merge_repo=mock.Mock(return_value=merge_resp), - merge_status=mock.Mock(return_value=(True, 'WRONG_MESSAGE'))) + merge_status=mock.Mock(return_value=(None, True, 'WRONG_MESSAGE'))) with model_patcher: response = self.app.post( @@ -891,6 +891,8 @@ class TestPullrequestsView(object): vcs = repo.scm_instance() vcs.remove_ref('refs/heads/{}'.format(branch_name)) + # NOTE(marcink): run GC to ensure the commits are gone + vcs.run_gc() response = self.app.get(route_path( 'pullrequest_show', 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 @@ -396,6 +396,7 @@ class RepoPullRequestsView(RepoAppView, pull_request_latest, auth_user=self._rhodecode_user, translator=self.request.translate, force_shadow_repo_refresh=force_refresh) + c.pr_merge_errors = _merge_check.error_details c.pr_merge_possible = not _merge_check.failed c.pr_merge_message = _merge_check.merge_msg @@ -537,6 +538,13 @@ class RepoPullRequestsView(RepoAppView, (ancestor_commit, commit_cache, missing_requirements, source_commit, target_commit) = cached_diff['commits'] else: + # NOTE(marcink): we reach potentially unreachable errors when a PR has + # merge errors resulting in potentially hidden commits in the shadow repo. + maybe_unreachable = _merge_check.MERGE_CHECK in _merge_check.error_details \ + and _merge_check.merge_response + maybe_unreachable = maybe_unreachable \ + and _merge_check.merge_response.metadata.get('unresolved_files') + log.debug("Using unreachable commits due to MERGE_CHECK in merge simulation") diff_commit_cache = \ (ancestor_commit, commit_cache, missing_requirements, source_commit, target_commit) = self.get_commits( @@ -547,7 +555,7 @@ class RepoPullRequestsView(RepoAppView, source_scm, target_commit, target_ref_id, - target_scm) + target_scm, maybe_unreachable=maybe_unreachable) # register our commit range for comm in commit_cache.values(): @@ -698,15 +706,22 @@ class RepoPullRequestsView(RepoAppView, def get_commits( self, commits_source_repo, pull_request_at_ver, source_commit, - source_ref_id, source_scm, target_commit, target_ref_id, target_scm): + source_ref_id, source_scm, target_commit, target_ref_id, target_scm, + maybe_unreachable=False): + commit_cache = collections.OrderedDict() missing_requirements = False + try: pre_load = ["author", "date", "message", "branch", "parents"] - show_revs = pull_request_at_ver.revisions - for rev in show_revs: - comm = commits_source_repo.get_commit( - commit_id=rev, pre_load=pre_load) + + pull_request_commits = pull_request_at_ver.revisions + log.debug('Loading %s commits from %s', + len(pull_request_commits), commits_source_repo) + + for rev in pull_request_commits: + comm = commits_source_repo.get_commit(commit_id=rev, pre_load=pre_load, + maybe_unreachable=maybe_unreachable) commit_cache[comm.raw_id] = comm # Order here matters, we first need to get target, and then @@ -715,14 +730,12 @@ class RepoPullRequestsView(RepoAppView, commit_id=safe_str(target_ref_id)) source_commit = commits_source_repo.get_commit( - commit_id=safe_str(source_ref_id)) + commit_id=safe_str(source_ref_id), maybe_unreachable=True) except CommitDoesNotExistError: - log.warning( - 'Failed to get commit from `{}` repo'.format( - commits_source_repo), exc_info=True) + log.warning('Failed to get commit from `{}` repo'.format( + commits_source_repo), exc_info=True) except RepositoryRequirementError: - log.warning( - 'Failed to get all required data from repo', exc_info=True) + log.warning('Failed to get all required data from repo', exc_info=True) missing_requirements = True ancestor_commit = None try: diff --git a/rhodecode/lib/dbmigrate/versions/104_version_4_19_0.py b/rhodecode/lib/dbmigrate/versions/104_version_4_19_0.py new file mode 100644 --- /dev/null +++ b/rhodecode/lib/dbmigrate/versions/104_version_4_19_0.py @@ -0,0 +1,52 @@ +# -*- coding: utf-8 -*- + +import logging +from sqlalchemy import * + +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import BigInteger + +from rhodecode.lib.dbmigrate.versions import _reset_base +from rhodecode.model import init_model_encryption + + +log = logging.getLogger(__name__) + + +def upgrade(migrate_engine): + """ + Upgrade operations go here. + Don't create your own engine; bind migrate_engine to your metadata + """ + _reset_base(migrate_engine) + from rhodecode.lib.dbmigrate.schema import db_4_18_0_1 as db + + init_model_encryption(db) + + context = MigrationContext.configure(migrate_engine.connect()) + op = Operations(context) + + pull_requests = db.PullRequest.__table__ + + with op.batch_alter_table(pull_requests.name) as batch_op: + new_column = Column( + 'last_merge_metadata', + db.JsonType(dialect_map=dict(mysql=UnicodeText(16384)))) + batch_op.add_column(new_column) + + pull_request_version = db.PullRequestVersion.__table__ + with op.batch_alter_table(pull_request_version.name) as batch_op: + new_column = Column( + 'last_merge_metadata', + db.JsonType(dialect_map=dict(mysql=UnicodeText(16384)))) + batch_op.add_column(new_column) + + +def downgrade(migrate_engine): + meta = MetaData() + meta.bind = migrate_engine + + +def fixups(models, _SESSION): + pass diff --git a/rhodecode/lib/utils2.py b/rhodecode/lib/utils2.py --- a/rhodecode/lib/utils2.py +++ b/rhodecode/lib/utils2.py @@ -688,14 +688,17 @@ def get_clone_url(request, uri_tmpl, rep return safe_unicode(url) -def get_commit_safe(repo, commit_id=None, commit_idx=None, pre_load=None): +def get_commit_safe(repo, commit_id=None, commit_idx=None, pre_load=None, + maybe_unreachable=False): """ Safe version of get_commit if this commit doesn't exists for a repository it returns a Dummy one instead :param repo: repository instance :param commit_id: commit id as str + :param commit_idx: numeric commit index :param pre_load: optional list of commit attributes to load + :param maybe_unreachable: translate unreachable commits on git repos """ # TODO(skreft): remove these circular imports from rhodecode.lib.vcs.backends.base import BaseRepository, EmptyCommit @@ -706,7 +709,8 @@ def get_commit_safe(repo, commit_id=None try: commit = repo.get_commit( - commit_id=commit_id, commit_idx=commit_idx, pre_load=pre_load) + commit_id=commit_id, commit_idx=commit_idx, pre_load=pre_load, + maybe_unreachable=maybe_unreachable) except (RepositoryError, LookupError): commit = EmptyCommit() return commit 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 @@ -216,6 +216,7 @@ class MergeResponse(object): 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: @@ -437,7 +438,8 @@ class BaseRepository(object): self._invalidate_prop_cache('commit_ids') self._is_empty = False - def get_commit(self, commit_id=None, commit_idx=None, pre_load=None, translate_tag=None): + def get_commit(self, commit_id=None, commit_idx=None, pre_load=None, + translate_tag=None, maybe_unreachable=False): """ Returns instance of `BaseCommit` class. If `commit_id` and `commit_idx` are both None, most recent commit is returned. 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 @@ -228,12 +228,13 @@ class GitRepository(BaseRepository): return [] return output.splitlines() - def _lookup_commit(self, commit_id_or_idx, translate_tag=True): + def _lookup_commit(self, commit_id_or_idx, translate_tag=True, maybe_unreachable=False): def is_null(value): return len(value) == commit_id_or_idx.count('0') if commit_id_or_idx in (None, '', 'tip', 'HEAD', 'head', -1): return self.commit_ids[-1] + commit_missing_err = "Commit {} does not exist for `{}`".format( *map(safe_str, [commit_id_or_idx, self.name])) @@ -248,7 +249,8 @@ class GitRepository(BaseRepository): elif is_bstr: # Need to call remote to translate id for tagging scenario try: - remote_data = self._remote.get_object(commit_id_or_idx) + remote_data = self._remote.get_object(commit_id_or_idx, + maybe_unreachable=maybe_unreachable) commit_id_or_idx = remote_data["commit_id"] except (CommitDoesNotExistError,): raise CommitDoesNotExistError(commit_missing_err) @@ -410,7 +412,8 @@ class GitRepository(BaseRepository): except Exception: return - def get_commit(self, commit_id=None, commit_idx=None, pre_load=None, translate_tag=True): + def get_commit(self, commit_id=None, commit_idx=None, pre_load=None, + translate_tag=True, maybe_unreachable=False): """ Returns `GitCommit` object representing commit from git repository at the given `commit_id` or head (most recent commit) if None given. @@ -440,7 +443,7 @@ class GitRepository(BaseRepository): commit_id = "tip" if translate_tag: - commit_id = self._lookup_commit(commit_id) + commit_id = self._lookup_commit(commit_id, maybe_unreachable=maybe_unreachable) try: idx = self._commit_ids[commit_id] @@ -662,6 +665,13 @@ class GitRepository(BaseRepository): self._remote.remove_ref(ref_name) self._invalidate_prop_cache('_refs') + def run_gc(self, prune=True): + cmd = ['gc', '--aggressive'] + if prune: + cmd += ['--prune=now'] + _stdout, stderr = self.run_git_command(cmd, fail_on_stderr=False) + return stderr + def _update_server_info(self): """ runs gits update-server-info command in this repo instance @@ -827,8 +837,7 @@ class GitRepository(BaseRepository): if self.is_empty(): # TODO(skreft): do something more robust in this case. - raise RepositoryError( - 'Do not know how to merge into empty repositories yet') + raise RepositoryError('Do not know how to merge into empty repositories yet') unresolved = None # N.B.(skreft): the --no-ff option is used to enforce the creation of a @@ -836,9 +845,11 @@ class GitRepository(BaseRepository): cmd = ['-c', 'user.name="%s"' % safe_str(user_name), '-c', 'user.email=%s' % safe_str(user_email), 'merge', '--no-ff', '-m', safe_str(merge_message)] - cmd.extend(heads) + + merge_cmd = cmd + heads + try: - output = self.run_git_command(cmd, fail_on_stderr=False) + self.run_git_command(merge_cmd, fail_on_stderr=False) except RepositoryError: files = self.run_git_command(['diff', '--name-only', '--diff-filter', 'U'], fail_on_stderr=False)[0].splitlines() @@ -846,6 +857,7 @@ class GitRepository(BaseRepository): unresolved = ['U {}'.format(f) for f in files] # Cleanup any merge leftovers + self._remote.invalidate_vcs_cache() self.run_git_command(['merge', '--abort'], fail_on_stderr=False) if unresolved: 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 @@ -429,7 +429,8 @@ class MercurialRepository(BaseRepository """ return os.path.join(self.path, '.hg', '.hgrc') - def get_commit(self, commit_id=None, commit_idx=None, pre_load=None, translate_tag=None): + def get_commit(self, commit_id=None, commit_idx=None, pre_load=None, + translate_tag=None, maybe_unreachable=False): """ Returns ``MercurialCommit`` object representing repository's commit at the given `commit_id` or `commit_idx`. diff --git a/rhodecode/lib/vcs/backends/svn/repository.py b/rhodecode/lib/vcs/backends/svn/repository.py --- a/rhodecode/lib/vcs/backends/svn/repository.py +++ b/rhodecode/lib/vcs/backends/svn/repository.py @@ -276,7 +276,8 @@ class SubversionRepository(base.BaseRepo """ return os.path.join(self.path, 'hooks') - def get_commit(self, commit_id=None, commit_idx=None, pre_load=None, translate_tag=None): + def get_commit(self, commit_id=None, commit_idx=None, pre_load=None, + translate_tag=None, maybe_unreachable=False): if self.is_empty(): raise EmptyRepositoryError("There are no commits yet") if commit_id is not None: diff --git a/rhodecode/model/db.py b/rhodecode/model/db.py --- a/rhodecode/model/db.py +++ b/rhodecode/model/db.py @@ -2339,9 +2339,10 @@ class Repository(Base, BaseModel): # SCM PROPERTIES #========================================================================== - def get_commit(self, commit_id=None, commit_idx=None, pre_load=None): + def get_commit(self, commit_id=None, commit_idx=None, pre_load=None, maybe_unreachable=False): return get_commit_safe( - self.scm_instance(), commit_id, commit_idx, pre_load=pre_load) + self.scm_instance(), commit_id, commit_idx, pre_load=pre_load, + maybe_unreachable=maybe_unreachable) def get_changeset(self, rev=None, pre_load=None): warnings.warn("Use get_commit", DeprecationWarning) @@ -4024,6 +4025,10 @@ class _PullRequestBase(BaseModel): _last_merge_target_rev = Column( 'last_merge_other_rev', String(40), nullable=True) _last_merge_status = Column('merge_status', Integer(), nullable=True) + last_merge_metadata = Column( + 'last_merge_metadata', MutationObj.as_mutable( + JsonType(dialect_map=dict(mysql=UnicodeText(16384))))) + merge_rev = Column('merge_rev', String(40), nullable=True) reviewer_data = Column( @@ -4123,10 +4128,11 @@ class _PullRequestBase(BaseModel): pull_request = self if with_merge_state: - merge_status = PullRequestModel().merge_status(pull_request) + merge_response, merge_status, msg = \ + PullRequestModel().merge_status(pull_request) merge_state = { - 'status': merge_status[0], - 'message': safe_unicode(merge_status[1]), + 'status': merge_status, + 'message': safe_unicode(msg), } else: merge_state = {'status': 'not_available', 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 @@ -885,6 +885,7 @@ class PullRequestModel(BaseModel): version._last_merge_source_rev = pull_request._last_merge_source_rev version._last_merge_target_rev = pull_request._last_merge_target_rev version.last_merge_status = pull_request.last_merge_status + version.last_merge_metadata = pull_request.last_merge_metadata version.shadow_merge_ref = pull_request.shadow_merge_ref version.merge_rev = pull_request.merge_rev version.reviewer_data = pull_request.reviewer_data @@ -1349,30 +1350,28 @@ class PullRequestModel(BaseModel): return comment, status - def merge_status(self, pull_request, translator=None, - force_shadow_repo_refresh=False): + def merge_status(self, pull_request, translator=None, force_shadow_repo_refresh=False): _ = translator or get_current_request().translate if not self._is_merge_enabled(pull_request): - return False, _('Server-side pull request merging is disabled.') + return None, False, _('Server-side pull request merging is disabled.') + if pull_request.is_closed(): - return False, _('This pull request is closed.') + return None, False, _('This pull request is closed.') + merge_possible, msg = self._check_repo_requirements( target=pull_request.target_repo, source=pull_request.source_repo, translator=_) if not merge_possible: - return merge_possible, msg + return None, merge_possible, msg try: - resp = self._try_merge( - pull_request, - force_shadow_repo_refresh=force_shadow_repo_refresh) - log.debug("Merge response: %s", resp) - status = resp.possible, resp.merge_status_message + merge_response = self._try_merge( + pull_request, force_shadow_repo_refresh=force_shadow_repo_refresh) + log.debug("Merge response: %s", merge_response) + return merge_response, merge_response.possible, merge_response.merge_status_message except NotImplementedError: - status = False, _('Pull request merging is not supported.') - - return status + return None, False, _('Pull request merging is not supported.') def _check_repo_requirements(self, target, source, translator): """ @@ -1439,6 +1438,9 @@ class PullRequestModel(BaseModel): 'target_ref': pull_request.target_ref_parts, 'source_ref': pull_request.source_ref_parts, } + if pull_request.last_merge_metadata: + metadata.update(pull_request.last_merge_metadata) + if not possible and target_ref.type == 'branch': # NOTE(marcink): case for mercurial multiple heads on branch heads = target_vcs._heads(target_ref.name) @@ -1447,6 +1449,7 @@ class PullRequestModel(BaseModel): metadata.update({ 'heads': heads }) + merge_state = MergeResponse( possible, False, None, pull_request.last_merge_status, metadata=metadata) @@ -1487,6 +1490,8 @@ class PullRequestModel(BaseModel): pull_request.source_ref_parts.commit_id pull_request._last_merge_target_rev = target_reference.commit_id pull_request.last_merge_status = merge_state.failure_reason + pull_request.last_merge_metadata = merge_state.metadata + pull_request.shadow_merge_ref = merge_state.merge_ref Session().add(pull_request) Session().commit() @@ -1627,7 +1632,7 @@ class PullRequestModel(BaseModel): target_commit = source_repo.get_commit( commit_id=safe_str(target_ref_id)) source_commit = source_repo.get_commit( - commit_id=safe_str(source_ref_id)) + commit_id=safe_str(source_ref_id), maybe_unreachable=True) if isinstance(source_repo, Repository): vcs_repo = source_repo.scm_instance() else: @@ -1730,10 +1735,15 @@ class MergeCheck(object): self.review_status = None self.merge_possible = None self.merge_msg = '' + self.merge_response = None self.failed = None self.errors = [] self.error_details = OrderedDict() + def __repr__(self): + return ''.format( + self.merge_possible, self.failed, self.errors) + def push_error(self, error_type, message, error_key, details): self.failed = True self.errors.append([error_type, message]) @@ -1822,11 +1832,14 @@ class MergeCheck(object): return merge_check # merge possible, here is the filesystem simulation + shadow repo - merge_status, msg = PullRequestModel().merge_status( + merge_response, merge_status, msg = PullRequestModel().merge_status( pull_request, translator=translator, force_shadow_repo_refresh=force_shadow_repo_refresh) + merge_check.merge_possible = merge_status merge_check.merge_msg = msg + merge_check.merge_response = merge_response + if not merge_status: log.debug("MergeCheck: cannot merge, pull request merge not possible.") merge_check.push_error('warning', msg, cls.MERGE_CHECK, None) 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 @@ -169,7 +169,7 @@ class TestPullRequestModel(object): assert pull_request._last_merge_target_rev is None assert pull_request.last_merge_status is None - status, msg = PullRequestModel().merge_status(pull_request) + merge_response, status, msg = PullRequestModel().merge_status(pull_request) assert status is True assert msg == 'This pull request can be automatically merged.' self.merge_mock.assert_called_with( @@ -184,7 +184,7 @@ class TestPullRequestModel(object): assert pull_request.last_merge_status is MergeFailureReason.NONE self.merge_mock.reset_mock() - status, msg = PullRequestModel().merge_status(pull_request) + merge_response, status, msg = PullRequestModel().merge_status(pull_request) assert status is True assert msg == 'This pull request can be automatically merged.' assert self.merge_mock.called is False @@ -198,7 +198,7 @@ class TestPullRequestModel(object): assert pull_request._last_merge_target_rev is None assert pull_request.last_merge_status is None - status, msg = PullRequestModel().merge_status(pull_request) + merge_response, status, msg = PullRequestModel().merge_status(pull_request) assert status is False assert msg == 'This pull request cannot be merged because of merge conflicts. file1' self.merge_mock.assert_called_with( @@ -213,9 +213,9 @@ class TestPullRequestModel(object): assert pull_request.last_merge_status is MergeFailureReason.MERGE_FAILED self.merge_mock.reset_mock() - status, msg = PullRequestModel().merge_status(pull_request) + merge_response, status, msg = PullRequestModel().merge_status(pull_request) assert status is False - assert msg == 'This pull request cannot be merged because of merge conflicts. ' + assert msg == 'This pull request cannot be merged because of merge conflicts. file1' assert self.merge_mock.called is False def test_merge_status_unknown_failure(self, pull_request): @@ -227,7 +227,7 @@ class TestPullRequestModel(object): assert pull_request._last_merge_target_rev is None assert pull_request.last_merge_status is None - status, msg = PullRequestModel().merge_status(pull_request) + merge_response, status, msg = PullRequestModel().merge_status(pull_request) assert status is False assert msg == ( 'This pull request cannot be merged because of an unhandled exception. ' @@ -244,7 +244,7 @@ class TestPullRequestModel(object): assert pull_request.last_merge_status is None self.merge_mock.reset_mock() - status, msg = PullRequestModel().merge_status(pull_request) + merge_response, status, msg = PullRequestModel().merge_status(pull_request) assert status is False assert msg == ( 'This pull request cannot be merged because of an unhandled exception. ' @@ -253,7 +253,7 @@ class TestPullRequestModel(object): 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) + merge_response, status, msg = PullRequestModel().merge_status(pull_request) assert status is False assert msg == ( 'This pull request cannot be merged because the target repository ' @@ -266,7 +266,7 @@ class TestPullRequestModel(object): patcher = mock.patch.object(PullRequestModel, '_has_largefiles', has_largefiles) with patcher: - status, msg = PullRequestModel().merge_status(pull_request) + merge_response, status, msg = PullRequestModel().merge_status(pull_request) assert status is False assert msg == 'Target repository large files support is disabled.' @@ -278,7 +278,7 @@ class TestPullRequestModel(object): patcher = mock.patch.object(PullRequestModel, '_has_largefiles', has_largefiles) with patcher: - status, msg = PullRequestModel().merge_status(pull_request) + merge_response, status, msg = PullRequestModel().merge_status(pull_request) assert status is False assert msg == 'Source repository large files support is disabled.'