pull-requests: fix way how pull-request calculates common ancestors.

- fixes  #5561
- added new preview for size (commits/files) of PRs before opening, this is now based on the special logic that calculates common ancestor and has access to preview diff
- store common ancestor in DB so updates of pull-requests are consistent

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

@@ -0,0 +1,47 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_19_0_0 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('common_ancestor_id', Unicode(255), nullable=True)
+        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('common_ancestor_id', Unicode(255), nullable=True)
+        batch_op.add_column(new_column)
+
+
+def downgrade(migrate_engine):
+    meta = MetaData()
+    meta.bind = migrate_engine
+
+
+def fixups(models, _SESSION):
+    pass

rhodecode/__init__.py

@@ -48,7 +48,7 b' PYRAMID_SETTINGS = {}'
 EXTENSIONS = {}
 
 __version__ = ('.'.join((str(each) for each in VERSION[:3])))
-__dbversion__ = 106  # defines current db version for migrations
+__dbversion__ = 107  # defines current db version for migrations
 __platform__ = platform.system()
 __license__ = 'AGPLv3, and Commercial License'
 __author__ = 'RhodeCode GmbH'

rhodecode/api/views/pull_request_api.py

@@ -686,28 +686,9 b' def create_pull_request('
     full_source_ref = resolve_ref_or_error(source_ref, source_db_repo)
     full_target_ref = resolve_ref_or_error(target_ref, target_db_repo)
 
-    source_scm = source_db_repo.scm_instance()
-    target_scm = target_db_repo.scm_instance()
-
     source_commit = get_commit_or_error(full_source_ref, source_db_repo)
     target_commit = get_commit_or_error(full_target_ref, target_db_repo)
 
-    ancestor = source_scm.get_common_ancestor(
-        source_commit.raw_id, target_commit.raw_id, target_scm)
-    if not ancestor:
-        raise JSONRPCError('no common ancestor found')
-
-    # recalculate target ref based on ancestor
-    target_ref_type, target_ref_name, __ = full_target_ref.split(':')
-    full_target_ref = ':'.join((target_ref_type, target_ref_name, ancestor))
-
-    commit_ranges = target_scm.compare(
-        target_commit.raw_id, source_commit.raw_id, source_scm,
-        merge=True, pre_load=[])
-
-    if not commit_ranges:
-        raise JSONRPCError('no commits found')
-
     reviewer_objects = Optional.extract(reviewers) or []
 
     # serialize and validate passed in given reviewers
@@ -727,16 +708,16 b' def create_pull_request('
         PullRequestModel().get_reviewer_functions()
 
     # recalculate reviewers logic, to make sure we can validate this
-    reviewer_rules = get_default_reviewers_data(
+    default_reviewers_data = get_default_reviewers_data(
         owner, source_db_repo,
         source_commit, target_db_repo, target_commit)
 
     # now MERGE our given with the calculated
-    reviewer_objects = reviewer_rules['reviewers'] + reviewer_objects
+    reviewer_objects = default_reviewers_data['reviewers'] + reviewer_objects
 
     try:
         reviewers = validate_default_reviewers(
-            reviewer_objects, reviewer_rules)
+            reviewer_objects, default_reviewers_data)
     except ValueError as e:
         raise JSONRPCError('Reviewers Validation: {}'.format(e))
 
@@ -748,6 +729,24 b' def create_pull_request('
             source_ref=title_source_ref,
             target=target_repo
         )
+
+    diff_info = default_reviewers_data['diff_info']
+    common_ancestor_id = diff_info['ancestor']
+    commits = diff_info['commits']
+
+    if not common_ancestor_id:
+        raise JSONRPCError('no common ancestor found')
+
+    if not commits:
+        raise JSONRPCError('no commits found')
+
+    # NOTE(marcink): reversed is consistent with how we open it in the WEB interface
+    revisions = [commit.raw_id for commit in reversed(commits)]
+
+    # recalculate target ref based on ancestor
+    target_ref_type, target_ref_name, __ = full_target_ref.split(':')
+    full_target_ref = ':'.join((target_ref_type, target_ref_name, common_ancestor_id))
+
     # fetch renderer, if set fallback to plain in case of PR
     rc_config = SettingsModel().get_all_settings()
     default_system_renderer = rc_config.get('rhodecode_markup_renderer', 'plain')
@@ -760,12 +759,13 b' def create_pull_request('
         source_ref=full_source_ref,
         target_repo=target_repo,
         target_ref=full_target_ref,
-        revisions=[commit.raw_id for commit in reversed(commit_ranges)],
+        common_ancestor_id=common_ancestor_id,
+        revisions=revisions,
         reviewers=reviewers,
         title=title,
         description=description,
         description_renderer=description_renderer,
-        reviewer_data=reviewer_rules,
+        reviewer_data=default_reviewers_data,
         auth_user=apiuser
     )
 

rhodecode/apps/repository/tests/test_repo_compare.py

@@ -484,7 +484,7 b' class TestCompareView(object):'
 
         # outgoing commits between those commits
         compare_page = ComparePage(response)
-        compare_page.contains_commits(commits=[commit1], ancestors=[commit0])
+        compare_page.contains_commits(commits=[commit1])
 
     def test_errors_when_comparing_unknown_source_repo(self, backend):
         repo = backend.repo
@@ -641,6 +641,7 b' class ComparePage(AssertResponse):'
             self.contains_one_link(
                 'r%s:%s' % (commit.idx, commit.short_id),
                 self._commit_url(commit))
+
         if ancestors:
             response.mustcontain('Ancestor')
             for ancestor in ancestors:

