Reland "presubmit: Skip owners checks if code-owners plugin is enabled."

This is a reland of 2cf835a9bad9ef5bfe477719d3376eccab3cac68

URL-encoded repo path, so chromium/src becomes chromium%2Fsrc
as Gerrit expects

Original change's description:
> presubmit: Skip owners checks if code-owners plugin is enabled.
>
> If code-owners plugin is enable for the repo, skip owners check on
> commit, and skip checking owners format, as that will be done by
> the plugin.
>
> Change-Id: I1663baef4f0f27b00423071343fe740f6da50ce7
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2727131
> Auto-Submit: Edward Lesmes <ehmaldonado@chromium.org>
> Reviewed-by: Gavin Mak <gavinmak@google.com>
> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

Change-Id: I3038590f3a92cbf7b6dc0ba6eb47f72593a2ccf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2775840
Auto-Submit: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Gavin Mak <gavinmak@google.com>

gerrit_util.py

@@ -771,13 +771,21 @@ def SetCommitMessage(host, change, description, notify='ALL'):
         'in change %s' % change)
 
 
-def IsCodeOwnersEnabled(host):
+def IsCodeOwnersEnabledOnHost(host):
   """Check if the code-owners plugin is enabled for the host."""
   path = 'config/server/capabilities'
   capabilities = ReadHttpJsonResponse(CreateHttpConn(host, path))
   return 'code-owners-checkCodeOwner' in capabilities
 
 
+def IsCodeOwnersEnabledOnRepo(host, repo):
+  """Check if the code-owners plugin is enabled for the repo."""
+  repo = PercentEncodeForGitRef(repo)
+  path = '/projects/%s/code_owners.project_config' % repo
+  config = ReadHttpJsonResponse(CreateHttpConn(host, path))
+  return config['status'].get('disabled', False)
+
+
 def GetOwnersForFile(host, project, branch, path, limit=100,
                      resolve_all_users=True, seed=None, o_params=('DETAILS',)):
   """Gets information about owners attached to a file."""

owners_client.py

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

presubmit_canned_checks.py

@@ -1108,6 +1108,8 @@ def CheckOwnersDirMetadataExclusive(input_api, output_api):
 
 
 def CheckOwnersFormat(input_api, output_api):
