reviewboard: we should not reset reviewers if none are specified in commit summary (bug 1169499); r=gps
authorDan Minor <dminor@mozilla.com>
Fri, 29 May 2015 07:21:39 -0400
changeset 2465 665dfb80447a3dce6831482100fc3c1c02caafb6
parent 2464 ab987198243b006b3c308841535c5a815213caf7
child 2466 281da957477e6f93cac0cbe31b452dd38aee7d98
push id807
push userdminor@mozilla.com
push dateSat, 30 May 2015 02:28:56 +0000
treeherderversion-control-tools@665dfb80447a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgps
bugs1169499
reviewboard: we should not reset reviewers if none are specified in commit summary (bug 1169499); r=gps This is a symptom of a larger problem where specifying reviewers in the commit summary causes any reviewers specified in the UI to be lost. Solving that larger problem requires us to have a good means of identifying reviewers which were specified in the UI and which were specified in the commit summary. In the mean time, if no reviewers are specified in the commit summary we can skip setting the reviewers on the draft, which will preserve reviewers that were set in the UI if none are specified in the commit.
hgext/reviewboard/tests/test-specify-reviewers.t
pylib/reviewboardmods/reviewboardmods/pushhooks.py
--- a/hgext/reviewboard/tests/test-specify-reviewers.t
+++ b/hgext/reviewboard/tests/test-specify-reviewers.t
@@ -256,17 +256,83 @@ again.
   
   review id:  bz://1/mynick
   review url: http://*:$HGPORT1/r/1 (draft) (glob)
   (visit review url to publish this review request so others can see it)
   [1]
   $ rbmanage list-reviewers 11 --draft
   admin+1, remus, romulus
 
-Amending a commit will reset the reviewers back to the default.
+We should not overwrite manually added reviewers if the revision is amended 
+and pushed with no reviewers specified.
+
+  $ rbmanage list-reviewers 11 --draft
+  admin+1, remus, romulus
+  $ echo blah >> foo
+  $ hg commit --amend -m 'Bug 1 - Amended stuff'
+  $ hg push
+  pushing to ssh://*:$HGPORT6/test-repo (glob)
+  searching for changes
+  remote: adding changesets
+  remote: adding manifests
+  remote: adding file changes
+  remote: added 1 changesets with 1 changes to 1 files (+1 heads)
+  remote: Trying to insert into pushlog.
+  remote: Inserted into the pushlog db successfully.
+  submitting 10 changesets for review
+  
+  changeset:  11:fcf566e4c32a
+  summary:    Bug 1 - some stuff; r?romulus
+  review:     http://*:$HGPORT1/r/2 (draft) (glob)
+  
+  changeset:  12:c62a829e2f0a
+  summary:    Bug 1 - More stuff; r?romulus, r?remus
+  review:     http://*:$HGPORT1/r/3 (draft) (glob)
+  
+  changeset:  13:955576a13e6c
+  summary:    Bug 1 - More stuff; r?romulus,r?remus
+  review:     http://*:$HGPORT1/r/4 (draft) (glob)
+  
+  changeset:  14:696e908c00aa
+  summary:    Bug 1 - More stuff; r?romulus, remus
+  review:     http://*:$HGPORT1/r/5 (draft) (glob)
+  
+  changeset:  15:92e037a5e92f
+  summary:    Bug 1 - More stuff; r?romulus,remus
+  review:     http://*:$HGPORT1/r/6 (draft) (glob)
+  
+  changeset:  16:a7c3071c6b54
+  summary:    Bug 1 - More stuff; (r?romulus)
+  review:     http://*:$HGPORT1/r/7 (draft) (glob)
+  
+  changeset:  17:7b03b2560ab0
+  summary:    Bug 1 - More stuff; (r?romulus,remus)
+  review:     http://*:$HGPORT1/r/8 (draft) (glob)
+  
+  changeset:  18:42c4d67a510e
+  summary:    Bug 1 - More stuff; [r?romulus]
+  review:     http://*:$HGPORT1/r/9 (draft) (glob)
+  
+  changeset:  19:2bc874a070ce
+  summary:    Bug 1 - More stuff; [r?remus, r?romulus]
+  review:     http://*:$HGPORT1/r/10 (draft) (glob)
+  
+  changeset:  24:4a950181ffd8
+  summary:    Bug 1 - Amended stuff
+  review:     http://*:$HGPORT1/r/11 (draft) (glob)
+  
+  review id:  bz://1/mynick
+  review url: http://*:$HGPORT1/r/1 (draft) (glob)
+  (visit review url to publish this review request so others can see it)
+
+  $ rbmanage list-reviewers 11 --draft
+  admin+1, remus, romulus
+
+Amending a commit with reviewers specified will reset the reviewers back to
+those specified in the commit summary.
 
   $ echo blah >> foo
   $ hg commit --amend -m 'Bug 1 - Amended stuff; r?romulus, r?remus'
   $ hg push
   pushing to ssh://*:$HGPORT6/test-repo (glob)
   searching for changes
   remote: adding changesets
   remote: adding manifests