rhodecode/apps/repository/utils.py

@@ -20,6 +20,9 b''
 
 from rhodecode.lib import helpers as h
 from rhodecode.lib.utils2 import safe_int
+from rhodecode.model.pull_request import get_diff_info
+
+REVIEWER_API_VERSION = 'V3'
 
 
 def reviewer_as_json(user, reasons=None, mandatory=False, rules=None, user_group=None):
@@ -47,15 +50,20 b' def reviewer_as_json(user, reasons=None,'
 
 def get_default_reviewers_data(
         current_user, source_repo, source_commit, target_repo, target_commit):
+    """
+    Return json for default reviewers of a repository
+    """
 
-    """ Return json for default reviewers of a repository """
+    diff_info = get_diff_info(
+        source_repo, source_commit.raw_id, target_repo, target_commit.raw_id)
 
     reasons = ['Default reviewer', 'Repository owner']
     json_reviewers = [reviewer_as_json(
         user=target_repo.user, reasons=reasons, mandatory=False, rules=None)]
 
     return {
-        'api_ver': 'v1',  # define version for later possible schema upgrade
+        'api_ver': REVIEWER_API_VERSION,  # define version for later possible schema upgrade
+        'diff_info': diff_info,
         'reviewers': json_reviewers,
         'rules': {},
         'rules_data': {},

rhodecode/apps/repository/views/repo_pull_requests.py

@@ -40,12 +40,12 b' from rhodecode.lib.auth import ('
     NotAnonymous, CSRFRequired)
 from rhodecode.lib.utils2 import str2bool, safe_str, safe_unicode
 from rhodecode.lib.vcs.backends.base import EmptyCommit, UpdateFailureReason
-from rhodecode.lib.vcs.exceptions import (CommitDoesNotExistError,
-    RepositoryRequirementError, EmptyRepositoryError)
+from rhodecode.lib.vcs.exceptions import (
+    CommitDoesNotExistError, RepositoryRequirementError, EmptyRepositoryError)
 from rhodecode.model.changeset_status import ChangesetStatusModel
 from rhodecode.model.comment import CommentsModel
-from rhodecode.model.db import (func, or_, PullRequest, PullRequestVersion,
-    ChangesetComment, ChangesetStatus, Repository)
+from rhodecode.model.db import (
+    func, or_, PullRequest, ChangesetComment, ChangesetStatus, Repository)
 from rhodecode.model.forms import PullRequestForm
 from rhodecode.model.meta import Session
 from rhodecode.model.pull_request import PullRequestModel, MergeCheck
@@ -210,10 +210,12 b' class RepoPullRequestsView(RepoAppView, '
         return caching_enabled
 
     def _get_diffset(self, source_repo_name, source_repo,
+                     ancestor_commit,
                      source_ref_id, target_ref_id,
                      target_commit, source_commit, diff_limit, file_limit,
                      fulldiff, hide_whitespace_changes, diff_context):
 
+        target_ref_id = ancestor_commit.raw_id
         vcs_diff = PullRequestModel().get_diff(
             source_repo, source_ref_id, target_ref_id,
             hide_whitespace_changes, diff_context)
@@ -278,6 +280,7 b' class RepoPullRequestsView(RepoAppView, '
         _new_state = {
             'created': PullRequest.STATE_CREATED,
         }.get(self.request.GET.get('force_state'))
+
         if c.is_super_admin and _new_state:
             with pull_request.set_state(PullRequest.STATE_UPDATING, final_state=_new_state):
                 h.flash(
@@ -557,7 +560,8 b' class RepoPullRequestsView(RepoAppView, '
                     source_scm,
                     target_commit,
                     target_ref_id,
-                    target_scm, maybe_unreachable=maybe_unreachable)
+                    target_scm,
+                maybe_unreachable=maybe_unreachable)
 
         # register our commit range
         for comm in commit_cache.values():
@@ -591,11 +595,10 b' class RepoPullRequestsView(RepoAppView, '
             has_proper_diff_cache = cached_diff and cached_diff.get('commits')
             if not force_recache and has_proper_diff_cache:
                 c.diffset = cached_diff['diff']
-                (ancestor_commit, commit_cache, missing_requirements,
-                 source_commit, target_commit) = cached_diff['commits']
             else:
                 c.diffset = self._get_diffset(
                     c.source_repo.repo_name, commits_source_repo,
+                    c.ancestor_commit,
                     source_ref_id, target_ref_id,
                     target_commit, source_commit,
                     diff_limit, file_limit, c.fulldiff,
@@ -675,8 +678,10 b' class RepoPullRequestsView(RepoAppView, '
 
             # calculate the diff for commits between versions
             c.commit_changes = []
-            mark = lambda cs, fw: list(
-                h.itertools.izip_longest([], cs, fillvalue=fw))
+
+            def mark(cs, fw):
+                return list(h.itertools.izip_longest([], cs, fillvalue=fw))
+
             for c_type, raw_id in mark(commit_changes.added, 'a') \
                                 + mark(commit_changes.removed, 'r') \
                                 + mark(commit_changes.common, 'c'):
@@ -739,13 +744,14 b' class RepoPullRequestsView(RepoAppView, '
         except RepositoryRequirementError:
             log.warning('Failed to get all required data from repo', exc_info=True)
             missing_requirements = True
-        ancestor_commit = None
+
+        pr_ancestor_id = pull_request_at_ver.common_ancestor_id
+
         try:
-            ancestor_id = source_scm.get_common_ancestor(
-                source_commit.raw_id, target_commit.raw_id, target_scm)
-            ancestor_commit = source_scm.get_commit(ancestor_id)
+            ancestor_commit = source_scm.get_commit(pr_ancestor_id)
         except Exception:
             ancestor_commit = None
+
         return ancestor_commit, commit_cache, missing_requirements, source_commit, target_commit
 
     def assure_not_empty_repo(self):
@@ -949,6 +955,7 b' class RepoPullRequestsView(RepoAppView, '
         target_repo = _form['target_repo']
         target_ref = _form['target_ref']
         commit_ids = _form['revisions'][::-1]
+        common_ancestor_id = _form['common_ancestor']
 
         # find the ancestor for this pr
         source_db_repo = Repository.get_by_repo_name(_form['source_repo'])
@@ -1035,6 +1042,7 b' class RepoPullRequestsView(RepoAppView, '
                 target_repo=target_repo,
                 target_ref=target_ref,
                 revisions=commit_ids,
+                common_ancestor_id=common_ancestor_id,
                 reviewers=reviewers,
                 title=pullrequest_title,
                 description=description,

rhodecode/apps/repository/views/repo_review_rules.py

@@ -54,8 +54,20 b' class RepoReviewRulesView(RepoAppView):'
         renderer='json_ext')
     def repo_default_reviewers_data(self):
         self.load_default_context()
