mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor draft
authorGregory Szorc <gps@mozilla.com>
Tue, 12 Jan 2016 15:32:08 -0800
changeset 6712 59c8dc21db3d89a69326f30c809ef54d467189ea
parent 6711 64f75ee4606a6bbbaec07ac15035c7ea0394448c
child 6713 af3905c7dfed34656415c20876cd32d514130ee1
push id513
push usergszorc@mozilla.com
push dateWed, 13 Jan 2016 02:13:15 +0000
reviewerssmacleod, dminor
bugs1229468
mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor The final step for moving the creation of review series into a unified web API is moving the management of child review requests to the new API, which this commit does. This commit is rather large. Unfortunately, there is no easy way to incrementally move this code since managing the state of child review requests is so complicated and intertwined. As part of migrating the code, the overall structure changed substantially. This is because we're able to do things differently from the context of the web api that we weren't from outside. For example, we can query the user database directly instead of going through the search interface. The best way to review this change is probably to open a copy of pushhooks.py in a separate window and read it alongside the new code in batch_review_request.py. Fortunately, our test coverage of review state is pretty robust. And, as you can see, there are no test changes as a result of this refactor! Presumably the new behavior is identical to the old behavior! There is still room to optimize this code. For example, the old code maintained a cache of reviewer lookups. This was dropped as part of the refactor. It can and should be added in later. The practical effect of this change is that review submission and publishing now requires far fewer HTTP requests to the Review Board Web API. On a local test cluster, submitting and publishing a review series of 20 commits with a single reviewer required the following: Before: 12.0s 134 BMO requests 158 RB requests After: 10.0s 154 BMO requests 54 RB requests Delta: -2.0s +20 HTTP requests -104 HTTP requests (The HTTP request count includes requests during cluster startup, so it is artificially high to start.) The regression in BMO HTTP requests is almost certainly due to redundant user lookups. This will be corrected later.
pylib/mozreview/mozreview/resources/batch_review_request.py
pylib/reviewboardmods/reviewboardmods/pushhooks.py
--- a/pylib/mozreview/mozreview/resources/batch_review_request.py
+++ b/pylib/mozreview/mozreview/resources/batch_review_request.py
@@ -1,57 +1,173 @@
 from __future__ import absolute_import, unicode_literals
 
 import json
 import logging
 
+from django.contrib.auth.models import User
 from django.db import transaction
+from django.db.models import Q
 from djblets.webapi.decorators import (
     webapi_login_required,
     webapi_request_fields,
     webapi_response_errors,
 )
 from djblets.webapi.errors import (
     INVALID_FORM_DATA,
     NOT_LOGGED_IN,
     PERMISSION_DENIED,
 )
+from reviewboard.accounts.backends import (
+    get_enabled_auth_backends,
+)
 from reviewboard.diffviewer.models import (
     DiffSet,
 )
 from reviewboard.reviews.models import (
     ReviewRequest,
     ReviewRequestDraft,
 )
 from reviewboard.scmtools.models import (
     Repository,
 )
 from reviewboard.webapi.decorators import (
     webapi_check_local_site,
 )
+from reviewboard.webapi.encoder import (
+    status_to_string,
+)
 from reviewboard.webapi.resources import (
     WebAPIResource,
 )
 
