Add AffectedSubmodules() to presubmit change.

+ Change scm GetAllFiles back to returning all files
  + should be safe to do since it's only used by presubmit code
+  add ignore_submodules option to CaptureStatus
  + It's used by other parts of depot_tools code
+ Add AffectedSubmodules to return submodules only
+ AffectedFiles still only returns non-submodule files.

Bug: 1475770
Change-Id: I457ad96099fb20eff4cc1a4fc84f59e7ae707abe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5216005
Auto-Submit: Joanna Wang <jojwang@chromium.org>
Commit-Queue: Joanna Wang <jojwang@chromium.org>
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>

presubmit_support.py

@@ -1311,12 +1311,25 @@ class Change(object):
         Returns:
             [AffectedFile(path, action), AffectedFile(path, action)]
         """
-        affected = list(filter(file_filter, self._affected_files))
+        submodule_list = scm.GIT.ListSubmodules(self.RepositoryRoot())
+        files = [
+            af for af in self._affected_files
+            if af.LocalPath() not in submodule_list
+        ]
+        affected = list(filter(file_filter, files))
 
         if include_deletes:
             return affected
         return list(filter(lambda x: x.Action() != 'D', affected))
 
+    def AffectedSubmodules(self):
+        """Returns a list of AffectedFile instances for submodules in the change."""
+        submodule_list = scm.GIT.ListSubmodules(self.RepositoryRoot())
+        return [
+            af for af in self._affected_files
+            if af.LocalPath() in submodule_list
+        ]
+
     def AffectedTestableFiles(self, include_deletes=None, **kwargs):
         """Return a list of the existing text files in a change."""
         if include_deletes is not None:
@@ -1965,9 +1978,9 @@ def _parse_change(parser, options):
     elif options.all_files:
         change_files = [('M', f) for f in scm.GIT.GetAllFiles(options.root)]
     else:
-        change_files = scm.GIT.CaptureStatus(options.root, options.upstream
-                                             or None)
-
+        change_files = scm.GIT.CaptureStatus(options.root,
+                                             options.upstream or None,
+                                             ignore_submodules=False)
     logging.info('Found %d file(s).', len(change_files))
 
     change_class = GitChange if change_scm == 'git' else Change

scm.py

@@ -127,7 +127,10 @@ class GIT(object):
         return output.strip() if strip_out else output
 
     @staticmethod
-    def CaptureStatus(cwd, upstream_branch, end_commit=None):
+    def CaptureStatus(cwd,
+                      upstream_branch,
+                      end_commit=None,
+                      ignore_submodules=True):
         # type: (str, str, Optional[str]) -> Sequence[Tuple[str, str]]
         """Returns git status.
 
@@ -138,11 +141,15 @@ class GIT(object):
             upstream_branch = GIT.GetUpstreamBranch(cwd)
             if upstream_branch is None:
                 raise gclient_utils.Error('Cannot determine upstream branch')
+
         command = [
             '-c', 'core.quotePath=false', 'diff', '--name-status',
-            '--no-renames', '--ignore-submodules=all', '-r',
-            '%s...%s' % (upstream_branch, end_commit)
+            '--no-renames'
         ]
+        if ignore_submodules:
+            command.append('--ignore-submodules=all')
+        command.extend(['-r', '%s...%s' % (upstream_branch, end_commit)])
+
         status = GIT.Capture(command, cwd)
         results = []
         if status:
@@ -398,10 +405,8 @@ class GIT(object):
     @staticmethod
     def GetAllFiles(cwd):
         """Returns the list of all files under revision control."""
-        command = ['-c', 'core.quotePath=false', 'ls-files', '-s', '--', '.']
-        files = GIT.Capture(command, cwd=cwd).splitlines(False)
-        # return only files
-        return [f.split(maxsplit=3)[-1] for f in files if f.startswith('100')]
+        command = ['-c', 'core.quotePath=false', 'ls-files', '--', '.']
+        return GIT.Capture(command, cwd=cwd).splitlines(False)
 
     @staticmethod
     def GetSubmoduleCommits(cwd, submodules):

tests/presubmit_unittest.py

@@ -1076,7 +1076,8 @@ def CheckChangeOnCommit(input_api, output_api):
                                                     options.author,
                                                     upstream=options.upstream)
         scm.GIT.CaptureStatus.assert_called_once_with(options.root,
-                                                      options.upstream)
+                                                      options.upstream,
+                                                      ignore_submodules=False)
 
     @mock.patch('presubmit_support.GitChange', mock.Mock())
     @mock.patch('scm.GIT.GetAllFiles', mock.Mock())
@@ -1722,12 +1723,23 @@ class AffectedFileUnittest(PresubmitTestsBase):
 
 
 class ChangeUnittest(PresubmitTestsBase):
-    def testAffectedFiles(self):
-        change = presubmit.Change('', '', self.fake_root_dir, [('Y', 'AA')], 3,
+
+    @mock.patch('scm.GIT.ListSubmodules', return_value=['BB'])
+    def testAffectedFiles(self, mockListSubmodules):
+        change = presubmit.Change('', '', self.fake_root_dir, [('Y', 'AA'),
+                                                               ('A', 'BB')], 3,
                                   5, '')
         self.assertEqual(1, len(change.AffectedFiles()))
         self.assertEqual('Y', change.AffectedFiles()[0].Action())
 
+    @mock.patch('scm.GIT.ListSubmodules', return_value=['BB'])
+    def testAffectedSubmodules(self, mockListSubmodules):
+        change = presubmit.Change('', '', self.fake_root_dir, [('Y', 'AA'),
+                                                               ('A', 'BB')], 3,
+                                  5, '')
+        self.assertEqual(1, len(change.AffectedSubmodules()))
+        self.assertEqual('A', change.AffectedSubmodules()[0].Action())
+
     def testSetDescriptionText(self):
         change = presubmit.Change('', 'foo\nDRU=ro', self.fake_root_dir, [], 3,
                                   5, '')