Revert "Use GetCodeOwnersClient in presubmit_support"

This reverts commit 7a67bca6.

Reason for revert: Probably broke chromium presubmit, see https://ci.chromium.org/ui/p/chromium/builders/try/chromium_presubmit/1152280/overview

Original change's description:
> 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>

Change-Id: I2cca469f14194f65306baab7928ddddd48033a3b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2718691
Auto-Submit: Stephen Martinis <martiniss@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>

git_cl.py

@@ -1294,8 +1294,6 @@ 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:
@@ -1304,8 +1302,6 @@ 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 host and gerrit_util.IsCodeOwnersEnabled(host):
+  if gerrit_util.IsCodeOwnersEnabled(host):
     return GerritClient(host, project, branch)
   return DepotToolsClient(root, branch)

presubmit_support.py

@@ -376,10 +376,8 @@ class GerritAccessor(object):
   To avoid excessive Gerrit calls, caches the results.
   """
 
-  def __init__(self, host, project=None, branch=None):
+  def __init__(self, host):
     self.host = host
-    self.project = project
-    self.branch = branch
     self.cache = {}
 
   def _FetchChangeDetail(self, issue):
@@ -652,19 +650,10 @@ class InputApi(object):
     # Temporary files we must manually remove at the end of a run.
     self._named_temporary_files = []
 
-    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)
+    # 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)
     self.owners_db = owners_db.Database(
         change.RepositoryRoot(), fopen=open, os_path=self.os_path)
     self.owners_finder = owners_finder.OwnersFinder
@@ -1518,7 +1507,7 @@ def DoPostUploadExecuter(change,
 
 class PresubmitExecuter(object):
   def __init__(self, change, committing, verbose, gerrit_obj, dry_run=None,
-               thread_pool=None, parallel=False):
+               thread_pool=None, parallel=False, gerrit_project=None):
     """
     Args:
       change: The Change object.
@@ -1536,6 +1525,7 @@ 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.
@@ -1578,10 +1568,9 @@ class PresubmitExecuter(object):
 
     # Get the URL of git remote origin and use it to identify host and project
     host = ''
-    project = ''
-    if self.gerrit:
-      host = self.gerrit.host or ''
-      project = self.gerrit.project or ''
+    if self.gerrit and self.gerrit.host:
+      host = self.gerrit.host
+    project = self.gerrit_project or ''
 
     # Prefix for test names
     prefix = 'presubmit:%s/%s:%s/' % (host, project, rel_path)
@@ -1680,7 +1669,8 @@ def DoPresubmitChecks(change,
                       gerrit_obj,
                       dry_run=None,
                       parallel=False,
-                      json_output=None):
+                      json_output=None,
+                      gerrit_project=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
@@ -1723,7 +1713,7 @@ def DoPresubmitChecks(change,
     results = []
     thread_pool = ThreadPool()
     executer = PresubmitExecuter(change, committing, verbose, gerrit_obj,
-                                 dry_run, thread_pool, parallel)
+                                 dry_run, thread_pool, parallel, gerrit_project)
     if default_presubmit:
       if verbose:
         sys.stdout.write('Running default presubmit script.\n')
@@ -1880,15 +1870,11 @@ 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 or options.gerrit_project are
-    set, or None otherwise.
+    A GerritAccessor object if options.gerrit_url is set, or None otherwise.
   """
   gerrit_obj = None
-  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 options.gerrit_url:
+    gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc)
 
   if not options.gerrit_fetch:
     return gerrit_obj
@@ -1974,8 +1960,6 @@ 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
@@ -2008,7 +1992,8 @@ def main(argv=None):
           gerrit_obj,
           options.dry_run,
           options.parallel,
-          options.json_output)
+          options.json_output,
+          options.gerrit_project)
   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,7 +98,6 @@ 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,9 +2958,6 @@ 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()
@@ -3003,7 +3000,6 @@ class ChangelistTest(unittest.TestCase):
         '--gerrit_url', 'https://chromium-review.googlesource.com',
         '--issue', '123456',
         '--patchset', '7',
-        '--target_ref', 'ref',
         '--commit',
         '--may_prompt',
         '--parallel',
@@ -3052,7 +3048,6 @@ 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',
@@ -3100,7 +3095,6 @@ 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',
@@ -3141,7 +3135,6 @@ 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,13 +275,6 @@ 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,7 +184,6 @@ 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()
@@ -1145,10 +1144,9 @@ def CheckChangeOnCommit(input_api, output_api):
         upstream=options.upstream)
     scm.GIT.GetAllFiles.assert_called_once_with(options.root)
 
-  def testParseGerritOptions_NoGerritUrlOrProject(self):
+  def testParseGerritOptions_NoGerritUrl(self):
     options = mock.Mock(
         gerrit_url=None,
-        gerrit_project=None,
         gerrit_fetch=False,
         author='author',
         description='description')
@@ -1162,16 +1160,14 @@ 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', 'baz/project', 'refs/heads/foobar')
+        'foo-review.googlesource.com')
     self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj)
     self.assertEqual('author', options.author)
     self.assertEqual('description', options.description)
@@ -1185,16 +1181,14 @@ 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', 'baz/project', 'refs/heads/foobar')
+        'foo-review.googlesource.com')
     self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj)
     self.assertEqual('new owner', options.author)
     self.assertEqual('new description', options.description)