From b54a78ed9d67798b7de4509ebcb0fdd10f34ff27 Mon Sep 17 00:00:00 2001
From: "dpranke@chromium.org"
 <dpranke@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Thu, 13 Dec 2012 23:37:23 +0000
Subject: [PATCH] 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                          |  6 +++--
 testing_support/filesystem_mock.py |  3 +++
 tests/owners_unittest.py           | 35 +++++++++++++++++-------------
 3 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/owners.py b/owners.py
index 109273e5c..1b1775f08 100644
--- a/owners.py
+++ b/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 = {}
diff --git a/testing_support/filesystem_mock.py b/testing_support/filesystem_mock.py
index 49f7c890b..07e1834ff 100644
--- a/testing_support/filesystem_mock.py
+++ b/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
 
diff --git a/tests/owners_unittest.py b/tests/owners_unittest.py
index e031ab87b..34272a133 100755
--- a/tests/owners_unittest.py
+++ b/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',
-        '/content/baz/froboz.h',
-        '/chrome/renderer/gpu/gpu_channel_host.h'], [brett])
+        'chrome/gpu/gpu_channel.h',
+        'content/baz/froboz.h',
+        'chrome/renderer/gpu/gpu_channel_host.h'], [brett])
 
   def test_reviewers_for__two_owners(self):
     self.assert_reviewers_for([
-        '/chrome/gpu/gpu_channel.h',
-        '/content/content.gyp',
-        '/content/baz/froboz.h',
-        '/content/views/pie.h'
+        'chrome/gpu/gpu_channel.h',
+        'content/content.gyp',
+        'content/baz/froboz.h',
+        'content/views/pie.h'
         ], [john, brett])
 
   def test_reviewers_for__all_files(self):
     self.assert_reviewers_for([
-        '/chrome/gpu/gpu_channel.h',
-        '/chrome/renderer/gpu/gpu_channel_host.h',
-        '/chrome/renderer/safe_browsing/scorer.h',
-        '/content/content.gyp',
-        '/content/bar/foo.cc',
-        '/content/baz/froboz.h',
-        '/content/views/pie.h'], [john, brett])
+        'chrome/gpu/gpu_channel.h',
+        'chrome/renderer/gpu/gpu_channel_host.h',
+        'chrome/renderer/safe_browsing/scorer.h',
+        'content/content.gyp',
+        'content/bar/foo.cc',
+        'content/baz/froboz.h',
+        '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'))