-        target_repo_name = self.request.GET.get('target_repo', self.db_repo.repo_name)
+
+        request = self.request
+        source_repo = self.db_repo
+        source_repo_name = source_repo.repo_name
+        target_repo_name = request.GET.get('target_repo', source_repo_name)
         target_repo = Repository.get_by_repo_name(target_repo_name)
+
+        source_ref = request.GET['source_ref']
+        target_ref = request.GET['target_ref']
+        source_commit = source_repo.get_commit(source_ref)
+        target_commit = target_repo.get_commit(target_ref)
+
+        current_user = request.user.get_instance()
         review_data = get_default_reviewers_data(
-            self.db_repo.user, None, None, target_repo, None)
+            current_user, source_repo, source_commit, target_repo, target_commit)
+
         return review_data

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

@@ -580,6 +580,9 b' class GitRepository(BaseRepository):'
         return len(self.commit_ids)
 
     def get_common_ancestor(self, commit_id1, commit_id2, repo2):
+        log.debug('Calculating common ancestor between %sc1:%s and %sc2:%s',
+                  self, commit_id1, repo2, commit_id2)
+
         if commit_id1 == commit_id2:
             return commit_id1
 
@@ -600,6 +603,8 b' class GitRepository(BaseRepository):'
                 ['merge-base', commit_id1, commit_id2])
             ancestor_id = re.findall(r'[0-9a-fA-F]{40}', output)[0]
 
+        log.debug('Found common ancestor with sha: %s', ancestor_id)
+
         return ancestor_id
 
     def compare(self, commit_id1, commit_id2, repo2, merge, pre_load=None):

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

@@ -297,13 +297,20 b' class MercurialRepository(BaseRepository'
         return update_cache
 
     def get_common_ancestor(self, commit_id1, commit_id2, repo2):
+        log.debug('Calculating common ancestor between %sc1:%s and %sc2:%s',
+                  self, commit_id1, repo2, commit_id2)
+
         if commit_id1 == commit_id2:
             return commit_id1
 
         ancestors = self._remote.revs_from_revspec(
             "ancestor(id(%s), id(%s))", commit_id1, commit_id2,
             other_path=repo2.path)
-        return repo2[ancestors[0]].raw_id if ancestors else None
+
+        ancestor_id = repo2[ancestors[0]].raw_id if ancestors else None
+
+        log.debug('Found common ancestor with sha: %s', ancestor_id)
+        return ancestor_id
 
     def compare(self, commit_id1, commit_id2, repo2, merge, pre_load=None):
         if commit_id1 == commit_id2:

rhodecode/model/db.py

@@ -3989,6 +3989,8 b' class _PullRequestBase(BaseModel):'
     _revisions = Column(
         'revisions', UnicodeText().with_variant(UnicodeText(20500), 'mysql'))
 
+    common_ancestor_id = Column('common_ancestor_id', Unicode(255), nullable=True)
+
     @declared_attr
     def source_repo_id(cls):
         # TODO: dan: rename column to source_repo_id
@@ -4302,7 +4304,7 b' class PullRequest(Base, _PullRequestBase'
         attrs.source_ref_parts = pull_request_obj.source_ref_parts
         attrs.target_ref_parts = pull_request_obj.target_ref_parts
         attrs.revisions = pull_request_obj.revisions
-
+        attrs.common_ancestor_id = pull_request_obj.common_ancestor_id
         attrs.shadow_merge_ref = org_pull_request_obj.shadow_merge_ref
         attrs.reviewer_data = org_pull_request_obj.reviewer_data
         attrs.reviewer_data_json = org_pull_request_obj.reviewer_data_json

rhodecode/model/pull_request.py

@@ -35,6 +35,7 b' import collections'
 from pyramid import compat
 from pyramid.threadlocal import get_current_request
 
+from rhodecode.lib.vcs.nodes import FileNode
 from rhodecode.translation import lazy_ugettext
 from rhodecode.lib import helpers as h, hooks_utils, diffs
 from rhodecode.lib import audit_logger
@@ -82,6 +83,126 b' class UpdateResponse(object):'
         self.target_changed = target_changed
 
 
