diff --git a/git_cl.py b/git_cl.py index 33f4437f21..d54b46c275 100755 --- a/git_cl.py +++ b/git_cl.py @@ -977,6 +977,7 @@ class Changelist(object): # Lazily cached values. self._gerrit_host = None # e.g. chromium-review.googlesource.com self._gerrit_server = None # e.g. https://chromium-review.googlesource.com + self._owners_client = None # Map from change number (issue) to its detail cache. self._detail_cache = {} @@ -985,6 +986,18 @@ class Changelist(object): self._gerrit_host = codereview_host self._gerrit_server = 'https://%s' % codereview_host + @property + def owners_client(self): + if self._owners_client is None: + remote, remote_branch = self.GetRemoteBranch() + branch = GetTargetRef(remote, remote_branch, None) + self._owners_client = owners_client.GetCodeOwnersClient( + root=settings.GetRoot(), + host=self.GetGerritHost(), + project=self.GetGerritProject(), + branch=branch) + return self._owners_client + def GetCCList(self): """Returns the users cc'd on this CL. @@ -1375,16 +1388,14 @@ class Changelist(object): # Fill gaps in OWNERS coverage to tbrs/reviewers if requested. if options.add_owners_to: assert options.add_owners_to in ('TBR', 'R'), options.add_owners_to - client = owners_client.DepotToolsClient( - root=settings.GetRoot(), - branch=self.GetCommonAncestorWithUpstream()) - status = client.GetFilesApprovalStatus( + status = self.owners_client.GetFilesApprovalStatus( files, [], options.tbrs + options.reviewers) missing_files = [ f for f in files - if status[f] == owners_client.OwnersClient.INSUFFICIENT_REVIEWERS + if status[f] == self._owners_client.INSUFFICIENT_REVIEWERS ] - owners = client.SuggestOwners(missing_files, exclude=[self.GetAuthor()]) + owners = self.owners_client.SuggestOwners( + missing_files, exclude=[self.GetAuthor()]) if options.add_owners_to == 'TBR': assert isinstance(options.tbrs, list), options.tbrs options.tbrs.extend(owners) @@ -4790,10 +4801,7 @@ def CMDowners(parser, args): if len(args) == 0: print('No files specified for --show-all. Nothing to do.') return 0 - client = owners_client.DepotToolsClient( - root=settings.GetRoot(), - branch=cl.GetCommonAncestorWithUpstream()) - owners_by_path = client.BatchListOwners(args) + owners_by_path = cl.owners_client.BatchListOwners(args) for path in args: print('Owners for %s:' % path) print('\n'.join( @@ -4809,15 +4817,14 @@ def CMDowners(parser, args): # Default to diffing against the common ancestor of the upstream branch. base_branch = cl.GetCommonAncestorWithUpstream() - root = settings.GetRoot() affected_files = cl.GetAffectedFiles(base_branch) if options.batch: - client = owners_client.DepotToolsClient(root, base_branch) - print('\n'.join( - client.SuggestOwners(affected_files, exclude=[cl.GetAuthor()]))) + owners = cl.owners_client.SuggestOwners(affected_files, exclude=[author]) + print('\n'.join(owners)) return 0 + root = settings.GetRoot() owner_files = [f for f in affected_files if 'OWNERS' in os.path.basename(f)] original_owner_files = { f: scm.GIT.GetOldContents(root, f, base_branch).splitlines() diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 6100f6eeea..fdc884cad4 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -1505,16 +1505,19 @@ class TestGitCl(unittest.TestCase): change_id = git_cl.GenerateGerritChangeId('line1\nline2\n') self.assertEqual(change_id, 'Ihashchange') + @mock.patch('gerrit_util.IsCodeOwnersEnabled') @mock.patch('git_cl.Settings.GetBugPrefix') - @mock.patch('git_cl.Settings.GetRoot') @mock.patch('git_cl.Changelist.FetchDescription') @mock.patch('git_cl.Changelist.GetBranch') - @mock.patch('git_cl.Changelist.GetCommonAncestorWithUpstream') + @mock.patch('git_cl.Changelist.GetGerritHost') + @mock.patch('git_cl.Changelist.GetGerritProject') + @mock.patch('git_cl.Changelist.GetRemoteBranch') @mock.patch('owners_client.OwnersClient.BatchListOwners') def getDescriptionForUploadTest( - self, mockBatchListOwners=None, mockGetCommonAncestorWithUpstream=None, - mockGetBranch=None, mockFetchDescription=None, mockGetRoot=None, - mockGetBugPrefix=None, + self, mockBatchListOwners=None, mockGetRemoteBranch=None, + mockGetGerritProject=None, mockGetGerritHost=None, mockGetBranch=None, + mockFetchDescription=None, mockGetBugPrefix=None, + mockIsCodeOwnersEnabled=None, initial_description='desc', bug=None, fixed=None, branch='branch', reviewers=None, tbrs=None, add_owners_to=None, expected_description='desc'): @@ -1525,10 +1528,10 @@ class TestGitCl(unittest.TestCase): 'b': ['b@example.com'], 'c': ['c@example.com'], } - mockGetRoot.return_value = 'root' + mockIsCodeOwnersEnabled.return_value = True mockGetBranch.return_value = branch mockGetBugPrefix.return_value = 'prefix' - mockGetCommonAncestorWithUpstream.return_value = 'upstream' + mockGetRemoteBranch.return_value = ('origin', 'refs/remotes/origin/main') mockFetchDescription.return_value = 'desc' mockBatchListOwners.side_effect = lambda ps: { p: owners_by_path.get(p) @@ -4104,8 +4107,18 @@ class CMDOwnersTestCase(CMDTestCaseBase): 'git_cl.Changelist.GetCommonAncestorWithUpstream', return_value='upstream').start() mock.patch( - 'owners_client.DepotToolsClient.BatchListOwners', + 'git_cl.Changelist.GetGerritHost', + return_value='host').start() + mock.patch( + 'git_cl.Changelist.GetGerritProject', + return_value='project').start() + mock.patch( + 'git_cl.Changelist.GetRemoteBranch', + return_value=('origin', 'refs/remotes/origin/main')).start() + mock.patch( + 'owners_client.OwnersClient.BatchListOwners', return_value=self.owners_by_path).start() + mock.patch('gerrit_util.IsCodeOwnersEnabled', return_value=True).start() self.addCleanup(mock.patch.stopall) def testShowAllNoArgs(self): @@ -4118,7 +4131,7 @@ class CMDOwnersTestCase(CMDTestCaseBase): self.assertEqual( 0, git_cl.main(['owners', '--show-all', 'foo', 'bar', 'baz'])) - owners_client.DepotToolsClient.BatchListOwners.assert_called_once_with( + owners_client.OwnersClient.BatchListOwners.assert_called_once_with( ['foo', 'bar', 'baz']) self.assertEqual( '\n'.join([