Change the OWNERS check to print files, not directories

Currently, when we run the OWNERS check, we print the list of directories
that contain the relevant OWNERS files for any modified files in a change
still needing approval. 

This has two problems:

1) if we bubble all the way up to the top level OWNERS, we print "" instead of
"src/" or something more useful (bug 157191)

2) for OWNERS files that contain per-file set-noparent entries (like changes to IPC messages), this can be really confusing because an owner of other stuff in the directory might've approved things already.

This change will now print the list of files in the CL that are still unapproved.
This might be a lot more verbose (since you get N lines rather than 1 for N files in a given directory), but hopefully it'll be clearer in the two cases above.

Also, this change takes care of some lingering clean-up in the code to rename some methods to be clearer.

R=maruel@chromium.org
BUG=157191




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

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

owners.py

@@ -129,13 +129,8 @@ class Database(object):
         suggested_owners = set(['<anyone>'])
     return suggested_owners
 
-  # TODO(dpranke): rename to objects_not_covered_by
-  def directories_not_covered_by(self, files, reviewers):
-    """Returns the set of directories that are not owned by a reviewer.
-
-    Determines which of the given files are not owned by at least one of the
-    reviewers, then returns a set containing the applicable enclosing
-    directories, i.e. the ones upward from the files that have OWNERS files.
+  def files_not_covered_by(self, files, reviewers):
+    """Returns the files not owned by one of the reviewers.
 
     Args:
         files is a sequence of paths relative to (and under) self.root.
@@ -145,20 +140,11 @@ class Database(object):
     self._check_reviewers(reviewers)
     self._load_data_needed_for(files)
 
-    objs = set()
-    for f in files:
-      if f in self.owners_for:
-        objs.add(f)
-      else:
-        objs.add(self.os_path.dirname(f))
-
     covered_objs = self._objs_covered_by(reviewers)
-    uncovered_objs = [self._enclosing_obj_with_owners(o) for o in objs
-                      if not self._is_obj_covered_by(o, covered_objs)]
+    uncovered_files = [f for f in files
+                       if not self._is_obj_covered_by(f, covered_objs)]
 
-    return set(uncovered_objs)
-
-  objects_not_covered_by = directories_not_covered_by
+    return set(uncovered_files)
 
   def _check_paths(self, files):
     def _is_under(f, pfx):
@@ -171,38 +157,29 @@ class Database(object):
     _assert_is_collection(reviewers)
     assert all(self.email_regexp.match(r) for r in reviewers)
 
-  # TODO(dpranke): Rename to _objs_covered_by and update_callers
-  def _dirs_covered_by(self, reviewers):
-    dirs = self.owned_by[EVERYONE]
+  def _objs_covered_by(self, reviewers):
+    objs = self.owned_by[EVERYONE]
     for r in reviewers:
-      dirs = dirs | self.owned_by.get(r, set())
-    return dirs
-
-  _objs_covered_by = _dirs_covered_by
+      objs = objs | self.owned_by.get(r, set())
+    return objs
 
-  def _stop_looking(self, dirname):
-    return dirname in self.stop_looking
+  def _stop_looking(self, objname):
+    return objname in self.stop_looking
 
-  # TODO(dpranke): Rename to _is_dir_covered_by and update callers.
-  def _is_dir_covered_by(self, dirname, covered_dirs):
-    while not dirname in covered_dirs and not self._stop_looking(dirname):
-      dirname = self.os_path.dirname(dirname)
-    return dirname in covered_dirs
+  def _is_obj_covered_by(self, objname, covered_objs):
+    while not objname in covered_objs and not self._stop_looking(objname):
+      objname = self.os_path.dirname(objname)
+    return objname in covered_objs
 
-  _is_obj_covered_by = _is_dir_covered_by
-
-  # TODO(dpranke): Rename to _enclosing_obj_with_owners and update callers.
-  def _enclosing_dir_with_owners(self, directory):
+  def _enclosing_dir_with_owners(self, objname):
     """Returns the innermost enclosing directory that has an OWNERS file."""
-    dirpath = directory
+    dirpath = objname
     while not dirpath in self.owners_for:
       if self._stop_looking(dirpath):
         break
       dirpath = self.os_path.dirname(dirpath)
     return dirpath
 
-  _enclosing_obj_with_owners = _enclosing_dir_with_owners
-
   def _load_data_needed_for(self, files):
     for f in files:
       dirpath = self.os_path.dirname(f)

