git-cl: Set Code-Review label as a git push argument.

Bug: 877717
Change-Id: I6541a971068aae662b086eba84448bd0769f1a09
Reviewed-on: https://chromium-review.googlesource.com/c/1362405Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

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,

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 = {

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

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(