Add a way to require approval from owners other than the author.

Right now we require approval from someone, and we require an owner
approval, but we don't require an approval from an owner *other than
the patch other*. It's conceivable that we might want this, so
I am making this a configurable argument to the presubmit check.

This will also be needed to ensure that we don't suggest you as an
owner for your own patches, when we actually know who you are.

R=maruel@chromium.org
BUG=


Review URL: https://chromiumcodereview.appspot.com/12326151

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

owners.py

@@ -115,13 +115,15 @@ class Database(object):
     # (This is implicitly true for the root directory).
     self.stop_looking = set([''])
 
-  def reviewers_for(self, files):
+  def reviewers_for(self, files, author):
     """Returns a suggested set of reviewers that will cover the files.
 
-    files is a sequence of paths relative to (and under) self.root."""
+    files is a sequence of paths relative to (and under) self.root.
+    If author is nonempty, we ensure it is not included in the set returned
+    in order avoid suggesting the author as a reviewer for their own changes."""
     self._check_paths(files)
     self._load_data_needed_for(files)
-    suggested_owners = self._covering_set_of_owners_for(files)
+    suggested_owners = self._covering_set_of_owners_for(files, author)
     if EVERYONE in suggested_owners:
       if len(suggested_owners) > 1:
         suggested_owners.remove(EVERYONE)
@@ -236,9 +238,9 @@ class Database(object):
           ('%s is not a "set" directive, "*", '
            'or an email address: "%s"' % (line_type, directive)))
 
-  def _covering_set_of_owners_for(self, files):
+  def _covering_set_of_owners_for(self, files, author):
     dirs_remaining = set(self._enclosing_dir_with_owners(f) for f in files)
-    all_possible_owners = self._all_possible_owners(dirs_remaining)
+    all_possible_owners = self._all_possible_owners(dirs_remaining, author)
     suggested_owners = set()
     while dirs_remaining:
       owner = self.lowest_cost_owner(all_possible_owners, dirs_remaining)
@@ -247,7 +249,7 @@ class Database(object):
       dirs_remaining -= dirs_to_remove
     return suggested_owners
 
-  def _all_possible_owners(self, dirs):
+  def _all_possible_owners(self, dirs, author):
     """Returns a list of (potential owner, distance-from-dir) tuples; a
     distance of 1 is the lowest/closest possible distance (which makes the
     subsequent math easier)."""
@@ -257,6 +259,8 @@ class Database(object):
       distance = 1
       while True:
         for owner in self.owners_for.get(dirname, []):
+          if author and owner == author:
+            continue
           all_possible_owners.setdefault(owner, [])
           # If the same person is in multiple OWNERS files above a given
           # directory, only count the closest one.

presubmit_canned_checks.py

@@ -769,7 +769,8 @@ def CheckBuildbotPendingBuilds(input_api, output_api, url, max_pendings,
   return []
 
 
-def CheckOwners(input_api, output_api, source_file_filter=None):
+def CheckOwners(input_api, output_api, source_file_filter=None,
+                author_counts_as_owner=True):
   if input_api.is_committing:
     if input_api.tbr:
       return [output_api.PresubmitNotifyResult(
@@ -792,25 +793,29 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
       owners_db.email_regexp,
       approval_needed=input_api.is_committing)
 
-  if owner_email:
-    message = ''
-    reviewers_plus_owner = reviewers.union(set([owner_email]))
+  if author_counts_as_owner:
+    if owner_email:
+      message = ''
+      reviewers_plus_owner = reviewers.union(set([owner_email]))
+    else:
+      message = ('\nUntil the issue is uploaded, this list will include '
+                 'files for which you are an OWNER.')
+      owner_email = ''
+      reviewers_plus_owner = set()
+    missing_files = owners_db.files_not_covered_by(affected_files,
+                                                   reviewers_plus_owner)
   else:
-    message = ('\nUntil the issue is uploaded, this list will include '
-               'files for which you are an OWNER.')
-    owner_email = ''
-    reviewers_plus_owner = set()
+    message = ''
+    missing_files = owners_db.files_not_covered_by(affected_files, reviewers)
 
-  missing_files = owners_db.files_not_covered_by(affected_files,
-                                                 reviewers_plus_owner)
   if missing_files:
     output_list = [
         output('Missing %s for these files:\n    %s%s' %
                (needed, '\n    '.join(missing_files), message))]
     if not input_api.is_committing:
-      suggested_owners = owners_db.reviewers_for(affected_files)
+      suggested_owners = owners_db.reviewers_for(affected_files, owner_email)
       output_list.append(output('Suggested OWNERS:\n    %s' %
-                                ('\n    '.join(suggested_owners))))
+                                ('\n    '.join(suggested_owners or []))))
     return output_list
 
   if input_api.is_committing and not reviewers:

tests/owners_unittest.py

@@ -214,7 +214,7 @@ class OwnersDatabaseTest(_BaseTestCase):
     self.files['/foo/OWNERS'] = owners_file_contents
     self.files['/foo/DEPS'] = ''
     try:
-      db.reviewers_for(['foo/DEPS'])
+      db.reviewers_for(['foo/DEPS'], None)
       self.fail()  # pragma: no cover
     except owners.SyntaxErrorInOwnersFile, e:
       self.assertTrue(str(e).startswith('/foo/OWNERS:1'))
@@ -230,44 +230,45 @@ class OwnersDatabaseTest(_BaseTestCase):
 
 
 class ReviewersForTest(_BaseTestCase):
-  def assert_reviewers_for(self, files, *potential_suggested_reviewers):
+  def assert_reviewers_for(self, files, potential_suggested_reviewers,
+                           author=None):
     db = self.db()
-    suggested_reviewers = db.reviewers_for(set(files))
+    suggested_reviewers = db.reviewers_for(set(files), author)
     self.assertTrue(suggested_reviewers in
         [set(suggestion) for suggestion in potential_suggested_reviewers])
 
   def test_reviewers_for__basic_functionality(self):
     self.assert_reviewers_for(['chrome/gpu/gpu_channel.h'],
-                              [ken])
+                              [[ken]])
 
   def test_reviewers_for__set_noparent_works(self):
     self.assert_reviewers_for(['content/content.gyp'],
-                              [john],
-                              [darin])
+                              [[john],
+                               [darin]])
 
   def test_reviewers_for__valid_inputs(self):
     db = self.db()
 
     # Check that we're passed in a sequence that isn't a string.
-    self.assertRaises(AssertionError, db.reviewers_for, 'foo')
+    self.assertRaises(AssertionError, db.reviewers_for, 'foo', None)
     if hasattr(owners.collections, 'Iterable'):
       self.assertRaises(AssertionError, db.reviewers_for,
-                        (f for f in ['x', 'y']))
+                        (f for f in ['x', 'y']), None)
 
     # Check that the files are under the root.
     db.root = '/checkout'
-    self.assertRaises(AssertionError, db.reviewers_for, ['/OWNERS'])
+    self.assertRaises(AssertionError, db.reviewers_for, ['/OWNERS'], None)
 
   def test_reviewers_for__wildcard_dir(self):
-    self.assert_reviewers_for(['DEPS'], ['<anyone>'])
-    self.assert_reviewers_for(['DEPS', 'chrome/gpu/gpu_channel.h'], [ken])
+    self.assert_reviewers_for(['DEPS'], [['<anyone>']])
+    self.assert_reviewers_for(['DEPS', 'chrome/gpu/gpu_channel.h'], [[ken]])
 
   def test_reviewers_for__one_owner(self):
     self.assert_reviewers_for([
         'chrome/gpu/gpu_channel.h',
         'content/baz/froboz.h',
         'chrome/renderer/gpu/gpu_channel_host.h'],
-        [brett])
+        [[brett]])
 
   def test_reviewers_for__two_owners(self):
     self.assert_reviewers_for([
@@ -275,7 +276,7 @@ class ReviewersForTest(_BaseTestCase):
         'content/content.gyp',
         'content/baz/froboz.h',
         'content/views/pie.h'],
-        [ken, john])
+        [[ken, john]])
 
   def test_reviewers_for__all_files(self):
     self.assert_reviewers_for([
@@ -286,25 +287,25 @@ class ReviewersForTest(_BaseTestCase):
         'content/bar/foo.cc',
         'content/baz/froboz.h',
         'content/views/pie.h'],
-        [peter, ken, john])
+        [[peter, ken, john]])
 
   def test_reviewers_for__per_file_owners_file(self):
     self.files['/content/baz/OWNERS'] = owners_file(lines=[
         'per-file ugly.*=tom@example.com'])
     self.assert_reviewers_for(['content/baz/OWNERS'],
-                              [john],
-                              [darin])
+                              [[john],
+                               [darin]])
 
   def test_reviewers_for__per_file(self):
     self.files['/content/baz/OWNERS'] = owners_file(lines=[
         'per-file ugly.*=tom@example.com'])
     self.assert_reviewers_for(['content/baz/ugly.cc'],
-                              [tom])
+                              [[tom]])
 
   def test_reviewers_for__two_nested_dirs(self):
     # The same owner is listed in two directories (one above the other)
     self.assert_reviewers_for(['chrome/browser/defaults.h'],
-                              [brett])
+                              [[brett]])
 
     # Here, although either ben or brett could review both files,
     # someone closer to the gpu_channel_host.h should also be suggested.
@@ -313,8 +314,13 @@ class ReviewersForTest(_BaseTestCase):
     self.files['/chrome/renderer/gpu/OWNERS'] = owners_file(ken)
     self.assert_reviewers_for(['chrome/OWNERS',
                                'chrome/renderer/gpu/gpu_channel_host.h'],
-                              [ben, ken],
-                              [brett, ken])
+                              [[ben, ken],
+                               [brett, ken]])
+
+  def test_reviewers_for__author_is_known(self):
+    # We should never suggest ken as a reviewer for his own changes.
+    self.assert_reviewers_for(['chrome/gpu/gpu_channel.h'],
+                              [[ben], [brett]], author=ken)
 
 
 class LowestCostOwnersTest(_BaseTestCase):

tests/presubmit_unittest.py

@@ -2219,12 +2219,15 @@ class CannedChecksUnittest(PresubmitTestsBase):
 
   def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None,
       reviewers=None, is_committing=True, rietveld_response=None,
-      uncovered_files=None, expected_output=''):
+      uncovered_files=None, expected_output='', author_counts_as_owner=True):
     if approvers is None:
+      # The set of people who lgtm'ed a change.
       approvers = set()
     if reviewers is None:
-      reviewers = set()
-    reviewers = reviewers.union(approvers)
+      # The set of people needed to lgtm a change. We default to
+      # the same list as the people who approved it. We use 'reviewers'
+      # to avoid a name collision w/ owners.py.
+      reviewers = approvers
     if uncovered_files is None:
       uncovered_files = set()
 
@@ -2263,15 +2266,20 @@ class CannedChecksUnittest(PresubmitTestsBase):
                 rietveld_response)
         people.add(owner_email)
 
-      fake_db.files_not_covered_by(set(['foo/xyz.cc']),
-          people).AndReturn(uncovered_files)
+      if author_counts_as_owner:
+        fake_db.files_not_covered_by(set(['foo/xyz.cc']),
+            people).AndReturn(uncovered_files)
+      else:
+        fake_db.files_not_covered_by(set(['foo/xyz.cc']),
+            people.difference(set([owner_email]))).AndReturn(uncovered_files)
       if not is_committing and uncovered_files:
-        fake_db.reviewers_for(set(['foo/xyz.cc'])).AndReturn(owner_email)
+        fake_db.reviewers_for(set(['foo/xyz.cc']),
+            owner_email if issue else '').AndReturn(owner_email)
 
     self.mox.ReplayAll()
     output = presubmit.PresubmitOutput()
     results = presubmit_canned_checks.CheckOwners(input_api,
-        presubmit.OutputApi)
+        presubmit.OutputApi, author_counts_as_owner=author_counts_as_owner)
     if results:
       results[0].handle(output)
     self.assertEquals(output.getvalue(), expected_output)
@@ -2376,6 +2384,16 @@ class CannedChecksUnittest(PresubmitTestsBase):
                            is_committing=False,
                            expected_output='')
 
+  def testCannedCheckOwners_AuthorCountsAsOwner(self):
+    self.AssertOwnersWorks(approvers=set(['john@example.com',
+                                          'brett@example.com']),
+                           reviewers=set(['john@example.com',
+                                          'ben@example.com']),
+                           uncovered_files=set(['foo/xyz.cc']),
+                           expected_output='Missing LGTM from an OWNER '
+                                           'for these files:\n    foo/xyz.cc\n',
+                           author_counts_as_owner=False)
+
   def testCannedCheckOwners_TBR(self):
     self.AssertOwnersWorks(tbr=True,
         expected_output='--tbr was specified, skipping OWNERS check\n')