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>
changes/64/5254364/13
Gavin Mak 1 year ago committed by LUCI CQ
parent a4dee1b821
commit dd1a596c3e

@ -880,7 +880,7 @@ class InputApi(object):
def ListSubmodules(self): def ListSubmodules(self):
"""Returns submodule paths for current change's repo.""" """Returns submodule paths for current change's repo."""
return scm.GIT.ListSubmodules(self.change.RepositoryRoot()) return self.change._repo_submodules()
@property @property
def tbr(self): def tbr(self):
@ -912,9 +912,6 @@ class InputApi(object):
class _DiffCache(object): class _DiffCache(object):
"""Caches diffs retrieved from a particular SCM.""" """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): def GetDiff(self, path, local_root):
"""Get the diff for a particular path.""" """Get the diff for a particular path."""
@ -927,8 +924,11 @@ class _DiffCache(object):
class _GitDiffCache(_DiffCache): class _GitDiffCache(_DiffCache):
"""DiffCache implementation for git; gets all file diffs at once.""" """DiffCache implementation for git; gets all file diffs at once."""
def __init__(self, upstream): 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 self._diffs_by_file = None
def GetDiff(self, path, local_root): def GetDiff(self, path, local_root):
@ -1138,21 +1138,13 @@ class Change(object):
'^[ \t]*(?P<key>[A-Z][A-Z_0-9]*)[ \t]*=[ \t]*(?P<value>.*?)[ \t]*$') '^[ \t]*(?P<key>[A-Z][A-Z_0-9]*)[ \t]*=[ \t]*(?P<value>.*?)[ \t]*$')
scm = '' scm = ''
def __init__(self, def __init__(self, name, description, local_root, files, issue, patchset,
name, author):
description,
local_root,
files,
issue,
patchset,
author,
upstream=None):
if files is None: if files is None:
files = [] files = []
self._name = name self._name = name
# Convert root into an absolute path. # Convert root into an absolute path.
self._local_root = os.path.abspath(local_root) self._local_root = os.path.abspath(local_root)
self._upstream = upstream
self.issue = issue self.issue = issue
self.patchset = patchset self.patchset = patchset
self.author_email = author self.author_email = author
@ -1165,15 +1157,13 @@ class Change(object):
assert all((isinstance(f, (list, tuple)) and len(f) == 2) assert all((isinstance(f, (list, tuple)) and len(f) == 2)
for f in files), files for f in files), files
diff_cache = self._AFFECTED_FILES.DIFF_CACHE(self._upstream)
self._affected_files = [ self._affected_files = [
self._AFFECTED_FILES(path, action.strip(), self._local_root, 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): def _diff_cache(self):
"""Returns the upstream branch for the change.""" return self._AFFECTED_FILES.DIFF_CACHE()
return self._upstream
def Name(self): def Name(self):
"""Returns the change name.""" """Returns the change name."""
@ -1310,29 +1300,22 @@ class Change(object):
Returns: Returns:
[AffectedFile(path, action), AffectedFile(path, action)] [AffectedFile(path, action), AffectedFile(path, action)]
""" """
submodule_list = scm.GIT.ListSubmodules(self.RepositoryRoot()) affected = list(filter(file_filter, self._affected_files))
files = [
af for af in self._affected_files
if af.LocalPath() not in submodule_list
]
affected = list(filter(file_filter, files))
if include_deletes: if include_deletes:
return affected return affected
return list(filter(lambda x: x.Action() != 'D', affected)) return list(filter(lambda x: x.Action() != 'D', affected))
def AffectedSubmodules(self): def AffectedSubmodules(self):
"""Returns a list of AffectedFile instances for submodules in the change.""" """Returns a list of AffectedFile instances for submodules in the change.
submodule_list = scm.GIT.ListSubmodules(self.RepositoryRoot())
return [ There is no SCM and no submodules, so return an empty list.
af for af in self._affected_files """
if af.LocalPath() in submodule_list return []
]
def AffectedTestableFiles(self, include_deletes=None, **kwargs): def AffectedTestableFiles(self, include_deletes=None, **kwargs):
"""Return a list of the existing text files in a change.""" """Return a list of the existing text files in a change."""
if include_deletes is not None: if include_deletes is not None:
warn('AffectedTeestableFiles(include_deletes=%s)' warn('AffectedTestableFiles(include_deletes=%s)'
' is deprecated and ignored' % str(include_deletes), ' is deprecated and ignored' % str(include_deletes),
category=DeprecationWarning, category=DeprecationWarning,
stacklevel=2) stacklevel=2)
@ -1386,11 +1369,33 @@ class Change(object):
files = self.AffectedFiles(file_filter=owners_file_filter) files = self.AffectedFiles(file_filter=owners_file_filter)
return {f.LocalPath(): f.OldContents() for f in files} 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): class GitChange(Change):
_AFFECTED_FILES = GitAffectedFile _AFFECTED_FILES = GitAffectedFile
scm = 'git' 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): def AllFiles(self, root=None):
"""List all files under source control in the repo.""" """List all files under source control in the repo."""
root = root or self.RepositoryRoot() root = root or self.RepositoryRoot()
@ -1398,6 +1403,33 @@ class GitChange(Change):
['git', '-c', 'core.quotePath=false', 'ls-files', '--', '.'], ['git', '-c', 'core.quotePath=false', 'ls-files', '--', '.'],
cwd=root).decode('utf-8', 'ignore').splitlines() 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): def ListRelevantPresubmitFiles(files, root):
"""Finds all presubmit files that apply to a given set of source files. """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) ignore_submodules=False)
logging.info('Found %d file(s).', len(change_files)) logging.info('Found %d file(s).', len(change_files))
change_class = GitChange if change_scm == 'git' else Change change_args = [
return change_class(options.name, options.name, options.description, options.root, change_files,
options.description, options.issue, options.patchset, options.author
options.root, ]
change_files, if change_scm == 'git':
options.issue, return GitChange(*change_args, upstream=options.upstream)
options.patchset, return Change(*change_args)
options.author,
upstream=options.upstream)
def _parse_gerrit_options(parser, options): def _parse_gerrit_options(parser, options):

@ -385,7 +385,7 @@ class PresubmitUnittest(PresubmitTestsBase):
0, 0,
0, 0,
None, None,
upstream=None) upstream='upstream')
self.assertIsNotNone(change.Name() == 'mychange') self.assertIsNotNone(change.Name() == 'mychange')
self.assertIsNotNone(change.DescriptionText( self.assertIsNotNone(change.DescriptionText(
) == 'Hello there\nthis is a change\nand some more regular text') ) == 'Hello there\nthis is a change\nand some more regular text')
@ -444,8 +444,13 @@ class PresubmitUnittest(PresubmitTestsBase):
def testInvalidChange(self): def testInvalidChange(self):
with self.assertRaises(AssertionError): with self.assertRaises(AssertionError):
presubmit.GitChange('mychange', 'description', self.fake_root_dir, presubmit.GitChange('mychange',
['foo/blat.cc', 'bar'], 0, 0, None) 'description',
self.fake_root_dir, ['foo/blat.cc', 'bar'],
0,
0,
None,
upstream='upstream')
def testExecPresubmitScript(self): def testExecPresubmitScript(self):
description_lines = ('Hello there', 'this is a change', 'BUG=123') 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) change = presubmit._parse_change(None, options)
self.assertEqual(presubmit.Change.return_value, change) self.assertEqual(presubmit.Change.return_value, change)
presubmit.Change.assert_called_once_with(options.name, presubmit.Change.assert_called_once_with(
options.description, options.name, options.description, options.root,
options.root, [('M', 'random_file.txt')], options.issue, options.patchset,
[('M', 'random_file.txt')], options.author)
options.issue,
options.patchset,
options.author,
upstream=options.upstream)
presubmit._parse_files.assert_called_once_with(options.files, presubmit._parse_files.assert_called_once_with(options.files,
options.recursive) options.recursive)
@ -1185,9 +1186,14 @@ class InputApiUnittest(PresubmitTestsBase):
os.path.isfile.side_effect = lambda f: f in known_files os.path.isfile.side_effect = lambda f: f in known_files
presubmit.scm.GIT.GenerateDiff.return_value = '\n'.join(diffs) 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, 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( input_api = presubmit.InputApi(
change, os.path.join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'), change, os.path.join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'),
False, None, False) False, None, False)
@ -1241,9 +1247,14 @@ class InputApiUnittest(PresubmitTestsBase):
known_files = [os.path.join(self.fake_root_dir, f) for _, f in files] 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 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, 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( input_api = presubmit.InputApi(
change, os.path.join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'), change, os.path.join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'),
False, None, False) False, None, False)
@ -1371,8 +1382,14 @@ class InputApiUnittest(PresubmitTestsBase):
] ]
os.path.isfile.side_effect = lambda f: f in known_files os.path.isfile.side_effect = lambda f: f in known_files
change = presubmit.GitChange('mychange', '', self.fake_root_dir, files, change = presubmit.GitChange('mychange',
0, 0, None) '',
self.fake_root_dir,
files,
0,
0,
None,
upstream='upstream')
input_api = presubmit.InputApi( input_api = presubmit.InputApi(
change, os.path.join(self.fake_root_dir, 'PRESUBMIT.py'), False, change, os.path.join(self.fake_root_dir, 'PRESUBMIT.py'), False,
None, False) None, False)
@ -1392,8 +1409,14 @@ class InputApiUnittest(PresubmitTestsBase):
] ]
os.path.isfile.side_effect = lambda f: f in known_files os.path.isfile.side_effect = lambda f: f in known_files
change = presubmit.GitChange('mychange', '', self.fake_root_dir, files, change = presubmit.GitChange('mychange',
0, 0, None) '',
self.fake_root_dir,
files,
0,
0,
None,
upstream='upstream')
input_api = presubmit.InputApi( input_api = presubmit.InputApi(
change, os.path.join(self.fake_root_dir, 'PRESUBMIT.py'), False, change, os.path.join(self.fake_root_dir, 'PRESUBMIT.py'), False,
None, False) None, False)
@ -1683,19 +1706,23 @@ class AffectedFileUnittest(PresubmitTestsBase):
class ChangeUnittest(PresubmitTestsBase): class ChangeUnittest(PresubmitTestsBase):
@mock.patch('scm.GIT.ListSubmodules', return_value=['BB']) def testAffectedFiles(self):
def testAffectedFiles(self, mockListSubmodules):
change = presubmit.Change('', '', self.fake_root_dir, [('Y', 'AA'), change = presubmit.Change('', '', self.fake_root_dir, [('Y', 'AA'),
('A', 'BB')], 3, ('A', 'BB')], 3,
5, '') 5, '')
self.assertEqual(1, len(change.AffectedFiles())) self.assertEqual(2, len(change.AffectedFiles()))
self.assertEqual('Y', change.AffectedFiles()[0].Action()) self.assertEqual('Y', change.AffectedFiles()[0].Action())
@mock.patch('scm.GIT.ListSubmodules', return_value=['BB']) @mock.patch('scm.GIT.ListSubmodules', return_value=['BB'])
def testAffectedSubmodules(self, mockListSubmodules): def testAffectedSubmodules(self, mockListSubmodules):
change = presubmit.Change('', '', self.fake_root_dir, [('Y', 'AA'), change = presubmit.GitChange('',
('A', 'BB')], 3, '',
5, '') self.fake_root_dir, [('Y', 'AA'),
('A', 'BB')],
3,
5,
'',
upstream='upstream')
self.assertEqual(1, len(change.AffectedSubmodules())) self.assertEqual(1, len(change.AffectedSubmodules()))
self.assertEqual('A', change.AffectedSubmodules()[0].Action()) self.assertEqual('A', change.AffectedSubmodules()[0].Action())

Loading…
Cancel
Save