From 46cb7d0aca592cd20ddc2f6cb16ee386b2abbf0d Mon Sep 17 00:00:00 2001 From: Joanna Wang Date: Fri, 19 Jan 2024 19:47:44 +0000 Subject: [PATCH] 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 Commit-Queue: Joanna Wang Reviewed-by: Josip Sokcevic --- presubmit_support.py | 21 +++++++++++++++++---- scm.py | 19 ++++++++++++------- tests/presubmit_unittest.py | 18 +++++++++++++++--- 3 files changed, 44 insertions(+), 14 deletions(-) diff --git a/presubmit_support.py b/presubmit_support.py index eff767e0e0..e0117bb884 100755 --- a/presubmit_support.py +++ b/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 diff --git a/scm.py b/scm.py index 58b2ca1469..b84662add5 100644 --- a/scm.py +++ b/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): diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 59cf76cf4c..fd147d0cdf 100755 --- a/tests/presubmit_unittest.py +++ b/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, '')