pr-versioning: implemented versioning for pull requests.

- users can navigate via each version or generate diffs between versions
- diff between versions will show changed commits and partial diffs
- redone the UI for versioning a bit
- small UI fixes

rhodecode/controllers/pullrequests.py

@@ -66,113 +66,10 b' log = logging.getLogger(__name__)'
 
 
 class PullrequestsController(BaseRepoController):
+
     def __before__(self):
         super(PullrequestsController, self).__before__()
 
-    def _load_compare_data(self, pull_request, inline_comments):
-        """
-        Load context data needed for generating compare diff
-
-        :param pull_request: object related to the request
-        :param enable_comments: flag to determine if comments are included
-        """
-        source_repo = pull_request.source_repo
-        source_ref_id = pull_request.source_ref_parts.commit_id
-
-        target_repo = pull_request.target_repo
-        target_ref_id = pull_request.target_ref_parts.commit_id
-
-        # despite opening commits for bookmarks/branches/tags, we always
-        # convert this to rev to prevent changes after bookmark or branch change
-        c.source_ref_type = 'rev'
-        c.source_ref = source_ref_id
-
-        c.target_ref_type = 'rev'
-        c.target_ref = target_ref_id
-
-        c.source_repo = source_repo
-        c.target_repo = target_repo
-
-        c.fulldiff = bool(request.GET.get('fulldiff'))
-
-        # diff_limit is the old behavior, will cut off the whole diff
-        # if the limit is applied  otherwise will just hide the
-        # big files from the front-end
-        diff_limit = self.cut_off_limit_diff
-        file_limit = self.cut_off_limit_file
-
-        pre_load = ["author", "branch", "date", "message"]
-
-        c.commit_ranges = []
-        source_commit = EmptyCommit()
-        target_commit = EmptyCommit()
-        c.missing_requirements = False
-        try:
-            c.commit_ranges = [
-                source_repo.get_commit(commit_id=rev, pre_load=pre_load)
-                for rev in pull_request.revisions]
-
-            c.statuses = source_repo.statuses(
-                [x.raw_id for x in c.commit_ranges])
-
-            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))
-        except RepositoryRequirementError:
-            c.missing_requirements = True
-
-        # auto collapse if we have more than limit
-        collapse_limit = diffs.DiffProcessor._collapse_commits_over
-        c.collapse_all_commits = len(c.commit_ranges) > collapse_limit
-
-        c.changes = {}
-        c.missing_commits = False
-        if (c.missing_requirements or
-                isinstance(source_commit, EmptyCommit) or
-                    source_commit == target_commit):
-            _parsed = []
-            c.missing_commits = True
-        else:
-            vcs_diff = PullRequestModel().get_diff(pull_request)
-            diff_processor = diffs.DiffProcessor(
-                vcs_diff, format='newdiff', diff_limit=diff_limit,
-                file_limit=file_limit, show_full_diff=c.fulldiff)
-
-            _parsed = diff_processor.prepare()
-            c.limited_diff = isinstance(_parsed, diffs.LimitedDiffContainer)
-
-            included_files = {}
-            for f in _parsed:
-                included_files[f['filename']] = f['stats']
-
-            c.deleted_files = [fname for fname in inline_comments if
-                               fname not in included_files]
-
-            c.deleted_files_comments = collections.defaultdict(dict)
-            for fname, per_line_comments in inline_comments.items():
-                if fname in c.deleted_files:
-                    c.deleted_files_comments[fname]['stats'] = 0
-                    c.deleted_files_comments[fname]['comments'] = list()
-                    for lno, comments in per_line_comments.items():
-                        c.deleted_files_comments[fname]['comments'].extend(comments)
-
-            def _node_getter(commit):
-                def get_node(fname):
-                    try:
-                        return commit.get_node(fname)
-                    except NodeDoesNotExistError:
-                        return None
-                return get_node
-
-            c.diffset = codeblocks.DiffSet(
-                repo_name=c.repo_name,
-                source_repo_name=c.source_repo.repo_name,
-                source_node_getter=_node_getter(target_commit),
-                target_node_getter=_node_getter(source_commit),
-                comments=inline_comments
-            ).render_patchset(_parsed, target_commit.raw_id, source_commit.raw_id)
-
     def _extract_ordering(self, request):
         column_index = safe_int(request.GET.get('order[0][column]'))
         order_dir = request.GET.get('order[0][dir]', 'desc')
@@ -693,71 +590,53 b' class PullrequestsController(BaseRepoCon'
 
         pull_request_display_obj = PullRequest.get_pr_display_object(
             pull_request_obj, _org_pull_request_obj)
+
         return _org_pull_request_obj, pull_request_obj, \
                pull_request_display_obj, at_version
 
-    def _get_pr_version_changes(self, version, pull_request_latest):
-        """
-        Generate changes commits, and diff data based on the current pr version
-        """
-
-        #TODO(marcink): save those changes as JSON metadata for chaching later.
-
-        # fake the version to add the "initial" state object
-        pull_request_initial = PullRequest.get_pr_display_object(
-            pull_request_latest, pull_request_latest,
-            internal_methods=['get_commit', 'versions'])
-        pull_request_initial.revisions = []
-        pull_request_initial.source_repo.get_commit = types.MethodType(
-            lambda *a, **k: EmptyCommit(), pull_request_initial)
-        pull_request_initial.source_repo.scm_instance = types.MethodType(
-            lambda *a, **k: EmptyRepository(), pull_request_initial)
-
-        _changes_versions = [pull_request_latest] + \
-                            list(reversed(c.versions)) + \
-                            [pull_request_initial]
-
-        if version == 'latest':
-            index = 0
-        else:
-            for pos, prver in enumerate(_changes_versions):
-                ver = getattr(prver, 'pull_request_version_id', -1)
-                if ver == safe_int(version):
-                    index = pos
-                    break
-            else:
-                index = 0
-
-        cur_obj = _changes_versions[index]
-        prev_obj = _changes_versions[index + 1]
-
-        old_commit_ids = set(prev_obj.revisions)
-        new_commit_ids = set(cur_obj.revisions)
-
-        changes = PullRequestModel()._calculate_commit_id_changes(
-            old_commit_ids, new_commit_ids)
-
-        old_diff_data, new_diff_data = PullRequestModel()._generate_update_diffs(
-            cur_obj, prev_obj)
-        file_changes = PullRequestModel()._calculate_file_changes(
-            old_diff_data, new_diff_data)
-        return changes, file_changes
-
     @LoginRequired()
     @HasRepoPermissionAnyDecorator('repository.read', 'repository.write',
                                    'repository.admin')
     def show(self, repo_name, pull_request_id):
         pull_request_id = safe_int(pull_request_id)
         version = request.GET.get('version')
