Allow TBR for CLs with approved changes to OWNERS files

https://chromium-review.googlesource.com/982601 disabled TBR for CLs that
modified OWNERS files. This CL relaxes the restriction to permit TBR for
CLs that have LGTMs for their modifications to OWNERS files.

Bug: 688115
Change-Id: I47fef6b1eb021ca7cdfc003dc57722643b174a6e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1797605
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Auto-Submit: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>

presubmit_canned_checks.py

@@ -971,7 +971,27 @@ def CheckOwnersFormat(input_api, output_api):
 def CheckOwners(input_api, output_api, source_file_filter=None):
   affected_files = set([f.LocalPath() for f in
       input_api.change.AffectedFiles(file_filter=source_file_filter)])
-  affects_owners = any('OWNERS' in name for name in affected_files)
+  owners_db = input_api.owners_db
+  owners_db.override_files = input_api.change.OriginalOwnersFiles()
+  owner_email, reviewers = GetCodereviewOwnerAndReviewers(
+      input_api,
+      owners_db.email_regexp,
+      approval_needed=input_api.is_committing)
+
+  owner_email = owner_email or input_api.change.author_email
+
+  finder = input_api.owners_finder(
+      affected_files,
+      input_api.change.RepositoryRoot(),
+      owner_email,
+      reviewers,
+      fopen=file,
+      os_path=input_api.os_path,
+      email_postfix='',
+      disable_color=True,
+      override_files=input_api.change.OriginalOwnersFiles())
+  missing_files = finder.unreviewed_files
+  affects_owners = any('OWNERS' in name for name in missing_files)
 
   if input_api.is_committing:
     if input_api.tbr and not affects_owners:
@@ -992,29 +1012,12 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
     needed = 'OWNER reviewers'
     output_fn = output_api.PresubmitNotifyResult
 
-  owners_db = input_api.owners_db
-  owners_db.override_files = input_api.change.OriginalOwnersFiles()
-  owner_email, reviewers = GetCodereviewOwnerAndReviewers(
-      input_api,
-      owners_db.email_regexp,
-      approval_needed=input_api.is_committing)
-
-  owner_email = owner_email or input_api.change.author_email
-
-  finder = input_api.owners_finder(
-      affected_files, input_api.change.RepositoryRoot(),
-      owner_email, reviewers, fopen=file, os_path=input_api.os_path,
-      email_postfix='', disable_color=True,
-      override_files=input_api.change.OriginalOwnersFiles())
-  missing_files = finder.unreviewed_files
-
   if missing_files:
     output_list = [
         output_fn('Missing %s for these files:\n    %s' %
                   (needed, '\n    '.join(sorted(missing_files))))]
     if input_api.tbr and affects_owners:
-      output_list.append(output_fn('The CL affects an OWNERS file, so TBR will '
-                                   'be ignored.'))
+      output_list.append(output_fn('TBR for OWNERS files are ignored.'))
     if not input_api.is_committing:
       suggested_owners = owners_db.reviewers_for(missing_files, owner_email)
       owners_with_comments = []

tests/presubmit_unittest.py

@@ -2211,7 +2211,7 @@ the current line as well!
 
     affected_file.LocalPath.return_value = modified_file
     change.AffectedFiles.return_value = [affected_file]
-    if not is_committing or (not tbr and issue) or ('OWNERS' in modified_file):
+    if not is_committing or issue or ('OWNERS' in modified_file):
       change.OriginalOwnersFiles.return_value = {}
       if issue and not response:
         response = {
@@ -2496,13 +2496,19 @@ the current line as well!
 
   def testCannedCheckOwners_TBROWNERSFile(self):
     self.AssertOwnersWorks(
-        tbr=True, uncovered_files=set(['foo']),
+        tbr=True,
+        uncovered_files=set(['foo/OWNERS']),
         modified_file='foo/OWNERS',
-        expected_output=re.compile(
-            'Missing LGTM from an OWNER for these files:\n'
-            '    foo\n'
-            '.*The CL affects an OWNERS file, so TBR will be ignored.',
-            re.MULTILINE))
+        expected_output='Missing LGTM from an OWNER for these files:\n'
+        '    foo/OWNERS\n'
+        'TBR for OWNERS files are ignored.\n')
+
+  def testCannedCheckOwners_TBRNonOWNERSFile(self):
+    self.AssertOwnersWorks(
+        tbr=True,
+        uncovered_files=set(['foo/xyz.cc']),
+        modified_file='foo/OWNERS',
+        expected_output='--tbr was specified, skipping OWNERS check\n')
 
   def testCannedCheckOwners_WithoutOwnerLGTM(self):
     self.AssertOwnersWorks(uncovered_files=set(['foo']),