diff --git a/gerrit_util.py b/gerrit_util.py index 4cfd65b45..985c7fb6c 100644 --- a/gerrit_util.py +++ b/gerrit_util.py @@ -613,6 +613,18 @@ def GetGerritFetchUrl(host): return '%s://%s/' % (GERRIT_PROTOCOL, host) +def GetCodeReviewTbrScore(host, project): + """Given a gerrit host name and project, return the Code-Review score for TBR. + """ + conn = CreateHttpConn(host, '/projects/%s' % urllib.quote(project, safe='')) + project = ReadHttpJsonResponse(conn) + if ('labels' not in project + or 'Code-Review' not in project['labels'] + or 'values' not in project['labels']['Code-Review']): + return 1 + return max([int(x) for x in project['labels']['Code-Review']['values']]) + + def GetChangePageUrl(host, change_number): """Given a gerrit host name and change number, return change page url.""" return '%s://%s/#/c/%d/' % (GERRIT_PROTOCOL, host, change_number) @@ -976,7 +988,7 @@ def tempdir(): def ChangeIdentifier(project, change_number): - """Returns change identifier "project~number" suitable for |chagne| arg of + """Returns change identifier "project~number" suitable for |change| arg of this module API. Such format is allows for more efficient Gerrit routing of HTTP requests, diff --git a/git_cl.py b/git_cl.py index a981b6e4a..eb5ba35a5 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1613,41 +1613,6 @@ class Changelist(object): ret = upload_branch_deps(self, orig_args) return ret - def SetLabels(self, enable_auto_submit, use_commit_queue, cq_dry_run): - """Sets labels on the change based on the provided flags. - - Sets labels if issue is already uploaded and known, else returns without - doing anything. - - Args: - enable_auto_submit: Sets Auto-Submit+1 on the change. - use_commit_queue: Sets Commit-Queue+2 on the change. - cq_dry_run: Sets Commit-Queue+1 on the change. Overrides Commit-Queue+2 if - both use_commit_queue and cq_dry_run are true. - """ - if not self.GetIssue(): - return - try: - self._codereview_impl.SetLabels(enable_auto_submit, use_commit_queue, - cq_dry_run) - return 0 - except KeyboardInterrupt: - raise - except: - labels = [] - if enable_auto_submit: - labels.append('Auto-Submit') - if use_commit_queue or cq_dry_run: - labels.append('Commit-Queue') - print('WARNING: Failed to set label(s) on your change: %s\n' - 'Either:\n' - ' * Your project does not have the above label(s),\n' - ' * You don\'t have permission to set the above label(s),\n' - ' * There\'s a bug in this code (see stack trace below).\n' % - (', '.join(labels))) - # Still raise exception so that stack trace is printed. - raise - def SetCQState(self, new_state): """Updates the CQ state for the latest patchset. @@ -1841,13 +1806,6 @@ class _ChangelistCodereviewBase(object): """Uploads a change to codereview.""" raise NotImplementedError() - def SetLabels(self, enable_auto_submit, use_commit_queue, cq_dry_run): - """Sets labels on the change based on the provided flags. - - Issue must have been already uploaded and known. - """ - raise NotImplementedError() - def SetCQState(self, new_state): """Updates the CQ state for the latest patchset. @@ -2670,16 +2628,18 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): # https://gerrit-review.googlesource.com/Documentation/user-upload.html#topic refspec_opts.append('topic=%s' % options.topic) - if not change_desc.get_reviewers(tbr_only=True): - # Change is not TBR, so we can inline setting other labels, too. - # TODO(crbug.com/877717): make this working for TBR, too, by figuring out - # max score for CR label somehow. - if options.enable_auto_submit: - refspec_opts.append('l=Auto-Submit+1') - if options.use_commit_queue: - refspec_opts.append('l=Commit-Queue+2') - elif options.cq_dry_run: - refspec_opts.append('l=Commit-Queue+1') + if options.enable_auto_submit: + refspec_opts.append('l=Auto-Submit+1') + if options.use_commit_queue: + refspec_opts.append('l=Commit-Queue+2') + elif options.cq_dry_run: + refspec_opts.append('l=Commit-Queue+1') + + if change_desc.get_reviewers(tbr_only=True): + score = gerrit_util.GetCodeReviewTbrScore( + self._GetGerritHost(), + self._GetGerritProject()) + refspec_opts.append('l=Code-Review+%s' % score) # Gerrit sorts hashtags, so order is not important. hashtags = {change_desc.sanitize_hash_tag(t) for t in options.hashtags} @@ -2740,20 +2700,6 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): reviewers, cc, notify=bool(options.send_mail)) - if change_desc.get_reviewers(tbr_only=True): - labels = self._GetChangeDetail(['LABELS']).get('labels', {}) - score = 1 - if 'Code-Review' in labels and 'values' in labels['Code-Review']: - score = max([int(x) for x in labels['Code-Review']['values'].keys()]) - print('Adding self-LGTM (Code-Review +%d) because of TBRs.' % score) - gerrit_util.SetReview( - self._GetGerritHost(), - self._GerritChangeIdentifier(), - msg='Self-approving for TBR', - labels={'Code-Review': score}) - # Labels aren't set through refspec only if tbr is set (see check above). - self.SetLabels(options.enable_auto_submit, options.use_commit_queue, - options.cq_dry_run) return 0 def _ComputeParent(self, remote, upstream_branch, custom_cl_base, force, @@ -2830,23 +2776,6 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): else: DieWithError('ERROR: Gerrit commit-msg hook not installed.') - def SetLabels(self, enable_auto_submit, use_commit_queue, cq_dry_run): - """Sets labels on the change based on the provided flags.""" - labels = {} - notify = None; - if enable_auto_submit: - labels['Auto-Submit'] = 1 - if use_commit_queue: - labels['Commit-Queue'] = 2 - elif cq_dry_run: - labels['Commit-Queue'] = 1 - notify = False - if labels: - gerrit_util.SetReview( - self._GetGerritHost(), - self._GerritChangeIdentifier(), - labels=labels, notify=notify) - def SetCQState(self, new_state): """Sets the Commit-Queue label assuming canonical CQ config for Gerrit.""" vote_map = { diff --git a/metrics_utils.py b/metrics_utils.py index 60328a6c4..37b668dae 100644 --- a/metrics_utils.py +++ b/metrics_utils.py @@ -147,6 +147,8 @@ KNOWN_SUBCOMMAND_ARGS = { 'cc', 'hashtag', 'l=Auto-Submit+1', + 'l=Code-Review+1', + 'l=Code-Review+2', 'l=Commit-Queue+1', 'l=Commit-Queue+2', 'label', diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 3aa17a06b..0525007ad 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -1059,10 +1059,17 @@ class TestGitCl(TestCase): if c in cc: cc.remove(c) - if not tbr: - for k, v in sorted((labels or {}).items()): - ref_suffix += ',l=%s+%d' % (k, v) - metrics_arguments.append('l=%s+%d' % (k, v)) + for k, v in sorted((labels or {}).items()): + ref_suffix += ',l=%s+%d' % (k, v) + metrics_arguments.append('l=%s+%d' % (k, v)) + + if tbr: + calls += [ + (('GetCodeReviewTbrScore', + '%s-review.googlesource.com' % short_hostname, + 'my/repo'), + 2,), + ] calls += [ (('time.time',), 1000,), @@ -1115,29 +1122,6 @@ class TestGitCl(TestCase): notify), ''), ] - if tbr: - calls += [ - (('GetChangeDetail', 'chromium-review.googlesource.com', - 'my%2Frepo~123456', ['LABELS']), { - 'labels': { - 'Code-Review': { - 'default_value': 0, - 'all': [], - 'values': { - '+2': 'lgtm, approved', - '+1': 'lgtm, but someone else must approve', - ' 0': 'No score', - '-1': 'Don\'t submit as-is', - } - } - } - }), - (('SetReview', - 'chromium-review.googlesource.com', - 'my%2Frepo~123456', - 'Self-approving for TBR', - {'Code-Review': 2}, None), ''), - ] calls += cls._git_post_upload_calls() return calls @@ -1261,6 +1245,8 @@ class TestGitCl(TestCase): notify=True) def test_gerrit_reviewer_multiple(self): + self.mock(git_cl.gerrit_util, 'GetCodeReviewTbrScore', + lambda *a: self._mocked_call('GetCodeReviewTbrScore', *a)) self._run_gerrit_upload_test( [], 'desc\nTBR=reviewer@example.com\nBUG=\nR=another@example.com\n' @@ -1269,7 +1255,8 @@ class TestGitCl(TestCase): ['reviewer@example.com', 'another@example.com'], expected_upstream_ref='origin/master', cc=['more@example.com', 'people@example.com'], - tbr='reviewer@example.com') + tbr='reviewer@example.com', + labels={'Code-Review': 2}) def test_gerrit_upload_squash_first_is_default(self): self._run_gerrit_upload_test(