Skip files owned by reviewers when quering for missing owners.

Bug: 728298
Change-Id: If813ff41b1668a2cab6c26b65e424fbe574e629c
Reviewed-on: https://chromium-review.googlesource.com/899086
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>

git_cl.py

@@ -1750,6 +1750,9 @@ class Changelist(object):
     """Get owner from codereview, which may differ from this checkout."""
     return self._codereview_impl.GetIssueOwner()
 
+  def GetReviewers(self):
+    return self._codereview_impl.GetReviewers()
+
   def GetMostRecentPatchset(self):
     return self._codereview_impl.GetMostRecentPatchset()
 
@@ -1908,6 +1911,9 @@ class _ChangelistCodereviewBase(object):
   def GetIssueOwner(self):
     raise NotImplementedError()
 
+  def GetReviewers(self):
+    raise NotImplementedError()
+
   def GetTryJobProperties(self, patchset=None):
     raise NotImplementedError()
 
@@ -2010,6 +2016,9 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
   def GetIssueOwner(self):
     return (self.GetIssueProperties() or {}).get('owner_email')
 
+  def GetReviewers(self):
+    return (self.GetIssueProperties() or {}).get('reviewers')
+
   def AddComment(self, message, publish=None):
     return self.RpcServer().add_comment(self.GetIssue(), message)
 
@@ -3278,6 +3287,10 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
   def GetIssueOwner(self):
     return self._GetChangeDetail(['DETAILED_ACCOUNTS'])['owner']['email']
 
+  def GetReviewers(self):
+    details = self._GetChangeDetail(['DETAILED_ACCOUNTS'])
+    return [reviewer['email'] for reviewer in details['reviewers']['REVIEWER']]
+
 
 _CODEREVIEW_IMPLEMENTATIONS = {
   'rietveld': _RietveldChangelistImpl,
@@ -5896,7 +5909,9 @@ def CMDowners(parser, args):
   return owners_finder.OwnersFinder(
       affected_files,
       change.RepositoryRoot(),
-      author, fopen=file, os_path=os.path,
+      author,
+      cl.GetReviewers(),
+      fopen=file, os_path=os.path,
       disable_color=options.no_color,
       override_files=change.OriginalOwnersFiles()).run()
 

owners_finder.py

@@ -22,7 +22,7 @@ class OwnersFinder(object):
 
   indentation = 0
 
-  def __init__(self, files, local_root, author,
+  def __init__(self, files, local_root, author, reviewers,
                fopen, os_path,
                email_postfix='@chromium.org',
                disable_color=False,
@@ -45,9 +45,13 @@ class OwnersFinder(object):
 
     filtered_files = files
 
-    # Eliminate files that the author can review.
+    reviewers = list(reviewers)
+    if author:
+      reviewers.append(author)
+
+    # Eliminate files that existing reviewers can review.
     filtered_files = list(self.db.files_not_covered_by(
-      filtered_files, [author] if author else []))
+        filtered_files, reviewers))
 
     # If some files are eliminated.
     if len(filtered_files) != len(files):

presubmit_canned_checks.py

@@ -870,12 +870,12 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
 
   owner_email = owner_email or input_api.change.author_email
 
-  if owner_email:
+  finder = input_api.owners_finder(
-    reviewers_plus_owner = set([owner_email]).union(reviewers)
+      affected_files, input_api.change.RepositoryRoot(),
-    missing_files = owners_db.files_not_covered_by(affected_files,
+      owner_email, reviewers, fopen=file, os_path=input_api.os_path,
-        reviewers_plus_owner)
+      email_postfix='', disable_color=True,
-  else:
+      override_files=input_api.change.OriginalOwnersFiles())
-    missing_files = owners_db.files_not_covered_by(affected_files, reviewers)
+  missing_files = finder.unreviewed_files
 
   if missing_files:
     output_list = [
@@ -883,11 +883,6 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
                   (needed, '\n    '.join(sorted(missing_files))))]
     if not input_api.is_committing:
       suggested_owners = owners_db.reviewers_for(missing_files, owner_email)