+        from_version = request.GET.get('from_version') or version
         merge_checks = request.GET.get('merge_checks')
+        c.fulldiff = str2bool(request.GET.get('fulldiff'))
+
+        # register for JS templates
+        c.template_context['pull_request_data']['pull_request_id'] = \
+            pull_request_id
 
         (pull_request_latest,
          pull_request_at_ver,
          pull_request_display_obj,
-         at_version) = self._get_pr_version(pull_request_id, version=version)
+         at_version) = self._get_pr_version(
+            pull_request_id, version=version)
+        versions = pull_request_display_obj.versions()
+
+        c.at_version = at_version
+        c.at_version_num = (at_version
+                            if at_version and at_version != 'latest'
+                            else None)
+        c.at_version_pos = ChangesetComment.get_index_from_version(
+            c.at_version_num, versions)
 
-        c.template_context['pull_request_data']['pull_request_id'] = \
-            pull_request_id
+        (prev_pull_request_latest,
+         prev_pull_request_at_ver,
+         prev_pull_request_display_obj,
+         prev_at_version) = self._get_pr_version(
+            pull_request_id, version=from_version)
+
+        c.from_version = prev_at_version
+        c.from_version_num = (prev_at_version
+                              if prev_at_version and prev_at_version != 'latest'
+                              else None)
+        c.from_version_pos = ChangesetComment.get_index_from_version(
+            c.from_version_num, versions)
+
+        # define if we're in COMPARE mode or VIEW at version mode
+        compare = at_version != prev_at_version
 
         # pull_requests repo_name we opened it against
         # ie. target_repo must match
@@ -767,12 +646,12 b' class PullrequestsController(BaseRepoCon'
         c.shadow_clone_url = PullRequestModel().get_shadow_clone_url(
             pull_request_at_ver)
 
-        c.ancestor = None  # TODO: add ancestor here
+        c.ancestor = None  # empty ancestor hidden in display
         c.pull_request = pull_request_display_obj
         c.pull_request_latest = pull_request_latest
 
         pr_closed = pull_request_latest.is_closed()
-        if at_version and not at_version == 'latest':
+        if compare or (at_version and not at_version == 'latest'):
             c.allowed_to_change_status = False
             c.allowed_to_update = False
             c.allowed_to_merge = False
@@ -789,35 +668,50 b' class PullrequestsController(BaseRepoCon'
                 pull_request_latest, c.rhodecode_user) and not pr_closed
             c.allowed_to_comment = not pr_closed
 
-        cc_model = CommentsModel()
+        # check merge capabilities
+        _merge_check = MergeCheck.validate(
+            pull_request_latest, user=c.rhodecode_user)
+        c.pr_merge_errors = _merge_check.error_details
+        c.pr_merge_possible = not _merge_check.failed
+        c.pr_merge_message = _merge_check.merge_msg
 
+        if merge_checks:
+            return render('/pullrequests/pullrequest_merge_checks.mako')
+
+        comments_model = CommentsModel()
+
+        # reviewers and statuses
         c.pull_request_reviewers = pull_request_at_ver.reviewers_statuses()
+        allowed_reviewers = [x[0].user_id for x in c.pull_request_reviewers]
         c.pull_request_review_status = pull_request_at_ver.calculated_review_status()
 
-        c.versions = pull_request_display_obj.versions()
-        c.at_version = at_version
-        c.at_version_num = at_version if at_version and at_version != 'latest' else None
-        c.at_version_pos = ChangesetComment.get_index_from_version(
-            c.at_version_num, c.versions)
-
         # GENERAL COMMENTS with versions #
-        q = cc_model._all_general_comments_of_pull_request(pull_request_latest)
+        q = comments_model._all_general_comments_of_pull_request(pull_request_latest)
+        q = q.order_by(ChangesetComment.comment_id.asc())
         general_comments = q.order_by(ChangesetComment.pull_request_version_id.asc())
 
         # pick comments we want to render at current version
-        c.comment_versions = cc_model.aggregate_comments(
-            general_comments, c.versions, c.at_version_num)
+        c.comment_versions = comments_model.aggregate_comments(
+            general_comments, versions, c.at_version_num)
         c.comments = c.comment_versions[c.at_version_num]['until']
 
         # INLINE COMMENTS with versions  #
-        q = cc_model._all_inline_comments_of_pull_request(pull_request_latest)
+        q = comments_model._all_inline_comments_of_pull_request(pull_request_latest)
+        q = q.order_by(ChangesetComment.comment_id.asc())
         inline_comments = q.order_by(ChangesetComment.pull_request_version_id.asc())
-        c.inline_versions = cc_model.aggregate_comments(
-            inline_comments, c.versions, c.at_version_num, inline=True)
+        c.inline_versions = comments_model.aggregate_comments(
+            inline_comments, versions, c.at_version_num, inline=True)
+
+        # inject latest version
+        latest_ver = PullRequest.get_pr_display_object(
+            pull_request_latest, pull_request_latest)
+
+        c.versions = versions + [latest_ver]
 
         # if we use version, then do not show later comments
         # than current version
-        display_inline_comments = collections.defaultdict(lambda: collections.defaultdict(list))
+        display_inline_comments = collections.defaultdict(
+            lambda: collections.defaultdict(list))
         for co in inline_comments:
             if c.at_version_num:
                 # pick comments that are at least UPTO given version, so we
