Suggest owners for OWNERS files that only have per-file owners.

If you were creating a new OWNERS file that only had per-file owners
in it (and no catch-all owners for the whole directory), then we
would not look for suggested owners in parent directories, and end up
suggesting nothing. See https://chromiumcodereview.appspot.com/11555036/
for the CL that revealed this.

Also, the unit tests were incorrectly using absolute paths in some cases,
making the code less predictable; I've fixed the unit tests and added
a check for this into owners.py (real changes never used absolute paths,
just paths relative to the checkout root).

R=maruel@chromium.org


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

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

owners.py

@@ -157,7 +157,8 @@ class Database(object):
     def _is_under(f, pfx):
       return self.os_path.abspath(self.os_path.join(pfx, f)).startswith(pfx)
     _assert_is_collection(files)
-    assert all(_is_under(f, self.os_path.abspath(self.root)) for f in files)
+    assert all(not self.os_path.isabs(f) and
+                _is_under(f, self.os_path.abspath(self.root)) for f in files)
 
   def _check_reviewers(self, reviewers):
     _assert_is_collection(reviewers)
@@ -256,7 +257,8 @@ class Database(object):
     # Get the set of directories from the files.
     dirs = set()
     for f in files:
-      dirs.add(self.os_path.dirname(f))
+      dirs.add(self._enclosing_dir_with_owners(f))
+
 
     owned_dirs = {}
     dir_owners = {}

testing_support/filesystem_mock.py

@@ -46,6 +46,9 @@ class MockFileSystem(object):
   def exists(self, path):
     return self.isfile(path) or self.isdir(path)
 
+  def isabs(self, path):
+    return path.startswith(self.sep)
+
   def isfile(self, path):
     return path in self.files and self.files[path] is not None
 

tests/owners_unittest.py

@@ -234,34 +234,39 @@ class OwnersDatabaseTest(unittest.TestCase):
 
   def test_reviewers_for__one_owner(self):
     self.assert_reviewers_for([
-        '/chrome/gpu/gpu_channel.h',
+        'chrome/gpu/gpu_channel.h',
-        '/content/baz/froboz.h',
+        'content/baz/froboz.h',
-        '/chrome/renderer/gpu/gpu_channel_host.h'], [brett])
+        'chrome/renderer/gpu/gpu_channel_host.h'], [brett])
 
   def test_reviewers_for__two_owners(self):
     self.assert_reviewers_for([
-        '/chrome/gpu/gpu_channel.h',
+        'chrome/gpu/gpu_channel.h',
-        '/content/content.gyp',
+        'content/content.gyp',
-        '/content/baz/froboz.h',
+        'content/baz/froboz.h',
-        '/content/views/pie.h'
+        'content/views/pie.h'
         ], [john, brett])
 
   def test_reviewers_for__all_files(self):
     self.assert_reviewers_for([
-        '/chrome/gpu/gpu_channel.h',
+        'chrome/gpu/gpu_channel.h',
-        '/chrome/renderer/gpu/gpu_channel_host.h',
+        'chrome/renderer/gpu/gpu_channel_host.h',
-        '/chrome/renderer/safe_browsing/scorer.h',
+        'chrome/renderer/safe_browsing/scorer.h',
-        '/content/content.gyp',
+        'content/content.gyp',
-        '/content/bar/foo.cc',
+        'content/bar/foo.cc',
-        '/content/baz/froboz.h',
+        'content/baz/froboz.h',
-        '/content/views/pie.h'], [john, brett])
+        'content/views/pie.h'], [john, brett])
+
+  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'], [darin])
 
   def assert_syntax_error(self, owners_file_contents):
     db = self.db()
     self.files['/foo/OWNERS'] = owners_file_contents
     self.files['/foo/DEPS'] = ''
     try:
-      db.reviewers_for(['/foo/DEPS'])
+      db.reviewers_for(['foo/DEPS'])
       self.fail()  # pragma: no cover
     except owners.SyntaxErrorInOwnersFile, e:
       self.assertTrue(str(e).startswith('/foo/OWNERS:1'))