From aa475e6a12dfa9f5c11a968e4c7b4c2703b69709 Mon Sep 17 00:00:00 2001 From: Arthur Sonzogni Date: Fri, 9 Feb 2024 15:54:57 +0000 Subject: [PATCH] Revert "Reland "Refactor git functionality out of Change and _DiffCache"" This reverts commit 4d2864f3a1e991754ab8ab984e33ad399ee335f7. Reason for revert: This still doesn't scale well with large commit: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8756578662173359185/+/u/presubmit/stdout?format=raw This is now a different instruction called repeatedly needlessly: ``` git -c core.quotePath=false diff -p --no-color --no-prefix --no-ext-diff f0b151b0e519ceb3efae2ad8951d8a9ce86e4fbd...HEAD --no-renames; cwd=D:\b\s\w\ir\cache\builder\win_presubmit\src ``` called for each affected files. We probably want to cache the output for this too? Original change's description: > Reland "Refactor git functionality out of Change and _DiffCache" > > This is a reland of commit dd1a596c3e2573f600110d91007b2522fb9602d0 > > It is not a pure reland and adds support for caching submodules. > > Original change's description: > > Refactor git functionality out of Change and _DiffCache > > > > A followup change will add support for change diff provided as user > > input through stdin/file. > > > > Bug: b/323243527 > > Change-Id: I8d3420370e134859c61e35e23d76803227e4a506 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5254364 > > Reviewed-by: Joanna Wang > > Commit-Queue: Gavin Mak > > Reviewed-by: Josip Sokcevic > > Bug: b/323243527 > Change-Id: I74bbb179d4cbda7431101a2d707131fab2093029 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5280620 > Reviewed-by: Josip Sokcevic > Commit-Queue: Gavin Mak Bug: b/323243527, b/324293047 Change-Id: Iffde46facefa9d2e3a0d3a9ab5f8e753c53ea6d4 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5272317 Auto-Submit: Arthur Sonzogni Reviewed-by: Gavin Mak Commit-Queue: Gavin Mak Bot-Commit: Rubber Stamper --- presubmit_support.py | 121 +++++++++++++----------------------- tests/presubmit_unittest.py | 92 ++++++++------------------- 2 files changed, 68 insertions(+), 145 deletions(-) diff --git a/presubmit_support.py b/presubmit_support.py index 56503e006..755e82547 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -880,7 +880,7 @@ class InputApi(object): def ListSubmodules(self): """Returns submodule paths for current change's repo.""" - return self.change._repo_submodules() + return scm.GIT.ListSubmodules(self.change.RepositoryRoot()) @property def tbr(self): @@ -912,6 +912,9 @@ class InputApi(object): class _DiffCache(object): """Caches diffs retrieved from a particular SCM.""" + def __init__(self, upstream=None): + """Stores the upstream revision against which all diffs will be computed.""" + self._upstream = upstream def GetDiff(self, path, local_root): """Get the diff for a particular path.""" @@ -924,11 +927,8 @@ class _DiffCache(object): class _GitDiffCache(_DiffCache): """DiffCache implementation for git; gets all file diffs at once.""" - def __init__(self, upstream): - """Stores the upstream revision against which all diffs are computed.""" - super(_GitDiffCache, self).__init__() - self._upstream = upstream + super(_GitDiffCache, self).__init__(upstream=upstream) self._diffs_by_file = None def GetDiff(self, path, local_root): @@ -1138,13 +1138,21 @@ class Change(object): '^[ \t]*(?P[A-Z][A-Z_0-9]*)[ \t]*=[ \t]*(?P.*?)[ \t]*$') scm = '' - def __init__(self, name, description, local_root, files, issue, patchset, - author): + def __init__(self, + name, + description, + local_root, + files, + issue, + patchset, + author, + upstream=None): if files is None: files = [] self._name = name # Convert root into an absolute path. self._local_root = os.path.abspath(local_root) + self._upstream = upstream self.issue = issue self.patchset = patchset self.author_email = author @@ -1157,13 +1165,15 @@ class Change(object): assert all((isinstance(f, (list, tuple)) and len(f) == 2) for f in files), files + diff_cache = self._AFFECTED_FILES.DIFF_CACHE(self._upstream) self._affected_files = [ self._AFFECTED_FILES(path, action.strip(), self._local_root, - self._diff_cache()) for action, path in files + diff_cache) for action, path in files ] - def _diff_cache(self): - return self._AFFECTED_FILES.DIFF_CACHE() + def UpstreamBranch(self): + """Returns the upstream branch for the change.""" + return self._upstream def Name(self): """Returns the change name.""" @@ -1300,22 +1310,29 @@ 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. - - There is no SCM and no submodules, so return an empty list. - """ - return [] + """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: - warn('AffectedTestableFiles(include_deletes=%s)' + warn('AffectedTeestableFiles(include_deletes=%s)' ' is deprecated and ignored' % str(include_deletes), category=DeprecationWarning, stacklevel=2) @@ -1369,38 +1386,11 @@ class Change(object): files = self.AffectedFiles(file_filter=owners_file_filter) return {f.LocalPath(): f.OldContents() for f in files} - def _repo_submodules(self): - """Returns submodule paths for current change's repo. - - There is no SCM, so return an empty list. - """ - return [] - class GitChange(Change): _AFFECTED_FILES = GitAffectedFile scm = 'git' - def __init__(self, *args, upstream, **kwargs): - self._upstream = upstream - super(GitChange, self).__init__(*args) - - # List of submodule paths in the repo. - self._submodules = None - - def _diff_cache(self): - return self._AFFECTED_FILES.DIFF_CACHE(self._upstream) - - def _repo_submodules(self): - """Returns submodule paths for current change's repo.""" - if not self._submodules: - self._submodules = scm.GIT.ListSubmodules(self.RepositoryRoot()) - return self._submodules - - def UpstreamBranch(self): - """Returns the upstream branch for the change.""" - return self._upstream - def AllFiles(self, root=None): """List all files under source control in the repo.""" root = root or self.RepositoryRoot() @@ -1408,33 +1398,6 @@ class GitChange(Change): ['git', '-c', 'core.quotePath=false', 'ls-files', '--', '.'], cwd=root).decode('utf-8', 'ignore').splitlines() - def AffectedFiles(self, include_deletes=True, file_filter=None): - """Returns a list of AffectedFile instances for all files in the change. - - Args: - include_deletes: If false, deleted files will be filtered out. - file_filter: An additional filter to apply. - - Returns: - [AffectedFile(path, action), AffectedFile(path, action)] - """ - files = [ - af for af in self._affected_files - if af.LocalPath() not in self._repo_submodules() - ] - 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.""" - return [ - af for af in self._affected_files - if af.LocalPath() in self._repo_submodules() - ] - def ListRelevantPresubmitFiles(files, root): """Finds all presubmit files that apply to a given set of source files. @@ -2017,13 +1980,15 @@ def _parse_change(parser, options): ignore_submodules=False) logging.info('Found %d file(s).', len(change_files)) - change_args = [ - options.name, options.description, options.root, change_files, - options.issue, options.patchset, options.author - ] - if change_scm == 'git': - return GitChange(*change_args, upstream=options.upstream) - return Change(*change_args) + change_class = GitChange if change_scm == 'git' else Change + return change_class(options.name, + options.description, + options.root, + change_files, + options.issue, + options.patchset, + options.author, + upstream=options.upstream) def _parse_gerrit_options(parser, options): diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index a510df36b..71942d5fe 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -385,7 +385,7 @@ class PresubmitUnittest(PresubmitTestsBase): 0, 0, None, - upstream='upstream') + upstream=None) self.assertIsNotNone(change.Name() == 'mychange') self.assertIsNotNone(change.DescriptionText( ) == 'Hello there\nthis is a change\nand some more regular text') @@ -444,13 +444,8 @@ class PresubmitUnittest(PresubmitTestsBase): def testInvalidChange(self): with self.assertRaises(AssertionError): - presubmit.GitChange('mychange', - 'description', - self.fake_root_dir, ['foo/blat.cc', 'bar'], - 0, - 0, - None, - upstream='upstream') + presubmit.GitChange('mychange', 'description', self.fake_root_dir, + ['foo/blat.cc', 'bar'], 0, 0, None) def testExecPresubmitScript(self): description_lines = ('Hello there', 'this is a change', 'BUG=123') @@ -970,10 +965,14 @@ def CheckChangeOnCommit(input_api, output_api): change = presubmit._parse_change(None, options) self.assertEqual(presubmit.Change.return_value, change) - presubmit.Change.assert_called_once_with( - options.name, options.description, options.root, - [('M', 'random_file.txt')], options.issue, options.patchset, - options.author) + presubmit.Change.assert_called_once_with(options.name, + options.description, + options.root, + [('M', 'random_file.txt')], + options.issue, + options.patchset, + options.author, + upstream=options.upstream) presubmit._parse_files.assert_called_once_with(options.files, options.recursive) @@ -1186,14 +1185,9 @@ class InputApiUnittest(PresubmitTestsBase): os.path.isfile.side_effect = lambda f: f in known_files presubmit.scm.GIT.GenerateDiff.return_value = '\n'.join(diffs) - change = presubmit.GitChange('mychange', - '\n'.join(description_lines), + change = presubmit.GitChange('mychange', '\n'.join(description_lines), self.fake_root_dir, - [[f[0], f[1]] for f in files], - 0, - 0, - None, - upstream='upstream') + [[f[0], f[1]] for f in files], 0, 0, None) input_api = presubmit.InputApi( change, os.path.join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'), False, None, False) @@ -1247,14 +1241,9 @@ class InputApiUnittest(PresubmitTestsBase): known_files = [os.path.join(self.fake_root_dir, f) for _, f in files] os.path.isfile.side_effect = lambda f: f in known_files - change = presubmit.GitChange('mychange', - 'description\nlines\n', + change = presubmit.GitChange('mychange', 'description\nlines\n', self.fake_root_dir, - [[f[0], f[1]] for f in files], - 0, - 0, - None, - upstream='upstream') + [[f[0], f[1]] for f in files], 0, 0, None) input_api = presubmit.InputApi( change, os.path.join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'), False, None, False) @@ -1382,14 +1371,8 @@ class InputApiUnittest(PresubmitTestsBase): ] os.path.isfile.side_effect = lambda f: f in known_files - change = presubmit.GitChange('mychange', - '', - self.fake_root_dir, - files, - 0, - 0, - None, - upstream='upstream') + change = presubmit.GitChange('mychange', '', self.fake_root_dir, files, + 0, 0, None) input_api = presubmit.InputApi( change, os.path.join(self.fake_root_dir, 'PRESUBMIT.py'), False, None, False) @@ -1409,14 +1392,8 @@ class InputApiUnittest(PresubmitTestsBase): ] os.path.isfile.side_effect = lambda f: f in known_files - change = presubmit.GitChange('mychange', - '', - self.fake_root_dir, - files, - 0, - 0, - None, - upstream='upstream') + change = presubmit.GitChange('mychange', '', self.fake_root_dir, files, + 0, 0, None) input_api = presubmit.InputApi( change, os.path.join(self.fake_root_dir, 'PRESUBMIT.py'), False, None, False) @@ -1706,41 +1683,22 @@ class AffectedFileUnittest(PresubmitTestsBase): class ChangeUnittest(PresubmitTestsBase): - def testAffectedFiles(self): + @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(2, len(change.AffectedFiles())) + 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.GitChange('', - '', - self.fake_root_dir, [('Y', 'AA'), - ('A', 'BB')], - 3, - 5, - '', - upstream='upstream') + 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()) - @mock.patch('scm.GIT.ListSubmodules', return_value=['BB']) - def testAffectedSubmodulesCachesSubmodules(self, mockListSubmodules): - change = presubmit.GitChange('', - '', - self.fake_root_dir, [('Y', 'AA'), - ('A', 'BB')], - 3, - 5, - '', - upstream='upstream') - change.AffectedSubmodules() - mockListSubmodules.assert_called_once() - change.AffectedSubmodules() - mockListSubmodules.assert_called_once() - def testSetDescriptionText(self): change = presubmit.Change('', 'foo\nDRU=ro', self.fake_root_dir, [], 3, 5, '')