From 6dadfbfcf77356b3cf4b972de93c0ba1f9472fe4 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Tue, 9 May 2017 14:27:58 -0700 Subject: [PATCH] git-cl-upload: Set all reviewers and ccs in a single batch api call The old system had two faults: * It set reviewers and ccs via different mechanisms, which is confusing * It set CCs with a single call for each, resulting in N separate emails, with each email going to the uploader and all reviewers and all previous CCs. This new system just collects all reviewers and CCs, and sets them in a single call. That call will fail if *any* of the individual reviewers or ccs fail, so it also parses the response and retries with only the ones which would have succeeded on their own. If that second call fails, or the first fails in an unexpected way, it raises an exception like normal Bug: 710028 Change-Id: I1be508487a41f0b68f9c41908229b8f5342830a3 Reviewed-on: https://chromium-review.googlesource.com/479712 Commit-Queue: Aaron Gable Reviewed-by: Andrii Shyshkalov --- gerrit_util.py | 60 ++++++++++++++++++++++++++------------------ git_cl.py | 32 ++++++++++++----------- tests/git_cl_test.py | 14 ++++------- 3 files changed, 59 insertions(+), 47 deletions(-) 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