@@ -307,17 +373,17 @@ Amending a commit will reset the reviewe
   changeset:  18:42c4d67a510e
   summary:    Bug 1 - More stuff; [r?romulus]
   review:     http://*:$HGPORT1/r/9 (draft) (glob)
   
   changeset:  19:2bc874a070ce
   summary:    Bug 1 - More stuff; [r?remus, r?romulus]
   review:     http://*:$HGPORT1/r/10 (draft) (glob)
   
-  changeset:  24:9a37c8988ef7
+  changeset:  26:3ec3b449ccca
   summary:    Bug 1 - Amended stuff; r?romulus, r?remus
   review:     http://*:$HGPORT1/r/11 (draft) (glob)
   
   review id:  bz://1/mynick
   review url: http://*:$HGPORT1/r/1 (draft) (glob)
   (visit review url to publish this review request so others can see it)
   $ rbmanage list-reviewers 11 --draft
   remus, romulus
@@ -334,17 +400,17 @@ Unrecognized reviewers should be ignored
   remote: adding changesets
   remote: adding manifests
   remote: adding file changes
   remote: added 1 changesets with 1 changes to 1 files
   remote: Trying to insert into pushlog.
   remote: Inserted into the pushlog db successfully.
   submitting 1 changesets for review
   
-  changeset:  25:8b7822987cba
+  changeset:  27:d9a3b1783a10
   summary:    Bug 2 - different stuff; r?cthulhu
   review:     http://*:$HGPORT1/r/13 (draft) (glob)
   
   review id:  bz://2/mynick
   review url: http://*:$HGPORT1/r/12 (draft) (glob)
   (visit review url to publish this review request so others can see it)
   $ rbmanage list-reviewers 12 --draft
   
@@ -360,21 +426,21 @@ from the client.
   remote: adding changesets
   remote: adding manifests
   remote: adding file changes
   remote: added 1 changesets with 1 changes to 1 files
   remote: Trying to insert into pushlog.
   remote: Inserted into the pushlog db successfully.
   submitting 2 changesets for review
   
-  changeset:  25:8b7822987cba
+  changeset:  27:d9a3b1783a10
   summary:    Bug 2 - different stuff; r?cthulhu
   review:     http://*:$HGPORT1/r/13 (draft) (glob)
   
-  changeset:  26:2d366e692ea2
+  changeset:  28:c486e8175a60
   summary:    Bug 2 - different stuff; r=romulus
   (It appears you are using r= to specify reviewers for a patch under review. Please use r? to avoid ambiguity as to whether or not review has been granted.)
   review:     http://*:$HGPORT1/r/14 (draft) (glob)
   
   review id:  bz://2/mynick
   review url: http://*:$HGPORT1/r/12 (draft) (glob)
   (visit review url to publish this review request so others can see it)
  
--- a/pylib/reviewboardmods/reviewboardmods/pushhooks.py
+++ b/pylib/reviewboardmods/reviewboardmods/pushhooks.py
@@ -193,22 +193,25 @@ def _post_reviews(api_root, repoid, iden
                     squashed_reviewers.add(username)
             except APIError:
                 pass
 
         return sorted(reviewers)
 
     def update_review_request(rid, commit):
         rr = api_root.get_review_request(review_request_id=rid)
-        draft = rr.get_or_create_draft(**{
+        props = {
             "summary": commit['message'].splitlines()[0],
             "description": commit['message'],
             "extra_data.p2rb.commit_id": commit['id'],
-            "target_people": ','.join(extract_reviewers(commit)),
-        })
+        }
+        reviewers = extract_reviewers(commit)
+        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))
 
         return rr
 
@@ -330,20 +333,24 @@ def _post_reviews(api_root, repoid, iden
             'extra_data.p2rb.identifier': identifier,
             'extra_data.p2rb.commit_id': commit['id'],
             'repository': repoid,
         })
         rr.get_diffs().upload_diff(
             commit['diff'],
             parent_diff=commit.get('parent_diff', None),
             base_commit_id=commit.get('base_commit_id', None))
-        draft = rr.get_or_create_draft(
-            summary=commit['message'].splitlines()[0],
-            description=commit['message'],
-            target_people=','.join(extract_reviewers(commit)))
+        props = {
+            "summary": commit['message'].splitlines()[0],
+            "description": commit['message'],
+        }
+        reviewers = extract_reviewers(commit)
+        if reviewers:
+            props["target_people"] = ','.join(reviewers)
+        draft = rr.get_or_create_draft(**props)
         processed_nodes.add(commit['id'])
         assert isinstance(rr.id, int)
         node_to_rid[node] = rr.id
         review_requests[rr.id] = 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
@@ -386,23 +393,25 @@ def _post_reviews(api_root, repoid, iden
     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()))
 
-    squashed_draft = squashed_rr.get_or_create_draft(**{
+    props = {
         'summary': identifier,
         'description': '%s\n' % '\n'.join(squashed_description),
         'depends_on': depends,
         'extra_data.p2rb.commits': commit_list_json,
-        'target_people': ','.join(sorted(squashed_reviewers)),
-    })
+    }
+    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),
     })