Clean up the parsing of approvals for OWNERS checks.

This fixes bugs 76724. We will now allow approvals from
only non-owners to be sufficient if the patch is from an OWNER.

Also, this strips out the code for suggesting reviewers during upload completely. I've come to believe that this should be done by gcl and git-cl directly rather than through a presubmit hook, when the change is being initially created. CheckOwners() is now a no-op on upload.

(We might need to add a new value to the codereview.settings file instead to indicate if we want this to happen).

Review URL: http://codereview.chromium.org/6730020

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

presubmit_canned_checks.py

@@ -627,35 +627,37 @@ def CheckBuildbotPendingBuilds(input_api, output_api, url, max_pendings,
   return []
 
 
-def CheckOwners(input_api, output_api, email_regexp=None,
-    source_file_filter=None):
-  affected_files = set([f.LocalPath() for f in
-      input_api.change.AffectedFiles(source_file_filter)])
-  owners_db = input_api.owners_db
-  if email_regexp:
-    owners_db.email_regexp = input_api.re.compile(email_regexp)
-
-  if input_api.is_committing and input_api.tbr:
-    return [output_api.PresubmitNotifyResult(
-        '--tbr was specified, skipping OWNERS check')]
-  elif input_api.is_committing:
-    approvers = _Approvers(input_api, owners_db.email_regexp)
-    missing_files = owners_db.files_not_covered_by(affected_files, approvers)
-    if missing_files:
-      return [output_api.PresubmitError('Missing LGTM from an OWNER for: %s' %
-              ','.join(missing_files))]
-    return []
-  elif input_api.change.tags.get('R'):
+def CheckOwners(input_api, output_api, source_file_filter=None):
+  if not input_api.is_committing:
     return []
+  if input_api.tbr:
+    return [output_api.PresubmitNotifyResult('--tbr was specified, '
+                                             'skipping OWNERS check')]
+  if not input_api.change.issue:
+    return [output_api.PresubmitError('Change not uploaded for review')]
 
-  suggested_reviewers = owners_db.reviewers_for(affected_files)
-  return [output_api.PresubmitAddReviewers(suggested_reviewers)]
+  affected_files = set([f.LocalPath() for f in
+      input_api.change.AffectedFiles(source_file_filter)])
 
+  owners_db = input_api.owners_db
+  owner_email, approvers = _RietveldOwnerAndApprovers(input_api,
+                                                      owners_db.email_regexp)
+  approvers_plus_owner = approvers.union(set([owner_email]))
+
+  missing_files = owners_db.files_not_covered_by(affected_files,
+                                                 approvers_plus_owner)
+  if missing_files:
+    return [output_api.PresubmitError('Missing LGTM from an OWNER for: %s' %
+                                      ','.join(missing_files))]
+
+  if not approvers:
+    return [output_api.PresubmitError('Missing LGTM from someone other than %s'
+                                      % owner_email)]
+  return []
 
-def _Approvers(input_api, email_regexp):
-  if not input_api.change.issue:
-    return []
 
+def _RietveldOwnerAndApprovers(input_api, email_regexp):
+  """Return the owner and approvers of a change, if any."""
   # TODO(dpranke): Should figure out if input_api.host_url is supposed to
   # be a host or a scheme+host and normalize it there.
   host = input_api.host_url
@@ -665,19 +667,56 @@ def _Approvers(input_api, email_regexp):
 
   f = input_api.urllib2.urlopen(url)
   issue_props = input_api.json.load(f)
-  owner = input_api.re.escape(issue_props['owner'])
+  messages = issue_props.get('messages', [])
+  owner_email = issue_props['owner_email']
+  owner_regexp = input_api.re.compile(input_api.re.escape(owner_email))
+  approvers = _GetApprovers(messages, email_regexp, owner_regexp)
+
+  return (owner_email, set(approvers))
+
+
+def _IsApprovingComment(text):
+  """Implements the logic for parsing a change comment for approval."""
+
+  # Any comment that contains a non-quoted line containing an 'lgtm' is an
+  # approval.
+  #
+  # TODO(dpranke): this differs from the logic used inside Google in a few
+  # ways. Inside Google,
+  #
+  # 1) the approving phrase must appear at the beginning of the first non
+  #    quoted-line in the comment.'
+  # 2) "LG", "Looks Good", and "Looks Good to Me" are also acceptable.
+  # 3) Subsequent comments from the reviewer can rescind approval, unless
+  #    the phrase "LGTM++" was used.
+  # We should consider implementing some or all of this here.
+  for l in text.splitlines():
+    l = l.strip().lower()
+    if l.startswith('>'):
+      continue
+
+    if 'lgtm' in l:
+      return True
+  return False
+
+
+def _GetApprovers(messages, email_regexp, owner_regexp):
+  """Returns the set of approvers for a change given the owner and messages.
+
+  Messages should be a list of dicts containing 'sender' and 'text' keys."""
 
   # TODO(dpranke): This mimics the logic in
   # /tools/commit-queue/verifiers/reviewer_lgtm.py
   # We should share the code and/or remove the check there where it is
   # redundant (since the commit queue also enforces the presubmit checks).
   def match_reviewer(r):
-    return email_regexp.match(r) and not input_api.re.match(owner, r)
+    return email_regexp.match(r) and not owner_regexp.match(r)
 
   approvers = []
-  for m in issue_props.get('messages', []):
-    if 'lgtm' in m['text'].lower() and match_reviewer(m['sender']):
-      approvers.append(m['sender'])
+  for m in messages:
+    sender = m['sender']
+    if _IsApprovingComment(m['text']) and match_reviewer(sender):
+      approvers.append(sender)
   return set(approvers)
 
 
@@ -774,10 +813,8 @@ def PanProjectChecks(input_api, output_api,
   text_files = lambda x: input_api.FilterSourceFile(x, black_list=black_list,
                                                     white_list=white_list)
 
-  # TODO(dpranke): enable upload as well
-  if input_api.is_committing:
-    results.extend(input_api.canned_checks.CheckOwners(
-        input_api, output_api, source_file_filter=sources))
+  results.extend(input_api.canned_checks.CheckOwners(
+      input_api, output_api, source_file_filter=sources))
 
   results.extend(input_api.canned_checks.CheckLongLines(
       input_api, output_api, source_file_filter=sources))

tests/presubmit_unittest.py

@@ -1884,43 +1884,46 @@ mac|success|blew
     self.assertEquals(results[0].__class__,
         presubmit.OutputApi.PresubmitNotifyResult)
 
-  def OwnersTest(self, is_committing, tbr=False, change_tags=None,
-      suggested_reviewers=None, approvers=None,
-      uncovered_files=None, expected_reviewers=None, expected_output='',
-      host_url=None):
-    affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile)
-    affected_file.LocalPath().AndReturn('foo.cc')
-    change = self.mox.CreateMock(presubmit.Change)
-    change.AffectedFiles(None).AndReturn([affected_file])
+  def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None,
+      rietveld_response=None, host_url=None,
+      uncovered_files=None, expected_output=''):
+    if approvers is None:
+      approvers = set()
+    if uncovered_files is None:
+      uncovered_files = set()
 
