diff --git a/owners.py b/owners.py index 30646ffa2..563675a15 100644 --- a/owners.py +++ b/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) diff --git a/testing_support/filesystem_mock.py b/testing_support/filesystem_mock.py index 07e1834ff..b45ea8e37 100644 --- a/testing_support/filesystem_mock.py +++ b/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 '' diff --git a/tests/owners_unittest.py b/tests/owners_unittest.py index f205aed69..5cb177411 100755 --- a/tests/owners_unittest.py +++ b/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