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 <jojwang@chromium.org>
> > Commit-Queue: Gavin Mak <gavinmak@google.com>
> > Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
>
> Bug: b/323243527
> Change-Id: I74bbb179d4cbda7431101a2d707131fab2093029
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5280620
> Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
> Commit-Queue: Gavin Mak <gavinmak@google.com>

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 <arthursonzogni@google.com>
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Gavin Mak <gavinmak@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>

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<key>[A-Z][A-Z_0-9]*)[ \t]*=[ \t]*(?P<value>.*?)[ \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):

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, '')