Make OWNERS review for added DEPS include_dirs opt-in

If review is required, DEPS files can add:

new_usages_require_review = True

Bug: 365797506
Change-Id: I3457e8d868fbf9715ce6ed4622d48a75c351c17a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5910107
Reviewed-by: Yaron Friedman <[email protected]>
Commit-Queue: Yaron Friedman <[email protected]>
Auto-Submit: Andrew Grieve <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1389797}

PRESUBMIT.py

@@ -3450,6 +3450,32 @@ def Lookup(self, var_name):
     return local_scope
 
 
+def _FindAllDepsFilesForSubpath(input_api, subpath):
+    ret = []
+    while subpath:
+        cur = input_api.os_path.join(input_api.change.RepositoryRoot(), subpath, 'DEPS')
+        if input_api.os_path.exists(cur):
+            ret.append(cur)
+        subpath = input_api.os_path.dirname(subpath)
+    return ret
+
+
+def _FindAddedDepsThatRequireReview(input_api, depended_on_paths):
+    """Filters to those whose DEPS set new_usages_require_review=True"""
+    ret = set()
+    cache = {}
+    for target_path in depended_on_paths:
+        for subpath in _FindAllDepsFilesForSubpath(input_api, target_path):
+            config = cache.get(subpath)
+            if config is None:
+                config = _ParseDeps(input_api.ReadFile(subpath))
+                cache[subpath] = config
+            if config.get('new_usages_require_review'):
+                ret.add(target_path)
+                break
+    return ret
+
+
 def _CalculateAddedDeps(os_path, old_contents, new_contents):
     """Helper method for CheckAddedDepsHaveTargetApprovals. Returns
     a set of DEPS entries that we should look up.
@@ -3599,7 +3625,9 @@ def CheckAddedDepsHaveTargetApprovals(input_api, output_api):
         return [output_api.PresubmitPromptWarning(
                 'Failed to retrieve owner override status - %s' % str(e))]
 
-    virtual_depended_on_files = set()
+    # A set of paths (that might not exist) that are being added as DEPS
+    # (via lines like "+foo/bar/baz").
+    depended_on_paths = set()
 
     # Consistently use / as path separator to simplify the writing of regex
     # expressions.
@@ -3610,12 +3638,14 @@ def CheckAddedDepsHaveTargetApprovals(input_api, output_api):
                                      file_filter=file_filter):
         filename = input_api.os_path.basename(f.LocalPath())
         if filename == 'DEPS':
-            virtual_depended_on_files.update(
+            depended_on_paths.update(
                 _CalculateAddedDeps(input_api.os_path,
                                     '\n'.join(f.OldContents()),
                                     '\n'.join(f.NewContents())))
 
-    if not virtual_depended_on_files:
+    # Requiring reviews is opt-in as of https://crbug.com/365797506
+    depended_on_paths = _FindAddedDepsThatRequireReview(input_api, depended_on_paths)
+    if not depended_on_paths:
         return []
 
     if input_api.is_committing:
@@ -3650,10 +3680,10 @@ def CheckAddedDepsHaveTargetApprovals(input_api, output_api):
     owner_email = owner_email or input_api.change.author_email
 
     approval_status = input_api.owners_client.GetFilesApprovalStatus(
-        virtual_depended_on_files, reviewers.union([owner_email]), [])
+        depended_on_paths, reviewers.union([owner_email]), [])
     missing_files = [
-        f for f in virtual_depended_on_files
-        if approval_status[f] != input_api.owners_client.APPROVED
+        p for p in depended_on_paths
+        if approval_status[p] != input_api.owners_client.APPROVED
     ]
 
     # We strip the /DEPS part that was added by

PRESUBMIT_test.py

@@ -445,6 +445,29 @@ def testCalculateAddedDepsIgnoresPermutations(self):
             set(), self.calculate(old_include_rules, {}, new_include_rules,
                                   {}))
 
+    def testFindAddedDepsThatRequireReview(self):
+        caring = ['new_usages_require_review = True']
+        self.input_api.InitFiles([
+            MockAffectedFile('cares/DEPS', caring),
+            MockAffectedFile('cares/inherits/DEPS', []),
+            MockAffectedFile('willynilly/DEPS', []),
+            MockAffectedFile('willynilly/butactually/DEPS', caring),
+        ])
+
+        expected = {
+            'cares': True,
+            'cares/sub/sub': True,
+            'cares/inherits': True,
+            'cares/inherits/sub': True,
+            'willynilly': False,
+            'willynilly/butactually': True,
+            'willynilly/butactually/sub': True,
+        }
+        results = PRESUBMIT._FindAddedDepsThatRequireReview(
+            self.input_api, set(expected))
+        actual = {k: k in results for k in expected}
+        self.assertEqual(expected, actual)
+
     class FakeOwnersClient(object):
         APPROVED = "APPROVED"
         PENDING = "PENDING"
@@ -487,6 +510,7 @@ def mock(*args, **kwargs):
     def testApprovedAdditionalDep(self):
         self.input_api.InitFiles([
             MockAffectedFile('pdf/DEPS', ['include_rules=["+v8/123"]']),
+            MockAffectedFile('v8/DEPS', ['new_usages_require_review=True']),
         ])
 
         # mark the additional dep as approved.
@@ -501,6 +525,7 @@ def testApprovedAdditionalDep(self):
     def testUnapprovedAdditionalDep(self):
         self.input_api.InitFiles([
             MockAffectedFile('pdf/DEPS', ['include_rules=["+v8/123"]']),
+            MockAffectedFile('v8/DEPS', ['new_usages_require_review=True']),
         ])
 
         # pending.

PRESUBMIT_test_mocks.py

@@ -106,6 +106,7 @@ def InitFiles(self, files):
         def mock_exists(path):
             if not os.path.isabs(path):
                 path = os.path.join(self.presubmit_local_path, path)
+            path = os.path.normpath(path)
             return path in files_that_exist
 
         def mock_glob(pattern, *args, **kwargs):
@@ -169,8 +170,10 @@ def PresubmitLocalPath(self):
     def ReadFile(self, filename, mode='r'):
         if hasattr(filename, 'AbsoluteLocalPath'):
             filename = filename.AbsoluteLocalPath()
+        norm_filename = os.path.normpath(filename)
         for file_ in self.files:
-            if filename in (file_.LocalPath(), file_.AbsoluteLocalPath()):
+            to_check = (file_.LocalPath(), file_.AbsoluteLocalPath())
+            if filename in to_check or norm_filename in to_check:
                 return '\n'.join(file_.NewContents())
         # Otherwise, file is not in our mock API.
         raise IOError("No such file or directory: '%s'" % filename)