Commit 7f6dec01 authored by Edward Lemur's avatar Edward Lemur Committed by LUCI CQ

git-cl: Simplify Change object construction.

The `author` parameter is never set.
The `local_description` parameter was only set with the
purpose of getting the local description, so separate it into its
own method and call that instead.
Use the absolute root when getting the list of modified files instead.

Bug: 1042324
Change-Id: I8505172edaacee0656dda98ebbd5f8ebfacb3cef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2038050
Auto-Submit: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: 's avatarAnthony Polito <apolito@google.com>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
parent 816c2b35
...@@ -1384,6 +1384,11 @@ class Changelist(object): ...@@ -1384,6 +1384,11 @@ class Changelist(object):
return None return None
return '%s/%s' % (self.GetCodereviewServer(), issue) return '%s/%s' % (self.GetCodereviewServer(), issue)
def GetLocalDescription(self, upstream_branch):
"""Return the log messages of all commits up to the branch point."""
args = ['log', '--pretty=format:%s%n%n%b', '%s...' % (upstream_branch)]
return RunGitWithCode(args)[1].strip()
def GetDescription(self, pretty=False, force=False): def GetDescription(self, pretty=False, force=False):
if not self.has_description or force: if not self.has_description or force:
if self.GetIssue(): if self.GetIssue():
...@@ -1464,20 +1469,15 @@ class Changelist(object): ...@@ -1464,20 +1469,15 @@ class Changelist(object):
self.issue = None self.issue = None
self.patchset = None self.patchset = None
def GetChange(self, upstream_branch, author, local_description=False): def GetChange(self, upstream_branch):
if not self.GitSanityChecks(upstream_branch): if not self.GitSanityChecks(upstream_branch):
DieWithError('\nGit sanity check failure') DieWithError('\nGit sanity check failure')
root = settings.GetRelativeRoot() root = settings.GetRoot()
if not root:
root = '.'
absroot = os.path.abspath(root)
# We use the sha1 of HEAD as a name of this change. # We use the sha1 of HEAD as a name of this change.
name = RunGitWithCode(['rev-parse', 'HEAD'])[1].strip() name = RunGitWithCode(['rev-parse', 'HEAD'])[1].strip()
# Need to pass a relative path for msysgit.
try: try:
files = scm.GIT.CaptureStatus([root], '.', upstream_branch) files = scm.GIT.CaptureStatus(root, upstream_branch)
except subprocess2.CalledProcessError: except subprocess2.CalledProcessError:
DieWithError( DieWithError(
('\nFailed to diff against upstream branch %s\n\n' ('\nFailed to diff against upstream branch %s\n\n'
...@@ -1489,21 +1489,19 @@ class Changelist(object): ...@@ -1489,21 +1489,19 @@ class Changelist(object):
issue = self.GetIssue() issue = self.GetIssue()
patchset = self.GetPatchset() patchset = self.GetPatchset()
if issue and not local_description: if issue:
description = self.GetDescription() description = self.GetDescription()
else: else:
# If the change was never uploaded, use the log messages of all commits # If the change was never uploaded, use the log messages of all commits
# up to the branch point, as git cl upload will prefill the description # up to the branch point, as git cl upload will prefill the description
# with these log messages. # with these log messages.
args = ['log', '--pretty=format:%s%n%n%b', '%s...' % (upstream_branch)] description = self.GetLocalDescription(upstream_branch)
description = RunGitWithCode(args)[1].strip()
if not author: author = RunGit(['config', 'user.email']).strip() or None
author = RunGit(['config', 'user.email']).strip() or None
return presubmit_support.GitChange( return presubmit_support.GitChange(
name, name,
description, description,
absroot, root,
files, files,
issue, issue,
patchset, patchset,
...@@ -1576,7 +1574,7 @@ class Changelist(object): ...@@ -1576,7 +1574,7 @@ class Changelist(object):
self.EnsureCanUploadPatchset(force=options.force) self.EnsureCanUploadPatchset(force=options.force)
# Apply watchlists on upload. # Apply watchlists on upload.
change = self.GetChange(base_branch, None) change = self.GetChange(base_branch)
watchlist = watchlists.Watchlists(change.RepositoryRoot()) watchlist = watchlists.Watchlists(change.RepositoryRoot())
files = [f.LocalPath() for f in change.AffectedFiles()] files = [f.LocalPath() for f in change.AffectedFiles()]
if not options.bypass_watchlists: if not options.bypass_watchlists:
...@@ -2138,7 +2136,7 @@ class Changelist(object): ...@@ -2138,7 +2136,7 @@ class Changelist(object):
committing=True, committing=True,
may_prompt=not force, may_prompt=not force,
verbose=verbose, verbose=verbose,
change=self.GetChange(self.GetCommonAncestorWithUpstream(), None), change=self.GetChange(self.GetCommonAncestorWithUpstream()),
parallel=parallel) parallel=parallel)
if not hook_results.should_continue(): if not hook_results.should_continue():
return 1 return 1
...@@ -4149,8 +4147,7 @@ def CMDdescription(parser, args): ...@@ -4149,8 +4147,7 @@ def CMDdescription(parser, args):
text = '\n'.join(l.rstrip() for l in sys.stdin) text = '\n'.join(l.rstrip() for l in sys.stdin)
elif text == '+': elif text == '+':
base_branch = cl.GetCommonAncestorWithUpstream() base_branch = cl.GetCommonAncestorWithUpstream()
change = cl.GetChange(base_branch, None, local_description=True) text = cl.GetLocalDescription(base_branch)
text = change.FullDescriptionText()
description.set_description(text) description.set_description(text)
else: else:
...@@ -4182,7 +4179,7 @@ def CMDlint(parser, args): ...@@ -4182,7 +4179,7 @@ def CMDlint(parser, args):
os.chdir(settings.GetRoot()) os.chdir(settings.GetRoot())
try: try:
cl = Changelist() cl = Changelist()
change = cl.GetChange(cl.GetCommonAncestorWithUpstream(), None) change = cl.GetChange(cl.GetCommonAncestorWithUpstream())
files = [f.LocalPath() for f in change.AffectedFiles()] files = [f.LocalPath() for f in change.AffectedFiles()]
if not files: if not files:
print('Cannot lint an empty CL') print('Cannot lint an empty CL')
...@@ -4240,7 +4237,7 @@ def CMDpresubmit(parser, args): ...@@ -4240,7 +4237,7 @@ def CMDpresubmit(parser, args):
base_branch = cl.GetCommonAncestorWithUpstream() base_branch = cl.GetCommonAncestorWithUpstream()
if options.all: if options.all:
base_change = cl.GetChange(base_branch, None) base_change = cl.GetChange(base_branch)
files = [('M', f) for f in base_change.AllFiles()] files = [('M', f) for f in base_change.AllFiles()]
change = presubmit_support.GitChange( change = presubmit_support.GitChange(
base_change.Name(), base_change.Name(),
...@@ -4252,7 +4249,7 @@ def CMDpresubmit(parser, args): ...@@ -4252,7 +4249,7 @@ def CMDpresubmit(parser, args):
base_change.author_email, base_change.author_email,
base_change._upstream) base_change._upstream)
else: else:
change = cl.GetChange(base_branch, None) change = cl.GetChange(base_branch)
cl.RunHook( cl.RunHook(
committing=not options.upload, committing=not options.upload,
...@@ -5065,8 +5062,7 @@ def CMDowners(parser, args): ...@@ -5065,8 +5062,7 @@ def CMDowners(parser, args):
if options.show_all: if options.show_all:
for arg in args: for arg in args:
base_branch = cl.GetCommonAncestorWithUpstream() base_branch = cl.GetCommonAncestorWithUpstream()
change = cl.GetChange(base_branch, None) database = owners.Database(settings.GetRoot(), file, os.path)
database = owners.Database(change.RepositoryRoot(), file, os.path)
database.load_data_needed_for([arg]) database.load_data_needed_for([arg])
print('Owners for %s:' % arg) print('Owners for %s:' % arg)
for owner in sorted(database.all_possible_owners([arg], None)): for owner in sorted(database.all_possible_owners([arg], None)):
...@@ -5081,7 +5077,7 @@ def CMDowners(parser, args): ...@@ -5081,7 +5077,7 @@ def CMDowners(parser, args):
# Default to diffing against the common ancestor of the upstream branch. # Default to diffing against the common ancestor of the upstream branch.
base_branch = cl.GetCommonAncestorWithUpstream() base_branch = cl.GetCommonAncestorWithUpstream()
change = cl.GetChange(base_branch, None) change = cl.GetChange(base_branch)
affected_files = [f.LocalPath() for f in change.AffectedFiles()] affected_files = [f.LocalPath() for f in change.AffectedFiles()]
if options.batch: if options.batch:
......
...@@ -1771,7 +1771,7 @@ def _parse_change(parser, options): ...@@ -1771,7 +1771,7 @@ def _parse_change(parser, options):
change_files = [('M', f) for f in scm.GIT.GetAllFiles(options.root)] change_files = [('M', f) for f in scm.GIT.GetAllFiles(options.root)]
else: else:
change_files = scm.GIT.CaptureStatus( change_files = scm.GIT.CaptureStatus(
[], options.root, options.upstream or None) options.root, options.upstream or None)
logging.info('Found %d file(s).', len(change_files)) logging.info('Found %d file(s).', len(change_files))
......
...@@ -118,11 +118,9 @@ class GIT(object): ...@@ -118,11 +118,9 @@ class GIT(object):
return output.strip() if strip_out else output return output.strip() if strip_out else output
@staticmethod @staticmethod
def CaptureStatus(files, cwd, upstream_branch): def CaptureStatus(cwd, upstream_branch):
"""Returns git status. """Returns git status.
@files is a list of files.
Returns an array of (status, file) tuples.""" Returns an array of (status, file) tuples."""
if upstream_branch is None: if upstream_branch is None:
upstream_branch = GIT.GetUpstreamBranch(cwd) upstream_branch = GIT.GetUpstreamBranch(cwd)
...@@ -130,8 +128,6 @@ class GIT(object): ...@@ -130,8 +128,6 @@ class GIT(object):
raise gclient_utils.Error('Cannot determine upstream branch') raise gclient_utils.Error('Cannot determine upstream branch')
command = ['-c', 'core.quotePath=false', 'diff', command = ['-c', 'core.quotePath=false', 'diff',
'--name-status', '--no-renames', '-r', '%s...' % upstream_branch] '--name-status', '--no-renames', '-r', '%s...' % upstream_branch]
if files:
command.extend(files)
status = GIT.Capture(command, cwd) status = GIT.Capture(command, cwd)
results = [] results = []
if status: if status:
......
...@@ -879,7 +879,7 @@ class TestGitCl(TestCase): ...@@ -879,7 +879,7 @@ class TestGitCl(TestCase):
((['git', 'rev-parse', 'HEAD'],), '12345'), ((['git', 'rev-parse', 'HEAD'],), '12345'),
((['git', '-c', 'core.quotePath=false', 'diff', '--name-status', ((['git', '-c', 'core.quotePath=false', 'diff', '--name-status',
'--no-renames', '-r', ancestor_revision + '...', '.'],), '--no-renames', '-r', ancestor_revision + '...'],),
'M\t.gitignore\n'), 'M\t.gitignore\n'),
((['git', 'config', 'branch.master.gerritpatchset'],), CERR1), ((['git', 'config', 'branch.master.gerritpatchset'],), CERR1),
] ]
......
...@@ -982,7 +982,7 @@ def CheckChangeOnCommit(input_api, output_api): ...@@ -982,7 +982,7 @@ def CheckChangeOnCommit(input_api, output_api):
options.author, options.author,
upstream=options.upstream) upstream=options.upstream)
scm.GIT.CaptureStatus.assert_called_once_with( scm.GIT.CaptureStatus.assert_called_once_with(
[], options.root, options.upstream) options.root, options.upstream)
@mock.patch('presubmit_support.GitChange', mock.Mock()) @mock.patch('presubmit_support.GitChange', mock.Mock())
@mock.patch('scm.GIT.GetAllFiles', mock.Mock()) @mock.patch('scm.GIT.GetAllFiles', mock.Mock())
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment