From 968b1fe7d7cb848e250ffb62e27f547450f5b9e9 Mon Sep 17 00:00:00 2001 From: Edward Lesmes Date: Fri, 4 Dec 2020 00:45:01 +0000 Subject: [PATCH] [git-cl] Use owners client when processing --[tb]r-owners. Change-Id: Id094bce2aa731359cd8af16f10ce79ae7e02bd85 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2572809 Commit-Queue: Edward Lesmes Auto-Submit: Edward Lesmes Reviewed-by: Josip Sokcevic --- git_cl.py | 35 +++++++++++++-------- tests/git_cl_test.py | 72 +++++++++++++++++++++----------------------- 2 files changed, 57 insertions(+), 50 deletions(-) diff --git a/git_cl.py b/git_cl.py index bca26568e..90427ef58 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1372,9 +1372,24 @@ class Changelist(object): # Set the reviewer list now so that presubmit checks can access it. if options.reviewers or options.tbrs or options.add_owners_to: + add_owners = [] + if options.add_owners_to: + # Fill gaps in OWNERS coverage to tbrs/reviewers if requested. + project = self.GetGerritProject() + branch = self.GetCommonAncestorWithUpstream() + client = owners_client.DepotToolsClient( + host=self.GetGerritHost(), + root=settings.GetRoot(), + branch=branch) + status = client.GetFilesApprovalStatus( + project, branch, files, [], options.tbrs + options.reviewers) + missing_files = [ + f for f in files + if status[f] == owners_client.INSUFFICIENT_REVIEWERS + ] + add_owners = client.SuggestOwners(project, branch, missing_files) change_description.update_reviewers( - options.reviewers, options.tbrs, options.add_owners_to, files, - self.GetAuthor()) + options.reviewers, options.tbrs, options.add_owners_to, add_owners) return change_description @@ -2607,8 +2622,7 @@ class ChangeDescription(object): description = git_footers.add_footer_change_id(description, change_id) self.set_description(description) - def update_reviewers( - self, reviewers, tbrs, add_owners_to, affected_files, author_email): + def update_reviewers(self, reviewers, tbrs, add_owners_to, add_owners): """Rewrites the R=/TBR= line(s) as a single line each. Args: @@ -2616,14 +2630,14 @@ class ChangeDescription(object): tbrs (list(str)) - list of additional emails to use for TBRs. add_owners_to (None|'R'|'TBR') - Pass to do an OWNERS lookup for files in the change that are missing OWNER coverage. If this is not None, you - must also pass a value for `change`. - change (Change) - The Change that should be used for OWNERS lookups. + must also pass a value for `add_owners`. + add_owners (list(str)) - Owners to add to R or TBR if requested. """ assert isinstance(reviewers, list), reviewers assert isinstance(tbrs, list), tbrs assert add_owners_to in (None, 'TBR', 'R'), add_owners_to - assert not add_owners_to or affected_files, add_owners_to + assert not add_owners_to or add_owners, add_owners_to if not reviewers and not tbrs and not add_owners_to: return @@ -2652,12 +2666,7 @@ class ChangeDescription(object): # Next, maybe fill in OWNERS coverage gaps to either tbrs/reviewers. if add_owners_to: - owners_db = owners.Database(settings.GetRoot(), - fopen=open, os_path=os.path) - missing_files = owners_db.files_not_covered_by(affected_files, - (tbrs | reviewers)) - LOOKUP[add_owners_to].update( - owners_db.reviewers_for(missing_files, author_email)) + LOOKUP[add_owners_to].update(add_owners) # If any folks ended up in both groups, remove them from tbrs. tbrs -= reviewers diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 6ec82b3f7..480678b44 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -562,9 +562,6 @@ class TestGitCl(unittest.TestCase): 'git_cl.gclient_utils.CheckCallAndFilter', self._mocked_call).start() mock.patch('git_common.is_dirty_git_tree', lambda x: False).start() - mock.patch( - 'git_common.get_or_create_merge_base', - lambda *a: self._mocked_call('get_or_create_merge_base', *a)).start() mock.patch('git_cl.FindCodereviewSettingsFile', return_value='').start() mock.patch( 'git_cl.SaveDescriptionBackup', @@ -707,17 +704,13 @@ class TestGitCl(unittest.TestCase): def _gerrit_base_calls(cls, issue=None, fetched_description=None, fetched_status=None, other_cl_owner=None, custom_cl_base=None, short_hostname='chromium', - change_id=None, new_default=False): + change_id=None, default_branch='master'): calls = [] if custom_cl_base: ancestor_revision = custom_cl_base else: # Determine ancestor_revision to be merge base. - ancestor_revision = 'fake_ancestor_sha' - calls += [ - (('get_or_create_merge_base', 'master', - 'refs/remotes/origin/master'), ancestor_revision), - ] + ancestor_revision = 'origin/' + default_branch if issue: gerrit_util.GetChangeDetail.return_value = { @@ -744,7 +737,7 @@ class TestGitCl(unittest.TestCase): ] calls += [ ((['git', 'show-branch', 'refs/remotes/origin/main'], ), - '1' if new_default else callError(1)), + '1' if default_branch == 'main' else callError(1)), ] return calls @@ -758,8 +751,7 @@ class TestGitCl(unittest.TestCase): labels=None, change_id=None, final_description=None, gitcookies_exists=True, force=False, edit_description=None, - new_default=False): - default_branch = 'main' if new_default else 'master'; + default_branch='master'): if post_amend_description is None: post_amend_description = description cc = cc or [] @@ -787,11 +779,8 @@ class TestGitCl(unittest.TestCase): ref_to_push = 'abcdef0123456789' if custom_cl_base is None: - calls += [ - (('get_or_create_merge_base', 'master', - 'refs/remotes/origin/master'), 'origin/' + default_branch), - ] parent = 'origin/' + default_branch + git_common.get_or_create_merge_base.return_value = parent else: calls += [ ((['git', 'merge-base', '--is-ancestor', custom_cl_base, @@ -1057,7 +1046,7 @@ class TestGitCl(unittest.TestCase): log_description=None, edit_description=None, fetched_description=None, - new_default=False): + default_branch='master'): """Generic gerrit upload test framework.""" if squash_mode is None: if '--no-squash' in upload_args: @@ -1109,6 +1098,9 @@ class TestGitCl(unittest.TestCase): return_value=post_amend_description or description).start() mock.patch( 'git_cl.GenerateGerritChangeId', return_value=change_id).start() + mock.patch( + 'git_common.get_or_create_merge_base', + return_value='origin/' + default_branch).start() mock.patch( 'gclient_utils.AskForData', lambda prompt: self._mocked_call('ask_for_data', prompt)).start() @@ -1128,7 +1120,7 @@ class TestGitCl(unittest.TestCase): custom_cl_base=custom_cl_base, short_hostname=short_hostname, change_id=change_id, - new_default=new_default) + default_branch=default_branch) if fetched_status != 'ABANDONED': mock.patch( 'gclient_utils.temporary_file', TemporaryFileMock()).start() @@ -1147,7 +1139,7 @@ class TestGitCl(unittest.TestCase): gitcookies_exists=gitcookies_exists, force=force, edit_description=edit_description, - new_default=new_default) + default_branch=default_branch) # Uncomment when debugging. # print('\n'.join(map(lambda x: '%2i: %s' % x, enumerate(self.calls)))) git_cl.main(['upload'] + upload_args) @@ -1468,47 +1460,53 @@ class TestGitCl(unittest.TestCase): def test_update_reviewers(self): data = [ - ('foo', [], [], + ('foo', [], [], None, None, 'foo'), - ('foo\nR=xx', [], [], + ('foo\nR=xx', [], [], None, None, 'foo\nR=xx'), - ('foo\nTBR=xx', [], [], + ('foo\nTBR=xx', [], [], None, None, 'foo\nTBR=xx'), - ('foo', ['a@c'], [], + ('foo', ['a@c'], [], None, None, 'foo\n\nR=a@c'), - ('foo\nR=xx', ['a@c'], [], + ('foo\nR=xx', ['a@c'], [], None, None, 'foo\n\nR=a@c, xx'), - ('foo\nTBR=xx', ['a@c'], [], + ('foo\nTBR=xx', ['a@c'], [], None, None, 'foo\n\nR=a@c\nTBR=xx'), - ('foo\nTBR=xx\nR=yy', ['a@c'], [], + ('foo\nTBR=xx\nR=yy', ['a@c'], [], None, None, 'foo\n\nR=a@c, yy\nTBR=xx'), - ('foo\nBUG=', ['a@c'], [], + ('foo\nBUG=', ['a@c'], [], None, None, 'foo\nBUG=\nR=a@c'), - ('foo\nR=xx\nTBR=yy\nR=bar', ['a@c'], [], + ('foo\nR=xx\nTBR=yy\nR=bar', ['a@c'], [], None, None, 'foo\n\nR=a@c, bar, xx\nTBR=yy'), - ('foo', ['a@c', 'b@c'], [], + ('foo', ['a@c', 'b@c'], [], None, None, 'foo\n\nR=a@c, b@c'), - ('foo\nBar\n\nR=\nBUG=', ['c@c'], [], + ('foo\nBar\n\nR=\nBUG=', ['c@c'], [], None, None, 'foo\nBar\n\nR=c@c\nBUG='), - ('foo\nBar\n\nR=\nBUG=\nR=', ['c@c'], [], + ('foo\nBar\n\nR=\nBUG=\nR=', ['c@c'], [], None, None, 'foo\nBar\n\nR=c@c\nBUG='), # Same as the line before, but full of whitespaces. ( - 'foo\nBar\n\n R = \n BUG = \n R = ', ['c@c'], [], + 'foo\nBar\n\n R = \n BUG = \n R = ', ['c@c'], [], None, None, 'foo\nBar\n\nR=c@c\n BUG =', ), # Whitespaces aren't interpreted as new lines. - ('foo BUG=allo R=joe ', ['c@c'], [], + ('foo BUG=allo R=joe ', ['c@c'], [], None, None, 'foo BUG=allo R=joe\n\nR=c@c'), # Redundant TBRs get promoted to Rs - ('foo\n\nR=a@c\nTBR=t@c', ['b@c', 'a@c'], ['a@c', 't@c'], + ('foo\n\nR=a@c\nTBR=t@c', ['b@c', 'a@c'], ['a@c', 't@c'], None, None, 'foo\n\nR=a@c, b@c\nTBR=t@c'), + # Add to R + ('foo', [], [], 'R', ['a@c'], + 'foo\n\nR=a@c'), + # Add to TBR + ('foo', [], [], 'TBR', ['a@c'], + 'foo\n\nTBR=a@c'), ] expected = [i[-1] for i in data] actual = [] - for orig, reviewers, tbrs, _expected in data: + for orig, reviewers, tbrs, add_to, add, _expected in data: obj = git_cl.ChangeDescription(orig) - obj.update_reviewers(reviewers, tbrs, None, None, None) + obj.update_reviewers(reviewers, tbrs, add_to, add) actual.append(obj.description) self.assertEqual(expected, actual) @@ -2746,7 +2744,7 @@ class TestGitCl(unittest.TestCase): squash=False, squash_mode='override_nosquash', change_id='I123456789', - new_default=True) + default_branch='main') class ChangelistTest(unittest.TestCase):