@@ -831,17 +725,126 b' class PullrequestsController(BaseRepoCon'
             if should_render:
                 display_inline_comments[co.f_path][co.line_no].append(co)
 
-        _merge_check = MergeCheck.validate(
-            pull_request_latest, user=c.rhodecode_user)
-        c.pr_merge_errors = _merge_check.error_details
-        c.pr_merge_possible = not _merge_check.failed
-        c.pr_merge_message = _merge_check.merge_msg
+        # load diff data into template context, if we use compare mode then
+        # diff is calculated based on changes between versions of PR
+
+        source_repo = pull_request_at_ver.source_repo
+        source_ref_id = pull_request_at_ver.source_ref_parts.commit_id
+
+        target_repo = pull_request_at_ver.target_repo
+        target_ref_id = pull_request_at_ver.target_ref_parts.commit_id
+
+        if compare:
+            # in compare switch the diff base to latest commit from prev version
+            target_ref_id = prev_pull_request_display_obj.revisions[0]
+
+        # despite opening commits for bookmarks/branches/tags, we always
+        # convert this to rev to prevent changes after bookmark or branch change
+        c.source_ref_type = 'rev'
+        c.source_ref = source_ref_id
+
+        c.target_ref_type = 'rev'
+        c.target_ref = target_ref_id
+
+        c.source_repo = source_repo
+        c.target_repo = target_repo
+
+        # diff_limit is the old behavior, will cut off the whole diff
+        # if the limit is applied  otherwise will just hide the
+        # big files from the front-end
+        diff_limit = self.cut_off_limit_diff
+        file_limit = self.cut_off_limit_file
+
+        c.commit_ranges = []
+        source_commit = EmptyCommit()
+        target_commit = EmptyCommit()
+        c.missing_requirements = False
+
+        # try first shadow repo, fallback to regular repo
+        try:
+            commits_source_repo = pull_request_latest.get_shadow_repo()
+        except Exception:
+            log.debug('Failed to get shadow repo', exc_info=True)
+            commits_source_repo = source_repo.scm_instance()
+
+        c.commits_source_repo = commits_source_repo
+        commit_cache = {}
+        try:
+            pre_load = ["author", "branch", "date", "message"]
+            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)
+                c.commit_ranges.append(comm)
+                commit_cache[comm.raw_id] = comm
 
-        if merge_checks:
-            return render('/pullrequests/pullrequest_merge_checks.mako')
+            target_commit = commits_source_repo.get_commit(
+                commit_id=safe_str(target_ref_id))
+            source_commit = commits_source_repo.get_commit(
+                commit_id=safe_str(source_ref_id))
+        except CommitDoesNotExistError:
+            pass
+        except RepositoryRequirementError:
+            log.warning(
+                'Failed to get all required data from repo', exc_info=True)
+            c.missing_requirements = True
+
+        c.statuses = source_repo.statuses(
+            [x.raw_id for x in c.commit_ranges])
+
+        # auto collapse if we have more than limit
+        collapse_limit = diffs.DiffProcessor._collapse_commits_over
+        c.collapse_all_commits = len(c.commit_ranges) > collapse_limit
+        c.compare_mode = compare
+
+        c.missing_commits = False
+        if (c.missing_requirements or isinstance(source_commit, EmptyCommit)
+            or source_commit == target_commit):
+
+            c.missing_commits = True
+        else:
+            vcs_diff = PullRequestModel().get_diff(
+                commits_source_repo, source_ref_id, target_ref_id)
+
+            diff_processor = diffs.DiffProcessor(
+                vcs_diff, format='newdiff', diff_limit=diff_limit,
+                file_limit=file_limit, show_full_diff=c.fulldiff)
 
-        # load compare data into template context
-        self._load_compare_data(pull_request_at_ver, display_inline_comments)
+            _parsed = diff_processor.prepare()
+            c.limited_diff = isinstance(_parsed, diffs.LimitedDiffContainer)
+
+            def _node_getter(commit):
+                def get_node(fname):
+                    try:
+                        return commit.get_node(fname)
+                    except NodeDoesNotExistError:
+                        return None
+
+                return get_node
+
+            diffset = codeblocks.DiffSet(
+                repo_name=c.repo_name,
+                source_repo_name=c.source_repo.repo_name,
+                source_node_getter=_node_getter(target_commit),
+                target_node_getter=_node_getter(source_commit),
+                comments=display_inline_comments
+            )
+            c.diffset = diffset.render_patchset(
+                _parsed, target_commit.raw_id, source_commit.raw_id)
+
+            # calculate removed files that are bound to comments
+            comment_deleted_files = [
+                fname for fname in display_inline_comments
+                if fname not in c.diffset.file_stats]
+
+            c.deleted_files_comments = collections.defaultdict(dict)
+            for fname, per_line_comments in display_inline_comments.items():
+                if fname in comment_deleted_files:
+                    c.deleted_files_comments[fname]['stats'] = 0
+                    c.deleted_files_comments[fname]['comments'] = list()
+                    for lno, comments in per_line_comments.items():
+                        c.deleted_files_comments[fname]['comments'].extend(
+                            comments)
 
         # this is a hack to properly display links, when creating PR, the
         # compare view and others uses different notation, and
@@ -856,21 +859,55 b' class PullrequestsController(BaseRepoCon'
             statuses = ChangesetStatus.STATUSES
         c.commit_statuses = statuses
 
-        c.changes = None
-        c.file_changes = None
+        c.show_version_changes = not pr_closed
+        if c.show_version_changes:
+            cur_obj = pull_request_at_ver
+            prev_obj = prev_pull_request_at_ver
+
+            old_commit_ids = prev_obj.revisions
+            new_commit_ids = cur_obj.revisions
+            commit_changes = PullRequestModel()._calculate_commit_id_changes(
+                old_commit_ids, new_commit_ids)
+            c.commit_changes_summary = commit_changes
+
+            # calculate the diff for commits between versions
+            c.commit_changes = []
+            mark = lambda cs, fw: 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'):
 
