Use GetCodeOwnersClient in presubmit_support

This change also adds a target_ref flag to presubmit_support.py.

Recipe-Nontrivial-Roll: build
Change-Id: I6de6bb87fc1482b88d9fbebe5e4ad1dbd8ce9748
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2702792
Commit-Queue: Gavin Mak <gavinmak@google.com>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>

git_cl.py

@@ -1294,6 +1294,8 @@ class Changelist(object):
     gerrit_url = self.GetCodereviewServer()
     issue = self.GetIssue()
     patchset = self.GetPatchset()
+    remote, remote_branch = self.GetRemoteBranch()
+    target_ref = GetTargetRef(remote, remote_branch, None)
     if author:
       args.extend(['--author', author])
     if gerrit_url:
@@ -1302,6 +1304,8 @@ class Changelist(object):
       args.extend(['--issue', str(issue)])
     if patchset:
       args.extend(['--patchset', str(patchset)])
+    if target_ref:
+      args.extend(['--target_ref', target_ref])
 
     return args
 

owners_client.py

@@ -228,6 +228,6 @@ def GetCodeOwnersClient(root, host, project, branch):
 
   Defaults to GerritClient, and falls back to DepotToolsClient if code-owners
   plugin is not available."""
-  if gerrit_util.IsCodeOwnersEnabled(host):
+  if host and gerrit_util.IsCodeOwnersEnabled(host):
     return GerritClient(host, project, branch)
   return DepotToolsClient(root, branch)

presubmit_support.py

@@ -376,8 +376,10 @@ class GerritAccessor(object):
   To avoid excessive Gerrit calls, caches the results.
   """
 
-  def __init__(self, host):
+  def __init__(self, host, project=None, branch=None):
     self.host = host
+    self.project = project
+    self.branch = branch
     self.cache = {}
 
   def _FetchChangeDetail(self, issue):
@@ -650,10 +652,19 @@ class InputApi(object):
     # Temporary files we must manually remove at the end of a run.
     self._named_temporary_files = []
 
-    # 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_client = owners_client.DepotToolsClient(
-        change.RepositoryRoot(), change.UpstreamBranch(), os_path=self.os_path)
+    host = ''
+    project = ''
+    branch = ''
+    if gerrit_obj:
+      host = gerrit_obj.host or ''
+      project = gerrit_obj.project or ''
+      branch = gerrit_obj.branch or ''
+
+    self.owners_client = owners_client.GetCodeOwnersClient(
+        root=change.RepositoryRoot(),
+        host=host,
+        project=project,
+        branch=branch)
     self.owners_db = owners_db.Database(
         change.RepositoryRoot(), fopen=open, os_path=self.os_path)
     self.owners_finder = owners_finder.OwnersFinder
@@ -1507,7 +1518,7 @@ def DoPostUploadExecuter(change,
 
 class PresubmitExecuter(object):
   def __init__(self, change, committing, verbose, gerrit_obj, dry_run=None,
-               thread_pool=None, parallel=False, gerrit_project=None):
+               thread_pool=None, parallel=False):
     """
     Args:
       change: The Change object.
@@ -1525,7 +1536,6 @@ class PresubmitExecuter(object):
     self.more_cc = []
     self.thread_pool = thread_pool
     self.parallel = parallel
-    self.gerrit_project = gerrit_project
 
   def ExecPresubmitScript(self, script_text, presubmit_path):
     """Executes a single presubmit script.
@@ -1568,9 +1578,10 @@ class PresubmitExecuter(object):
 
     # Get the URL of git remote origin and use it to identify host and project
     host = ''
-    if self.gerrit and self.gerrit.host:
-      host = self.gerrit.host
-    project = self.gerrit_project or ''
+    project = ''
+    if self.gerrit:
+      host = self.gerrit.host or ''
+      project = self.gerrit.project or ''
 
     # Prefix for test names
     prefix = 'presubmit:%s/%s:%s/' % (host, project, rel_path)
