diff --git a/git_cl.py b/git_cl.py index f8c536a7e..a3b8b860d 100755 --- a/git_cl.py +++ b/git_cl.py @@ -3244,7 +3244,6 @@ class ChangeDescription(object): reviewers.append(name) if add_owners_tbr: owners_db = owners.Database(change.RepositoryRoot(), - change.GetOwnersStatusFile(), fopen=file, os_path=os.path) all_reviewers = set(tbr_names + reviewers) missing_files = owners_db.files_not_covered_by(change.LocalPaths(), @@ -3464,9 +3463,6 @@ def LoadCodereviewSettingsFromFile(fileobj): SetProperty('run-post-upload-hook', 'RUN_POST_UPLOAD_HOOK', unset_error_ok=True) - if 'OWNERS_STATUS_FILE' in keyvals: - SetProperty('owners-status-file', 'OWNERS_STATUS_FILE', unset_error_ok=True) - if 'GERRIT_HOST' in keyvals: RunGit(['config', 'gerrit.host', keyvals['GERRIT_HOST']]) @@ -5517,7 +5513,6 @@ def CMDowners(parser, args): [f.LocalPath() for f in cl.GetChange(base_branch, None).AffectedFiles()], change.RepositoryRoot(), - change.GetOwnersStatusFile(), author, fopen=file, os_path=os.path, disable_color=options.no_color).run() diff --git a/owners.py b/owners.py index 2a44a9da9..c7ff26949 100644 --- a/owners.py +++ b/owners.py @@ -100,16 +100,14 @@ class Database(object): of changed files, and see if a list of changed files is covered by a list of reviewers.""" - def __init__(self, root, status_file, fopen, os_path): + def __init__(self, root, fopen, os_path): """Args: root: the path to the root of the Repository - status_file: the path relative to root to global status entries or None open: function callback to open a text file for reading os_path: module/object callback with fields for 'abspath', 'dirname', 'exists', 'join', and 'relpath' """ self.root = root - self.status_file = status_file self.fopen = fopen self.os_path = os_path @@ -140,6 +138,9 @@ class Database(object): # being included from another file. self._included_files = {} + # File with global status lines for owners. + self._status_file = None + def reviewers_for(self, files, author): """Returns a suggested set of reviewers that will cover the files. @@ -233,6 +234,8 @@ class Database(object): self.read_files.add(owners_path) + is_toplevel = path == 'OWNERS' + comment = [] dirpath = self.os_path.dirname(path) in_comment = False @@ -241,6 +244,11 @@ class Database(object): lineno += 1 line = line.strip() if line.startswith('#'): + if is_toplevel: + m = re.match('#\s*OWNERS_STATUS\s+=\s+(.+)$', line) + if m: + self._status_file = m.group(1).strip() + continue if not in_comment: comment = [] comment.append(line[1:].strip()) @@ -276,12 +284,15 @@ class Database(object): ' '.join(comment)) def _read_global_comments(self): - if not self.status_file: - return + if not self._status_file: + if not 'OWNERS' in self.read_files: + self._read_owners('OWNERS') + if not self._status_file: + return - owners_status_path = self.os_path.join(self.root, self.status_file) + owners_status_path = self.os_path.join(self.root, self._status_file) if not self.os_path.exists(owners_status_path): - raise IOError('Could not find global status file "%s"' % + raise IOError('Could not find global status file "%s"' % owners_status_path) if owners_status_path in self.read_files: diff --git a/owners_finder.py b/owners_finder.py index 9fd011963..a3610bd29 100644 --- a/owners_finder.py +++ b/owners_finder.py @@ -22,7 +22,7 @@ class OwnersFinder(object): indentation = 0 - def __init__(self, files, local_root, owners_status_file, author, + def __init__(self, files, local_root, author, fopen, os_path, email_postfix='@chromium.org', disable_color=False): @@ -34,8 +34,7 @@ class OwnersFinder(object): self.COLOR_GREY = '' self.COLOR_RESET = '' - self.db = owners_module.Database(local_root, owners_status_file, fopen, - os_path) + self.db = owners_module.Database(local_root, fopen, os_path) self.db.load_data_needed_for(files) self.os_path = os_path @@ -52,8 +51,7 @@ class OwnersFinder(object): if len(filtered_files) != len(files): files = filtered_files # Reload the database. - self.db = owners_module.Database(local_root, owners_status_file, fopen, - os_path) + self.db = owners_module.Database(local_root, fopen, os_path) self.db.load_data_needed_for(files) self.all_possible_owners = self.db.all_possible_owners(files, None) diff --git a/presubmit_support.py b/presubmit_support.py index 087b3e4e2..ae7263a52 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -422,7 +422,6 @@ class InputApi(object): # TODO(dpranke): figure out a list of all approved owners for a repo # in order to be able to handle wildcard OWNERS files? self.owners_db = owners.Database(change.RepositoryRoot(), - change.GetOwnersStatusFile(), fopen=file, os_path=self.os_path) self.verbose = verbose self.Command = CommandData @@ -918,10 +917,6 @@ class Change(object): x for x in self.AffectedFiles(include_deletes=False) if x.IsTestableFile()) - def GetOwnersStatusFile(self): - """Returns the name of the global OWNERS status file.""" - return None - class GitChange(Change): _AFFECTED_FILES = GitAffectedFile @@ -933,19 +928,6 @@ class GitChange(Change): return subprocess.check_output( ['git', 'ls-files', '--', '.'], cwd=root).splitlines() - def GetOwnersStatusFile(self): - """Returns the name of the global OWNERS status file.""" - - try: - status_file = subprocess.check_output( - ['git', 'config', 'rietveld.owners-status-file'], - cwd=self.RepositoryRoot()) - return status_file - except subprocess.CalledProcessError: - pass - - return None - def ListRelevantPresubmitFiles(files, root): """Finds all presubmit files that apply to a given set of source files. diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index cfae719a5..f5cbb7213 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -684,25 +684,6 @@ class TestGitCl(TestCase): ] self.assertIsNone(git_cl.LoadCodereviewSettingsFromFile(codereview_file)) - def test_LoadCodereviewSettingsFromFile_owners_status(self): - codereview_file = StringIO.StringIO('OWNERS_STATUS_FILE: status') - self.calls = [ - ((['git', 'config', '--unset-all', 'rietveld.server'],), ''), - ((['git', 'config', '--unset-all', 'rietveld.cc'],), CERR1), - ((['git', 'config', '--unset-all', 'rietveld.private'],), CERR1), - ((['git', 'config', '--unset-all', 'rietveld.tree-status-url'],), CERR1), - ((['git', 'config', '--unset-all', 'rietveld.viewvc-url'],), CERR1), - ((['git', 'config', '--unset-all', 'rietveld.bug-prefix'],), CERR1), - ((['git', 'config', '--unset-all', 'rietveld.cpplint-regex'],), CERR1), - ((['git', 'config', '--unset-all', 'rietveld.cpplint-ignore-regex'],), - CERR1), - ((['git', 'config', '--unset-all', 'rietveld.project'],), CERR1), - ((['git', 'config', '--unset-all', 'rietveld.run-post-upload-hook'],), - CERR1), - ((['git', 'config', 'rietveld.owners-status-file', 'status'],), ''), - ] - self.assertIsNone(git_cl.LoadCodereviewSettingsFromFile(codereview_file)) - @classmethod def _is_gerrit_calls(cls, gerrit=False): return [((['git', 'config', 'rietveld.autoupdate'],), ''), diff --git a/tests/owners_finder_test.py b/tests/owners_finder_test.py index 2933b1b19..2801aea05 100755 --- a/tests/owners_finder_test.py +++ b/tests/owners_finder_test.py @@ -41,7 +41,8 @@ def owners_file(*email_addresses, **kwargs): def test_repo(): return filesystem_mock.MockFileSystem(files={ '/DEPS': '', - '/OWNERS': owners_file(ken, peter, tom), + '/OWNERS': owners_file(ken, peter, tom, + comment='OWNERS_STATUS = build/OWNERS.status'), '/build/OWNERS.status': '%s: bar' % jochen, '/base/vlog.h': '', '/chrome/OWNERS': owners_file(ben, brett), @@ -73,8 +74,7 @@ class OutputInterceptedOwnersFinder(owners_finder.OwnersFinder): def __init__(self, files, local_root, fopen, os_path, disable_color=False): super(OutputInterceptedOwnersFinder, self).__init__( - files, local_root, os_path.join('build', 'OWNERS.status'), None, - fopen, os_path, disable_color=disable_color) + files, local_root, None, fopen, os_path, disable_color=disable_color) self.output = [] self.indentation_stack = [] diff --git a/tests/owners_unittest.py b/tests/owners_unittest.py index adbe7192f..9956d5080 100755 --- a/tests/owners_unittest.py +++ b/tests/owners_unittest.py @@ -74,11 +74,12 @@ class _BaseTestCase(unittest.TestCase): self.root = '/' self.fopen = self.repo.open_for_reading - def db(self, root=None, fopen=None, os_path=None, status_file=None): + def db(self, root=None, fopen=None, os_path=None): root = root or self.root fopen = fopen or self.fopen os_path = os_path or self.repo - return owners.Database(root, status_file, fopen, os_path) + # pylint: disable=no-value-for-parameter + return owners.Database(root, fopen, os_path) class OwnersDatabaseTest(_BaseTestCase): @@ -335,8 +336,9 @@ class OwnersDatabaseTest(_BaseTestCase): self.assert_syntax_error('file:foo/bar/baz\n') def test_non_existant_status_file(self): - db = self.db(status_file='does_not_exist') - self.files['/foo/OWNERS'] = brett + db = self.db() + self.files['/OWNERS'] = owners_file(brett, + comment='OWNERS_STATUS = nonexistant') self.files['/foo/DEPS'] = '' self.assertRaises(IOError, db.reviewers_for, ['foo/DEPS'], None) diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 0b3ceaccd..76ea971cc 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -120,8 +120,6 @@ index fe3de7b..54ae6e1 100755 self._root = obj.fake_root_dir def RepositoryRoot(self): return self._root - def GetOwnersStatusFile(self): - return None self.mox.StubOutWithMock(presubmit, 'random') self.mox.StubOutWithMock(presubmit, 'warn') @@ -513,7 +511,6 @@ class PresubmitUnittest(PresubmitTestsBase): 0, 0, None) - change.GetOwnersStatusFile = lambda: None executer = presubmit.PresubmitExecuter(change, False, None, False) self.failIf(executer.ExecPresubmitScript('', fake_presubmit)) # No error if no on-upload entry point @@ -1068,7 +1065,6 @@ class InputApiUnittest(PresubmitTestsBase): 0, 0, None) - change.GetOwnersStatusFile = lambda: None input_api = presubmit.InputApi( change, presubmit.os.path.join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'), @@ -1197,7 +1193,6 @@ class InputApiUnittest(PresubmitTestsBase): change = presubmit.GitChange( 'mychange', '', self.fake_root_dir, files, 0, 0, None) - change.GetOwnersStatusFile = lambda: None input_api = presubmit.InputApi( change, presubmit.os.path.join(self.fake_root_dir, 'PRESUBMIT.py'), @@ -1218,7 +1213,6 @@ class InputApiUnittest(PresubmitTestsBase): change = presubmit.GitChange( 'mychange', '', self.fake_root_dir, files, 0, 0, None) - change.GetOwnersStatusFile = lambda: None input_api = presubmit.InputApi( change, './PRESUBMIT.py', False, None, False) # Sample usage of overiding the default white and black lists. @@ -1542,7 +1536,7 @@ class ChangeUnittest(PresubmitTestsBase): 'AbsoluteLocalPaths', 'AffectedFiles', 'AffectedTestableFiles', 'AffectedTextFiles', 'AllFiles', 'DescriptionText', 'FullDescriptionText', - 'GetOwnersStatusFile', 'LocalPaths', 'Name', 'RepositoryRoot', + 'LocalPaths', 'Name', 'RepositoryRoot', 'RightHandSideLines', 'SetDescriptionText', 'TAG_LINE_RE', 'author_email', 'issue', 'patchset', 'scm', 'tags', ]