presubmit_canned_checks.py

@@ -797,16 +797,16 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
     reviewers_plus_owner = reviewers.union(set([owner_email]))
   else:
     message = ('\nUntil the issue is uploaded, this list will include '
-               'directories for which you \nare an OWNER.')
+               'files for which you are an OWNER.')
     owner_email = ''
     reviewers_plus_owner = set()
 
-  missing_directories = owners_db.directories_not_covered_by(affected_files,
-      reviewers_plus_owner)
-  if missing_directories:
+  missing_files = owners_db.files_not_covered_by(affected_files,
+                                                 reviewers_plus_owner)
+  if missing_files:
     output_list = [
-        output('Missing %s for files in these directories:\n    %s%s' %
-               (needed, '\n    '.join(missing_directories), message))]
+        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)
       output_list.append(output('Suggested OWNERS:\n    %s' %

tests/owners_unittest.py

@@ -80,59 +80,59 @@ class OwnersDatabaseTest(_BaseTestCase):
   def test_constructor(self):
     self.assertNotEquals(self.db(), None)
 
-  def test_dirs_not_covered_by__valid_inputs(self):
+  def test_files_not_covered_by__valid_inputs(self):
     db = self.db()
 
     # Check that we're passed in a sequence that isn't a string.
-    self.assertRaises(AssertionError, db.directories_not_covered_by, 'foo', [])
+    self.assertRaises(AssertionError, db.files_not_covered_by, 'foo', [])
     if hasattr(owners.collections, 'Iterable'):
-      self.assertRaises(AssertionError, db.directories_not_covered_by,
+      self.assertRaises(AssertionError, db.files_not_covered_by,
                         (f for f in ['x', 'y']), [])
 
     # Check that the files are under the root.
     db.root = '/checkout'
-    self.assertRaises(AssertionError, db.directories_not_covered_by,
+    self.assertRaises(AssertionError, db.files_not_covered_by,
                       ['/OWNERS'], [])
     db.root = '/'
 
     # Check invalid email address.
-    self.assertRaises(AssertionError, db.directories_not_covered_by,
+    self.assertRaises(AssertionError, db.files_not_covered_by,
                       ['OWNERS'], ['foo'])
 
-  def assert_dirs_not_covered_by(self, files, reviewers, unreviewed_dirs):
+  def assert_files_not_covered_by(self, files, reviewers, unreviewed_files):
     db = self.db()
-    self.assertEquals(
-        db.directories_not_covered_by(set(files), set(reviewers)),
-        set(unreviewed_dirs))
+    self.assertEquals(db.files_not_covered_by(set(files), set(reviewers)),
+                      set(unreviewed_files))
 
-  def test_dirs_not_covered_by__owners_propagates_down(self):
-    self.assert_dirs_not_covered_by(
+  def test_files_not_covered_by__owners_propagates_down(self):
+    self.assert_files_not_covered_by(
       ['chrome/gpu/gpu_channel.h', 'chrome/renderer/gpu/gpu_channel_host.h'],
       [ben], [])
 
-  def test_dirs_not_covered_by__partial_covering(self):
-    self.assert_dirs_not_covered_by(
+  def test_files_not_covered_by__partial_covering(self):
+    self.assert_files_not_covered_by(
       ['content/content.gyp', 'chrome/renderer/gpu/gpu_channel_host.h'],
-      [peter], ['content'])
+      [peter], ['content/content.gyp'])
 
-  def test_dirs_not_covered_by__set_noparent_works(self):
-    self.assert_dirs_not_covered_by(['content/content.gyp'], [ben],
-                                    ['content'])
+  def test_files_not_covered_by__set_noparent_works(self):
+    self.assert_files_not_covered_by(['content/content.gyp'], [ben],
+                                    ['content/content.gyp'])
 
-  def test_dirs_not_covered_by__no_reviewer(self):
-    self.assert_dirs_not_covered_by(
+  def test_files_not_covered_by__no_reviewer(self):
+    self.assert_files_not_covered_by(
       ['content/content.gyp', 'chrome/renderer/gpu/gpu_channel_host.h'],
-      [], ['content'])
+      [], ['content/content.gyp'])
 
-  def test_dirs_not_covered_by__combines_directories(self):
-    self.assert_dirs_not_covered_by(['content/content.gyp',
+  def test_files_not_covered_by__combines_directories(self):
+    self.assert_files_not_covered_by(['content/content.gyp',
                                      'content/bar/foo.cc',
                                      'chrome/renderer/gpu/gpu_channel_host.h'],
                                     [peter],
-                                    ['content'])
+                                    ['content/content.gyp',
+                                     'content/bar/foo.cc'])
 
-  def test_dirs_not_covered_by__multiple_directories(self):
-    self.assert_dirs_not_covered_by(
+  def test_files_not_covered_by__multiple_directories(self):
+    self.assert_files_not_covered_by(
         ['content/content.gyp',                    # Not covered
          'content/bar/foo.cc',                     # Not covered (combines in)
          'content/baz/froboz.h',                   # Not covered
@@ -140,23 +140,23 @@ class OwnersDatabaseTest(_BaseTestCase):
          'chrome/renderer/gpu/gpu_channel_host.h'  # Owned by * via parent
         ],
         [ken],
-        ['content', 'content/baz'])
+        ['content/content.gyp', 'content/bar/foo.cc', 'content/baz/froboz.h'])
 
   def test_per_file(self):
     # brett isn't allowed to approve ugly.cc
     self.files['/content/baz/OWNERS'] = owners_file(brett,
         lines=['per-file ugly.*=tom@example.com'])
-    self.assert_dirs_not_covered_by(['content/baz/ugly.cc'],
+    self.assert_files_not_covered_by(['content/baz/ugly.cc'],
                                     [brett],
                                     [])
 
     # tom is allowed to approve ugly.cc, but not froboz.h
-    self.assert_dirs_not_covered_by(['content/baz/ugly.cc'],
+    self.assert_files_not_covered_by(['content/baz/ugly.cc'],
                                     [tom],
                                     [])
-    self.assert_dirs_not_covered_by(['content/baz/froboz.h'],
+    self.assert_files_not_covered_by(['content/baz/froboz.h'],
                                     [tom],
-                                    ['content/baz'])
+                                    ['content/baz/froboz.h'])
 
   def test_per_file_with_spaces(self):
     # This is the same as test_per_file(), except that we include spaces
@@ -164,16 +164,16 @@ class OwnersDatabaseTest(_BaseTestCase):
     # tom is allowed to approve ugly.cc, but not froboz.h
     self.files['/content/baz/OWNERS'] = owners_file(brett,
         lines=['per-file ugly.* = tom@example.com'])
-    self.assert_dirs_not_covered_by(['content/baz/ugly.cc'],
+    self.assert_files_not_covered_by(['content/baz/ugly.cc'],
                                     [brett],
                                     [])
 
-    self.assert_dirs_not_covered_by(['content/baz/ugly.cc'],
+    self.assert_files_not_covered_by(['content/baz/ugly.cc'],
                                     [tom],
                                     [])
-    self.assert_dirs_not_covered_by(['content/baz/froboz.h'],
+    self.assert_files_not_covered_by(['content/baz/froboz.h'],
                                     [tom],
-                                    ['content/baz'])
+                                    ['content/baz/froboz.h'])
 
   def test_per_file__set_noparent(self):
     self.files['/content/baz/OWNERS'] = owners_file(brett,
@@ -181,22 +181,22 @@ class OwnersDatabaseTest(_BaseTestCase):
                'per-file ugly.*=set noparent'])
 
     # brett isn't allowed to approve ugly.cc
-    self.assert_dirs_not_covered_by(['content/baz/ugly.cc'],
+    self.assert_files_not_covered_by(['content/baz/ugly.cc'],
                                     [brett],
                                     ['content/baz/ugly.cc'])
 
     # tom is allowed to approve ugly.cc, but not froboz.h
-    self.assert_dirs_not_covered_by(['content/baz/ugly.cc'],
+    self.assert_files_not_covered_by(['content/baz/ugly.cc'],
                                     [tom],
                                     [])
 
-    self.assert_dirs_not_covered_by(['content/baz/froboz.h'],
+    self.assert_files_not_covered_by(['content/baz/froboz.h'],
                                     [tom],
-                                    ['content/baz'])
+                                    ['content/baz/froboz.h'])
 
   def test_per_file_wildcard(self):
     self.files['/OWNERS'] = 'per-file DEPS=*\n'
-    self.assert_dirs_not_covered_by(['DEPS'], [brett], [])
+    self.assert_files_not_covered_by(['DEPS'], [brett], [])
 
   def test_mock_relpath(self):
     # This test ensures the mock relpath has the arguments in the right
@@ -207,7 +207,7 @@ class OwnersDatabaseTest(_BaseTestCase):
   def test_per_file_glob_across_dirs_not_allowed(self):
     self.files['/OWNERS'] = 'per-file content/*=john@example.org\n'
     self.assertRaises(owners.SyntaxErrorInOwnersFile,
-        self.db().directories_not_covered_by, ['DEPS'], [brett])
+        self.db().files_not_covered_by, ['DEPS'], [brett])
 
   def assert_syntax_error(self, owners_file_contents):
     db = self.db()

tests/presubmit_unittest.py

@@ -2219,14 +2219,14 @@ class CannedChecksUnittest(PresubmitTestsBase):
 
   def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None,
       reviewers=None, is_committing=True, rietveld_response=None,
-      uncovered_dirs=None, expected_output=''):
+      uncovered_files=None, expected_output=''):
     if approvers is None:
       approvers = set()
     if reviewers is None:
       reviewers = set()
     reviewers = reviewers.union(approvers)
-    if uncovered_dirs is None:
-      uncovered_dirs = set()
+    if uncovered_files is None:
+      uncovered_files = set()
 
     change = self.mox.CreateMock(presubmit.Change)
     change.issue = issue
@@ -2263,9 +2263,9 @@ class CannedChecksUnittest(PresubmitTestsBase):
                 rietveld_response)
         people.add(owner_email)
 
-      fake_db.directories_not_covered_by(set(['foo/xyz.cc']),
-          people).AndReturn(uncovered_dirs)
-      if not is_committing and uncovered_dirs:
+      fake_db.files_not_covered_by(set(['foo/xyz.cc']),
+          people).AndReturn(uncovered_files)
+      if not is_committing and uncovered_files:
         fake_db.reviewers_for(set(['foo/xyz.cc'])).AndReturn(owner_email)
 
     self.mox.ReplayAll()
@@ -2352,18 +2352,16 @@ class CannedChecksUnittest(PresubmitTestsBase):
 
   def testCannedCheckOwners_NoIssue(self):
     self.AssertOwnersWorks(issue=None,
-        uncovered_dirs=set(['foo']),
+        uncovered_files=set(['foo']),
         expected_output="OWNERS check failed: this change has no Rietveld "
                         "issue number, so we can't check it for approvals.\n")
     self.AssertOwnersWorks(issue=None,
         is_committing=False,
-        uncovered_dirs=set(['foo']),
-        expected_output='Missing OWNER reviewers for files in these '
-                        'directories:\n'
+        uncovered_files=set(['foo']),
+        expected_output='Missing OWNER reviewers for these files:\n'
                         '    foo\n'
                         'Until the issue is uploaded, this list will include '
-                        'directories for which you \n'
-                        'are an OWNER.\n')
+                        'files for which you are an OWNER.\n')
 
   def testCannedCheckOwners_NoLGTM(self):
     self.AssertOwnersWorks(expected_output='Missing LGTM from someone '
@@ -2384,22 +2382,20 @@ class CannedChecksUnittest(PresubmitTestsBase):
     self.AssertOwnersWorks(tbr=True, is_committing=False, expected_output='')
 
   def testCannedCheckOwners_WithoutOwnerLGTM(self):
-    self.AssertOwnersWorks(uncovered_dirs=set(['foo']),
-        expected_output='Missing LGTM from an OWNER for files in these '
-                        'directories:\n'
+    self.AssertOwnersWorks(uncovered_files=set(['foo']),
+        expected_output='Missing LGTM from an OWNER for these files:\n'
                         '    foo\n')
-    self.AssertOwnersWorks(uncovered_dirs=set(['foo']),
-                           is_committing=False,
-        expected_output='Missing OWNER reviewers for files in these '
-                        'directories:\n'
+    self.AssertOwnersWorks(uncovered_files=set(['foo']),
+        is_committing=False,
+        expected_output='Missing OWNER reviewers for these files:\n'
                         '    foo\n')
 
   def testCannedCheckOwners_WithLGTMs(self):
     self.AssertOwnersWorks(approvers=set(['ben@example.com']),
-                           uncovered_dirs=set())
+                           uncovered_files=set())
     self.AssertOwnersWorks(approvers=set(['ben@example.com']),
                            is_committing=False,
-                           uncovered_dirs=set())
+                           uncovered_files=set())
 
   def testCannedRunUnitTests(self):
     change = presubmit.Change(