mozreview: don't reuse review requests if commit is new (bug 1242246); r?dminor draft
authorGregory Szorc <gps@mozilla.com>
Fri, 05 Feb 2016 15:13:54 -0800
changeset 7122 e6283ed65eb3952bd4bca932c79d1c75a7b10cad
parent 7119 d0376ce2f372c431f473b43a1dda4ed7d115a9b6
child 7123 29c1717f6203321f11e1fe4a3cc98618846d11d8
push id597
push usergszorc@mozilla.com
push dateFri, 05 Feb 2016 23:15:12 +0000
reviewersdminor
bugs1242246
mozreview: don't reuse review requests if commit is new (bug 1242246); r?dminor test-specify-reviewers.t changed slightly because it has obsolescence enabled and exercises a code path where a new commit is pushed to an existing series. MozReview-Commit-ID: LRi48ckMsWq
hgext/reviewboard/tests/test-commits-deleted-obsolescence.t
hgext/reviewboard/tests/test-specify-reviewers.t
pylib/mozreview/mozreview/resources/batch_review_request.py
--- a/hgext/reviewboard/tests/test-commits-deleted-obsolescence.t
+++ b/hgext/reviewboard/tests/test-commits-deleted-obsolescence.t
@@ -761,17 +761,16 @@ The parent review should have been updat
     - '@@ -0,0 +1,1 @@'
     - +foo4
     - ''
   approved: false
   approval_failure: Commit eeb6d49dcb0950d771959358f662cf2e5ddc9dc1 is not approved.
 
 Deleting a commit and adding a commit will not recycle a review request
 because the new commit is logically different
-(TODO this is wrong - bug 1242246)
 
   $ hg -q up eeb6d49dcb09
 
   $ echo foo6 > foo6
   $ hg -q commit -A -m 'Bug 1 - Foo 6'
   $ hg --config mozreview.autopublish=true push
   pushing to ssh://$DOCKER_HOSTNAME:$HGPORT6/test-repo
   searching for changes
@@ -783,17 +782,17 @@ because the new commit is logically diff
   submitting 2 changesets for review
   
   changeset:  10:eeb6d49dcb09
   summary:    Bug 1 - Foo 2
   review:     http://$DOCKER_HOSTNAME:$HGPORT1/r/3
   
   changeset:  14:3b99865d1bab
   summary:    Bug 1 - Foo 6
-  review:     http://$DOCKER_HOSTNAME:$HGPORT1/r/5 (draft)
+  review:     http://$DOCKER_HOSTNAME:$HGPORT1/r/7 (draft)
   
   review id:  bz://1/mynick
   review url: http://$DOCKER_HOSTNAME:$HGPORT1/r/1 (draft)
   (review requests lack reviewers; visit review url to assign reviewers)
   (visit review url to publish these review requests so others can see them)
 
   $ rbmanage dumpreview 1
   id: 1
@@ -809,21 +808,21 @@ because the new commit is logically diff
   extra_data:
     calculated_trophies: true
     p2rb.reviewer_map: '{"3": [], "5": [], "4": []}'
   commit_extra_data:
     p2rb: true
     p2rb.base_commit: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
     p2rb.commits: '[["eeb6d49dcb0950d771959358f662cf2e5ddc9dc1", 3], ["a27a94c54524d4331dec2f92f647067bfd6dfbd4",
       5]]'
-    p2rb.discard_on_publish_rids: '[]'
+    p2rb.discard_on_publish_rids: '[5]'
     p2rb.first_public_ancestor: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
     p2rb.identifier: bz://1/mynick
     p2rb.is_squashed: true
-    p2rb.unpublished_rids: '[]'
+    p2rb.unpublished_rids: '[7]'
   diffs:
   - id: 1
     revision: 1
     base_commit_id: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
     name: diff
     extra: {}
     patch:
     - diff --git a/foo1 b/foo1
@@ -943,17 +942,17 @@ because the new commit is logically diff
     target_people: []
     extra:
       calculated_trophies: true
       p2rb.reviewer_map: '{"3": [], "5": []}'
     commit_extra_data:
       p2rb: true
       p2rb.base_commit: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
       p2rb.commits: '[["eeb6d49dcb0950d771959358f662cf2e5ddc9dc1", 3], ["3b99865d1bab8480235d913f4bcfc951fd9e3032",
