From d0573ec067f852c3a967b8e6aa38ad07615a6dc9 Mon Sep 17 00:00:00 2001 From: Jochen Eisinger Date: Thu, 13 Apr 2017 10:55:06 +0200 Subject: [PATCH] Use OldContents() for OWNERS files to do checks BUG=141253 R=dpranke@chromium.org Change-Id: Iacbc2f0571e725e4f2ccf5ea7878f101972289bb Reviewed-on: https://chromium-review.googlesource.com/476610 Reviewed-by: Dirk Pranke Commit-Queue: Jochen Eisinger --- git_cl.py | 3 ++- owners.py | 15 ++++++++++++++- owners_finder.py | 5 ++++- presubmit_canned_checks.py | 11 ++++++----- presubmit_support.py | 7 +++++++ tests/owners_unittest.py | 10 +++++++++- tests/presubmit_unittest.py | 4 +++- 7 files changed, 45 insertions(+), 10 deletions(-) diff --git a/git_cl.py b/git_cl.py index ab98b457ab..426b752570 100755 --- a/git_cl.py +++ b/git_cl.py @@ -5553,7 +5553,8 @@ def CMDowners(parser, args): cl.GetChange(base_branch, None).AffectedFiles()], change.RepositoryRoot(), author, fopen=file, os_path=os.path, - disable_color=options.no_color).run() + disable_color=options.no_color, + override_files=change.OriginalOwnersFiles()).run() def BuildGitDiffCmd(diff_type, upstream_commit, args): diff --git a/owners.py b/owners.py index c7ff269497..89ce219b3c 100644 --- a/owners.py +++ b/owners.py @@ -114,6 +114,11 @@ class Database(object): # Pick a default email regexp to use; callers can override as desired. self.email_regexp = re.compile(BASIC_EMAIL_REGEXP) + # Replacement contents for the given files. Maps the file name of an + # OWNERS file (relative to root) to an iterator returning the replacement + # file contents. + self.override_files = {} + # Mapping of owners to the paths or globs they own. self._owners_to_paths = {EVERYONE: set()} @@ -240,7 +245,13 @@ class Database(object): dirpath = self.os_path.dirname(path) in_comment = False lineno = 0 - for line in self.fopen(owners_path): + + if path in self.override_files: + file_iter = self.override_files[path] + else: + file_iter = self.fopen(owners_path) + + for line in file_iter: lineno += 1 line = line.strip() if line.startswith('#'): @@ -395,6 +406,8 @@ class Database(object): dirs_remaining = set(self._enclosing_dir_with_owners(f) for f in files) all_possible_owners = self.all_possible_owners(dirs_remaining, author) suggested_owners = set() + if len(all_possible_owners) == 0: + return suggested_owners while dirs_remaining: owner = self.lowest_cost_owner(all_possible_owners, dirs_remaining) suggested_owners.add(owner) diff --git a/owners_finder.py b/owners_finder.py index a3610bd298..afc364033e 100644 --- a/owners_finder.py +++ b/owners_finder.py @@ -25,7 +25,8 @@ class OwnersFinder(object): def __init__(self, files, local_root, author, fopen, os_path, email_postfix='@chromium.org', - disable_color=False): + disable_color=False, + override_files=None): self.email_postfix = email_postfix if os.name == 'nt' or disable_color: @@ -35,6 +36,7 @@ class OwnersFinder(object): self.COLOR_RESET = '' self.db = owners_module.Database(local_root, fopen, os_path) + self.db.override_files = override_files or {} self.db.load_data_needed_for(files) self.os_path = os_path @@ -52,6 +54,7 @@ class OwnersFinder(object): files = filtered_files # Reload the database. self.db = owners_module.Database(local_root, fopen, os_path) + self.db.override_files = override_files or {} self.db.load_data_needed_for(files) self.all_possible_owners = self.db.all_possible_owners(files, None) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 9d676e5a7b..0f6b72755a 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -883,6 +883,7 @@ def CheckOwners(input_api, output_api, source_file_filter=None): input_api.change.AffectedFiles(file_filter=source_file_filter)]) owners_db = input_api.owners_db + owners_db.override_files = input_api.change.OriginalOwnersFiles() owner_email, reviewers = GetCodereviewOwnerAndReviewers( input_api, owners_db.email_regexp, @@ -903,11 +904,11 @@ def CheckOwners(input_api, output_api, source_file_filter=None): (needed, '\n '.join(sorted(missing_files))))] if not input_api.is_committing: suggested_owners = owners_db.reviewers_for(missing_files, owner_email) - finder = input_api.owners_finder(missing_files, - input_api.change.RepositoryRoot(), - owner_email, - fopen=file, os_path=input_api.os_path, - email_postfix='', disable_color=True) + finder = input_api.owners_finder( + missing_files, input_api.change.RepositoryRoot(), + owner_email, fopen=file, os_path=input_api.os_path, + email_postfix='', disable_color=True, + override_files=input_api.change.OriginalOwnersFiles()) owners_with_comments = [] def RecordComments(text): owners_with_comments.append(finder.print_indent() + text) diff --git a/presubmit_support.py b/presubmit_support.py index f914f0fe3b..ab43d97f69 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -916,6 +916,13 @@ class Change(object): x for x in self.AffectedFiles(include_deletes=False) if x.IsTestableFile()) + def OriginalOwnersFiles(self): + """A map from path names of affected OWNERS files to their old content.""" + def owners_file_filter(f): + return 'OWNERS' in os.path.split(f.LocalPath())[1] + files = self.AffectedFiles(file_filter=owners_file_filter) + return dict([(f.LocalPath(), f.OldContents()) for f in files]) + class GitChange(Change): _AFFECTED_FILES = GitAffectedFile diff --git a/tests/owners_unittest.py b/tests/owners_unittest.py index 9956d50803..698d8f0603 100755 --- a/tests/owners_unittest.py +++ b/tests/owners_unittest.py @@ -18,6 +18,7 @@ import owners ben = 'ben@example.com' brett = 'brett@example.com' darin = 'darin@example.com' +jochen = 'jochen@example.com' john = 'john@example.com' ken = 'ken@example.com' peter = 'peter@example.com' @@ -345,8 +346,9 @@ class OwnersDatabaseTest(_BaseTestCase): class ReviewersForTest(_BaseTestCase): def assert_reviewers_for(self, files, potential_suggested_reviewers, - author=None): + author=None, override_files=None): db = self.db() + db.override_files = override_files or {} suggested_reviewers = db.reviewers_for(set(files), author) self.assertTrue(suggested_reviewers in [set(suggestion) for suggestion in potential_suggested_reviewers]) @@ -463,6 +465,12 @@ class ReviewersForTest(_BaseTestCase): self.assert_reviewers_for(['content/garply/bar.cc'], [[brett]]) + def test_override_files(self): + self.assert_reviewers_for(['content/baz/froboz.h'], [[jochen]], + override_files={'content/baz/OWNERS': [jochen]}) + self.assert_reviewers_for(['content/baz/froboz.h'], [[john],[darin]], + override_files={'content/baz/OWNERS': []}) + class LowestCostOwnersTest(_BaseTestCase): # Keep the data in the test_lowest_cost_owner* methods as consistent with diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 1328407109..3053c7e44e 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -1538,7 +1538,7 @@ class ChangeUnittest(PresubmitTestsBase): 'AbsoluteLocalPaths', 'AffectedFiles', 'AffectedTestableFiles', 'AffectedTextFiles', 'AllFiles', 'DescriptionText', 'FullDescriptionText', - 'LocalPaths', 'Name', 'RepositoryRoot', + 'LocalPaths', 'Name', 'OriginalOwnersFiles', 'RepositoryRoot', 'RightHandSideLines', 'SetDescriptionText', 'TAG_LINE_RE', 'author_email', 'issue', 'patchset', 'scm', 'tags', ] @@ -2276,6 +2276,7 @@ class CannedChecksUnittest(PresubmitTestsBase): if not is_committing or (not tbr and issue): affected_file.LocalPath().AndReturn('foo/xyz.cc') change.AffectedFiles(file_filter=None).AndReturn([affected_file]) + change.OriginalOwnersFiles().AndReturn({}) if issue and not rietveld_response and not gerrit_response: rietveld_response = { "owner_email": change.author_email, @@ -2305,6 +2306,7 @@ class CannedChecksUnittest(PresubmitTestsBase): if not is_committing and uncovered_files: fake_db.reviewers_for(set(['foo']), change.author_email).AndReturn(change.author_email) + change.OriginalOwnersFiles().AndReturn({}) self.mox.ReplayAll() output = presubmit.PresubmitOutput()