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.

rhodecode/lib/dbmigrate/versions/104_version_4_19_0.py

@@ -0,0 +1,52 b''
+# -*- 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

rhodecode/__init__.py

@@ -45,7 +45,7 b' 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'

rhodecode/apps/repository/tests/test_repo_pullrequests.py

@@ -629,7 +629,7 b' 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 b' 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',

rhodecode/apps/repository/views/repo_pull_requests.py

@@ -396,6 +396,7 b' 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 b' 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 b' 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 b' 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 b' 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(
+            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:

rhodecode/lib/utils2.py

@@ -688,14 +688,17 b' 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 b' 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

rhodecode/lib/vcs/backends/base.py

@@ -216,6 +216,7 b' 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 b' 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.

rhodecode/lib/vcs/backends/git/repository.py

@@ -228,12 +228,13 b' 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 b' 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 b' 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 b' 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 b' 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 b' 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 b' 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 b' 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:

rhodecode/lib/vcs/backends/hg/repository.py

@@ -429,7 +429,8 b' 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`.

rhodecode/lib/vcs/backends/svn/repository.py

@@ -276,7 +276,8 b' 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:

rhodecode/model/db.py

@@ -2339,9 +2339,10 b' 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 b' 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 b' 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',

rhodecode/model/pull_request.py

@@ -885,6 +885,7 b' 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 b' 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 b' 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 b' class PullRequestModel(BaseModel):'
                     metadata.update({
                         'heads': heads
                     })
+
             merge_state = MergeResponse(
                 possible, False, None, pull_request.last_merge_status, metadata=metadata)
 
@@ -1487,6 +1490,8 b' 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 b' 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 b' 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 '<MergeCheck(possible:{}, failed:{}, errors:{})>'.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 b' 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)

rhodecode/tests/models/test_pullrequest.py

@@ -169,7 +169,7 b' 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 b' 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 b' 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 b' 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 b' 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 b' 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 b' 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 b' 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 b' 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.'