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
--- 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,