+def get_diff_info(
+        source_repo, source_ref, target_repo, target_ref, get_authors=False,
+        get_commit_authors=True):
+    """
+    Calculates detailed diff information for usage in preview of creation of a pull-request.
+    This is also used for default reviewers logic
+    """
+
+    source_scm = source_repo.scm_instance()
+    target_scm = target_repo.scm_instance()
+
+    ancestor_id = target_scm.get_common_ancestor(target_ref, source_ref, source_scm)
+    if not ancestor_id:
+        raise ValueError(
+            'cannot calculate diff info without a common ancestor. '
+            'Make sure both repositories are related, and have a common forking commit.')
+
+    # case here is that want a simple diff without incoming commits,
+    # previewing what will be merged based only on commits in the source.
+    log.debug('Using ancestor %s as source_ref instead of %s',
+              ancestor_id, source_ref)
+
+    # source of changes now is the common ancestor
+    source_commit = source_scm.get_commit(commit_id=ancestor_id)
+    # target commit becomes the source ref as it is the last commit
+    # for diff generation this logic gives proper diff
+    target_commit = source_scm.get_commit(commit_id=source_ref)
+
+    vcs_diff = \
+        source_scm.get_diff(commit1=source_commit, commit2=target_commit,
+                            ignore_whitespace=False, context=3)
+
+    diff_processor = diffs.DiffProcessor(
+        vcs_diff, format='newdiff', diff_limit=None,
+        file_limit=None, show_full_diff=True)
+
+    _parsed = diff_processor.prepare()
+
+    all_files = []
+    all_files_changes = []
+    changed_lines = {}
+    stats = [0, 0]
+    for f in _parsed:
+        all_files.append(f['filename'])
+        all_files_changes.append({
+            'filename': f['filename'],
+            'stats': f['stats']
+        })
+        stats[0] += f['stats']['added']
+        stats[1] += f['stats']['deleted']
+
+        changed_lines[f['filename']] = []
+        if len(f['chunks']) < 2:
+            continue
+        # first line is "context" information
+        for chunks in f['chunks'][1:]:
+            for chunk in chunks['lines']:
+                if chunk['action'] not in ('del', 'mod'):
+                    continue
+                changed_lines[f['filename']].append(chunk['old_lineno'])
+
+    commit_authors = []
+    user_counts = {}
+    email_counts = {}
+    author_counts = {}
+    _commit_cache = {}
+
+    commits = []
+    if get_commit_authors:
+        commits = target_scm.compare(
+            target_ref, source_ref, source_scm, merge=True,
+            pre_load=["author"])
+
+        for commit in commits:
+            user = User.get_from_cs_author(commit.author)
+            if user and user not in commit_authors:
+                commit_authors.append(user)
+
+    # lines
+    if get_authors:
+        target_commit = source_repo.get_commit(ancestor_id)
+
+        for fname, lines in changed_lines.items():
+            try:
+                node = target_commit.get_node(fname)
+            except Exception:
+                continue
+
+            if not isinstance(node, FileNode):
+                continue
+
+            for annotation in node.annotate:
+                line_no, commit_id, get_commit_func, line_text = annotation
+                if line_no in lines:
+                    if commit_id not in _commit_cache:
+                        _commit_cache[commit_id] = get_commit_func()
+                    commit = _commit_cache[commit_id]
+                    author = commit.author
+                    email = commit.author_email
+                    user = User.get_from_cs_author(author)
+                    if user:
+                        user_counts[user] = user_counts.get(user, 0) + 1
+                    author_counts[author] = author_counts.get(author, 0) + 1
+                    email_counts[email] = email_counts.get(email, 0) + 1
+
+    return {
+        'commits': commits,
+        'files': all_files_changes,
+        'stats': stats,
+        'ancestor': ancestor_id,
+        # original authors of modified files
+        'original_authors': {
+            'users': user_counts,
+            'authors': author_counts,
+            'emails': email_counts,
+        },
+        'commit_authors': commit_authors
+    }
+
+
 class PullRequestModel(BaseModel):
 
     cls = PullRequest
@@ -453,6 +574,7 b' class PullRequestModel(BaseModel):'
 
     def create(self, created_by, source_repo, source_ref, target_repo,
                target_ref, revisions, reviewers, title, description=None,
+               common_ancestor_id=None,
                description_renderer=None,
                reviewer_data=None, translator=None, auth_user=None):
         translator = translator or get_current_request().translate
@@ -474,6 +596,8 b' class PullRequestModel(BaseModel):'
         pull_request.author = created_by_user
         pull_request.reviewer_data = reviewer_data
         pull_request.pull_request_state = pull_request.STATE_CREATING
+        pull_request.common_ancestor_id = common_ancestor_id
+
         Session().add(pull_request)
         Session().flush()
 
@@ -805,8 +929,17 b' class PullRequestModel(BaseModel):'
             target_commit.raw_id, source_commit.raw_id, source_repo, merge=True,
             pre_load=pre_load)
 
-        ancestor_commit_id = source_repo.get_common_ancestor(
-            source_commit.raw_id, target_commit.raw_id, target_repo)
+        target_ref = target_commit.raw_id
+        source_ref = source_commit.raw_id
+        ancestor_commit_id = target_repo.get_common_ancestor(
+            target_ref, source_ref, source_repo)
+
+        if not ancestor_commit_id:
+            raise ValueError(
+                'cannot calculate diff info without a common ancestor. '
+                'Make sure both repositories are related, and have a common forking commit.')
+
+        pull_request.common_ancestor_id = ancestor_commit_id
 
         pull_request.source_ref = '%s:%s:%s' % (
             source_ref_type, source_ref_name, source_commit.raw_id)
@@ -913,6 +1046,7 b' class PullRequestModel(BaseModel):'
         version.reviewer_data = pull_request.reviewer_data
 
         version.revisions = pull_request.revisions