-        c.show_version_changes = 1  # control flag, not used yet
+                if raw_id in commit_cache:
+                    commit = commit_cache[raw_id]
+                else:
+                    try:
+                        commit = commits_source_repo.get_commit(raw_id)
+                    except CommitDoesNotExistError:
+                        # in case we fail extracting still use "dummy" commit
+                        # for display in commit diff
+                        commit = h.AttributeDict(
+                            {'raw_id': raw_id,
+                             'message': 'EMPTY or MISSING COMMIT'})
+                c.commit_changes.append([c_type, commit])
 
-        if at_version and c.show_version_changes:
-            c.changes, c.file_changes = self._get_pr_version_changes(
-                version, pull_request_latest)
+            # current user review statuses for each version
+            c.review_versions = {}
+            if c.rhodecode_user.user_id in allowed_reviewers:
+                for co in general_comments:
+                    if co.author.user_id == c.rhodecode_user.user_id:
+                        # each comment has a status change
+                        status = co.status_change
+                        if status:
+                            _ver_pr = status[0].comment.pull_request_version_id
+                            c.review_versions[_ver_pr] = status[0]
 
         return render('/pullrequests/pullrequest_show.mako')
 
     @LoginRequired()
     @NotAnonymous()
-    @HasRepoPermissionAnyDecorator('repository.read', 'repository.write',
-                                   'repository.admin')
+    @HasRepoPermissionAnyDecorator(
+        'repository.read', 'repository.write', 'repository.admin')
     @auth.CSRFRequired()
     @jsonify
     def comment(self, repo_name, pull_request_id):

rhodecode/lib/codeblocks.py

@@ -382,6 +382,7 b' class DiffSet(object):'
             lines_deleted=0,
             changed_files=0,
             files=[],
+            file_stats={},
             limited_diff=isinstance(patchset, LimitedDiffContainer),
             repo_name=self.repo_name,
             source_repo_name=self.source_repo_name,
@@ -389,6 +390,7 b' class DiffSet(object):'
             target_ref=target_ref,
         ))
         for patch in patchset:
+            diffset.file_stats[patch['filename']] = patch['stats']
             filediff = self.render_patch(patch)
             filediff.diffset = diffset
             diffset.files.append(filediff)

rhodecode/lib/helpers.py

@@ -37,6 +37,7 b' import time'
 import string
 import hashlib
 import pygments
+import itertools
 
 from datetime import datetime
 from functools import partial

rhodecode/model/db.py

@@ -3028,7 +3028,7 b' class ChangesetStatus(Base, BaseModel):'
     pull_request = relationship('PullRequest', lazy='joined')
 
     def __unicode__(self):
-        return u"<%s('%s[%s]:%s')>" % (
+        return u"<%s('%s[v%s]:%s')>" % (
             self.__class__.__name__,
             self.status, self.version, self.author
         )
@@ -3254,7 +3254,6 b' class PullRequest(Base, _PullRequestBase'
                             cascade="all, delete, delete-orphan",
                             lazy='dynamic')
 
-
     @classmethod
     def get_pr_display_object(cls, pull_request_obj, org_pull_request_obj,
                               internal_methods=None):
@@ -3290,6 +3289,10 b' class PullRequest(Base, _PullRequestBase'
             def is_closed(self):
                 return pull_request_obj.is_closed()
 
