diff --git a/gerrit_util.py b/gerrit_util.py index 4bdbb56dd..d1ef98957 100755 --- a/gerrit_util.py +++ b/gerrit_util.py @@ -653,32 +653,44 @@ def GetReview(host, change, revision): return ReadHttpJsonResponse(CreateHttpConn(host, path)) -def AddReviewers(host, change, add=None, is_reviewer=True, notify=True): +def AddReviewers(host, change, reviewers=None, ccs=None, notify=True, + accept_statuses=frozenset([200, 400, 422])): """Add reviewers to a change.""" - errors = None - if not add: + if not reviewers and not ccs: return None - if isinstance(add, basestring): - add = (add,) - path = 'changes/%s/reviewers' % change - for r in add: - state = 'REVIEWER' if is_reviewer else 'CC' - notify = 'ALL' if notify else 'NONE' - body = { - 'reviewer': r, - 'state': state, - 'notify': notify, - } - try: - conn = CreateHttpConn(host, path, reqtype='POST', body=body) - _ = ReadHttpJsonResponse(conn) - except GerritError as e: - if e.http_status == 422: # "Unprocessable Entity" - LOGGER.warn('Note: "%s" not added as a %s' % (r, state.lower())) - errors = True - else: - raise - return errors + reviewers = frozenset(reviewers or []) + ccs = frozenset(ccs or []) + path = 'changes/%s/revisions/current/review' % change + + body = { + 'reviewers': [], + 'notify': 'ALL' if notify else 'NONE', + } + for r in sorted(reviewers | ccs): + state = 'REVIEWER' if r in reviewers else 'CC' + body['reviewers'].append({ + 'reviewer': r, + 'state': state, + 'notify': 'NONE', # We handled `notify` argument above. + }) + + conn = CreateHttpConn(host, path, reqtype='POST', body=body) + # Gerrit will return 400 if one or more of the requested reviewers are + # unprocessable. We read the response object to see which were rejected, + # warn about them, and retry with the remainder. + resp = ReadHttpJsonResponse(conn, accept_statuses=accept_statuses) + + errored = set() + for result in resp.get('reviewers', {}).itervalues(): + r = result.get('input') + state = 'REVIEWER' if r in reviewers else 'CC' + if result.get('error'): + errored.add(r) + LOGGER.warn('Note: "%s" not added as a %s' % (r, state.lower())) + if errored: + # Try again, adding only those that didn't fail, and only accepting 200. + AddReviewers(host, change, reviewers=(reviewers-errored), + ccs=(ccs-errored), notify=notify, accept_statuses=[200]) def RemoveReviewers(host, change, remove=None): diff --git a/git_cl.py b/git_cl.py index 69c23a7ae..9dff8952b 100755 --- a/git_cl.py +++ b/git_cl.py @@ -2937,6 +2937,14 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): 'single commit.') confirm_or_exit(action='upload') + if options.reviewers or options.tbrs or options.add_owners_to: + change_desc.update_reviewers(options.reviewers, options.tbrs, + options.add_owners_to, change) + + if options.send_mail: + if not change_desc.get_reviewers(): + DieWithError('Must specify reviewers to send email.', change_desc) + # Extra options that can be specified at push time. Doc: # https://gerrit-review.googlesource.com/Documentation/user-upload.html refspec_opts = [] @@ -2960,16 +2968,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): # reverse on its side. refspec_opts.append('m=' + title.replace(' ', '_')) - if options.send_mail: - if not change_desc.get_reviewers(): - DieWithError('Must specify reviewers to send email.', change_desc) - refspec_opts.append('notify=ALL') - else: - refspec_opts.append('notify=NONE') - - reviewers = change_desc.get_reviewers() - if reviewers: - refspec_opts.extend('r=' + email.strip() for email in reviewers) + # Never notify now because no one is on the review. Notify when we add + # reviewers and CCs below. + refspec_opts.append('notify=NONE') if options.private: refspec_opts.append('draft') @@ -3013,6 +3014,8 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): self.SetIssue(change_numbers[0]) self._GitSetBranchConfigValue('gerritsquashhash', ref_to_push) + reviewers = sorted(change_desc.get_reviewers()) + # Add cc's from the CC_LIST and --cc flag (if any). cc = self.GetCCList().split(',') if options.cc: @@ -3020,10 +3023,11 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): cc = filter(None, [email.strip() for email in cc]) if change_desc.get_cced(): cc.extend(change_desc.get_cced()) - if cc: - gerrit_util.AddReviewers( - self._GetGerritHost(), self.GetIssue(), cc, - is_reviewer=False, notify=bool(options.send_mail)) + + gerrit_util.AddReviewers( + self._GetGerritHost(), self.GetIssue(), reviewers, cc, + notify=bool(options.send_mail)) + return 0 def _ComputeParent(self, remote, upstream_branch, custom_cl_base, force, diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index a1a19520c..95d75d146 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -594,8 +594,8 @@ class TestGitCl(TestCase): lambda *args, **kwargs: self._mocked_call( 'GetChangeDetail', *args, **kwargs)) self.mock(git_cl.gerrit_util, 'AddReviewers', - lambda h, i, add, is_reviewer, notify: self._mocked_call( - 'AddReviewers', h, i, add, is_reviewer, notify)) + lambda h, i, reviewers, ccs, notify: self._mocked_call( + 'AddReviewers', h, i, reviewers, ccs, notify)) self.mock(git_cl.gerrit_util.GceAuthenticator, 'is_gce', classmethod(lambda _: False)) self.mock(git_cl, 'DieWithError', @@ -1515,16 +1515,12 @@ class TestGitCl(TestCase): else: ref_suffix = '%m=' + title - notify_suffix = 'notify=%s' % ('ALL' if notify else 'NONE') + notify_suffix = 'notify=NONE' if ref_suffix: ref_suffix += ',' + notify_suffix else: ref_suffix = '%' + notify_suffix - if reviewers: - ref_suffix += ',' + ','.join('r=%s' % email - for email in sorted(reviewers)) - if git_mirror is None: calls += [ ((['git', 'config', 'remote.origin.url'],), @@ -1571,8 +1567,8 @@ class TestGitCl(TestCase): calls += [ ((['git', 'config', 'rietveld.cc'],), ''), (('AddReviewers', 'chromium-review.googlesource.com', - 123456 if squash else None, - ['joe@example.com'] + cc, False, notify), ''), + 123456 if squash else None, sorted(reviewers), + ['joe@example.com'] + cc, notify), ''), ] calls += cls._git_post_upload_calls() return calls