+        version.common_ancestor_id = pull_request.common_ancestor_id
         version.pull_request = pull_request
         Session().add(version)
         Session().flush()

rhodecode/public/css/main.less

@@ -467,9 +467,9 b' ul.auth_plugins {'
 #pr_open_message {
     border: @border-thickness solid #fff;
     border-radius: @border-radius;
-    padding: @padding-large-vertical @padding-large-vertical @padding-large-vertical 0;
     text-align: left;
     overflow: hidden;
+    white-space: pre-line;
 }
 
 .pr-details-title {
@@ -1796,7 +1796,7 b' table.integrations {'
 }
 
 div.ancestor {
-    line-height: 33px;
+
 }
 
 .cs_icon_td input[type="checkbox"] {

rhodecode/public/css/select2.less

@@ -205,7 +205,9 b' select.select2{height:28px;visibility:hi'
             font-family: @text-regular;
             color: @grey2;
             cursor: pointer;
+            white-space: nowrap;
         }
+
         &.select2-result-with-children {
 
             .select2-result-label {
@@ -220,6 +222,7 b' select.select2{height:28px;visibility:hi'
                 font-family: @text-regular;
                 color: @grey2;
                 cursor: pointer;
+                white-space: nowrap;
             }
         }
     }

rhodecode/public/js/src/i18n_utils.js

@@ -22,7 +22,7 b' var _gettext = function (s) {'
     if (_TM.hasOwnProperty(s)) {
         return _TM[s];
     }
-    i18nLog.error(
+    i18nLog.warning(
         'String `' + s + '` was requested but cannot be ' +
         'found in translation table');
     return s
@@ -34,3 +34,5 b' var _ngettext = function (singular, plur'
     }
     return _gettext(plural)
 };
+
+

rhodecode/public/js/src/rhodecode/pullrequests.js

@@ -75,12 +75,12 b' var getTitleAndDescription = function(so'
   var desc = '';
 
   $.each($(elements).get().reverse().slice(0, limit), function(idx, value) {
-      var rawMessage = $(value).find('td.td-description .message').data('messageRaw').toString();
+      var rawMessage = value['message'];
       desc += '- ' + rawMessage.split('\n')[0].replace(/\n+$/, "") + '\n';
   });
   // only 1 commit, use commit message as title
   if (elements.length === 1) {
-      var rawMessage = $(elements[0]).find('td.td-description .message').data('messageRaw').toString();
+      var rawMessage = elements[0]['message'];
       title = rawMessage.split('\n')[0];
   }
   else {
@@ -92,7 +92,6 b' var getTitleAndDescription = function(so'
 };
 
 