@@ -1669,8 +1680,7 @@ def DoPresubmitChecks(change,
                       gerrit_obj,
                       dry_run=None,
                       parallel=False,
-                      json_output=None,
-                      gerrit_project=None):
+                      json_output=None):
   """Runs all presubmit checks that apply to the files in the change.
 
   This finds all PRESUBMIT.py files in directories enclosing the files in the
@@ -1713,7 +1723,7 @@ def DoPresubmitChecks(change,
     results = []
     thread_pool = ThreadPool()
     executer = PresubmitExecuter(change, committing, verbose, gerrit_obj,
-                                 dry_run, thread_pool, parallel, gerrit_project)
+                                 dry_run, thread_pool, parallel)
     if default_presubmit:
       if verbose:
         sys.stdout.write('Running default presubmit script.\n')
@@ -1870,11 +1880,15 @@ def _parse_gerrit_options(parser, options):
     parser: The parser used to parse the arguments from command line.
     options: The arguments parsed from command line.
   Returns:
-    A GerritAccessor object if options.gerrit_url is set, or None otherwise.
+    A GerritAccessor object if options.gerrit_url or options.gerrit_project are
+    set, or None otherwise.
   """
   gerrit_obj = None
-  if options.gerrit_url:
-    gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc)
+  if options.gerrit_url or options.gerrit_project:
+    gerrit_obj = GerritAccessor(
+        urlparse.urlparse(options.gerrit_url or '').netloc,
+        options.gerrit_project,
+        options.target_ref)
 
   if not options.gerrit_fetch:
     return gerrit_obj
@@ -1960,6 +1974,8 @@ def main(argv=None):
                       'executing presubmit or post-upload hooks. fnmatch '
                       'wildcards can also be used.')
   parser.add_argument('--gerrit_project', help=argparse.SUPPRESS)
+  parser.add_argument('--target_ref',
+                      help='The remote branch ref to use for the change.')
   options = parser.parse_args(argv)
 
   log_level = logging.ERROR
@@ -1992,8 +2008,7 @@ def main(argv=None):
           gerrit_obj,
           options.dry_run,
           options.parallel,
-          options.json_output,
-          options.gerrit_project)
+          options.json_output)
   except PresubmitFailure as e:
     print(e, file=sys.stderr)
     print('Maybe your depot_tools is out of date?', file=sys.stderr)

recipes/recipe_modules/presubmit/api.py

@@ -98,6 +98,7 @@ class PresubmitApi(recipe_api.RecipeApi):
       '--issue', self.m.tryserver.gerrit_change.change,
       '--patchset', self.m.tryserver.gerrit_change.patchset,
       '--gerrit_url', 'https://%s' % self.m.tryserver.gerrit_change.host,
+      '--target_ref', self.m.tryserver.gerrit_change_target_ref,
       '--gerrit_fetch',
     ]
     if self.m.cq.state == self.m.cq.DRY:

tests/git_cl_test.py

@@ -2958,6 +2958,9 @@ class ChangelistTest(unittest.TestCase):
     mock.patch('git_cl.Changelist.GetAuthor', return_value='author').start()
     mock.patch('git_cl.Changelist.GetIssue', return_value=123456).start()
     mock.patch('git_cl.Changelist.GetPatchset', return_value=7).start()
+    mock.patch('git_cl.Changelist.GetRemoteBranch',
+        return_value=('foo', 'bar')).start()
+    mock.patch('git_cl.GetTargetRef', return_value='ref').start()
     mock.patch('git_cl.PRESUBMIT_SUPPORT', 'PRESUBMIT_SUPPORT').start()
     mock.patch('git_cl.Settings.GetRoot', return_value='root').start()
     mock.patch('git_cl.time_time').start()
@@ -3000,6 +3003,7 @@ class ChangelistTest(unittest.TestCase):
         '--gerrit_url', 'https://chromium-review.googlesource.com',
         '--issue', '123456',
         '--patchset', '7',
+        '--target_ref', 'ref',
         '--commit',
         '--may_prompt',
         '--parallel',
@@ -3048,6 +3052,7 @@ class ChangelistTest(unittest.TestCase):
         'vpython', 'PRESUBMIT_SUPPORT',
         '--root', 'root',
         '--upstream', 'upstream',