+    change = self.mox.CreateMock(presubmit.Change)
+    change.issue = issue
+    affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile)
     input_api = self.MockInputApi(change, False)
-    expected_host = 'http://localhost'
-    if host_url:
-      input_api.host_url = host_url
-      if host_url.startswith('https'):
-        expected_host = host_url
     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
-    input_api.is_committing = is_committing
+    input_api.is_committing = True
     input_api.tbr = tbr
 
-    if is_committing and not tbr:
-      change.issue = '1'
+    if not tbr and issue:
+      affected_file.LocalPath().AndReturn('foo.cc')
+      change.AffectedFiles(None).AndReturn([affected_file])
+
+      expected_host = 'http://localhost'
+      if host_url:
+        input_api.host_url = host_url
+        if host_url.startswith('https'):
+          expected_host = host_url
+
+      owner_email = 'john@example.com'
       messages = list('{"sender": "' + a + '","text": "lgtm"}' for
                       a in approvers)
-      rietveld_response = ('{"owner": "john@example.com",'
-                           '"messages": [' + ','.join(messages) + ']}')
+      if not rietveld_response:
+        rietveld_response = ('{"owner_email": "' + owner_email + '",'
+                            '"messages": [' + ','.join(messages) + ']}')
       input_api.urllib2.urlopen(
           expected_host + '/api/1?messages=true').AndReturn(
           StringIO.StringIO(rietveld_response))
       input_api.json = presubmit.json
-      fake_db.files_not_covered_by(set(['foo.cc']), approvers).AndReturn(
-          uncovered_files)
-    elif not is_committing:
-      change.tags = change_tags
-      if not change_tags.get('R'):
-        fake_db.reviewers_for(set(['foo.cc'])).AndReturn(suggested_reviewers)
+      fake_db.files_not_covered_by(set(['foo.cc']),
+         approvers.union(set([owner_email]))).AndReturn(uncovered_files)
 
     self.mox.ReplayAll()
     output = presubmit.PresubmitOutput()
@@ -1928,49 +1931,77 @@ mac|success|blew
         presubmit.OutputApi)
     if results:
       results[0].handle(output)
-    if expected_reviewers is not None:
-      self.assertEquals(output.reviewers, expected_reviewers)
     self.assertEquals(output.getvalue(), expected_output)
 
