Reland Implement owners check in presubmit for Gerrit.

R=machenbach@chromium.org,phajdan.jr@chromium.org
BUG=605563

Review-Url: https://codereview.chromium.org/1927773002

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@300353 0039d316-1c4b-4281-b951-d872f2087c98

git_cl.py

@@ -1317,7 +1317,8 @@ class Changelist(object):
       return presubmit_support.DoPresubmitChecks(change, committing,
           verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin,
           default_presubmit=None, may_prompt=may_prompt,
-          rietveld_obj=self._codereview_impl.GetRieveldObjForPresubmit())
+          rietveld_obj=self._codereview_impl.GetRieveldObjForPresubmit(),
+          gerrit_obj=self._codereview_impl.GetGerritObjForPresubmit())
     except presubmit_support.PresubmitFailure, e:
       DieWithError(
           ('%s\nMaybe your depot_tools is out of date?\n'
@@ -1501,6 +1502,10 @@ class _ChangelistCodereviewBase(object):
     # For non-Rietveld codereviews, this probably should return a dummy object.
     raise NotImplementedError()
 
+  def GetGerritObjForPresubmit(self):
+    # None is valid return value, otherwise presubmit_support.GerritAccessor.
+    return None
+
   def UpdateDescriptionRemote(self, description):
     """Update the description on codereview site."""
     raise NotImplementedError()
@@ -2107,6 +2112,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
         raise NotImplementedError()
     return ThisIsNotRietveldIssue()
 
+  def GetGerritObjForPresubmit(self):
+    return presubmit_support.GerritAccessor(self._GetGerritHost())
+
   def GetStatus(self):
     """Apply a rough heuristic to give a simple summary of an issue's review
     or CQ status, assuming adherence to a common workflow.

presubmit_canned_checks.py

@@ -875,8 +875,7 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
       input_api.change.AffectedFiles(file_filter=source_file_filter)])
 
   owners_db = input_api.owners_db
-  # TODO(tandrii): this will always return None, set() in case of Gerrit.
-  owner_email, reviewers = _RietveldOwnerAndReviewers(
+  owner_email, reviewers = _CodereviewOwnersAndReviewers(
       input_api,
       owners_db.email_regexp,
       approval_needed=input_api.is_committing)
@@ -905,6 +904,26 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
     return [output('Missing LGTM from someone other than %s' % owner_email)]
   return []
 
+def _CodereviewOwnersAndReviewers(input_api, email_regexp, approval_needed):
+  """Return the owner and reviewers of a change, if any.
+
+  If approval_needed is True, only reviewers who have approved the change
+  will be returned.
+  """
+  if input_api.change.issue:
+    if input_api.gerrit:
+      res = _GerritOwnerAndReviewers(input_api, email_regexp, approval_needed)
+    else:
+      # Rietveld is default.
+      res = _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed)
+    if res:
+      return res
+
+  reviewers = set()
+  if not approval_needed:
+    reviewers = _ReviewersFromChange(input_api.change)
+  return None, reviewers
+
 
 def _GetRietveldIssueProps(input_api, messages):
   """Gets the issue properties from rietveld."""
@@ -926,35 +945,51 @@ def _ReviewersFromChange(change):
   return set(reviewer for reviewer in reviewers if '@' in reviewer)
 
 
+def _match_reviewer_email(r, owner_email, email_regexp):
+  return email_regexp.match(r) and r != owner_email
+
 def _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed=False):
   """Return the owner and reviewers of a change, if any.
 
   If approval_needed is True, only reviewers who have approved the change
   will be returned.
+  Returns None if can't fetch issue properties from codereview.
   """
   issue_props = _GetRietveldIssueProps(input_api, True)
   if not issue_props:
-    reviewers = set()
-    if not approval_needed:
-      reviewers = _ReviewersFromChange(input_api.change)
-    return None, reviewers
+    return None
 
   if not approval_needed:
     return issue_props['owner_email'], set(issue_props['reviewers'])
 
   owner_email = issue_props['owner_email']
 
-  def match_reviewer(r):
-    return email_regexp.match(r) and r != owner_email
-
   messages = issue_props.get('messages', [])
   approvers = set(
       m['sender'] for m in messages
-      if m.get('approval') and match_reviewer(m['sender']))
-
+      if m.get('approval') and _match_reviewer_email(m['sender'], owner_email,
+                                                     email_regexp))
   return owner_email, approvers
 
 
+def _GerritOwnerAndReviewers(input_api, email_regexp, approval_needed=False):
+  """Return the owner and reviewers of a change, if any.
+
+  If approval_needed is True, only reviewers who have approved the change
+  will be returned.
+  Returns None if can't fetch issue properties from codereview.
+  """
+  issue = input_api.change.issue
+  if not issue:
+    return None
+
+  owner_email = input_api.gerrit.GetChangeOwner(issue)
+  reviewers = set(
+      r for r in input_api.gerrit.GetChangeReviewers(issue, approval_needed)
+      if _match_reviewer_email(r, owner_email, email_regexp))
+  return owner_email, reviewers
+
+
 def _CheckConstNSObject(input_api, output_api, source_file_filter):
   """Checks to make sure no objective-c files have |const NSSomeClass*|."""
   pattern = input_api.re.compile(

presubmit_support.py

@@ -197,6 +197,73 @@ class _MailTextResult(_PresubmitResult):
     super(_MailTextResult, self).__init__()
     raise NotImplementedError()
 
+class GerritAccessor(object):
+  """Limited Gerrit functionality for canned presubmit checks to work.
+
+  To avoid excessive Gerrit calls, caches the results.
+  """
+
+  def __init__(self, host):
+    self.host = host
+    self.cache = {}
+
+  def _FetchChangeDetail(self, issue):
+    # Separate function to be easily mocked in tests.
+    return gerrit_util.GetChangeDetail(
+        self.host, str(issue),
+        ['ALL_REVISIONS', 'DETAILED_LABELS'])
+
+  def GetChangeInfo(self, issue):
+    """Returns labels and all revisions (patchsets) for this issue.
+
+    The result is a dictionary according to Gerrit REST Api.
+    https://gerrit-review.googlesource.com/Documentation/rest-api.html
+
+    However, API isn't very clear what's inside, so see tests for example.
+    """
+    assert issue
+    cache_key = int(issue)
+    if cache_key not in self.cache:
+      self.cache[cache_key] = self._FetchChangeDetail(issue)
+    return self.cache[cache_key]
+
+  def GetChangeDescription(self, issue, patchset=None):
+    """If patchset is none, fetches current patchset."""
+    info = self.GetChangeInfo(issue)
+    # info is a reference to cache. We'll modify it here adding description to
+    # it to the right patchset, if it is not yet there.
+
+    # Find revision info for the patchset we want.
+    if patchset is not None:
+      for rev, rev_info in info['revisions'].iteritems():
+        if str(rev_info['_number']) == str(patchset):
+          break
+      else:
+        raise Exception('patchset %s doesn\'t exist in issue %s' % (
+            patchset, issue))
+    else:
+      rev = info['current_revision']
+      rev_info = info['revisions'][rev]
+
+    # Updates revision info, which is part of cached issue info.
+    if 'real_description' not in rev_info:
+      rev_info['real_description'] = (
+          gerrit_util.GetChangeDescriptionFromGitiles(
+              rev_info['fetch']['http']['url'], rev))
+    return rev_info['real_description']
+
+  def GetChangeOwner(self, issue):
+    return self.GetChangeInfo(issue)['owner']['email']
+
+  def GetChangeReviewers(self, issue, approving_only=True):
+    # Gerrit has 'approved' sub-section, but it only lists 1 approver.
+    # So, if we look only for approvers, we have to look at all anyway.
+    # Also, assume LGTM means Code-Review label == 2. Other configurations
+    # aren't supported.
+    return [r['email']
+            for r in self.GetChangeInfo(issue)['labels']['Code-Review']['all']
+            if not approving_only or '2' == str(r.get('value', 0))]
+
 
 class OutputApi(object):
   """An instance of OutputApi gets passed to presubmit scripts so that they
@@ -265,7 +332,7 @@ class InputApi(object):
   )
 
   def __init__(self, change, presubmit_path, is_committing,
-      rietveld_obj, verbose, dry_run=None):
+      rietveld_obj, verbose, gerrit_obj=None, dry_run=None):
     """Builds an InputApi object.
 
     Args:
@@ -273,12 +340,15 @@ class InputApi(object):
       presubmit_path: The path to the presubmit script being processed.
       is_committing: True if the change is about to be committed.
       rietveld_obj: rietveld.Rietveld client object
+      gerrit_obj: provides basic Gerrit codereview functionality.
+      dry_run: if true, some Checks will be skipped.
     """
     # Version number of the presubmit_support script.
     self.version = [int(x) for x in __version__.split('.')]
     self.change = change
     self.is_committing = is_committing
     self.rietveld = rietveld_obj
