From 93248c5ccbaab4d3a43bc86681ff21bdbb932f6c Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Mon, 15 May 2017 11:23:02 -0700 Subject: [PATCH] owners: handle CLs which only have partial OWNERS If every file in a change has OWNERS, then the code would find them fine. Similarly, if no files has OWNERS, then this code would return an empty set just fine. But if some files had OWNERS while others didn't, it would crash when it tried to find an OWNER for file 'foo' while all the possible OWNERS only provided coverage for file 'bar'. This code purges the list of possible OWNERS as they become useless for providing additional coverage, and returns whatever set we have accumulated so far when the set of possible OWNERS becomes empty. R=iannucci@chromium.org Bug: 715062 Change-Id: I408601bd89379381db1cc7df56beed97ab3c27e6 Reviewed-on: https://chromium-review.googlesource.com/506239 Commit-Queue: Aaron Gable Reviewed-by: Robbie Iannucci --- owners.py | 21 +++++++++++++++------ tests/owners_unittest.py | 7 +++++++ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/owners.py b/owners.py index 407567b86..c11780786 100644 --- a/owners.py +++ b/owners.py @@ -418,19 +418,28 @@ 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: + while dirs_remaining and all_possible_owners: owner = self.lowest_cost_owner(all_possible_owners, dirs_remaining) suggested_owners.add(owner) dirs_to_remove = set(el[0] for el in all_possible_owners[owner]) dirs_remaining -= dirs_to_remove + # Now that we've used `owner` and covered all their dirs, remove them + # from consideration. + del all_possible_owners[owner] + for o, dirs in all_possible_owners.items(): + new_dirs = [(d, dist) for (d, dist) in dirs if d not in dirs_to_remove] + if not new_dirs: + del all_possible_owners[o] + else: + all_possible_owners[o] = new_dirs return suggested_owners def all_possible_owners(self, dirs, author): - """Returns a list of (potential owner, distance-from-dir) tuples; a - distance of 1 is the lowest/closest possible distance (which makes the - subsequent math easier).""" + """Returns a dict of {potential owner: (dir, distance)} mappings. + + A distance of 1 is the lowest/closest possible distance (which makes the + subsequent math easier). + """ all_possible_owners = {} for current_dir in dirs: dirname = current_dir diff --git a/tests/owners_unittest.py b/tests/owners_unittest.py index 0cb0a1ede..b72a9b6cb 100755 --- a/tests/owners_unittest.py +++ b/tests/owners_unittest.py @@ -467,6 +467,13 @@ class ReviewersForTest(_BaseTestCase): self.assert_reviewers_for(['chrome/gpu/gpu_channel.h'], [[ben], [brett]], author=ken) + + def test_reviewers_for__ignores_unowned_files(self): + # Clear the root OWNERS file. + self.files['/OWNERS'] = '' + self.assert_reviewers_for(['base/vlog.h', 'chrome/browser/deafults/h'], + [[brett]]) + def test_reviewers_file_includes__absolute(self): self.assert_reviewers_for(['content/qux/foo.cc'], [[peter], [brett], [john], [darin]])