-  def testCannedCheckOwners_WithReviewer(self):
-    self.OwnersTest(is_committing=False, change_tags={'R': 'ben@example.com'})
-    self.OwnersTest(is_committing=False, tbr=True,
-        change_tags={'R': 'ben@example.com'})
-
-  def testCannedCheckOwners_NoReviewer(self):
-    self.OwnersTest(is_committing=False, change_tags={},
-        suggested_reviewers=['ben@example.com'],
-        expected_reviewers=['ben@example.com'])
-    self.OwnersTest(is_committing=False, tbr=True, change_tags={},
-        suggested_reviewers=['ben@example.com'],
-        expected_reviewers=['ben@example.com'])
-
-  def testCannedCheckOwners_CommittingWithoutOwnerLGTM(self):
-    self.OwnersTest(is_committing=True,
-        approvers=set(),
-        uncovered_files=set(['foo.cc']),
-        expected_output='Missing LGTM from an OWNER for: foo.cc\n')
+  def testCannedCheckOwners_LGTMPhrases(self):
+    def phrase_test(phrase, approvers=None, expected_output=''):
+      if approvers is None:
+        approvers = set(['ben@example.com'])
+      self.AssertOwnersWorks(approvers=approvers,
+          rietveld_response='{"owner_email": "john@example.com",' +
+                            '"messages": [{"sender": "ben@example.com",' +
+                                          '"text": "' + phrase + '"}]}',
+          expected_output=expected_output)
+
+    phrase_test('LGTM')
+    phrase_test('\\nlgtm')
+    phrase_test('> foo\\n> bar\\nlgtm\\n')
+    phrase_test('> LGTM', approvers=set(),
+                expected_output='Missing LGTM from someone other than '
+                                'john@example.com\n')
+
+    # TODO(dpranke): these probably should pass.
+    phrase_test('Looks Good To Me', approvers=set(),
+                expected_output='Missing LGTM from someone other than '
+                                'john@example.com\n')
+    phrase_test('looks good to me', approvers=set(),
+                expected_output='Missing LGTM from someone other than '
+                                'john@example.com\n')
+
+    # TODO(dpranke): this probably shouldn't pass.
+    phrase_test('no lgtm for you')
+
+  def testCannedCheckOwners_HTTPS_HostURL(self):
+    self.AssertOwnersWorks(approvers=set(['ben@example.com']),
+                           host_url='https://localhost')
 
-  def testCannedCheckOwners_CommittingWithLGTMs(self):
-    self.OwnersTest(is_committing=True,
-        approvers=set(['ben@example.com']),
-        uncovered_files=set())
+  def testCannedCheckOwners_MissingSchemeInHostURL(self):
+    self.AssertOwnersWorks(approvers=set(['ben@example.com']),
+                           host_url='localhost')
+
+  def testCannedCheckOwners_NoIssue(self):
+    self.AssertOwnersWorks(issue=None,
+                           expected_output='Change not uploaded for review\n')
+
+  def testCannedCheckOwners_NoLGTM(self):
+    self.AssertOwnersWorks(expected_output='Missing LGTM from someone '
+                                           'other than john@example.com\n')
+
+  def testCannedCheckOwners_OnlyOwnerLGTM(self):
+    self.AssertOwnersWorks(approvers=set(['john@example.com']),
+                           expected_output='Missing LGTM from someone '
+                                           'other than john@example.com\n')
 
   def testCannedCheckOwners_TBR(self):
-    self.OwnersTest(is_committing=True, tbr=True,
-        approvers=set(),
-        uncovered_files=set(),
+    self.AssertOwnersWorks(tbr=True,
         expected_output='--tbr was specified, skipping OWNERS check\n')
 
-  def testCannedCheckOwners_MissingSchemeInHostURL(self):
-    self.OwnersTest(is_committing=True,
-        approvers=set(['ben@example.com']),
-        uncovered_files=set(), host_url='localhost')
+  def testCannedCheckOwners_Upload(self):
+    class FakeInputAPI(object):
+      is_committing = False
+
+    results = presubmit_canned_checks.CheckOwners(FakeInputAPI(),
+                                                  presubmit.OutputApi)
+    self.assertEqual(results, [])
+
+  def testCannedCheckOwners_WithoutOwnerLGTM(self):
+    self.AssertOwnersWorks(uncovered_files=set(['foo.cc']),
+        expected_output='Missing LGTM from an OWNER for: foo.cc\n')
+
+  def testCannedCheckOwners_WithLGTMs(self):
+    self.AssertOwnersWorks(approvers=set(['ben@example.com']),
+                           uncovered_files=set())
 
-  def testCannedCheckOwners_HTTPS_HostURL(self):
-    self.OwnersTest(is_committing=True,
-        approvers=set(['ben@example.com']),
-        uncovered_files=set(), host_url='https://localhost')
 
 
 if __name__ == '__main__':