+    self.gerrit = gerrit_obj
     self.dry_run = dry_run
     # TBD
     self.host_url = 'http://codereview.chromium.org'
@@ -1349,16 +1419,19 @@ def DoPostUploadExecuter(change,
 
 class PresubmitExecuter(object):
   def __init__(self, change, committing, rietveld_obj, verbose,
-               dry_run=None):
+               gerrit_obj=None, dry_run=None):
     """
     Args:
       change: The Change object.
       committing: True if 'gcl commit' is running, False if 'gcl upload' is.
       rietveld_obj: rietveld.Rietveld client object.
+      gerrit_obj: provides basic Gerrit codereview functionality.
+      dry_run: if true, some Checks will be skipped.
     """
     self.change = change
     self.committing = committing
     self.rietveld = rietveld_obj
+    self.gerrit = gerrit_obj
     self.verbose = verbose
     self.dry_run = dry_run
 
@@ -1381,7 +1454,7 @@ class PresubmitExecuter(object):
     # Load the presubmit script into context.
     input_api = InputApi(self.change, presubmit_path, self.committing,
                          self.rietveld, self.verbose,
-                         dry_run=self.dry_run)
+                         gerrit_obj=self.gerrit, dry_run=self.dry_run)
     context = {}
     try:
       exec script_text in context