-      finder = input_api.owners_finder(
-          missing_files, input_api.change.RepositoryRoot(),
-          owner_email, fopen=file, os_path=input_api.os_path,
-          email_postfix='', disable_color=True,
-          override_files=input_api.change.OriginalOwnersFiles())
       owners_with_comments = []
       def RecordComments(text):
         owners_with_comments.append(finder.print_indent() + text)

tests/owners_finder_test.py

@@ -26,6 +26,7 @@ john = 'john@example.com'
 ken = 'ken@example.com'
 peter = 'peter@example.com'
 tom = 'tom@example.com'
+nonowner = 'nonowner@example.com'
 
 
 def owners_file(*email_addresses, **kwargs):
@@ -70,10 +71,11 @@ def test_repo():
 
 
 class OutputInterceptedOwnersFinder(owners_finder.OwnersFinder):
-  def __init__(self, files, local_root,
+  def __init__(self, files, local_root, author, reviewers,
                fopen, os_path, disable_color=False):
     super(OutputInterceptedOwnersFinder, self).__init__(
-      files, local_root, None, fopen, os_path, disable_color=disable_color)
+      files, local_root, author, reviewers, fopen, os_path,
+      disable_color=disable_color)
     self.output = []
     self.indentation_stack = []
 
@@ -113,8 +115,12 @@ class _BaseTestCase(unittest.TestCase):
     self.root = '/'
     self.fopen = self.repo.open_for_reading
 
-  def ownersFinder(self, files):
+  def ownersFinder(self, files, author=nonowner, reviewers=None):
-    finder = OutputInterceptedOwnersFinder(files, self.root,
+    reviewers = reviewers or []
+    finder = OutputInterceptedOwnersFinder(files,
+                                           self.root,
+                                           author,
+                                           reviewers,
                                            fopen=self.fopen,
                                            os_path=self.repo,
                                            disable_color=True)
@@ -128,6 +134,22 @@ class OwnersFinderTests(_BaseTestCase):
   def test_constructor(self):
     self.assertNotEquals(self.defaultFinder(), None)
 
+  def test_skip_files_owned_by_reviewers(self):
+    files = [
+        'chrome/browser/defaults.h',  # owned by brett
+        'content/bar/foo.cc',         # not owned by brett
+    ]
+    finder = self.ownersFinder(files, reviewers=[brett])
+    self.assertEqual(finder.unreviewed_files, {'content/bar/foo.cc'})
+
+  def test_skip_files_owned_by_author(self):
+    files = [
+        'chrome/browser/defaults.h',  # owned by brett
+        'content/bar/foo.cc',         # not owned by brett
+    ]
+    finder = self.ownersFinder(files, author=brett)
+    self.assertEqual(finder.unreviewed_files, {'content/bar/foo.cc'})
+
   def test_reset(self):
     finder = self.defaultFinder()
     i = 0

tests/presubmit_unittest.py

@@ -2440,6 +2440,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
     input_api.owners_db = fake_db
 
     fake_finder = self.mox.CreateMock(owners_finder.OwnersFinder)
+    fake_finder.unreviewed_files = uncovered_files
     fake_finder.print_indent = lambda: ''
     # pylint: disable=unnecessary-lambda
     fake_finder.print_comments = lambda owner: fake_finder.writeln(owner)
@@ -2476,12 +2477,10 @@ class CannedChecksUnittest(PresubmitTestsBase):
           input_api.gerrit._FetchChangeDetail = lambda _: gerrit_response
 
       people.add(change.author_email)
-      fake_db.files_not_covered_by(set(['foo/xyz.cc']),
+      change.OriginalOwnersFiles().AndReturn({})
-          people).AndReturn(uncovered_files)
       if not is_committing and uncovered_files:
         fake_db.reviewers_for(set(['foo']),
             change.author_email).AndReturn(change.author_email)
-        change.OriginalOwnersFiles().AndReturn({})
 
     self.mox.ReplayAll()
     output = presubmit.PresubmitOutput()