+        '--target_ref', 'ref',
         '--upload',
         '--json_output', '/tmp/fake-temp2',
         '--description_file', '/tmp/fake-temp1',
@@ -3095,6 +3100,7 @@ class ChangelistTest(unittest.TestCase):
         'vpython', 'PRESUBMIT_SUPPORT',
         '--root', 'root',
         '--upstream', 'upstream',
+        '--target_ref', 'ref',
         '--upload',
         '--json_output', '/tmp/fake-temp2',
         '--description_file', '/tmp/fake-temp1',
@@ -3135,6 +3141,7 @@ class ChangelistTest(unittest.TestCase):
         '--gerrit_url', 'https://chromium-review.googlesource.com',
         '--issue', '123456',
         '--patchset', '7',
+        '--target_ref', 'ref',
         '--post_upload',
         '--description_file', '/tmp/fake-temp1',
     ])

tests/owners_client_test.py

@@ -275,6 +275,13 @@ class GetCodeOwnersClientTest(unittest.TestCase):
     mock.patch('gerrit_util.IsCodeOwnersEnabled').start()
     self.addCleanup(mock.patch.stopall)
 
+  def testGetCodeOwnersClient_NoHost(self):
+    owners_client.GetCodeOwnersClient('root', None, 'project', 'branch')
+    gerrit_util.IsCodeOwnersEnabled.assert_not_called()
+
+    owners_client.GetCodeOwnersClient('root', '', 'project', 'branch')
+    gerrit_util.IsCodeOwnersEnabled.assert_not_called()
+
   def testGetCodeOwnersClient_GerritClient(self):
     gerrit_util.IsCodeOwnersEnabled.return_value = True
     self.assertIsInstance(

tests/presubmit_unittest.py

@@ -184,6 +184,7 @@ index fe3de7b..54ae6e1 100755
     mock.patch('os.path.abspath', lambda f: f).start()
     mock.patch('os.path.isfile').start()
     mock.patch('os.remove').start()
+    mock.patch('owners_client.GetCodeOwnersClient').start()
     mock.patch('presubmit_support._parse_files').start()
     mock.patch('presubmit_support.rdb_wrapper.client',
                return_value=self.rdb_client).start()
@@ -1144,9 +1145,10 @@ def CheckChangeOnCommit(input_api, output_api):
         upstream=options.upstream)
     scm.GIT.GetAllFiles.assert_called_once_with(options.root)
 
-  def testParseGerritOptions_NoGerritUrl(self):
+  def testParseGerritOptions_NoGerritUrlOrProject(self):
     options = mock.Mock(
         gerrit_url=None,
+        gerrit_project=None,
         gerrit_fetch=False,
         author='author',
         description='description')
@@ -1160,14 +1162,16 @@ def CheckChangeOnCommit(input_api, output_api):
   def testParseGerritOptions_NoGerritFetch(self):
     options = mock.Mock(
         gerrit_url='https://foo-review.googlesource.com/bar',
+        gerrit_project='baz/project',
         gerrit_fetch=False,
+        target_ref='refs/heads/foobar',
         author='author',
         description='description')
 
     gerrit_obj = presubmit._parse_gerrit_options(None, options)
 
     presubmit.GerritAccessor.assert_called_once_with(
-        'foo-review.googlesource.com')
+        'foo-review.googlesource.com', 'baz/project', 'refs/heads/foobar')
     self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj)
     self.assertEqual('author', options.author)
     self.assertEqual('description', options.description)
@@ -1181,14 +1185,16 @@ def CheckChangeOnCommit(input_api, output_api):
 
     options = mock.Mock(
         gerrit_url='https://foo-review.googlesource.com/bar',
+        gerrit_project='baz/project',
         gerrit_fetch=True,
+        target_ref='refs/heads/foobar',
         issue=123,
         patchset=4)
 
     gerrit_obj = presubmit._parse_gerrit_options(None, options)
 
     presubmit.GerritAccessor.assert_called_once_with(
-        'foo-review.googlesource.com')
+        'foo-review.googlesource.com', 'baz/project', 'refs/heads/foobar')
     self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj)
     self.assertEqual('new owner', options.author)
     self.assertEqual('new description', options.description)