+  if input_api.gerrit and input_api.gerrit.IsCodeOwnersEnabledOnRepo():
+    return []
   affected_files = set([
       f.LocalPath()
       for f in input_api.change.AffectedFiles()
@@ -1134,8 +1136,17 @@ def CheckOwners(
       and input_api.gerrit.IsOwnersOverrideApproved(input_api.change.issue)):
     return []
 
-  # Ignore tbr if not allowed for this repo.
-  tbr = input_api.tbr and allow_tbr
+  code_owners_enabled = False
+  if input_api.gerrit and input_api.gerrit.IsCodeOwnersEnabledOnRepo():
+    code_owners_enabled = True
+
+  # Skip OWNERS check on commit if code-owners plugin is enabled, as owners
+  # enforcement is done by the plugin.
+  if input_api.is_committing and code_owners_enabled:
+    return []
+
+  # Ignore tbr if the code-owners plugin is enabled on this repo.
+  tbr = not code_owners_enabled and input_api.tbr
 
   affected_files = set([f.LocalPath() for f in
       input_api.change.AffectedFiles(file_filter=source_file_filter)])

presubmit_support.py

@@ -381,6 +381,7 @@ class GerritAccessor(object):
     self.project = project
     self.branch = branch
     self.cache = {}
+    self.code_owners_enabled = None
 
   def _FetchChangeDetail(self, issue):
     # Separate function to be easily mocked in tests.
@@ -466,6 +467,12 @@ class GerritAccessor(object):
   def UpdateDescription(self, description, issue):
     gerrit_util.SetCommitMessage(self.host, issue, description, notify='NONE')
 
+  def IsCodeOwnersEnabledOnRepo(self):
+    if self.code_owners_enabled is None:
+      self.code_owners_enabled = gerrit_util.IsCodeOwnersEnabledOnRepo(
+          self.host, self.repo)
+    return self.code_owners_enabled
+
 
 class OutputApi(object):
   """An instance of OutputApi gets passed to presubmit scripts so that they

tests/git_cl_test.py

@@ -1505,7 +1505,7 @@ class TestGitCl(unittest.TestCase):
     change_id = git_cl.GenerateGerritChangeId('line1\nline2\n')
     self.assertEqual(change_id, 'Ihashchange')
 
-  @mock.patch('gerrit_util.IsCodeOwnersEnabled')
+  @mock.patch('gerrit_util.IsCodeOwnersEnabledOnHost')
   @mock.patch('git_cl.Settings.GetBugPrefix')
   @mock.patch('git_cl.Changelist.FetchDescription')
   @mock.patch('git_cl.Changelist.GetBranch')
@@ -1519,7 +1519,7 @@ class TestGitCl(unittest.TestCase):
       mockGetGerritProject=None, mockGetGerritHost=None,
       mockGetCommonAncestorWithUpstream=None, mockGetBranch=None,
       mockFetchDescription=None, mockGetBugPrefix=None,
-      mockIsCodeOwnersEnabled=None,
+      mockIsCodeOwnersEnabledOnHost=None,
       initial_description='desc', bug=None, fixed=None, branch='branch',
       reviewers=None, tbrs=None, add_owners_to=None,
       expected_description='desc'):
@@ -1530,7 +1530,7 @@ class TestGitCl(unittest.TestCase):
       'b': ['b@example.com'],
       'c': ['c@example.com'],
     }
-    mockIsCodeOwnersEnabled.return_value = True
+    mockIsCodeOwnersEnabledOnHost.return_value = True
     mockGetBranch.return_value = branch
     mockGetBugPrefix.return_value = 'prefix'
     mockGetCommonAncestorWithUpstream.return_value = 'upstream'
@@ -4139,7 +4139,8 @@ class CMDOwnersTestCase(CMDTestCaseBase):
     mock.patch(
         'owners_client.OwnersClient.BatchListOwners',
         return_value=self.owners_by_path).start()
-    mock.patch('gerrit_util.IsCodeOwnersEnabled', return_value=True).start()
+    mock.patch(
+        'gerrit_util.IsCodeOwnersEnabledOnHost', return_value=True).start()
     self.addCleanup(mock.patch.stopall)
 
   def testShowAllNoArgs(self):

tests/owners_client_test.py

@@ -292,17 +292,17 @@ class OwnersClientTest(unittest.TestCase):
 
 class GetCodeOwnersClientTest(unittest.TestCase):
   def setUp(self):
-    mock.patch('gerrit_util.IsCodeOwnersEnabled').start()
+    mock.patch('gerrit_util.IsCodeOwnersEnabledOnHost').start()
     self.addCleanup(mock.patch.stopall)
 
   def testGetCodeOwnersClient_GerritClient(self):
-    gerrit_util.IsCodeOwnersEnabled.return_value = True
+    gerrit_util.IsCodeOwnersEnabledOnHost.return_value = True
     self.assertIsInstance(
         owners_client.GetCodeOwnersClient('', '', 'host', 'project', 'branch'),
         owners_client.GerritClient)
 
   def testGetCodeOwnersClient_DepotToolsClient(self):
-    gerrit_util.IsCodeOwnersEnabled.return_value = False
+    gerrit_util.IsCodeOwnersEnabledOnHost.return_value = False
     self.assertIsInstance(
         owners_client.GetCodeOwnersClient('root', 'branch', '', '', ''),
         owners_client.DepotToolsClient)

tests/presubmit_unittest.py

@@ -178,7 +178,7 @@ index fe3de7b..54ae6e1 100755
     mock.patch('gclient_utils.FileWrite').start()
     mock.patch('json.load').start()
     mock.patch('multiprocessing.cpu_count', lambda: 2)
-    mock.patch('gerrit_util.IsCodeOwnersEnabled').start()
+    mock.patch('gerrit_util.IsCodeOwnersEnabledOnHost').start()
     mock.patch('os.chdir').start()
     mock.patch('os.getcwd', self.RootDir)
     mock.patch('os.listdir').start()
@@ -2669,13 +2669,14 @@ the current line as well!
     self.assertEqual(1, len(results))
     self.assertIsInstance(results[0], presubmit.OutputApi.PresubmitError)
 
-  def GetInputApiWithOWNERS(self, owners_content):
+  def GetInputApiWithOWNERS(self, owners_content, code_owners_enabled=False):
     input_api = self.GetInputApiWithFiles({'OWNERS': ('M', owners_content)})
 
     owners_file = StringIO(owners_content)
     fopen = lambda *args: owners_file
 
     input_api.owners_db = owners.Database('', fopen, os.path)
+    input_api.gerrit.IsCodeOwnersEnabledOnRepo = lambda: code_owners_enabled
 
     return input_api
 
@@ -2690,6 +2691,17 @@ the current line as well!
             input_api, presubmit.OutputApi)
     )
 
+  def testCheckOwnersFormatWorks_CodeOwners(self):
+    # If code owners is enabled, we rely on it to check owners format instead of
+    # depot tools.
+    input_api = self.GetInputApiWithOWNERS(
+        'any content', code_owners_enabled=True)
+    self.assertEqual(
+        [],
+        presubmit_canned_checks.CheckOwnersFormat(
+            input_api, presubmit.OutputApi)
+    )
+
   def testCheckOwnersFormatFails(self):
     input_api = self.GetInputApiWithOWNERS('\n'.join([
         'set noparent',
@@ -2705,7 +2717,7 @@ the current line as well!
       self, tbr=False, issue='1', approvers=None, modified_files=None,
       owners_by_path=None, is_committing=True, response=None,
       expected_output='', manually_specified_reviewers=None, dry_run=None,
-      allow_tbr=True):
+      code_owners_enabled=False):
     # The set of people who lgtm'ed a change.
     approvers = approvers or set()
     manually_specified_reviewers = manually_specified_reviewers or []
@@ -2749,13 +2761,14 @@ the current line as well!
     input_api.tbr = tbr
     input_api.dry_run = dry_run
     input_api.gerrit._FetchChangeDetail = lambda _: response
+    input_api.gerrit.IsCodeOwnersEnabledOnRepo = lambda: code_owners_enabled
 
     input_api.owners_client = owners_client.OwnersClient()
 
     with mock.patch('owners_client.OwnersClient.ListOwners',
                     side_effect=lambda f: owners_by_path.get(f, [])):
       results = presubmit_canned_checks.CheckOwners(
-          input_api, presubmit.OutputApi, allow_tbr=allow_tbr)
+          input_api, presubmit.OutputApi)
     for result in results:
       result.handle()
     if expected_output:
@@ -3037,12 +3050,11 @@ the current line as well!
   def testCannedCheckOwners_TBRIgnored(self):
     self.AssertOwnersWorks(
         tbr=True,
-        allow_tbr=False,
-        expected_output='Missing LGTM from someone other than '
-                        'john@example.com\n')
+        code_owners_enabled=True,
+        expected_output='')
     self.AssertOwnersWorks(
         tbr=True,
-        allow_tbr=False,
+        code_owners_enabled=True,
         is_committing=False,
         expected_output='')