-
 ReviewersController = function () {
     var self = this;
     this.$reviewRulesContainer = $('#review_rules');
@@ -100,28 +99,35 b' ReviewersController = function () {'
     this.forbidReviewUsers = undefined;
     this.$reviewMembers = $('#review_members');
     this.currentRequest = null;
+    this.diffData = null;
+    //dummy handler, we might register our own later
+    this.diffDataHandler = function(data){};
 
-    this.defaultForbidReviewUsers = function() {
+    this.defaultForbidReviewUsers = function () {
         return [
-            {'username': 'default',
-             'user_id': templateContext.default_user.user_id}
+            {
+                'username': 'default',
+                'user_id': templateContext.default_user.user_id
+            }
         ];
     };
 
-    this.hideReviewRules = function() {
+    this.hideReviewRules = function () {
         self.$reviewRulesContainer.hide();
     };
 
-    this.showReviewRules = function() {
+    this.showReviewRules = function () {
         self.$reviewRulesContainer.show();
     };
 
-    this.addRule = function(ruleText) {
+    this.addRule = function (ruleText) {
         self.showReviewRules();
         return '<div>- {0}</div>'.format(ruleText)
     };
 
-    this.loadReviewRules = function(data) {
+    this.loadReviewRules = function (data) {
+        self.diffData = data;
+
         // reset forbidden Users
         this.forbidReviewUsers = self.defaultForbidReviewUsers();
 
@@ -141,7 +147,7 b' ReviewersController = function () {'
             if (data.rules.voting < 0) {
                 self.$rulesList.append(
                     self.addRule(
-                         _gettext('All individual reviewers must vote.'))
+                        _gettext('All individual reviewers must vote.'))
                 )
             } else if (data.rules.voting === 1) {
                 self.$rulesList.append(
@@ -158,7 +164,7 b' ReviewersController = function () {'
         }
 
         if (data.rules.voting_groups !== undefined) {
-            $.each(data.rules.voting_groups, function(index, rule_data) {
+            $.each(data.rules.voting_groups, function (index, rule_data) {
                 self.$rulesList.append(
                     self.addRule(rule_data.text)
                 )
@@ -188,7 +194,7 b' ReviewersController = function () {'
         if (data.rules.forbid_commit_author_to_review) {
 
             if (data.rules_data.forbidden_users) {
-                $.each(data.rules_data.forbidden_users, function(index, member_data) {
+                $.each(data.rules_data.forbidden_users, function (index, member_data) {
                     self.forbidReviewUsers.push(member_data)
                 });
 
@@ -203,11 +209,10 b' ReviewersController = function () {'
         return self.forbidReviewUsers
     };
 
-    this.loadDefaultReviewers = function(sourceRepo, sourceRef, targetRepo, targetRef) {
+    this.loadDefaultReviewers = function (sourceRepo, sourceRef, targetRepo, targetRef) {
 
         if (self.currentRequest) {
-            // make sure we cleanup old running requests before triggering this
-            // again
+            // make sure we cleanup old running requests before triggering this again
             self.currentRequest.abort();
         }
 
@@ -218,6 +223,9 b' ReviewersController = function () {'
         prButtonLock(true, null, 'reviewers');
         $('#user').hide(); // hide user autocomplete before load
 
+        // lock PR button, so we cannot send PR before it's calculated
+        prButtonLock(true, _gettext('Loading diff ...'), 'compare');
+
         if (sourceRef.length !== 3 || targetRef.length !== 3) {
             // don't load defaults in case we're missing some refs...
             $('.calculate-reviewers').hide();
@@ -225,58 +233,80 b' ReviewersController = function () {'
         }
 
         var url = pyroutes.url('repo_default_reviewers_data',
-                {
-                    'repo_name': templateContext.repo_name,
-                    'source_repo': sourceRepo,
-                    'source_ref': sourceRef[2],
-                    'target_repo': targetRepo,
-                    'target_ref': targetRef[2]
-                });
+            {
+                'repo_name': templateContext.repo_name,
+                'source_repo': sourceRepo,
+                'source_ref': sourceRef[2],
+                'target_repo': targetRepo,
+                'target_ref': targetRef[2]
+            });
 
-        self.currentRequest = $.get(url)
-            .done(function(data) {
+        self.currentRequest = $.ajax({
+            url: url,
+            headers: {'X-PARTIAL-XHR': true},
+            type: 'GET',
+            success: function (data) {
+
                 self.currentRequest = null;
 
                 // review rules
                 self.loadReviewRules(data);
+                self.handleDiffData(data["diff_info"]);
 
                 for (var i = 0; i < data.reviewers.length; i++) {
-                  var reviewer = data.reviewers[i];
-                  self.addReviewMember(
-                      reviewer, reviewer.reasons, reviewer.mandatory);
+                    var reviewer = data.reviewers[i];
+                    self.addReviewMember(reviewer, reviewer.reasons, reviewer.mandatory);
                 }
                 $('.calculate-reviewers').hide();
                 prButtonLock(false, null, 'reviewers');
                 $('#user').show(); // show user autocomplete after load
-            });
+
+                var commitElements = data["diff_info"]['commits'];
+                if (commitElements.length === 0) {
+                    prButtonLock(true, _gettext('no commits'), 'all');
+
+                } else {
+                    // un-lock PR button, so we cannot send PR before it's calculated
+                    prButtonLock(false, null, 'compare');
+                }
+
+            },
+            error: function (jqXHR, textStatus, errorThrown) {
+                var prefix = "Loading diff and reviewers failed\n"
+                var message = formatErrorMessage(jqXHR, textStatus, errorThrown, prefix);
+                ajaxErrorSwal(message);
+            }
+        });
+
     };
 
     // check those, refactor
-    this.removeReviewMember = function(reviewer_id, mark_delete) {
+    this.removeReviewMember = function (reviewer_id, mark_delete) {
         var reviewer = $('#reviewer_{0}'.format(reviewer_id));
 
-        if(typeof(mark_delete) === undefined){
+        if (typeof (mark_delete) === undefined) {
             mark_delete = false;
         }
 
-        if(mark_delete === true){
-            if (reviewer){
+        if (mark_delete === true) {
+            if (reviewer) {
                 // now delete the input
                 $('#reviewer_{0} input'.format(reviewer_id)).remove();
                 // mark as to-delete
                 var obj = $('#reviewer_{0}_name'.format(reviewer_id));
                 obj.addClass('to-delete');
-                obj.css({"text-decoration":"line-through", "opacity": 0.5});
+                obj.css({"text-decoration": "line-through", "opacity": 0.5});
             }
-        }
-        else{
+        } else {
             $('#reviewer_{0}'.format(reviewer_id)).remove();
         }
     };
-    this.reviewMemberEntry = function() {
+
+    this.reviewMemberEntry = function () {
 
     };
-    this.addReviewMember = function(reviewer_obj, reasons, mandatory) {
+
+    this.addReviewMember = function (reviewer_obj, reasons, mandatory) {
         var members = self.$reviewMembers.get(0);
         var id = reviewer_obj.user_id;
         var username = reviewer_obj.username;
@@ -287,13 +317,13 b' ReviewersController = function () {'
         // register IDS to check if we don't have this ID already in
         var currentIds = [];
         var _els = self.$reviewMembers.find('li').toArray();
-        for (el in _els){
+        for (el in _els) {
             currentIds.push(_els[el].id)
         }
 
-        var userAllowedReview = function(userId) {
+        var userAllowedReview = function (userId) {
             var allowed = true;
-            $.each(self.forbidReviewUsers, function(index, member_data) {
+            $.each(self.forbidReviewUsers, function (index, member_data) {
                 if (parseInt(userId) === member_data['user_id']) {
                     allowed = false;
                     return false // breaks the loop
@@ -303,35 +333,38 b' ReviewersController = function () {'
         };
 
         var userAllowed = userAllowedReview(id);
-        if (!userAllowed){
-           alert(_gettext('User `{0}` not allowed to be a reviewer').format(username));
+        if (!userAllowed) {
+            alert(_gettext('User `{0}` not allowed to be a reviewer').format(username));
         } else {
             // only add if it's not there
-            var alreadyReviewer = currentIds.indexOf('reviewer_'+id) != -1;
+            var alreadyReviewer = currentIds.indexOf('reviewer_' + id) != -1;
 
             if (alreadyReviewer) {
                 alert(_gettext('User `{0}` already in reviewers').format(username));
             } else {
                 members.innerHTML += renderTemplate('reviewMemberEntry', {
-                        'member': reviewer_obj,
-                        'mandatory': mandatory,
-                        'allowed_to_update': true,
-                        'review_status': 'not_reviewed',
-                        'review_status_label': _gettext('Not Reviewed'),
-                        'reasons': reasons,
-                        'create': true
-                        });
+                    'member': reviewer_obj,
+                    'mandatory': mandatory,
+                    'allowed_to_update': true,
+                    'review_status': 'not_reviewed',
+                    'review_status_label': _gettext('Not Reviewed'),
+                    'reasons': reasons,
+                    'create': true
+                });
                 tooltipActivate();
             }
         }
 
     };
 
-    this.updateReviewers = function(repo_name, pull_request_id){
+    this.updateReviewers = function (repo_name, pull_request_id) {
         var postData = $('#reviewers input').serialize();
         _updatePullRequest(repo_name, pull_request_id, postData);
     };
 
+    this.handleDiffData = function (data) {
+        self.diffDataHandler(data)
+    }
 };
 
 

rhodecode/public/js/src/select2/select2.css

@@ -393,6 +393,7 b' html[dir="rtl"] .select2-results {'
          -moz-user-select: none;
           -ms-user-select: none;
               user-select: none;
+    white-space: nowrap;
 }
 
 .select2-results-dept-1 .select2-result-label { padding-left: 20px }

rhodecode/templates/compare/compare_commits.mako

@@ -2,10 +2,8 b''
 <%namespace name="base" file="/base/base.mako"/>
 
 %if c.ancestor:
-<div class="ancestor">${_('Common Ancestor Commit')}:
-    <a href="${h.route_path('repo_commit', repo_name=c.repo_name, commit_id=c.ancestor)}">
-        ${h.short_id(c.ancestor)}
-    </a>. ${_('Compare was calculated based on this shared commit.')}
+<div class="ancestor">${_('Compare was calculated based on this common ancestor commit')}:
+    <a href="${h.route_path('repo_commit', repo_name=c.repo_name, commit_id=c.ancestor)}">${h.short_id(c.ancestor)}</a>
     <input id="common_ancestor" type="hidden" name="common_ancestor" value="${c.ancestor}">
 </div>
 %endif

rhodecode/templates/pullrequests/pullrequest.mako

@@ -241,7 +241,93 b''
    // custom code mirror
    var codeMirrorInstance = $('#pullrequest_desc').get(0).MarkupForm.cm;
 
+   var diffDataHandler = function(data) {
+
+       $('#pull_request_overview').html(data);
+
+       var commitElements = data['commits'];
+       var files = data['files'];
+       var added = data['stats'][0]
+       var deleted = data['stats'][1]
+       var commonAncestorId = data['ancestor'];
+
+       var prTitleAndDesc = getTitleAndDescription(
+           sourceRef()[1], commitElements, 5);
+
+       var title = prTitleAndDesc[0];
+       var proposedDescription = prTitleAndDesc[1];
+
+       var useGeneratedTitle = (
+            $('#pullrequest_title').hasClass('autogenerated-title') ||
+            $('#pullrequest_title').val() === "");
+
+       if (title && useGeneratedTitle) {
+           // use generated title if we haven't specified our own
+           $('#pullrequest_title').val(title);
+           $('#pullrequest_title').addClass('autogenerated-title');
+
+       }
+
+       var useGeneratedDescription = (
+           !codeMirrorInstance._userDefinedValue ||
+            codeMirrorInstance.getValue() === "");
+
+       if (proposedDescription && useGeneratedDescription) {
+            // set proposed content, if we haven't defined our own,
+            // or we don't have description written
+            codeMirrorInstance._userDefinedValue = false; // reset state
+            codeMirrorInstance.setValue(proposedDescription);
+       }
+
+       // refresh our codeMirror so events kicks in and it's change aware
+       codeMirrorInstance.refresh();
+
+       var url_data = {
+           'repo_name': targetRepo(),
+           'target_repo': sourceRepo(),
+           'source_ref': targetRef()[2],
+           'source_ref_type': 'rev',
+           'target_ref': sourceRef()[2],
+           'target_ref_type': 'rev',
+           'merge': true,
+           '_': Date.now() // bypass browser caching
+       }; // gather the source/target ref and repo here
+       var url = pyroutes.url('repo_compare', url_data);
+
+       var msg = '<input id="common_ancestor" type="hidden" name="common_ancestor" value="{0}">'.format(commonAncestorId);
+       msg += '<input type="hidden" name="__start__" value="revisions:sequence">'
+
+       $.each(commitElements, function(idx, value) {
+           msg += '<input type="hidden" name="revisions" value="{0}">'.format(value["raw_id"]);
+       });
+
+       msg += '<input type="hidden" name="__end__" value="revisions:sequence">'
+       msg += _ngettext(
+           'This pull requests will consist of <strong>{0} commit</strong>.',
+           'This pull requests will consist of <strong>{0} commits</strong>.',
+           commitElements.length).format(commitElements.length)
+
+       msg += '\n';
+       msg += _ngettext(
+           '<strong>{0} file</strong> changed, ',
+           '<strong>{0} files</strong> changed, ',
+           files.length).format(files.length)
+       msg += '<span class="op-added">{0} lines inserted</span>, <span class="op-deleted">{1} lines deleted</span>.'.format(added, deleted)
+
+       msg += '\n\n <a class="" id="pull_request_overview_url" href="{0}" target="_blank">${_('Show detailed compare.')}</a>'.format(url);
+
+       if (commitElements.length) {
+           var commitsLink = '<a href="#pull_request_overview"><strong>{0}</strong></a>'.format(commitElements.length);
+           prButtonLock(false, msg.replace('__COMMITS__', commitsLink), 'compare');
+       }
+       else {
+           prButtonLock(true, "${_('There are no commits to merge.')}", 'compare');
+       }
+
+   };
+
    reviewersController = new ReviewersController();
+   reviewersController.diffDataHandler = diffDataHandler;
 
    var queryTargetRepo = function(self, query) {
        // cache ALL results if query is empty
@@ -286,99 +372,6 b''
        query.callback({results: data.results});
    };
 
-   var loadRepoRefDiffPreview = function() {
-
-       var url_data = {
-           'repo_name': targetRepo(),
-           'target_repo': sourceRepo(),
-           'source_ref': targetRef()[2],
-           'source_ref_type': 'rev',
-           'target_ref': sourceRef()[2],
-           'target_ref_type': 'rev',
-           'merge': true,
-           '_': Date.now() // bypass browser caching
-       }; // gather the source/target ref and repo here
-
-       if (sourceRef().length !== 3 || targetRef().length !== 3) {
-           prButtonLock(true, "${_('Please select source and target')}");
-           return;
-       }
-       var url = pyroutes.url('repo_compare', url_data);
-
-       // lock PR button, so we cannot send PR before it's calculated
-       prButtonLock(true, "${_('Loading compare ...')}", 'compare');
-
-       if (loadRepoRefDiffPreview._currentRequest) {
-           loadRepoRefDiffPreview._currentRequest.abort();
-       }
-
-       loadRepoRefDiffPreview._currentRequest = $.get(url)
-           .error(function(jqXHR, textStatus, errorThrown) {
-                 if (textStatus !== 'abort') {
-                    var prefix = "Error while processing request.\n"
-                    var message = formatErrorMessage(jqXHR, textStatus, errorThrown, prefix);
-                    ajaxErrorSwal(message);
-                 }
-
-           })
-           .done(function(data) {
-               loadRepoRefDiffPreview._currentRequest = null;
-               $('#pull_request_overview').html(data);
-
-               var commitElements = $(data).find('tr[commit_id]');
-
-               var prTitleAndDesc = getTitleAndDescription(
-                   sourceRef()[1], commitElements, 5);
-
-               var title = prTitleAndDesc[0];
-               var proposedDescription = prTitleAndDesc[1];
-
-               var useGeneratedTitle = (
-                    $('#pullrequest_title').hasClass('autogenerated-title') ||
-                    $('#pullrequest_title').val() === "");
-
-               if (title && useGeneratedTitle) {
-                   // use generated title if we haven't specified our own
-                   $('#pullrequest_title').val(title);
-                   $('#pullrequest_title').addClass('autogenerated-title');
-
-               }
-
-               var useGeneratedDescription = (
-                   !codeMirrorInstance._userDefinedValue ||
-                    codeMirrorInstance.getValue() === "");
-
-               if (proposedDescription && useGeneratedDescription) {
-                    // set proposed content, if we haven't defined our own,
-                    // or we don't have description written
-                    codeMirrorInstance._userDefinedValue = false; // reset state
-                    codeMirrorInstance.setValue(proposedDescription);
-               }
-
-               // refresh our codeMirror so events kicks in and it's change aware
-               codeMirrorInstance.refresh();
-
-               var msg = '';
-               if (commitElements.length === 1) {
-                   msg = "${_ungettext('This pull request will consist of __COMMITS__ commit.', 'This pull request will consist of __COMMITS__ commits.', 1)}";
-               } else {
-                   msg = "${_ungettext('This pull request will consist of __COMMITS__ commit.', 'This pull request will consist of __COMMITS__ commits.', 2)}";
-               }
-
-               msg += ' <a id="pull_request_overview_url" href="{0}" target="_blank">${_('Show detailed compare.')}</a>'.format(url);
-
-               if (commitElements.length) {
-                   var commitsLink = '<a href="#pull_request_overview"><strong>{0}</strong></a>'.format(commitElements.length);
-                   prButtonLock(false, msg.replace('__COMMITS__', commitsLink), 'compare');
-               }
-               else {
-                   prButtonLock(true, "${_('There are no commits to merge.')}", 'compare');
-               }
-
-
-           });
-   };
-
    var Select2Box = function(element, overrides) {
      var globalDefaults = {
          dropdownAutoWidth: true,
@@ -476,13 +469,11 b''
    targetRepoSelect2.initRepo(defaultTargetRepo, false);
 
    $sourceRef.on('change', function(e){
-     loadRepoRefDiffPreview();
      reviewersController.loadDefaultReviewers(
          sourceRepo(), sourceRef(), targetRepo(), targetRef());
    });
 
    $targetRef.on('change', function(e){
-     loadRepoRefDiffPreview();
      reviewersController.loadDefaultReviewers(
          sourceRepo(), sourceRef(), targetRepo(), targetRef());
    });
@@ -502,7 +493,6 b''
            success: function(data) {
                $('#target_ref_loading').hide();
                targetRepoChanged(data);
-               loadRepoRefDiffPreview();
            },
            error: function(jqXHR, textStatus, errorThrown) {
                 var prefix = "Error while fetching entries.\n"
@@ -533,8 +523,8 b''
    % if c.default_source_ref:
    // in case we have a pre-selected value, use it now
    $sourceRef.select2('val', '${c.default_source_ref}');
-   // diff preview load
-   loadRepoRefDiffPreview();
+
+
    // default reviewers
    reviewersController.loadDefaultReviewers(
        sourceRepo(), sourceRef(), targetRepo(), targetRef());