+from ..review_helpers import (
+    gen_latest_reviews,
+)
 
 logger = logging.getLogger(__name__)
 
 
 class SquashedDiffProcessingException(Exception):
     pass
 
 
 class BatchReviewRequestResource(WebAPIResource):
     """Resource for creating a series of review requests with a single request.
 
     Submitting multiple review requests using the traditional Web API requires
     several HTTP requests and the count grows in proportion to the number of
     review requests being submitted. In addition, changes are only atomic
     within each HTTP request. This API exists to make submitting a review
     series a single HTTP request while also being atomic.
+
+    Each commit will become its own review request.
+    Additionally, a review request with a diff encompassing all the commits
+    will be created; This "squashed" review request will represent the push
+    for the provided ``identifier``.
+
+    The ``identifier`` is a unique string which represents a series of pushed
+    commit sets. This identifier is used to update review requests with a new
+    set of diffs from a new push. Generally this identifier will represent
+    some unit of work, such as a bug.
+
+    The ``commits`` argument takes the following form::
+
+        {
+            'squashed': {
+                'diff': <squashed-diff-string>,
+                'base_commit_id': <commit-id-to-apply-diff-to> (optional),
+                'first_public_ancestor': <commit of first public ancestor> (optional),
+            },
+            'individual': [
+                {
+                    'id': <commit-id>,
+                    'precursors': [<previous changeset>],
+                    'message': <commit-message>,
+                    'diff': <diff>,
+                    'bug': <bug-id>,
+                    'parent_diff': <diff-from-base-to-commit> (optional),
+                    'base_commit_id': <commit-id-to-apply-diffs-to> (optional),
+                    'first_public_ancestor': <commit of first public ancestor> (optional),
+                    'reviewers': [<user1>, <user2>, ...] (optional),
+                    'requal_reviewers': [<user1>, <user2>, ...] (optional),
+                },
+                {
+                    ...
+                },
+                ...
+            ]
+        }
+
+    When representing the commits on Review Board, we store meta data in the
+    extra_data dictionaries. We use a number of fields to keep track of review
+    requests and the state they are in.
+
+    The following ASCII Venn Diagram represents sets the related review requests
+    may be in and how they overlap.
+
+    Legend:
+
+    * "unpublished_rids" = squashed_rr.extra_data['p2rb.unpublished_rids']
+    * "discard_on_publish_rids" = squashed_rr.extra_data['p2rb.discard_on_publish_rids']
+    * "squashed.commits" = squashed_rr.extra_data['p2rb.commits']
+    * "draft.commits" = squashed_rr.draft.extra_data['p2rb.commits']
+
+    * A = unpublished_rids - draft.commits
+    * B = draft.commits - squashed.commits
+    * C = draft.commits - unpublished rids
+    * D = delete_on_publish_rids
+
+    Diagram::
+
+                unpublished_rids                       squashed.commits
+         ________________________________________________________________
+        |                             |                                  |
+        |                             |                                  |
+        |                _____________|_____________                     |
+        |               |             |             |                    |
+        |        A      |       draft.commits       |           D        |
+        |               |             |             |                    |
+        |               |             |             |                    |
+        |               |      B      |        C    |                    |
+        |               |             |             |                    |
+        |               |             |             |                    |
+        |               |_____________|_____________|                    |
+        |                             |                                  |
+        |                             |         discard_on_publish_rids  |
+        |                             |                                  |
+        |_____________________________|__________________________________|
+
+
+    The following rules should apply to the review request sets when publishing
+    or discarding.
+
+    When publishing the squashed review request:
+
+    * A: close "discarded" because it was never used
+    * B: publish draft
+    * C: publish draft
+    * D: close "discarded" because it is obsolete
+    * set unpublished_rids to empty '[]'
+    * set discard_on_publish_rids to empty '[]'
+
+    When discarding the draft of a published squashed review request:
+
+    * A: close "discarded" because it was never used (so it does not appear in
+      the owners dashboard)
+    * B: close "discarded" because it was never used (so it does not appear in
+      the owners dashboard)
+    * C: DELETE the review request draft
+    * D: do nothing
+    * set unpublished_rids to empty '[]'
+    * set discard_on_publish_rids to empty '[]'
+
+    When discarding an unpublished squashed review request (always a close "discarded"):
+
+    * TODO Bug 1047465
     """
     name = 'batch_review_request'
     allowed_methods = ('GET', 'POST',)
 
     @webapi_check_local_site
     @webapi_login_required
     @webapi_response_errors(INVALID_FORM_DATA, NOT_LOGGED_IN, PERMISSION_DENIED)
     @webapi_request_fields(
@@ -119,24 +235,25 @@ class BatchReviewRequestResource(WebAPIR
             logger.warn('repo %d does not exist' % repo_id)
             return INVALID_FORM_DATA, {'fields': {'repo_id': 'Invalid repo_id'}}
 
         if not repo.is_accessible_by(user):
             return self.get_no_access_error(request)
 
         try:
             with transaction.atomic():
-                squashed_rr = self._process_submission(
+                squashed_rr, node_to_rid, review_data, warnings = self._process_submission(
                     request, local_site, repo, identifier, commits)
-
                 return 200, {
                     self.item_result_key: {
+                        'nodes': node_to_rid,
                         'squashed_rr': squashed_rr.id,
-                    }
-                }
+                        'review_requests': review_data,
+                        'warnings': warnings,
+                    }}
 
         # Need to catch this outside the transaction so db changes are rolled back.
         except SquashedDiffProcessingException:
             return INVALID_FORM_DATA, {
                 'fields': {'commits': 'error processing squashed diff'}}
 
     def _process_submission(self, request, local_site, repo, identifier, commits):
         user = request.user
@@ -221,16 +338,20 @@ class BatchReviewRequestResource(WebAPIR
         node_to_rid = {}
 
         # A mapping from review request id to the corresponding ReviewRequest.
         review_requests = {}
 
         # A mapping of review request id to dicts of additional metadata.
         review_data = {}
 
+        squashed_reviewers = set()
+
+        warnings = []
+
         # Do a pass and find all commits that map cleanly to old review requests.
         for commit in commits['individual']:
             node = commit['id']
 
             if node not in remaining_nodes:
                 continue
 
             # If the commit appears in an old review request, by definition of
@@ -264,31 +385,153 @@ class BatchReviewRequestResource(WebAPIR
                 rid = remaining_nodes.get(precursor)
                 if not rid:
                     continue
 
                 del remaining_nodes[precursor]
                 unclaimed_rids.remove(rid)
 
                 rr = ReviewRequest.objects.get(pk=rid)
-                # TODO implement
-                #update_review_request(rr, commit)
+                draft, warns = update_review_request(local_site, request, rr, commit)
+                squashed_reviewers.update(u for u in draft.target_people.all())
+                warnings.extend(warns)
                 processed_nodes.add(node)
                 node_to_rid[node] = rid
                 review_requests[rid] = rr
                 review_data[rid] = get_review_request_data(rr)
 
                 try:
                     discard_on_publish_rids.remove(rid)
                 except ValueError:
                     pass
 
                 break
 
-        return squashed_rr
+        # Now do a pass over the commits that didn't map cleanly.
+        for commit in commits['individual']:
+            node = commit['id']
+            if node in processed_nodes:
+                continue
+
+            # We haven't seen this commit before *and* our mapping above didn't
+            # do anything useful with it.
+
+            # This is where things could get complicated. We could involve
+            # heuristic based matching (comparing commit messages, changed
+            # files, etc). We may do that in the future.
+
+            # For now, match the commit up against the next one in the index.
+            # The unclaimed rids list contains review requests which were created
+            # when previously updating this review identifier, but not published.
+            # If we have more commits than were previously published we'll start
+            # reusing these private review requests before creating new ones.
+            if unclaimed_rids:
+                assumed_old_rid = unclaimed_rids.pop(0)
+                rr = ReviewRequest.objects.get(pk=assumed_old_rid)
+                draft, warns = update_review_request(local_site, request, rr, commit)
+                squashed_reviewers.update(u for u in draft.target_people.all())
+                warnings.extend(warns)
+                processed_nodes.add(commit['id'])
+                node_to_rid[node] = assumed_old_rid
+                review_requests[assumed_old_rid] = rr
+                review_data[assumed_old_rid] = get_review_request_data(rr)
+
+                try:
+                    discard_on_publish_rids.remove(assumed_old_rid)
+                except ValueError:
+                    pass
+
+                continue
+
+            # There are no more unclaimed review request IDs. This means we have
+            # more commits than before. Create new review requests as appropriate.
+            rr = ReviewRequest.objects.create(user=user,
+                                              repository=repo,
+                                              commit_id=None,
+                                              local_site=local_site)
+
+            rr.extra_data['p2rb'] = True
+            rr.extra_data['p2rb.is_squashed'] = False
+            rr.extra_data['p2rb.identifier'] = identifier
+            rr.save(update_fields=['extra_data'])
+            logger.info('created commit review request #%d' % rr.id)
+            draft, warns = update_review_request(local_site, request, rr, commit)
+            squashed_reviewers.update(u for u in draft.target_people.all())
+            warnings.extend(warns)
+            processed_nodes.add(commit['id'])
+            node_to_rid[node] = rr.id
+            review_requests[rr.id] = rr
+            review_data[rr.id] = get_review_request_data(rr)
+            unpublished_rids.append(rr.id)
+
+        # At this point every incoming commit has been accounted for.
+        # If there are any remaining review requests, they must belong to
+        # deleted commits. (Or, we made a mistake and updated the wrong review
+        # request)
+        for rid in unclaimed_rids:
+            rr = ReviewRequest.objects.get(pk=rid)
+
+            if rr.public and rid not in discard_on_publish_rids:
+                # This review request has already been published so we'll need to
+                # discard it when we publish the squashed review request.
+                discard_on_publish_rids.append(rid)
+            elif not rr.public and rid not in unpublished_rids:
+                # We've never published this review request so it may be reused in
+                # the future for *any* commit. Keep track of it.
+                unpublished_rids.append(rid)
+            else:
+                # This means we've already marked the review request properly
+                # in a previous push, so do nothing.
+                pass
+
+        commit_list = []
+        for commit in commits['individual']:
+            node = commit['id']
+            commit_list.append([node, node_to_rid[node]])
+
+        # We need to refresh the squashed rr and draft because post save hooks
+        # in ReviewBoard result in magical changes to some of its fields.
+        squashed_rr = ReviewRequest.objects.get(pk=squashed_rr.id)
+        squashed_draft = squashed_rr.draft.get()
+
+        squashed_draft.summary = identifier
+
+        # Reviewboard does not allow review requests with empty descriptions to
+        # be published, so we insert some filler here.
+        squashed_draft.description = 'This is the parent review request'
+        squashed_draft.bugs_closed = ','.join(sorted(set(commit['bug'] for commit in commits['individual'])))
+
+        squashed_draft.depends_on.clear()
+        for rrid in sorted(node_to_rid.values()):
+            rr = ReviewRequest.objects.for_id(rrid, local_site)
+            squashed_draft.depends_on.add(rr)
+
+        squashed_draft.target_people.clear()
+        for user in sorted(squashed_reviewers):
+            squashed_draft.target_people.add(user)
+
+        squashed_draft.extra_data['p2rb.commits'] = json.dumps(commit_list)
+
+        if 'base_commit_id' in commits['squashed']:
+            squashed_draft.extra_data['p2rb.base_commit'] = commits['squashed']['base_commit_id']
+
+        squashed_rr.extra_data.update({
+            'p2rb.discard_on_publish_rids': json.dumps(discard_on_publish_rids),
+            'p2rb.unpublished_rids': json.dumps(unpublished_rids),
+            'p2rb.first_public_ancestor': commits['squashed']['first_public_ancestor'],
+        })
+
+        squashed_draft.save()
+        squashed_rr.save(update_fields=['extra_data'])
+
+        review_requests[squashed_rr.id] = squashed_rr
+        review_data[squashed_rr.id] = get_review_request_data(squashed_rr)
+
+        return squashed_rr, node_to_rid, review_data, warnings
+
 
 batch_review_request_resource = BatchReviewRequestResource()
 
 
 def update_diffset_history(rr, diffset):
     """Update the diffset revision from a review request.
 
     This should be called when creating new DiffSet so that the new item
@@ -325,25 +568,148 @@ def update_review_request_draft_diffset(
     draft.save()
 
     if discarded_diffset:
         discarded_diffset.delete()
 
     return draft
 
 
+def previous_reviewers(rr):
+    """Return the result of the most recent review given by each reviewer"""
+
+    # TODO: Ideally this would get approval state for each reviewer as
+    # calculated in the MozReviewApprovalHook to be absolutely sure we're
+    # consistent. For now, we duplicate that logic here.
+    pr = {}
+    for review in gen_latest_reviews(rr):
+        pr[review.user.username] = review.ship_it
+
+    return pr
+
+
+def get_user_from_reviewer(request, reviewer):
+    for backend in get_enabled_auth_backends():
+        backend.query_users(reviewer, request)
+
+    q = Q(username__icontains=reviewer)
+    q = q | Q(is_active=True)
+
+    users = User.objects.filter(q).all()
+    # Try exact username match.
+    for user in users:
+        if user.username == reviewer:
+            return user
+
+    # Try case insensitive username match.
+    for user in users:
+        if user.username.lower() == reviewer.lower():
+            return user
+
+    return None
+
+
+def resolve_reviewers(request, requested_reviewers):
+    reviewers = set()
+    unrecognized = set()
+    # TODO track mapping for multiple review requests and cache results.
+    for reviewer in requested_reviewers:
+        user = get_user_from_reviewer(request, reviewer)
+        if user:
+            if not any(u.username == user.username for u in reviewers):
+                reviewers.add(user)
+        else:
+            unrecognized.add(reviewer)
+
+    return reviewers, unrecognized
+
+
+def update_review_request(local_site, request, rr, commit):
+    """Synchronize the state of a review request with a commit.
+
+    Updates the commit message, refreshes the diff, etc.
+    """
+    try:
+        draft = rr.draft.get()
+    except ReviewRequestDraft.DoesNotExist:
+        draft = ReviewRequestDraft.create(rr)
+
+    draft.summary = commit['message'].splitlines()[0]
+    draft.description = commit['message']
+    draft.bugs_closed = commit['bug']
+    draft.extra_data['p2rb.commit_id'] = commit['id']
+    draft.extra_data['p2rb.first_public_ancestor'] = commit['first_public_ancestor']
+
+    reviewer_users, unrecognized_reviewers = \
+        resolve_reviewers(request, commit.get('reviewers', []))
+    requal_reviewer_users, unrecognized_requal_reviewers = \
+        resolve_reviewers(request, commit.get('requal_reviewers', []))
+
+    warnings = []
+
+    for reviewer in unrecognized_reviewers | unrecognized_requal_reviewers:
+        warnings.append('unrecognized reviewer: %s' % reviewer)
+        logger.info('unrecognized reviewer: %s' % reviewer)
+
+    if requal_reviewer_users:
+        pr = previous_reviewers(rr)
+        for user in requal_reviewer_users:
+            if not pr.get(user.username, False):
+                warnings.append('commit message for %s has r=%s but they '
+                                'have not granted a ship-it. review will be '
+                                'requested on your behalf' % (
+                                commit['id'][:12], user.username))
+
+        reviewer_users |= requal_reviewer_users
+
+    # Carry over from last time unless commit message overrules.
+    if reviewer_users:
+        draft.target_people.clear()
+    for user in sorted(reviewer_users):
+        draft.target_people.add(user)
+        logger.debug('adding reviewer %s to #%d' % (user.username, rr.id))
+
+    diffset = DiffSet.objects.create_from_data(
+        repository=rr.repository,
+        diff_file_name='diff',
+        diff_file_contents=commit['diff'],
+        parent_diff_file_name='diff',
+        parent_diff_file_contents=commit.get('parent_diff'),
+        diffset_history=None,
+        basedir='',
+        request=request,
+        base_commit_id=commit.get('base_commit_id'),
+        save=True,
+    )
+    update_diffset_history(rr, diffset)
+    diffset.save()
+
+    discarded_diffset = None
+    if draft.diffset and draft.diffset != diffset:
+        discarded_diffset = draft.diffset
+
+    draft.diffset = diffset
+    draft.save()
+
+    if discarded_diffset:
+        discarded_diffset.delete()
+
+    return draft, warnings
+
+
+
 def get_review_request_data(rr):
     """Obtain a dictionary containing review request metadata.
 
     The dict consists of plain types (as opposed to ReviewBoard types).
 
     Some values may be unicode, not str.
     """
     rd = {
-        'status': rr.status,
+        'status': status_to_string(rr.status),
     }
 
     thing = rr
     try:
         thing = rr.draft.get()
         rd['public'] = False
     except ReviewRequestDraft.DoesNotExist:
         rd['public'] = rr.public
--- a/pylib/reviewboardmods/reviewboardmods/pushhooks.py
+++ b/pylib/reviewboardmods/reviewboardmods/pushhooks.py
@@ -4,458 +4,54 @@ This module contains code for taking com
 Mercurial, etc) and adding them to Review Board.
 
 It is intended for this module to be generic and applicable to any
 Review Board install. Please abstract away Mozilla implementation
 details.
 """
 
 from contextlib import contextmanager
-import datetime
 import os
 import json
 import tempfile
 
 from rbtools.api.client import RBClient
 from rbtools.api.errors import APIError
 from rbtools.api.transport.sync import SyncTransport
 
 
 def post_reviews(url, repoid, identifier, commits, hgresp,
                  username=None, apikey=None):
     """Post a set of commits to Review Board.
 
     Repository hooks can use this function to post a set of pushed commits
-    to Review Board. Each commit will become its own review request.
-    Additionally, a review request with a diff encompassing all the commits
-    will be created; This "squashed" review request will represent the push
-    for the provided ``identifier``.
-
-    The ``identifier`` is a unique string which represents a series of pushed
-    commit sets. This identifier is used to update review requests with a new
-    set of diffs from a new push. Generally this identifier will represent
-    some unit of work, such as a bug.
-
-    The ``commits`` argument takes the following form::
-
-        {
-            'squashed': {
-                'diff': <squashed-diff-string>,
-                'base_commit_id': <commit-id-to-apply-diff-to> (optional),
-                'first_public_ancestor': <commit of first public ancestor> (optional),
-            },
-            'individual': [
-                {
-                    'id': <commit-id>,
-                    'precursors': [<previous changeset>],
-                    'message': <commit-message>,
-                    'diff': <diff>,
-                    'bug': <bug-id>,
-                    'parent_diff': <diff-from-base-to-commit> (optional),
-                    'base_commit_id': <commit-id-to-apply-diffs-to> (optional),
-                    'first_public_ancestor': <commit of first public ancestor> (optional),
-                    'reviewers': [<user1>, <user2>, ...] (optional),
-                    'requal_reviewers': [<user1>, <user2>, ...] (optional),
-                },
-                {
-                    ...
-                },
-                ...
-            ]
-        }
-
-    When representing the commits on Review Board, we store meta data in the
-    extra_data dictionaries. We use a number of fields to keep track of review
-    requests and the state they are in.
-
-    The following ASCII Venn Diagram represents sets the related review requests
-    may be in and how they overlap.
-
-    Legend:
-
-    * "unpublished_rids" = squashed_rr.extra_data['p2rb.unpublished_rids']
-    * "discard_on_publish_rids" = squashed_rr.extra_data['p2rb.discard_on_publish_rids']
-    * "squashed.commits" = squashed_rr.extra_data['p2rb.commits']
-    * "draft.commits" = squashed_rr.draft.extra_data['p2rb.commits']
-
-    * A = unpublished_rids - draft.commits
-    * B = draft.commits - squashed.commits
-    * C = draft.commits - unpublished rids
-    * D = delete_on_publish_rids
-
-    Diagram::
-
-                unpublished_rids                       squashed.commits
-         ________________________________________________________________
-        |                             |                                  |
-        |                             |                                  |
-        |                _____________|_____________                     |
-        |               |             |             |                    |
-        |        A      |       draft.commits       |           D        |
-        |               |             |             |                    |
-        |               |             |             |                    |
-        |               |      B      |        C    |                    |
-        |               |             |             |                    |
-        |               |             |             |                    |
-        |               |_____________|_____________|                    |
-        |                             |                                  |
-        |                             |         discard_on_publish_rids  |
-        |                             |                                  |
-        |_____________________________|__________________________________|
-
-
-    The following rules should apply to the review request sets when publishing
-    or discarding.
-
-    When publishing the squashed review request:
-
-    * A: close "discarded" because it was never used
-    * B: publish draft
-    * C: publish draft
-    * D: close "discarded" because it is obsolete
-    * set unpublished_rids to empty '[]'
-    * set discard_on_publish_rids to empty '[]'
-
-    When discarding the draft of a published squashed review request:
-
-    * A: close "discarded" because it was never used (so it does not appear in
-      the owners dashboard)
-    * B: close "discarded" because it was never used (so it does not appear in
-      the owners dashboard)
-    * C: DELETE the review request draft
-    * D: do nothing
-    * set unpublished_rids to empty '[]'
-    * set discard_on_publish_rids to empty '[]'
-
-    When discarding an unpublished squashed review request (always a close "discarded"):
-
-    * TODO Bug 1047465
+    to Review Board.
     """
     with ReviewBoardClient(url, username=username, apikey=apikey) as rbc:
         root = rbc.get_root()
         return _post_reviews(root, repoid, identifier, commits, hgresp)
 
 def _post_reviews(api_root, repoid, identifier, commits, hgresp):
     batch_request_resource = api_root.get_extension(
         extension_name='mozreview.extension.MozReviewExtension')\
         .get_batch_review_requests()
     series_result = batch_request_resource.create(
         # This assumes that we pushed to the repository/URL that Review Board is
         # configured to use. This assumption may not always hold.
         repo_id=repoid,
         identifier=identifier,
         commits=json.dumps(commits, encoding='utf-8'))
 
-    # Retrieve the squashed review request or create it.
-    rrs = api_root.get_review_requests(commit_id=identifier,
-                                       repository=repoid)
-
-    squashed_reviewers = set()
-    squashed_rr = rrs[0]
-    assert squashed_rr.id == series_result.squashed_rr
-
-    def extract_reviewers(requested_reviewers):
-        reviewers = set()
-        for reviewer in requested_reviewers:
-            # we've already seen this reviewer so we can just add them as a
-            # reviewer for this commit.
-            if reviewer in squashed_reviewers:
-                reviewers.add(reviewer)
-                continue
-
-            try:
-                # Check to see if this user exists (and sync things up
-                # between reviewboard and bugzilla, if necessary).
-                r = api_root.get_users(q=reviewer)
-                rsp_users = r.rsp['users']
-
-                # first try exact match
-                for user in rsp_users:
-                    username = user['username']
-                    if username == reviewer:
-                        reviewers.add(username)
-                        squashed_reviewers.add(username)
-                        break
-                else:
-                    # then attempt case insensitive match
-                    for user in rsp_users:
-                        username = user['username']
-                        if username.lower() == reviewer.lower():
-                            reviewers.add(username)
-                            squashed_reviewers.add(username)
-                            break
-                    else:
-                        hgresp.append('display unrecognized reviewer: %s' %
-                                      reviewer)
-            except APIError as e:
-                if e.http_status == 404 and e.error_code == 100:
-                    hgresp.append('display unrecognized reviewer: %s' %
-                                  str(reviewer))
-                else:
-                    hgresp.append('display error identifying reviewer: %s: %s' %
-                                  (str(reviewer), str(e)))
-
-        return sorted(reviewers)
-
-    def update_review_request(rr, commit):
-        """Synchronize the state of a review request with a commit.
-
-        Updates the commit message, refreshes the diff, etc.
-        """
-        props = {
-            # Mercurial guarantees UTF-8 internally, so no need to "replace"
-            # during decode.
-            "summary": commit['message'].decode('utf-8').splitlines()[0],
-            "bugs_closed": commit['bug'],
-            "description": commit['message'].decode('utf-8'),
-            "extra_data.p2rb.commit_id": commit['id'],
-            "extra_data.p2rb.first_public_ancestor": commit['first_public_ancestor'],
-        }
-        reviewers = extract_reviewers(commit.get('reviewers', []))
-
-        requal_reviewers = extract_reviewers(commit.get('requal_reviewers', []))
-        if requal_reviewers:
-            pr = previous_reviewers(rr)
-            for reviewer in requal_reviewers:
-                # We've claimed a r=, but the most recent review was not
-                # a ship-it
-                if not pr.get(reviewer, False):
-                    hgresp.append('display commit message for %s has r=%s '
-                                  'but they have not granted a ship-it. '
-                                  'review will be requested on your behalf'
-                                  % ((str(commit['id'][:12]), str(reviewer))))
-
-        reviewers.extend(requal_reviewers)
-
-        if reviewers:
-            props["target_people"] = ','.join(reviewers)
-        draft = rr.get_or_create_draft(**props)
-
-        rr.get_diffs().upload_diff(
-            commit['diff'],
-            parent_diff=commit.get('parent_diff', None),
-            base_commit_id=commit.get('base_commit_id', None))
-
-    def get_review_data(rr):
-        """Obtain a dictionary containing review metadata.
-
-        The dict consists of plain types (as opposed to RBTools types).
-
-        Some values may be unicode, not str.
-        """
-        rd = {
-            'status': rr.status,
-        }
-
-        thing = rr
-        try:
-            thing = rr.get_draft()
-        except APIError as e:
-            # error_code 100 is RB API code for "does not exist."
-            if e.http_status != 404 or e.error_code != 100:
-                raise
-
-        rd['reviewers'] = [p.title for p in thing.target_people]
-        rd['public'] = thing.public
-
-        return rd
-
-    # TODO: We need to take into account the commits data from the squashed
-    # review request's draft. This data represents the mapping from commit
-    # to rid in the event that we would have published. We're overwritting
-    # this data. This will only come into play if we start trusting the server
-    # instead of the client when matching review request ids. Bug 1047516
-
-    previous_commits = get_previous_commits(squashed_rr)
-    remaining_nodes = get_remaining_nodes(previous_commits)
-    discard_on_publish_rids = get_discard_on_publish_rids(squashed_rr)
-    unpublished_rids = get_unpublished_rids(squashed_rr)
-    unclaimed_rids = get_unclaimed_rids(previous_commits,
-                                        discard_on_publish_rids,
-                                        unpublished_rids)
-
-    # Previously pushed nodes which have been processed and had their review
-    # request updated or did not require updating.
-    processed_nodes = set()
-
-    node_to_rid = {}
-
-    # A mapping from review request id to the corresponding review request
-    # API object.
-    review_requests = {}
-
-    # A mapping of review request id to dicts of additional metadata.
-    review_data = {}
-
-    # Do a pass and find all commits that map cleanly to old review requests.
-    for commit in commits['individual']:
-        node = commit['id']
-
-        if node not in remaining_nodes:
-            continue
+    for w in series_result.warnings:
+        hgresp.append(b'display %s' % w.encode('utf-8'))
 
-        # If the commit appears in an old review request, by definition of
-        # commits deriving from content, the commit has not changed and there
-        # is nothing to update. Update our accounting and move on.
-        rid = remaining_nodes[node]
-        del remaining_nodes[node]
-        unclaimed_rids.remove(rid)
-        processed_nodes.add(node)
-        node_to_rid[node] = rid
-
-        rr = api_root.get_review_request(review_request_id=rid)
-        review_requests[rid] = rr
-        review_data[rid] = get_review_data(rr)
-
-        try:
-            discard_on_publish_rids.remove(rid)
-        except ValueError:
-            pass
-
-    # Find commits that map to a previous version.
-    for commit in commits['individual']:
-        node = commit['id']
-        if node in processed_nodes:
-            continue
-
-        # The client may have sent obsolescence data saying which commit this
-        # commit has derived from. Use that data (if available) to try to find
-        # a mapping to an old review request.
-        for precursor in commit['precursors']:
-            rid = remaining_nodes.get(precursor)
-            if not rid:
-                continue
-
-            del remaining_nodes[precursor]
-            unclaimed_rids.remove(rid)
-
-            rr = api_root.get_review_request(review_request_id=rid)
-            update_review_request(rr, commit)
-            processed_nodes.add(node)
-            node_to_rid[node] = rid
-            review_requests[rid] = rr
-            review_data[rid] = get_review_data(rr)
-
-            try:
-                discard_on_publish_rids.remove(rid)
-            except ValueError:
-                pass
-
-            break
-
-    # Now do a pass over the commits that didn't map cleanly.
-    for commit in commits['individual']:
-        node = commit['id']
-        if node in processed_nodes:
-            continue
-
-        # We haven't seen this commit before *and* our mapping above didn't
-        # do anything useful with it.
-
-        # This is where things could get complicated. We could involve
-        # heuristic based matching (comparing commit messages, changed
-        # files, etc). We may do that in the future.
-
-        # For now, match the commit up against the next one in the index.
-        # The unclaimed rids list contains review requests which were created
-        # when previously updating this review identifier, but not published.
-        # If we have more commits than were previously published we'll start
-        # reusing these private review requests before creating new ones.
-        if unclaimed_rids:
-            assumed_old_rid = unclaimed_rids[0]
-            unclaimed_rids.pop(0)
-            rr = api_root.get_review_request(review_request_id=assumed_old_rid)
-            update_review_request(rr, commit)
-            processed_nodes.add(commit['id'])
-            node_to_rid[node] = assumed_old_rid
-            review_requests[assumed_old_rid] = rr
-            review_data[assumed_old_rid] = get_review_data(rr)
+    nodes = {node.encode('utf-8'): str(rid)
+             for node, rid in series_result.nodes.iteritems()}
 
-            try:
-                discard_on_publish_rids.remove(assumed_old_rid)
-            except ValueError:
-                pass
-
-            continue
-
-        # There are no more unclaimed review request IDs. This means we have
-        # more commits than before. Create new review requests as appropriate.
-        rr = rrs.create(**{
-            'extra_data.p2rb': 'True',
-            'extra_data.p2rb.is_squashed': 'False',
-            'extra_data.p2rb.identifier': identifier,
-            'repository': repoid,
-        })
-        update_review_request(rr, commit)
-        processed_nodes.add(commit['id'])
-        assert isinstance(rr.id, int)
-        node_to_rid[node] = rr.id
-        review_requests[rr.id] = rr
-        review_data[rr.id] = get_review_data(rr)
-        unpublished_rids.append(rr.id)
-
-    # At this point every incoming commit has been accounted for.
-    # If there are any remaining review requests, they must belong to
-    # deleted commits. (Or, we made a mistake and updated the wrong review
-    # request)
-    for rid in unclaimed_rids:
-        rr = api_root.get_review_request(review_request_id=rid)
-
-        if rr.public and rid not in discard_on_publish_rids:
-            # This review request has already been published so we'll need to
-            # discard it when we publish the squashed review request.
-            discard_on_publish_rids.append(rid)
-        elif not rr.public and rid not in unpublished_rids:
-            # We've never published this review request so it may be reused in
-            # the future for *any* commit. Keep track of it.
-            unpublished_rids.append(rid)
-        else:
-            # This means we've already marked the review request properly
-            # in a previous push, so do nothing.
-            pass
-
-    commit_list = []
-    for commit in commits['individual']:
-        node = commit['id']
-        commit_list.append([node, node_to_rid[node]])
-
-    commit_list_json = json.dumps(commit_list)
-    depends = ','.join(str(i) for i in sorted(node_to_rid.values()))
-    all_bugs = ','.join(set(commit['bug'] for commit in commits['individual']))
-
-    props = {
-        'summary': identifier,
-        'bugs_closed': all_bugs,
-        # Reviewboard does not allow review requests with empty descriptions to
-        # be published, so we insert some filler here.
-        'description': u'This is the parent review request',
-        'depends_on': depends,
-        'extra_data.p2rb.commits': commit_list_json,
-    }
-
-    if 'base_commit_id' in commits['squashed']:
-        props['extra_data.p2rb.base_commit'] = (
-            commits['squashed']['base_commit_id'])
-
-    if squashed_reviewers:
-        props['target_people'] = ','.join(sorted(squashed_reviewers))
-    squashed_draft = squashed_rr.get_or_create_draft(**props)
-
-    squashed_rr.update(**{
-        'extra_data.p2rb.discard_on_publish_rids': json.dumps(
-            discard_on_publish_rids),
-        'extra_data.p2rb.unpublished_rids': json.dumps(
-            unpublished_rids),
-        'extra_data.p2rb.first_public_ancestor': commits['squashed']['first_public_ancestor'],
-    })
-
-    review_requests[squashed_rr.id] = squashed_rr
-    review_data[squashed_rr.id] = get_review_data(squashed_rr)
-
-    return squashed_rr.id, node_to_rid, review_data
+    return str(series_result.squashed_rr), nodes, series_result.review_requests
 
 
 def associate_ldap_username(url, ldap_username, privileged_username,
                             privileged_password, username, apikey):
     """Associate a Review Board user with an ldap username.
 
     Will return True if an ldap_username is successfully associated
     with a Review Board account, False otherwise.
@@ -553,123 +149,8 @@ def ReviewBoardClient(url, username=None
                            cookie_file=path, transport_cls=NoCacheTransport)
 
         yield rbc
     finally:
         try:
             os.unlink(path)
         except Exception:
             pass
-
-
-def get_previous_commits(squashed_rr):
-    """Retrieve the previous commits from a squashed review request.
-
-    This will return a list of tuples specifying the previous commit
-    id as well as the review request it is represented by. ex::
-
-        [
-            # (<commit-id>, <review-request-id>),
-            ('d4bd89322f54', 13),
-            ('373537353134', 14),
-        ]
-    """
-    extra_data = None
-
-    if not squashed_rr.public:
-        extra_data = squashed_rr.get_draft().extra_data
-    else:
-        extra_data = squashed_rr.extra_data
-
-    if 'p2rb.commits' not in extra_data:
-        return []
-
-    commits = []
-    for node, rid in json.loads(extra_data['p2rb.commits']):
-        # JSON decoding likes to give us unicode types. We speak str
-        # internally, so convert.
-        if isinstance(node, unicode):
-            node = node.encode('utf-8')
-
-        assert isinstance(node, str)
-
-        commits.append((node, int(rid)))
-
-    return commits
-
-
-def get_remaining_nodes(previous_commits):
-    """A mapping from previously pushed node, which has not been processed
-    yet, to the review request id associated with that node.
-    """
-    return dict((t[0], t[1]) for i, t in enumerate(previous_commits))
-
-
-def get_discard_on_publish_rids(squashed_rr):
-    """A list of review request ids that should be discarded when publishing.
-    Adding to this list will mark a review request as to-be-discarded when
-    the squashed draft is published on Review Board.
-    """
-    return map(int, json.loads(
-               squashed_rr.extra_data['p2rb.discard_on_publish_rids']))
-
-
-def get_unpublished_rids(squashed_rr):
-    """A list of review request ids that have been created for individual commits
-    but have not been published. If this list contains an item, it should be
-    re-used for indiviual commits instead of creating a brand new review
-    request.
-    """
-    return map(int, json.loads(
-               squashed_rr.extra_data['p2rb.unpublished_rids']))
-
-
-def get_unclaimed_rids(previous_commits, discard_on_publish_rids,
-                       unpublished_rids):
-    """Set of review request ids which have not been matched to a commit
-    from the current push. We use a list to represent this set because
-    if any entries are left over we need to process them in order.
-    This list includes currently published rids that were part of the
-    previous push and rids which have been used for drafts on this
-    reviewid but have not been published.
-    """
-    unclaimed_rids = [t[1] for t in previous_commits]
-
-    for rid in (discard_on_publish_rids + unpublished_rids):
-        if rid not in unclaimed_rids:
-            unclaimed_rids.append(rid)
-
-    return unclaimed_rids
-
-
-def previous_reviewers(rr):
-    """Return the result of the most recent review given by each reviewer"""
-
-    # TODO: Ideally this would hit an API endpoint that exposes the approval
-    #       state for each reviewer as calculated in the MozReviewApprovalHook
-    #       to be absolutely sure we're consistent. For now, we duplicate that
-    #       logic here.
-    start = 0
-    reviews = []
-    while True:
-        some_reviews = rr.get_reviews(start=start, max_results=50)
-        for review in some_reviews:
-            # We could use username = review.get_user()['username'] at a cost
-            # of one extra request per reviewer. For a review request with
-            # four reviews, this was twice as slow.
-            username = review.rsp['links']['user']['title']
-            if review['public']:
-                reviews.append((username, review['ship_it'], review['id']))
-        start += some_reviews.num_items
-        if start >= some_reviews.total_results:
-            break
-
-    # We seem to get reviews in sorted order already, but sort them anyway
-    # to be safe.
-    reviews = sorted(reviews, key=lambda x: int(x[2]))
-
-    pr = {}
-    for review in reviews:
-        # We only consider the most recent review from each reviewer, so we
-        # can overwrite any previous results here.
-        pr[review[0]] = review[1]
-
-    return pr