-        5]]'
+        7]]'
       p2rb.discard_on_publish_rids: '[]'
       p2rb.first_public_ancestor: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
       p2rb.identifier: bz://1/mynick
       p2rb.is_squashed: true
       p2rb.unpublished_rids: '[]'
     diffs:
     - id: 14
       revision: 5
@@ -971,17 +970,16 @@ because the new commit is logically diff
       - new file mode 100644
       - '--- /dev/null'
       - +++ b/foo6
       - '@@ -0,0 +1,1 @@'
       - +foo6
       - ''
 
 Review request 5 (whose commit was deleted) should be discarded
-(TODO this is wrong)
 
   $ rbmanage dumpreview 5
   id: 5
   status: pending
   public: true
   bugs:
   - '1'
   commit: null
@@ -1037,37 +1035,55 @@ Review request 5 (whose commit was delet
     - new file mode 100644
     - '--- /dev/null'
     - +++ b/foo4
     - '@@ -0,0 +1,1 @@'
     - +foo4
     - ''
   approved: false
   approval_failure: A suitable reviewer has not given a "Ship It!"
+
+  $ rbmanage dumpreview 7
+  id: 7
+  status: pending
+  public: false
+  bugs: []
+  commit: null
+  submitter: default+5
+  summary: ''
+  description: ''
+  target_people: []
+  extra_data: {}
+  commit_extra_data:
+    p2rb: true
+    p2rb.identifier: bz://1/mynick
+    p2rb.is_squashed: false
+  diffs: []
+  approved: false
+  approval_failure: The review request is not public.
   draft:
     bugs:
     - '1'
     commit: null
     summary: Bug 1 - Foo 6
     description:
     - Bug 1 - Foo 6
     - ''
     - 'MozReview-Commit-ID: OTOPw0'
     target_people: []
-    extra:
-      calculated_trophies: true
+    extra: {}
     commit_extra_data:
       p2rb: true
       p2rb.commit_id: 3b99865d1bab8480235d913f4bcfc951fd9e3032
       p2rb.first_public_ancestor: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
       p2rb.identifier: bz://1/mynick
       p2rb.is_squashed: false
     diffs:
     - id: 15
-      revision: 4
+      revision: 1
       base_commit_id: eeb6d49dcb0950d771959358f662cf2e5ddc9dc1
       name: diff
       extra: {}
       patch:
       - diff --git a/foo6 b/foo6
       - new file mode 100644
       - '--- /dev/null'
       - +++ b/foo6
--- a/hgext/reviewboard/tests/test-specify-reviewers.t
+++ b/hgext/reviewboard/tests/test-specify-reviewers.t
@@ -564,24 +564,24 @@ Reviewer identification should be case i
   remote: adding manifests
   remote: adding file changes
   remote: added 1 changesets with 1 changes to 1 files
   remote: recorded push in pushlog
   submitting 1 changesets for review
   
   changeset:  28:dce90596f792
   summary:    Bug 2 - better stuff; r?ryanvm
-  review:     http://$DOCKER_HOSTNAME:$HGPORT1/r/13 (draft)
+  review:     http://$DOCKER_HOSTNAME:$HGPORT1/r/14 (draft)
   
   review id:  bz://2/mynick
   review url: http://$DOCKER_HOSTNAME:$HGPORT1/r/12 (draft)
   
   publish these review requests now (Yn)? y
   (published review request 12)
 
-  $ rbmanage list-reviewers 13
+  $ rbmanage list-reviewers 14
   RyanVM
 
 
 Cleanup
 
   $ mozreview stop
   stopped 9 containers
--- a/pylib/mozreview/mozreview/resources/batch_review_request.py
+++ b/pylib/mozreview/mozreview/resources/batch_review_request.py
@@ -553,17 +553,23 @@ class BatchReviewRequestResource(WebAPIR
             # 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:
+            #
+            # We don't reuse existing review requests when obsolescence data is
+            # available because the lack of a clean commit mapping (from above)
+            # means that the commit is logically new and shouldn't be
+            # associated with a review request that belonged to a different
+            # logical commit.
+            if unclaimed_rids and not commits.get('obsolescence', False):
                 assumed_old_rid = unclaimed_rids.pop(0)
 
                 logger.info('%s: mapping %s to unclaimed request %d' % (
                             identifier, node, assumed_old_rid))
 
                 rr = ReviewRequest.objects.get(pk=assumed_old_rid)
                 draft, warns = update_review_request(local_site, request,
                                                      privileged_user,