@@ -1424,6 +1497,7 @@ def DoPresubmitChecks(change,
                       default_presubmit,
                       may_prompt,
                       rietveld_obj,
+                      gerrit_obj=None,
                       dry_run=None):
   """Runs all presubmit checks that apply to the files in the change.
 
@@ -1443,6 +1517,7 @@ def DoPresubmitChecks(change,
     default_presubmit: A default presubmit script to execute in any case.
     may_prompt: Enable (y/n) questions on warning or error.
     rietveld_obj: rietveld.Rietveld object.
+    gerrit_obj: provides basic Gerrit codereview functionality.
     dry_run: if true, some Checks will be skipped.
 
   Warning:
@@ -1471,7 +1546,7 @@ def DoPresubmitChecks(change,
       output.write("Warning, no PRESUBMIT.py found.\n")
     results = []
     executer = PresubmitExecuter(change, committing, rietveld_obj, verbose,
-                                 dry_run=dry_run)
+                                 gerrit_obj, dry_run)
     if default_presubmit:
       if verbose:
         output.write("Running default presubmit script.\n")
@@ -1696,7 +1771,8 @@ def main(argv=None):
     parser.error('For unversioned directory, <files> is not optional.')
   logging.info('Found %d file(s).' % len(files))
 
-  rietveld_obj = None
+  rietveld_obj, gerrit_obj = None, None
+
   if options.rietveld_url:
     # The empty password is permitted: '' is not None.
     if options.rietveld_private_key_file:
@@ -1718,21 +1794,12 @@ def main(argv=None):
       logging.info('Got description: """\n%s\n"""', options.description)
 
   if options.gerrit_url and options.gerrit_fetch:
-    rietveld_obj = None
     assert options.issue and options.patchset
-    props = gerrit_util.GetChangeDetail(
-        urlparse.urlparse(options.gerrit_url).netloc, str(options.issue),
-        ['ALL_REVISIONS'])
-    options.author = props['owner']['email']
-    for rev, rev_info in props['revisions'].iteritems():
-      if str(rev_info['_number']) == str(options.patchset):
-        options.description = gerrit_util.GetChangeDescriptionFromGitiles(
-            rev_info['fetch']['http']['url'], rev)
-        break
-    else:
-      print >> sys.stderr, ('Patchset %d was not found in Gerrit issue %d' %
-                            options.patchset, options.issue)
-      return 2
+    rietveld_obj = None
+    gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc)
+    options.author = gerrit_obj.GetChangeOwner(options.issue)
+    options.description = gerrit_obj.GetChangeDescription(options.issue,
+                                                          options.patchset)
     logging.info('Got author: "%s"', options.author)
     logging.info('Got description: """\n%s\n"""', options.description)
 
@@ -1754,6 +1821,7 @@ def main(argv=None):
           options.default_presubmit,
           options.may_prompt,
           rietveld_obj,
+          gerrit_obj,
           options.dry_run)
     return not results.should_continue()
   except NonexistantCannedCheckFilter, e:

tests/presubmit_unittest.py

@@ -184,6 +184,7 @@ class PresubmitUnittest(PresubmitTestsBase):
       'subprocess', 'sys', 'tempfile', 'time', 'traceback', 'types', 'unittest',
       'urllib2', 'warn', 'multiprocessing', 'DoGetTryMasters',
       'GetTryMastersExecuter', 'itertools', 'urlparse', 'gerrit_util',
+      'GerritAccessor',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(presubmit, members)
@@ -828,7 +829,8 @@ def CheckChangeOnCommit(input_api, output_api):
         0,
         None)
     output = presubmit.DoPresubmitChecks(
-        change, False, True, None, input_buf, DEFAULT_SCRIPT, False, None)
+        change, False, True, None, input_buf, DEFAULT_SCRIPT, False, None, None,
+        None)
     self.failIf(output.should_continue())
     text = (
         'Running presubmit upload checks ...\n'
@@ -910,7 +912,8 @@ def CheckChangeOnCommit(input_api, output_api):
         0,
         None)
     self.failUnless(presubmit.DoPresubmitChecks(
-        change, False, True, output, input_buf, DEFAULT_SCRIPT, False, None))
+        change, False, True, output, input_buf, DEFAULT_SCRIPT, False, None,
+        None))
     self.assertEquals(output.getvalue(),
                       ('Running presubmit upload checks ...\n'
                        'Warning, no PRESUBMIT.py found.\n'
@@ -1153,7 +1156,7 @@ def CheckChangeOnCommit(input_api, output_api):
     presubmit.DoPresubmitChecks(mox.IgnoreArg(), False, False,
                                 mox.IgnoreArg(),
                                 mox.IgnoreArg(),
-                                None, False, None, None).AndReturn(output)
+                                None, False, None, None, None).AndReturn(output)
     self.mox.ReplayAll()
 
     self.assertEquals(
@@ -1197,7 +1200,7 @@ class InputApiUnittest(PresubmitTestsBase):
       'os_walk', 'os_path', 'os_stat', 'owners_db', 'pickle', 'platform',
       'python_executable', 're', 'rietveld', 'subprocess', 'tbr', 'tempfile',
       'time', 'traceback', 'unittest', 'urllib2', 'version', 'verbose',
-      'dry_run',
+      'dry_run', 'gerrit',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(
@@ -1839,6 +1842,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
     input_api.os_path = presubmit.os.path
     input_api.re = presubmit.re
     input_api.rietveld = self.mox.CreateMock(rietveld.Rietveld)
+    input_api.gerrit = None
     input_api.traceback = presubmit.traceback
     input_api.urllib2 = self.mox.CreateMock(presubmit.urllib2)
     input_api.unittest = unittest
@@ -2561,7 +2565,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
         presubmit.OutputApi.PresubmitNotifyResult)
 
   def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None,
-      reviewers=None, is_committing=True, rietveld_response=None,
+      reviewers=None, is_committing=True,
+      rietveld_response=None, gerrit_response=None,
       uncovered_files=None, expected_output='',
       manually_specified_reviewers=None, dry_run=None):
     if approvers is None:
@@ -2584,6 +2589,11 @@ class CannedChecksUnittest(PresubmitTestsBase):
     change.TBR = ''
     affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile)
     input_api = self.MockInputApi(change, False)
+    if gerrit_response:
+      assert not rietveld_response
+      input_api.rietveld = None
+      input_api.gerrit = presubmit.GerritAccessor('host')
+
     fake_db = self.mox.CreateMock(owners.Database)
     fake_db.email_regexp = input_api.re.compile(owners.BASIC_EMAIL_REGEXP)
     input_api.owners_db = fake_db
@@ -2595,7 +2605,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
       if not dry_run:
         affected_file.LocalPath().AndReturn('foo/xyz.cc')
         change.AffectedFiles(file_filter=None).AndReturn([affected_file])
-      if issue and not rietveld_response:
+      if issue and not rietveld_response and not gerrit_response:
         rietveld_response = {
           "owner_email": change.author_email,
           "messages": [
@@ -2612,9 +2622,12 @@ class CannedChecksUnittest(PresubmitTestsBase):
 
       if not dry_run:
         if issue:
+          if rietveld_response:
             input_api.rietveld.get_issue_properties(
                 issue=int(input_api.change.issue), messages=True).AndReturn(
                     rietveld_response)
+          elif gerrit_response:
+            input_api.gerrit._FetchChangeDetail = lambda _: gerrit_response
 
         people.add(change.author_email)
         fake_db.files_not_covered_by(set(['foo/xyz.cc']),
@@ -2647,6 +2660,39 @@ class CannedChecksUnittest(PresubmitTestsBase):
         rietveld_response=response,
         expected_output='')
 
+  def testCannedCheckOwners_Approved_Gerrit(self):
+    response = {
+      "owner": {"email": "john@example.com"},
+      "labels": {"Code-Review": {
+        u'all': [
+          {
+            u'email': u'john@example.com',  # self +1 :)
+            u'value': 1
+          },
+          {
+            u'email': u'ben@example.com',
+            u'value': 2
+          },
+        ],
+        u'approved': {u'email': u'ben@example.org'},
+        u'default_value': 0,
+        u'values': {u' 0': u'No score',
+                    u'+1': u'Looks good to me, but someone else must approve',
+                    u'+2': u'Looks good to me, approved',
+                    u'-1': u"I would prefer that you didn't submit this",
+                    u'-2': u'Do not submit'}
+      }},
+    }
+    self.AssertOwnersWorks(approvers=set(['ben@example.com']),
+        gerrit_response=response,
+        is_committing=True,
+        expected_output='')
+
+    self.AssertOwnersWorks(approvers=set(['ben@example.com']),
+        is_committing=False,
+        gerrit_response=response,
+        expected_output='')
+
   def testCannedCheckOwners_Approved(self):
     response = {
       "owner_email": "john@example.com",