diff --git a/presubmit_support.py b/presubmit_support.py index 755e82547..beb51db83 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 scm.GIT.ListSubmodules(self.change.RepositoryRoot()) + return self.change._repo_submodules() @property def tbr(self): @@ -912,9 +912,6 @@ 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.""" @@ -927,8 +924,11 @@ class _DiffCache(object): class _GitDiffCache(_DiffCache): """DiffCache implementation for git; gets all file diffs at once.""" + def __init__(self, upstream): - super(_GitDiffCache, self).__init__(upstream=upstream) + """Stores the upstream revision against which all diffs are computed.""" + super(_GitDiffCache, self).__init__() + self._upstream = upstream self._diffs_by_file = None def GetDiff(self, path, local_root): @@ -1138,21 +1138,13 @@ 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, - upstream=None): + def __init__(self, name, description, local_root, files, issue, patchset, + author): 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 @@ -1165,15 +1157,13 @@ 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, - diff_cache) for action, path in files + self._diff_cache()) for action, path in files ] - def UpstreamBranch(self): - """Returns the upstream branch for the change.""" - return self._upstream + def _diff_cache(self): + return self._AFFECTED_FILES.DIFF_CACHE() def Name(self): """Returns the change name.""" @@ -1310,29 +1300,22 @@ class Change(object): Returns: [AffectedFile(path, action), AffectedFile(path, action)] """ - 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)) - + affected = list(filter(file_filter, self._affected_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 - ] + """Returns a list of AffectedFile instances for submodules in the change. + + There is no SCM and no submodules, so return an empty list. + """ + return [] 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('AffectedTeestableFiles(include_deletes=%s)' + warn('AffectedTestableFiles(include_deletes=%s)' ' is deprecated and ignored' % str(include_deletes), category=DeprecationWarning, stacklevel=2) @@ -1386,11 +1369,33 @@ 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) + + def _diff_cache(self): + return self._AFFECTED_FILES.DIFF_CACHE(self._upstream) + + def _repo_submodules(self): + """Returns submodule paths for current change's repo.""" + return scm.GIT.ListSubmodules(self.RepositoryRoot()) + + 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() @@ -1398,6 +1403,33 @@ 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. @@ -1980,15 +2012,13 @@ def _parse_change(parser, options): ignore_submodules=False) logging.info('Found %d file(s).', len(change_files)) - 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) + 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) def _parse_gerrit_options(parser, options): diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 71942d5fe..06e97c9a7 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -385,7 +385,7 @@ class PresubmitUnittest(PresubmitTestsBase): 0, 0, None, - upstream=None) + upstream='upstream') self.assertIsNotNone(change.Name() == 'mychange') self.assertIsNotNone(change.DescriptionText( ) == 'Hello there\nthis is a change\nand some more regular text') @@ -444,8 +444,13 @@ 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) + presubmit.GitChange('mychange', + 'description', + self.fake_root_dir, ['foo/blat.cc', 'bar'], + 0, + 0, + None, + upstream='upstream') def testExecPresubmitScript(self): description_lines = ('Hello there', 'this is a change', 'BUG=123') @@ -965,14 +970,10 @@ 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, - upstream=options.upstream) + presubmit.Change.assert_called_once_with( + options.name, options.description, options.root, + [('M', 'random_file.txt')], options.issue, options.patchset, + options.author) presubmit._parse_files.assert_called_once_with(options.files, options.recursive) @@ -1185,9 +1186,14 @@ 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) + [[f[0], f[1]] for f in files], + 0, + 0, + None, + upstream='upstream') input_api = presubmit.InputApi( change, os.path.join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'), False, None, False) @@ -1241,9 +1247,14 @@ 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) + [[f[0], f[1]] for f in files], + 0, + 0, + None, + upstream='upstream') input_api = presubmit.InputApi( change, os.path.join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'), False, None, False) @@ -1371,8 +1382,14 @@ 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) + change = presubmit.GitChange('mychange', + '', + self.fake_root_dir, + files, + 0, + 0, + None, + upstream='upstream') input_api = presubmit.InputApi( change, os.path.join(self.fake_root_dir, 'PRESUBMIT.py'), False, None, False) @@ -1392,8 +1409,14 @@ 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) + change = presubmit.GitChange('mychange', + '', + self.fake_root_dir, + files, + 0, + 0, + None, + upstream='upstream') input_api = presubmit.InputApi( change, os.path.join(self.fake_root_dir, 'PRESUBMIT.py'), False, None, False) @@ -1683,19 +1706,23 @@ class AffectedFileUnittest(PresubmitTestsBase): class ChangeUnittest(PresubmitTestsBase): - @mock.patch('scm.GIT.ListSubmodules', return_value=['BB']) - def testAffectedFiles(self, mockListSubmodules): + def testAffectedFiles(self): change = presubmit.Change('', '', self.fake_root_dir, [('Y', 'AA'), ('A', 'BB')], 3, 5, '') - self.assertEqual(1, len(change.AffectedFiles())) + self.assertEqual(2, 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, '') + change = presubmit.GitChange('', + '', + self.fake_root_dir, [('Y', 'AA'), + ('A', 'BB')], + 3, + 5, + '', + upstream='upstream') self.assertEqual(1, len(change.AffectedSubmodules())) self.assertEqual('A', change.AffectedSubmodules()[0].Action())