Implement support for file: includes in OWNERS files.

This CL implements support for file: include lines in OWNERS files,
both as top-level directives and as per-file directives. The
paths can be either relative or absolute.

Examples of lines in OWNERS files:

  file:test/OWNERS  (relative, top-level)
  file://content/OWNERS  (absolute, top-level)
  per-file mock_impl.h=file:test/OWNERS  (relative, per-file)
  per-file mock_impl.h=file://content/OWNERS  (absolute, per-file)

A whole series of tests to cover this feature have been added
to owners_unittest.py as well.

BUG=119396, 147633

Review URL: https://codereview.chromium.org/1085993004

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

owners.py

@@ -18,6 +18,7 @@ line      := directive
           | comment
 
 directive := "set noparent"
+          |  "file:" glob
           |  email_address
           |  "*"
 
@@ -45,6 +46,11 @@ If "per-file glob=set noparent" is used, then global directives are ignored
 for the glob, and only the "per-file" owners are used for files matching that
 glob.
 
+If the "file:" directive is used, the referred to OWNERS file will be parsed and
+considered when determining the valid set of OWNERS. If the filename starts with
+"//" it is relative to the root of the repository, otherwise it is relative to
+the current file
+
 Examples for all of these combinations can be found in tests/owners_unittest.py.
 """
 
@@ -118,6 +124,9 @@ class Database(object):
     # (This is implicitly true for the root directory).
     self.stop_looking = set([''])
 
+    # Set of files which have already been read.
+    self.read_files = set()
+
   def reviewers_for(self, files, author):
     """Returns a suggested set of reviewers that will cover the files.
 
@@ -189,16 +198,23 @@ class Database(object):
     for f in files:
       dirpath = self.os_path.dirname(f)
       while not dirpath in self.owners_for:
-        self._read_owners_in_dir(dirpath)
+        self._read_owners(self.os_path.join(dirpath, 'OWNERS'))
         if self._stop_looking(dirpath):
           break
         dirpath = self.os_path.dirname(dirpath)
 
-  def _read_owners_in_dir(self, dirpath):
-    owners_path = self.os_path.join(self.root, dirpath, 'OWNERS')
+  def _read_owners(self, path):
+    owners_path = self.os_path.join(self.root, path)
     if not self.os_path.exists(owners_path):
       return
+
+    if owners_path in self.read_files:
+      return
+
+    self.read_files.add(owners_path)
+
     comment = []
+    dirpath = self.os_path.dirname(path)
     in_comment = False
     lineno = 0
     for line in self.fopen(owners_path):
@@ -244,6 +260,23 @@ class Database(object):
                  line_type, owners_path, lineno, comment):
     if directive == 'set noparent':
       self.stop_looking.add(path)
+    elif directive.startswith('file:'):
+      owners_file = self._resolve_include(directive[5:], owners_path)
+      if not owners_file:
+        raise SyntaxErrorInOwnersFile(owners_path, lineno,
+            ('%s does not refer to an existing file.' % directive[5:]))
+
+      self._read_owners(owners_file)
+
+      dirpath = self.os_path.dirname(owners_file)
+      for key in self.owned_by:
+        if not dirpath in self.owned_by[key]:
+          continue
+        self.owned_by[key].add(path)
+
+      if dirpath in self.owners_for:
+        self.owners_for.setdefault(path, set()).update(self.owners_for[dirpath])
+
     elif self.email_regexp.match(directive) or directive == EVERYONE:
       self.comments.setdefault(directive, {})
       self.comments[directive][path] = comment