+            @property
+            def pull_request_version_id(self):
+                return getattr(pull_request_obj, 'pull_request_version_id', None)
+
         attrs = StrictAttributeDict(pull_request_obj.get_api_data())
 
         attrs.author = StrictAttributeDict(

rhodecode/model/notification.py

@@ -235,7 +235,7 b' class NotificationModel(BaseModel):'
             .filter(UserNotification.notification == notification)\
             .filter(UserNotification.user == user).scalar()
 
-    def make_description(self, notification, show_age=True):
+    def make_description(self, notification, show_age=True, translate=None):
         """
         Creates a human readable description based on properties
         of notification object
@@ -273,6 +273,8 b' class NotificationModel(BaseModel):'
         if show_age:
             template = templates[0]
             date_or_age = h.age(notification.created_on)
+            if translate:
+                date_or_age = translate(date_or_age)
         else:
             template = templates[1]
             date_or_age = h.format_date(notification.created_on)

rhodecode/model/pull_request.py

@@ -50,7 +50,7 b' from rhodecode.model.changeset_status im'
 from rhodecode.model.comment import CommentsModel
 from rhodecode.model.db import (
     PullRequest, PullRequestReviewers, ChangesetStatus,
-    PullRequestVersion, ChangesetComment)
+    PullRequestVersion, ChangesetComment, Repository)
 from rhodecode.model.meta import Session
 from rhodecode.model.notification import NotificationModel, \
     EmailNotificationModel
@@ -728,13 +728,23 b' class PullRequestModel(BaseModel):'
         return version
 
     def _generate_update_diffs(self, pull_request, pull_request_version):
+
         diff_context = (
             self.DIFF_CONTEXT +
             CommentsModel.needed_extra_diff_context())
+
+        source_repo = pull_request_version.source_repo
+        source_ref_id = pull_request_version.source_ref_parts.commit_id
+        target_ref_id = pull_request_version.target_ref_parts.commit_id
         old_diff = self._get_diff_from_pr_or_version(
-            pull_request_version, context=diff_context)
+            source_repo, source_ref_id, target_ref_id, context=diff_context)
+
+        source_repo = pull_request.source_repo
+        source_ref_id = pull_request.source_ref_parts.commit_id
+        target_ref_id = pull_request.target_ref_parts.commit_id
+
         new_diff = self._get_diff_from_pr_or_version(
-            pull_request, context=diff_context)
+            source_repo, source_ref_id, target_ref_id, context=diff_context)
 
         old_diff_data = diffs.DiffProcessor(old_diff)
         old_diff_data.prepare()
@@ -768,10 +778,11 b' class PullRequestModel(BaseModel):'
             Session().add(comment)
 
     def _calculate_commit_id_changes(self, old_ids, new_ids):
-        added = new_ids.difference(old_ids)
-        common = old_ids.intersection(new_ids)
-        removed = old_ids.difference(new_ids)
-        return ChangeTuple(added, common, removed)
+        added = [x for x in new_ids if x not in old_ids]
+        common = [x for x in new_ids if x in old_ids]
+        removed = [x for x in old_ids if x not in new_ids]
+        total = new_ids
+        return ChangeTuple(added, common, removed, total)
 
     def _calculate_file_changes(self, old_diff_data, new_diff_data):
 
@@ -1261,20 +1272,20 b' class PullRequestModel(BaseModel):'
                 raise EmptyRepositoryError()
         return groups, selected
 
-    def get_diff(self, pull_request, context=DIFF_CONTEXT):
-        pull_request = self.__get_pull_request(pull_request)
-        return self._get_diff_from_pr_or_version(pull_request, context=context)
+    def get_diff(self, source_repo, source_ref_id, target_ref_id, context=DIFF_CONTEXT):
+        return self._get_diff_from_pr_or_version(
+            source_repo, source_ref_id, target_ref_id, context=context)
 
-    def _get_diff_from_pr_or_version(self, pr_or_version, context):
-        source_repo = pr_or_version.source_repo
-
-        # we swap org/other ref since we run a simple diff on one repo
-        target_ref_id = pr_or_version.target_ref_parts.commit_id
-        source_ref_id = pr_or_version.source_ref_parts.commit_id
+    def _get_diff_from_pr_or_version(
+            self, source_repo, source_ref_id, target_ref_id, context):
         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))
-        vcs_repo = source_repo.scm_instance()
+        source_commit = source_repo.get_commit(
+            commit_id=safe_str(source_ref_id))
+        if isinstance(source_repo, Repository):
+            vcs_repo = source_repo.scm_instance()
+        else:
+            vcs_repo = source_repo
 
         # TODO: johbo: In the context of an update, we cannot reach
         # the old commit anymore with our normal mechanisms. It needs
@@ -1403,7 +1414,7 b' class MergeCheck(object):'
 
 
 ChangeTuple = namedtuple('ChangeTuple',
-                         ['added', 'common', 'removed'])
+                         ['added', 'common', 'removed', 'total'])
 
 FileChangeTuple = namedtuple('FileChangeTuple',
                              ['added', 'modified', 'removed'])

rhodecode/public/css/main.less

@@ -1393,6 +1393,14 b' table.integrations {'
   }
 }
 
+.subtitle-compare {
+  margin: -15px 0px 0px 0px;
+}
+
+.comments-summary-td {
+  border-top: 1px dashed @grey5;
+}
+
 // new entry in group_members
 .td-author-new-entry {
   background-color: rgba(red(@alert1), green(@alert1), blue(@alert1), 0.3);
@@ -1422,6 +1430,57 b' table.integrations {'
     margin-left: 5px;
 }
 
+.compare_view_commits {
+  .color-a {
+    color: @alert1;
+  }
+
+  .color-c {
+    color: @color3;
+  }
+
+  .color-r {
+    color: @color5;
+  }
+
+  .color-a-bg {
+    background-color: @alert1;
+  }
+
+  .color-c-bg {
+    background-color: @alert3;
+  }
+
+  .color-r-bg {
+    background-color: @alert2;
+  }
+
+  .color-a-border {
+    border: 1px solid @alert1;
+  }
+
+  .color-c-border {
+    border: 1px solid @alert3;
+  }
+
+  .color-r-border {
+    border: 1px solid @alert2;
+  }
+
+  .commit-change-indicator {
+    width: 15px;
+    height: 15px;
+    position: relative;
+    left: 15px;
+  }
+
+  .commit-change-content {
+    text-align: center;
+    vertical-align: middle;
+    line-height: 15px;
+  }
+}
+
 .compare_view_files {
     width: 100%;
 
@@ -1663,10 +1722,27 b' BIN_FILENODE = 7'
 }
 
 .pr-versions {
-    position: relative;
-    top: 6px;
+    font-size: 1.1em;
+
+    table {
+      padding: 0px 5px;
+    }
+
+    td {
+      line-height: 15px;
+    }
+
+    .flag_status {
+        margin: 0;
+    }
+
+    .compare-radio-button {
+      position: relative;
+      top: -3px;
+    }
 }
 
+
 #close_pull_request {
     margin-right: 0px;
 }

rhodecode/templates/changelog/changelog.mako

@@ -310,20 +310,42 b''
                 }
             }
 
-            $('.expand_commit').on('click',function(e){
-              var target_expand = $(this);
-              var cid = target_expand.data('commitId');
+            $('.expand_commit').on('click', function (e) {
+                var target_expand = $(this);
+                var cid = target_expand.data('commitId');
 
-              if (target_expand.hasClass('open')){
-                $('#c-'+cid).css({'height': '1.5em', 'white-space': 'nowrap', 'text-overflow': 'ellipsis', 'overflow':'hidden'});
-                $('#t-'+cid).css({'height': 'auto', 'line-height': '.9em', 'text-overflow': 'ellipsis', 'overflow':'hidden', 'white-space':'nowrap'});
-                target_expand.removeClass('open');
-              }
-              else {
-                $('#c-'+cid).css({'height': 'auto', 'white-space': 'pre-line', 'text-overflow': 'initial', 'overflow':'visible'});
-                $('#t-'+cid).css({'height': 'auto', 'max-height': 'none', 'text-overflow': 'initial', 'overflow':'visible', 'white-space':'normal'});
-                target_expand.addClass('open');
-              }
+                if (target_expand.hasClass('open')) {
+                    $('#c-' + cid).css({
+                        'height': '1.5em',
+                        'white-space': 'nowrap',
+                        'text-overflow': 'ellipsis',
+                        'overflow': 'hidden'
+                    });
+                    $('#t-' + cid).css({
+                        'height': 'auto',
+                        'line-height': '.9em',
+                        'text-overflow': 'ellipsis',
+                        'overflow': 'hidden',
+                        'white-space': 'nowrap'
+                    });
+                    target_expand.removeClass('open');
+                }
+                else {
+                    $('#c-' + cid).css({
+                        'height': 'auto',
+                        'white-space': 'pre-line',
+                        'text-overflow': 'initial',
+                        'overflow': 'visible'
+                    });
+                    $('#t-' + cid).css({
+                        'height': 'auto',
+                        'max-height': 'none',
+                        'text-overflow': 'initial',
+                        'overflow': 'visible',
+                        'white-space': 'normal'
+                    });
+                    target_expand.addClass('open');
+                }
                 // redraw the graph
                 graph_options.height = $("#changesets").height();
                 $("canvas").remove();

rhodecode/templates/codeblocks/diffs.mako

@@ -134,13 +134,6 b' collapse_all = len(diffset.files) > coll'
                     '%(num)s files changed: %(linesadd)s inserted, %(linesdel)s deleted', diffset.changed_files) % {'num': diffset.changed_files, 'linesadd': diffset.lines_added, 'linesdel': diffset.lines_deleted}}
     %endif
 
-        <% at_ver = getattr(c, 'at_version_pos', None) %>
-        % if at_ver:
-            <div class="pull-right">
-            ${_('Showing changes at version %d') % at_ver}
-            </div>
-        % endif
-
         </h2>
     </div>
 

rhodecode/templates/pullrequests/pullrequest_show.mako

@@ -1,4 +1,5 b''
 <%inherit file="/base/base.mako"/>
+<%namespace name="base" file="/base/base.mako"/>
 
 <%def name="title()">
     ${_('%s Pull Request #%s') % (c.repo_name, c.pull_request.pull_request_id)}
@@ -165,47 +166,23 b''
            </div>
 
            <div class="field">
-                <div class="label-summary">
-                    <label>${_('Versions')} (${len(c.versions)+1}):</label>
-                </div>
+               <div class="label-summary">
+                   <label>${_('Versions')}:</label>
+               </div>
+
+               <% outdated_comm_count_ver = len(c.inline_versions[None]['outdated']) %>
+               <% general_outdated_comm_count_ver = len(c.comment_versions[None]['outdated']) %>
 
                <div class="pr-versions">
                % if c.show_version_changes:
+                   <% outdated_comm_count_ver = len(c.inline_versions[c.at_version_num]['outdated']) %>
+                   <% general_outdated_comm_count_ver = len(c.comment_versions[c.at_version_num]['outdated']) %>
+                   <div id="show-pr-versions" class="input btn btn-link" onclick="return versionController.toggleVersionView(this)"
+                        data-toggle-on="${ungettext('{} version available for this pull request, show it.', '{} versions available for this pull request, show them.', len(c.versions)).format(len(c.versions))}"
+                        data-toggle-off="${_('Hide all versions of this pull request')}">
+                       ${ungettext('{} version available for this pull request, show it.', '{} versions available for this pull request, show them.', len(c.versions)).format(len(c.versions))}
+                   </div>
                    <table>
-                       ## CURRENTLY SELECT PR VERSION
-                       <tr class="version-pr" style="display: ${'' if c.at_version_num is None else 'none'}">
-                           <td>
-                               % if c.at_version_num is None:
-                                <i class="icon-ok link"></i>
-                               % else:
-                                <i class="icon-comment"></i>
-                                   <code>
-                                   ${len(c.comment_versions[None]['at'])}/${len(c.inline_versions[None]['at'])}
-                                   </code>
-                               % endif
-                           </td>
-                           <td>
-                               <code>
-                               % if c.versions:
-                                <a href="${h.url.current(version='latest')}">${_('latest')}</a>
-                               % else:
-                                ${_('initial')}
-                               % endif
-                               </code>
-                           </td>
-                           <td>
-                               <code>${c.pull_request_latest.source_ref_parts.commit_id[:6]}</code>
-                           </td>
-                           <td>
-                               ${_('created')} ${h.age_component(c.pull_request_latest.updated_on)}
-                           </td>
-                           <td align="right">
-                               % if c.versions and c.at_version_num in [None, 'latest']:
-                               <span id="show-pr-versions" class="btn btn-link" onclick="$('.version-pr').show(); $(this).hide(); return false">${_('Show all versions')}</span>
-                               % endif
-                           </td>
-                       </tr>
-
                        ## SHOW ALL VERSIONS OF PR
                        <% ver_pr = None %>
 
@@ -213,46 +190,52 b''
                            <% ver_pos = data[0] %>
                            <% ver = data[1] %>
                            <% ver_pr = ver.pull_request_version_id %>
+                           <% display_row = '' if c.at_version and (c.at_version_num == ver_pr or c.from_version_num == ver_pr) else 'none' %>
 
-                           <tr class="version-pr" style="display: ${'' if c.at_version_num == ver_pr else 'none'}">
+                           <tr class="version-pr" style="display: ${display_row}">
+                               <td>
+                                    <code>
+                                        <a href="${h.url.current(version=ver_pr or 'latest')}">v${ver_pos}</a>
+                                    </code>
+                               </td>
                                <td>
-                                   % if c.at_version_num == ver_pr:
-                                    <i class="icon-ok link"></i>
-                                   % else:
+                                   <input ${'checked="checked"' if c.from_version_num == ver_pr else ''} class="compare-radio-button" type="radio" name="ver_source" value="${ver_pr or 'latest'}" data-ver-pos="${ver_pos}"/>
+                                   <input ${'checked="checked"' if c.at_version_num == ver_pr else ''} class="compare-radio-button" type="radio" name="ver_target" value="${ver_pr or 'latest'}" data-ver-pos="${ver_pos}"/>
+                               </td>
+                               <td>
+                                <% review_status = c.review_versions[ver_pr].status if ver_pr in c.review_versions else 'not_reviewed' %>
+                                <div class="${'flag_status %s' % review_status} tooltip pull-left" title="${_('Your review status at this version')}">
+                                </div>
+                               </td>
+                               <td>
+                                   % if c.at_version_num != ver_pr:
                                     <i class="icon-comment"></i>
-                                       <code class="tooltip" title="${_('Comment from pull request version {0}, general:{1} inline{2}').format(ver_pos, len(c.comment_versions[ver_pr]['at']), len(c.inline_versions[ver_pr]['at']))}">
+                                       <code class="tooltip" title="${_('Comment from pull request version {0}, general:{1} inline:{2}').format(ver_pos, len(c.comment_versions[ver_pr]['at']), len(c.inline_versions[ver_pr]['at']))}">
                                            ${len(c.comment_versions[ver_pr]['at'])}/${len(c.inline_versions[ver_pr]['at'])}
                                        </code>
                                    % endif
                                </td>
                                <td>
-                                    <code>
-                                        <a href="${h.url.current(version=ver_pr)}">v${ver_pos}</a>
-                                    </code>
+                                   ##<code>${ver.source_ref_parts.commit_id[:6]}</code>
                                </td>
                                <td>
-                                   <code>${ver.source_ref_parts.commit_id[:6]}</code>
-                               </td>
-                               <td>
-                                   ${_('created')} ${h.age_component(ver.updated_on)}
-                               </td>
-                               <td align="right">
-                                   % if c.at_version_num == ver_pr:
-                                   <span id="show-pr-versions" class="btn btn-link" onclick="$('.version-pr').show(); $(this).hide(); return false">${_('Show all versions')}</span>
-                                   % endif
+                                   ${h.age_component(ver.updated_on)}
                                </td>
                            </tr>
                        % endfor
 
-                       ## show comment/inline comments summary
                        <tr>
-                           <td>
+                           <td colspan="5">
+                               <button id="show-version-diff" onclick="return versionController.showVersionDiff()" class="btn btn-sm" style="display: none" data-label-text="${_('show changes between versions')}">
+                                   ${_('show changes between versions')}
+                               </button>
                            </td>
+                       </tr>
 
-                           <td colspan="4" style="border-top: 1px dashed #dbd9da">
-                            <% outdated_comm_count_ver = len(c.inline_versions[c.at_version_num]['outdated']) %>
-                            <% general_outdated_comm_count_ver = len(c.comment_versions[c.at_version_num]['outdated']) %>
-
+                       ## show comment/inline comments summary
+                       <%def name="comments_summary()">
+                       <tr>
+                           <td colspan="6" class="comments-summary-td">
 
                             % if c.at_version:
                                 <% inline_comm_count_ver = len(c.inline_versions[c.at_version_num]['display']) %>
@@ -264,6 +247,7 b''
                                 ${_('Comments for this pull request')}:
                             % endif
 
+
                             %if general_comm_count_ver:
                                 <a href="#comments">${_("%d General ") % general_comm_count_ver}</a>
                             %else:
@@ -285,37 +269,18 b''
                             %endif
                            </td>
                        </tr>
-
-                       <tr>
-                           <td></td>
-                           <td colspan="4">
-                           % if c.at_version:
-<pre>
-Changed commits:
- * added: ${len(c.changes.added)}
- * removed: ${len(c.changes.removed)}
-
-% if not (c.file_changes.added+c.file_changes.modified+c.file_changes.removed):
-No file changes found
-% else:
-Changed files:
- %for file_name in c.file_changes.added:
- * A <a href="#${'a_' + h.FID('', file_name)}">${file_name}</a>
- %endfor
- %for file_name in c.file_changes.modified:
- * M <a href="#${'a_' + h.FID('', file_name)}">${file_name}</a>
- %endfor
- %for file_name in c.file_changes.removed:
- * R ${file_name}
- %endfor
-% endif
-</pre>
-                           % endif
-                           </td>
-                       </tr>
+                       </%def>
+                       ${comments_summary()}
                    </table>
                % else:
+                   <div class="input">
                    ${_('Pull request versions not available')}.
+                   </div>
+                   <div>
+                      <table>
+                        ${comments_summary()}
+                      </table>
+                   </div>
                % endif
                </div>
            </div>
@@ -426,7 +391,15 b' Changed files:'
                   </div>
                 </div>
               % endif
-                <div class="compare_view_commits_title">
+
+              <div class="compare_view_commits_title">
+                  % if not c.compare_mode:
+
+                    % if c.at_version_pos:
+                        <h4>
+                        ${_('Showing changes at v%d, commenting is disabled.') % c.at_version_pos}
+                        </h4>
+                    % endif
 
                     <div class="pull-left">
                       <div class="btn-group">
@@ -453,11 +426,113 b' Changed files:'
                         % endif
 
                     </div>
-
-                </div>
+                  % endif
+              </div>
 
               % if not c.missing_commits:
-                <%include file="/compare/compare_commits.mako" />
+                % if c.compare_mode:
+                    % if c.at_version:
+                    <h4>
+                        ${_('Commits and changes between v{ver_from} and {ver_to} of this pull request, commenting is disabled').format(ver_from=c.from_version_pos, ver_to=c.at_version_pos if c.at_version_pos else 'latest')}:
+                    </h4>
+
+                    <div class="subtitle-compare">
+                        ${_('commits added: {}, removed: {}').format(len(c.commit_changes_summary.added), len(c.commit_changes_summary.removed))}
+                    </div>
+
+                    <div class="container">
+                        <table class="rctable compare_view_commits">
+                            <tr>
+                                <th></th>
+                                <th>${_('Time')}</th>
+                                <th>${_('Author')}</th>
+                                <th>${_('Commit')}</th>
+                                <th></th>
+                                <th>${_('Description')}</th>
+                            </tr>
+
+                            % for c_type, commit in c.commit_changes:
+                              % if c_type in ['a', 'r']:
+                                <%
+                                    if c_type == 'a':
+                                        cc_title = _('Commit added in displayed changes')
+                                    elif c_type == 'r':
+                                        cc_title = _('Commit removed in displayed changes')
+                                    else:
+                                        cc_title = ''
+                                %>
+                                <tr id="row-${commit.raw_id}" commit_id="${commit.raw_id}" class="compare_select">
+                                <td>
+                                    <div class="commit-change-indicator color-${c_type}-border">
+                                      <div class="commit-change-content color-${c_type} tooltip" title="${cc_title}">
+                                        ${c_type.upper()}
+                                      </div>
+                                    </div>
+                                </td>
+                                <td class="td-time">
+                                    ${h.age_component(commit.date)}
+                                </td>
+                                <td class="td-user">
+                                    ${base.gravatar_with_user(commit.author, 16)}
+                                </td>
+                                <td class="td-hash">
+                                    <code>
+                                        <a href="${h.url('changeset_home', repo_name=c.target_repo.repo_name, revision=commit.raw_id)}">
+                                            r${commit.revision}:${h.short_id(commit.raw_id)}
+                                        </a>
+                                        ${h.hidden('revisions', commit.raw_id)}
+                                    </code>
+                                </td>
+                                <td class="expand_commit" data-commit-id="${commit.raw_id}" title="${_( 'Expand commit message')}">
+                                    <div class="show_more_col">
+                                    <i class="show_more"></i>
+                                    </div>
+                                </td>
+                                <td class="mid td-description">
+                                    <div class="log-container truncate-wrap">
+                                        <div class="message truncate" id="c-${commit.raw_id}" data-message-raw="${commit.message}">
+                                            ${h.urlify_commit_message(commit.message, c.repo_name)}
+                                        </div>
+                                    </div>
+                                </td>
+                            </tr>
+                              % endif
+                            % endfor
+                        </table>
+                    </div>
+
+                    <script>
+                    $('.expand_commit').on('click',function(e){
+                      var target_expand = $(this);
+                      var cid = target_expand.data('commitId');
+
+                      if (target_expand.hasClass('open')){
+                        $('#c-'+cid).css({
+                            'height': '1.5em',
+                            'white-space': 'nowrap',
+                            'text-overflow': 'ellipsis',
+                            'overflow':'hidden'
+                        });
+                        target_expand.removeClass('open');
+                      }
+                      else {
+                        $('#c-'+cid).css({
+                            'height': 'auto',
+                            'white-space': 'pre-line',
+                            'text-overflow': 'initial',
+                            'overflow':'visible'
+                        });
+                        target_expand.addClass('open');
+                      }
+                    });
+                    </script>
+
+                    % endif
+
+                % else:
+                    <%include file="/compare/compare_commits.mako" />
+                % endif
+
                 <div class="cs_files">
                     <%namespace name="cbdiffs" file="/codeblocks/diffs.mako"/>
                     ${cbdiffs.render_diffset_menu()}
@@ -528,6 +603,107 b' Changed files:'
             }
         }
 
+        VersionController = function() {
+            var self = this;
+            this.$verSource = $('input[name=ver_source]');
+            this.$verTarget = $('input[name=ver_target]');
+
+            this.adjustRadioSelectors = function (curNode) {
+                var getVal = function(item) {
+                    if (item == 'latest'){
+                        return Number.MAX_SAFE_INTEGER
+                    }
+                    else {
+                        return parseInt(item)
+                    }
+                };
+
+                var curVal = getVal($(curNode).val());
+                $.each(self.$verSource, function(index, value){
+                    var elVal = getVal($(value).val());
+                    if(elVal > curVal){
+                        $(value).attr('disabled', 'disabled');
+                        $(value).removeAttr('checked');
+                    }
+                    else{
+                        $(value).removeAttr('disabled');
+                    }
+                });
+
+                self.setLockAction(false, $(curNode).data('verPos'));
+            };
+
+
+            this.attachVersionListener = function () {
+                self.$verTarget.change(function(e){
+                    self.adjustRadioSelectors(this)
+                });
+                self.$verSource.change(function(e){
+                    self.adjustRadioSelectors(self.$verTarget.filter(':checked'))
+                });
+            };
+
+            this.init = function () {
+
+                var curNode = self.$verTarget.filter(':checked');
+                self.adjustRadioSelectors(curNode);
+                self.setLockAction(true);
+                self.attachVersionListener();
+
+            };
+
+            this.setLockAction = function (state, selectedVersion) {
+                if(state){
+                    $('#show-version-diff').attr('disabled','disabled')
+                    $('#show-version-diff').addClass('disabled')
+                    $('#show-version-diff').html($('#show-version-diff').data('labelText'));
+                }
+                else{
+                    $('#show-version-diff').removeAttr('disabled');
+                    $('#show-version-diff').removeClass('disabled')
+                    //$('#show-version-diff').html(_gettext('show changes for v') + selectedVersion)
+                }
+
+            };
+
+            this.showVersionDiff = function(){
+                var target = self.$verTarget.filter(':checked');
+                var source = self.$verSource.filter(':checked');
+
+                if (target.val() && source.val()) {
+                    var params = {
+                        'pull_request_id': ${c.pull_request.pull_request_id},
+                        'repo_name': templateContext.repo_name,
+                        'version': target.val(),
+                        'from_version': source.val()
+                    };
+                    window.location = pyroutes.url('pullrequest_show', params)
+                }
+
+                return false;
+            };
+
+            this.toggleVersionView = function (elem) {
+
+                if ($('#show-version-diff').is(':visible')) {
+                    $('.version-pr').hide();
+                    $('#show-version-diff').hide();
+                    $(elem).html($(elem).data('toggleOn'))
+                } else {
+                    $('.version-pr').show();
+                    $('#show-version-diff').show();
+                    $(elem).html($(elem).data('toggleOff'))
+                }
+
+                return false
+            }
+
+        };
+
+        versionController = new VersionController();
+        versionController.init();
+
+
         $(function(){
             ReviewerAutoComplete('user');
             // custom code mirror