From 046e175ff04dd0c2c1b6becad2ed352802a6208b Mon Sep 17 00:00:00 2001
From: "zork@chromium.org"
 <zork@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Mon, 7 May 2012 05:56:12 +0000
Subject: [PATCH] Output a list of suggested OWNERS reviewers when needed.

BUG=None
TEST=Change files that need OWNERS review.  Upload the patch.  Check that the warning suggests a minimum set of reviewers.

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@135619 0039d316-1c4b-4281-b951-d872f2087c98
---
 owners.py                   | 55 ++++++++++++++++++++++++++++++++-----
 presubmit_canned_checks.py  | 10 +++++--
 tests/owners_unittest.py    |  4 +--
 tests/presubmit_unittest.py |  2 ++
 4 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/owners.py b/owners.py
index d8692c362..a7067cba4 100644
--- a/owners.py
+++ b/owners.py
@@ -165,15 +165,56 @@ class Database(object):
              'or an email address: "%s"' % line))
 
   def _covering_set_of_owners_for(self, files):
-    # TODO(dpranke): implement the greedy algorithm for covering sets, and
-    # consider returning multiple options in case there are several equally
-    # short combinations of owners.
-    every_owner = set()
+    # Get the set of directories from the files.
+    dirs = set()
     for f in files:
-      dirname = self.os_path.dirname(f)
+      dirs.add(self.os_path.dirname(f))
+
+    owned_dirs = {}
+    dir_owners = {}
+
+    for current_dir in dirs:
+      # Get the list of owners for each directory.
+      current_owners = set()
+      dirname = current_dir
       while dirname in self.owners_for:
-        every_owner |= self.owners_for[dirname]
+        for owner in self.owners_for[dirname]:
+          current_owners.add(owner)
         if self._stop_looking(dirname):
           break
         dirname = self.os_path.dirname(dirname)
-    return every_owner
+
+      # Map each directory to a list of its owners.
+      dir_owners[current_dir] = current_owners
+
+      # Add the directory to the list of each owner.
+      for owner in current_owners:
+        if not owner in owned_dirs:
+          owned_dirs[owner] = set()
+        owned_dirs[owner].add(current_dir)
+
+    final_owners = set()
+    while dirs:
+      # Find the owner that has the most directories.
+      max_count = 0
+      max_owner = None
+      owner_count = {}
+      for dirname in dirs:
+        for owner in dir_owners[dirname]:
+          count = owner_count.get(owner, 0) + 1
+          owner_count[owner] = count
+          if count >= max_count:
+            max_owner = owner
+
+      # If no more directories have OWNERS, we're done.
+      if not max_owner:
+        break
+
+      final_owners.add(max_owner)
+
+      # Remove all directories owned by the current owner from the remaining
+      # list.
+      for dirname in owned_dirs[max_owner]:
+        dirs.remove(dirname)
+
+    return final_owners
diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py
index e7b503e0a..013effdc5 100644
--- a/presubmit_canned_checks.py
+++ b/presubmit_canned_checks.py
@@ -769,8 +769,14 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
   missing_directories = owners_db.directories_not_covered_by(affected_files,
       reviewers_plus_owner)
   if missing_directories:
-    return [output('Missing %s for files in these directories:\n    %s%s' %
-                   (needed, '\n    '.join(missing_directories), message))]
+    output_list = [
+        output('Missing %s for files in these directories:\n    %s%s' %
+               (needed, '\n    '.join(missing_directories), message))]
+    if not input_api.is_committing:
+      suggested_owners = owners_db.reviewers_for(affected_files)
+      output_list.append(output('Suggested OWNERS:\n    %s' %
+                                ('\n    '.join(suggested_owners))))
+    return output_list
 
   if input_api.is_committing and not reviewers:
     return [output('Missing LGTM from someone other than %s' % owner_email)]
diff --git a/tests/owners_unittest.py b/tests/owners_unittest.py
index 22629418b..4f9db7f24 100755
--- a/tests/owners_unittest.py
+++ b/tests/owners_unittest.py
@@ -136,10 +136,10 @@ class OwnersDatabaseTest(unittest.TestCase):
 
   def test_reviewers_for__basic_functionality(self):
     self.assert_reviewers_for(['chrome/gpu/gpu_channel.h'],
-                             [ken, ben, brett, owners.EVERYONE])
+                             [brett])
 
   def test_reviewers_for__set_noparent_works(self):
-    self.assert_reviewers_for(['content/content.gyp'], [john, darin])
+    self.assert_reviewers_for(['content/content.gyp'], [darin])
 
   def test_reviewers_for__valid_inputs(self):
     db = self.db()
diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py
index 75d140ada..d122cf77a 100755
--- a/tests/presubmit_unittest.py
+++ b/tests/presubmit_unittest.py
@@ -2229,6 +2229,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
 
       fake_db.directories_not_covered_by(set(['foo/xyz.cc']),
           people).AndReturn(uncovered_dirs)
+      if not is_committing and uncovered_dirs:
+        fake_db.reviewers_for(set(['foo/xyz.cc'])).AndReturn(owner_email)
 
     self.mox.ReplayAll()
     output = presubmit.PresubmitOutput()