@@ -251,9 +284,23 @@ class Database(object):
       self.owners_for.setdefault(path, set()).add(directive)
     else:
       raise SyntaxErrorInOwnersFile(owners_path, lineno,
-          ('%s is not a "set" directive, "*", '
+          ('%s is not a "set" directive, file include, "*", '
            'or an email address: "%s"' % (line_type, directive)))
 
+  def _resolve_include(self, path, start):
+    if path.startswith('//'):
+      include_path = path[2:]
+    else:
+      assert start.startswith(self.root)
+      start = self.os_path.dirname(start[len(self.root):])
+      include_path = self.os_path.join(start, path)
+
+    owners_path = self.os_path.join(self.root, include_path)
+    if not self.os_path.exists(owners_path):
+      return None
+
+    return include_path
+
   def _covering_set_of_owners_for(self, files, author):
     dirs_remaining = set(self._enclosing_dir_with_owners(f) for f in files)
     all_possible_owners = self.all_possible_owners(dirs_remaining, author)

testing_support/filesystem_mock.py

@@ -38,6 +38,11 @@ class MockFileSystem(object):
       return path[:-1]
     return path
 
+  def basename(self, path):
+    if self.sep not in path:
+      return ''
+    return self._split(path)[-1] or self.sep
+
   def dirname(self, path):
     if self.sep not in path:
       return ''

tests/owners_unittest.py

@@ -30,6 +30,8 @@ def owners_file(*email_addresses, **kwargs):
     s += '# %s\n' % kwargs.get('comment')
   if kwargs.get('noparent'):
     s += 'set noparent\n'
+  if kwargs.get('file'):
+    s += 'file:%s\n' % kwargs.get('file')
   s += '\n'.join(kwargs.get('lines', [])) + '\n'
   return s + '\n'.join(email_addresses) + '\n'
 
@@ -54,6 +56,11 @@ def test_repo():
     '/content/baz/froboz.h': '',
     '/content/baz/ugly.cc': '',
     '/content/baz/ugly.h': '',
+    '/content/garply/OWNERS': owners_file(file='test/OWNERS'),
+    '/content/garply/foo.cc': '',
+    '/content/garply/test/OWNERS': owners_file(peter),
+    '/content/qux/OWNERS': owners_file(peter, file='//content/baz/OWNERS'),
+    '/content/qux/foo.cc': '',
     '/content/views/OWNERS': owners_file(ben, john, owners.EVERYONE,
                                          noparent=True),
     '/content/views/pie.h': '',
@@ -209,6 +216,50 @@ class OwnersDatabaseTest(_BaseTestCase):
     self.assertRaises(owners.SyntaxErrorInOwnersFile,
         self.db().files_not_covered_by, ['DEPS'], [brett])
 
+  def test_file_include_absolute_path(self):
+    self.assert_files_not_covered_by(['content/qux/foo.cc'], [brett], [])
+    self.assert_files_not_covered_by(['content/qux/bar.cc'], [peter], [])
+    self.assert_files_not_covered_by(['content/qux/baz.cc'],
+                                     [tom], ['content/qux/baz.cc'])
+
+  def test_file_include_relative_path(self):
+    self.assert_files_not_covered_by(['content/garply/foo.cc'], [peter], [])
+    self.assert_files_not_covered_by(['content/garply/bar.cc'], [darin], [])
+    self.assert_files_not_covered_by(['content/garply/baz.cc'],
+                                     [tom], ['content/garply/baz.cc'])
+
+  def test_file_include_per_file_absolute_path(self):
+    self.files['/content/qux/OWNERS'] = owners_file(peter,
+        lines=['per-file foo.*=file://content/baz/OWNERS'])
+
+    self.assert_files_not_covered_by(['content/qux/foo.cc'], [brett], [])
+    self.assert_files_not_covered_by(['content/qux/baz.cc'],
+                                     [brett], ['content/qux/baz.cc'])
+
+  def test_file_include_per_file_relative_path(self):
+    self.files['/content/garply/OWNERS'] = owners_file(brett,
+        lines=['per-file foo.*=file:test/OWNERS'])
+
+    self.assert_files_not_covered_by(['content/garply/foo.cc'], [peter], [])
+    self.assert_files_not_covered_by(['content/garply/baz.cc'],
+                                     [peter], ['content/garply/baz.cc'])
+
+  def test_file_include_recursive(self):
+    self.files['/content/baz/OWNERS'] = owners_file(file='//chrome/gpu/OWNERS')
+    self.assert_files_not_covered_by(['content/qux/foo.cc'], [ken], [])
+
+  def test_file_include_recursive_loop(self):
+    self.files['/content/baz/OWNERS'] = owners_file(brett,
+        file='//content/qux/OWNERS')
+    self.test_file_include_absolute_path()
+
+  def test_file_include_different_filename(self):
+    self.files['/owners/garply'] = owners_file(peter)
+    self.files['/content/garply/OWNERS'] = owners_file(john,
+        lines=['per-file foo.*=file://owners/garply'])
+
+    self.assert_files_not_covered_by(['content/garply/foo.cc'], [peter], [])
+
   def assert_syntax_error(self, owners_file_contents):
     db = self.db()
     self.files['/foo/OWNERS'] = owners_file_contents
@@ -228,6 +279,12 @@ class OwnersDatabaseTest(_BaseTestCase):
   def test_syntax_error__bad_email(self):
     self.assert_syntax_error('ben\n')
 
+  def test_syntax_error__invalid_absolute_file(self):
+    self.assert_syntax_error('file://foo/bar/baz\n')
+
+  def test_syntax_error__invalid_relative_file(self):
+    self.assert_syntax_error('file:foo/bar/baz\n')
+
 
 class ReviewersForTest(_BaseTestCase):
   def assert_reviewers_for(self, files, potential_suggested_reviewers,
@@ -322,6 +379,33 @@ class ReviewersForTest(_BaseTestCase):
     self.assert_reviewers_for(['chrome/gpu/gpu_channel.h'],
                               [[ben], [brett]], author=ken)
 
+  def test_reviewers_file_includes__absolute(self):
+    self.assert_reviewers_for(['content/qux/foo.cc'],
+                              [[peter], [brett], [john], [darin]])
+
+  def test_reviewers_file_includes__relative(self):
+    self.assert_reviewers_for(['content/garply/foo.cc'],
+                              [[peter], [john], [darin]])
+
+  def test_reviewers_file_includes__per_file(self):
+    self.files['/content/garply/OWNERS'] = owners_file(brett,
+        lines=['per-file foo.*=file:test/OWNERS'])
+
+    self.assert_reviewers_for(['content/garply/foo.cc'],
+                              [[brett], [peter]])
+    self.assert_reviewers_for(['content/garply/bar.cc'],
+                              [[brett]])
+
+  def test_reviewers_file_includes__per_file_noparent(self):
+    self.files['/content/garply/OWNERS'] = owners_file(brett,
+        lines=['per-file foo.*=set noparent',
+               'per-file foo.*=file:test/OWNERS'])
+
+    self.assert_reviewers_for(['content/garply/foo.cc'],
+                              [[peter]])
+    self.assert_reviewers_for(['content/garply/bar.cc'],
+                              [[brett]])
+
 
 class LowestCostOwnersTest(_BaseTestCase):
   # Keep the data in the test_lowest_cost_owner* methods as consistent with