From a12175c2a7a9f79c3296068a022ac4f3051f8600 Mon Sep 17 00:00:00 2001 From: Edward Lemur Date: Mon, 9 Mar 2020 16:58:26 +0000 Subject: [PATCH] git-cl: Use _create_description_from_log instead of GetLocalDescription Also refactor code to eliminate multiple calls to _create_description_from_log. Change-Id: I113134fbd90f396bdb6d561ed0369ea5ee9c78ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2090448 Reviewed-by: Anthony Polito Commit-Queue: Edward Lesmes --- git_cl.py | 74 ++++++++++++++++---------------------------- split_cl.py | 2 +- tests/git_cl_test.py | 32 +++---------------- 3 files changed, 32 insertions(+), 76 deletions(-) diff --git a/git_cl.py b/git_cl.py index e306baf57..ff391a236 100755 --- a/git_cl.py +++ b/git_cl.py @@ -966,7 +966,7 @@ def _create_description_from_log(args): log_args = [args[0] + '..' + args[1]] else: log_args = args[:] # Hope for the best! - return RunGit(['log', '--pretty=format:%s\n\n%b'] + log_args) + return RunGit(['log', '--pretty=format:%s%n%n%b'] + log_args) class GerritChangeNotExists(Exception): @@ -1016,7 +1016,6 @@ class Changelist(object): self.lookedup_issue = False self.issue = issue or None self.description = None - self.local_description = None self.lookedup_patchset = False self.patchset = None self.cc = None @@ -1266,13 +1265,6 @@ class Changelist(object): return None return '%s/%s' % (self.GetCodereviewServer(), issue) - def GetLocalDescription(self, upstream_branch): - """Return the log messages of all commits up to the branch point.""" - if self.local_description is None: - args = ['log', '--pretty=format:%s%n%n%b', '%s...' % (upstream_branch)] - self.local_description = RunGitWithCode(args)[1].strip() - return self.local_description - def FetchDescription(self, pretty=False): assert self.GetIssue(), 'issue is required to query Gerrit' @@ -1348,7 +1340,7 @@ class Changelist(object): self.issue = None self.patchset = None - def GetChange(self, upstream_branch): + def GetChange(self, upstream_branch, description): if not self.GitSanityChecks(upstream_branch): DieWithError('\nGit sanity check failure') @@ -1368,13 +1360,6 @@ class Changelist(object): issue = self.GetIssue() patchset = self.GetPatchset() - if issue: - description = self.FetchDescription() - else: - # 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 - # with these log messages. - description = self.GetLocalDescription(upstream_branch) author = self.GetAuthor() return presubmit_support.GitChange( @@ -1493,17 +1478,23 @@ class Changelist(object): self.EnsureAuthenticated(force=options.force) self.EnsureCanUploadPatchset(force=options.force) + # Get description message for upload. + if self.GetIssue(): + description = self.FetchDescription() + elif options.message: + description = options.message + else: + description = _create_description_from_log(git_diff_args) + if options.title and options.squash: + description = options.title + '\n\n' + message + # Apply watchlists on upload. - change = self.GetChange(base_branch) + change = self.GetChange(base_branch, description) watchlist = watchlists.Watchlists(change.RepositoryRoot()) files = [f.LocalPath() for f in change.AffectedFiles()] if not options.bypass_watchlists: self.ExtendCC(watchlist.GetWatchersForPaths(files)) - if self.GetIssue(): - description = self.FetchDescription() - else: - description = self.GetLocalDescription(base_branch) if options.reviewers or options.tbrs or options.add_owners_to: # Set the reviewer list now so that presubmit checks can access it. change_description = ChangeDescription(description) @@ -1524,7 +1515,8 @@ class Changelist(object): self.ExtendCC(hook_results['more_cc']) print_stats(git_diff_args) - ret = self.CMDUploadChange(options, git_diff_args, custom_cl_base, change) + ret = self.CMDUploadChange( + options, git_diff_args, custom_cl_base, change, description) if not ret: self._GitSetBranchConfigValue( 'last-upload-hash', RunGit(['rev-parse', 'HEAD']).strip()) @@ -2021,7 +2013,7 @@ class Changelist(object): if self.GetIssue(): description = self.FetchDescription() else: - description = self.GetLocalDescription(upstream) + description = _create_description_from_log([upstream]) self.RunHook( committing=True, may_prompt=not force, @@ -2258,7 +2250,8 @@ class Changelist(object): return push_stdout - def CMDUploadChange(self, options, git_diff_args, custom_cl_base, change): + def CMDUploadChange( + self, options, git_diff_args, custom_cl_base, change, message): """Upload the current branch to Gerrit.""" if options.squash is None: # Load default for user, repo, squash=true, in this order. @@ -2283,12 +2276,6 @@ class Changelist(object): if options.squash: self._GerritCommitMsgHookCheck(offer_removal=not options.force) if self.GetIssue(): - # Try to get the message from a previous upload. - message = self.FetchDescription() - if not message: - DieWithError( - 'failed to fetch description from current Gerrit change %d\n' - '%s' % (self.GetIssue(), self.GetIssueURL())) if not title: if options.message: # When uploading a subsequent patchset, -m|--message is taken @@ -2347,12 +2334,6 @@ class Changelist(object): assert [change_id] == git_footers.get_footer_change_id(message) change_desc = ChangeDescription(message, bug=bug, fixed=fixed) else: # if not self.GetIssue() - if options.message: - message = options.message - else: - message = _create_description_from_log(git_diff_args) - if options.title: - message = options.title + '\n\n' + message change_desc = ChangeDescription(message, bug=bug, fixed=fixed) if not options.force: change_desc.prompt() @@ -2389,15 +2370,14 @@ class Changelist(object): ref_to_push = RunGit( ['commit-tree', tree, '-p', parent, '-F', desc_tempfile]).strip() else: # if not options.squash - change_desc = ChangeDescription( - options.message or _create_description_from_log(git_diff_args)) + change_desc = ChangeDescription(message) if not change_desc.description: DieWithError("Description is empty. Aborting...") if not git_footers.get_footer_change_id(change_desc.description): DownloadGerritHook(False) change_desc.set_description( - self._AddChangeIdToCommitMessage(options, git_diff_args)) + self._AddChangeIdToCommitMessage(message, git_diff_args)) if options.reviewers or options.tbrs or options.add_owners_to: change_desc.update_reviewers(options.reviewers, options.tbrs, options.add_owners_to, change) @@ -2609,13 +2589,11 @@ class Changelist(object): change_desc) return parent - def _AddChangeIdToCommitMessage(self, options, args): + def _AddChangeIdToCommitMessage(self, log_desc, args): """Re-commits using the current message, assumes the commit hook is in place. """ - log_desc = options.message or _create_description_from_log(args) - git_command = ['commit', '--amend', '-m', log_desc] - RunGit(git_command) + RunGit(['commit', '--amend', '-m', log_desc]) new_log_desc = _create_description_from_log(args) if git_footers.get_footer_change_id(new_log_desc): print('git-cl: Added Change-Id to commit message.') @@ -4038,7 +4016,7 @@ def CMDdescription(parser, args): text = '\n'.join(l.rstrip() for l in sys.stdin) elif text == '+': base_branch = cl.GetCommonAncestorWithUpstream() - text = cl.GetLocalDescription(base_branch) + text = _create_description_from_log([base_branch]) description.set_description(text) else: @@ -4070,7 +4048,7 @@ def CMDlint(parser, args): os.chdir(settings.GetRoot()) try: cl = Changelist() - change = cl.GetChange(cl.GetCommonAncestorWithUpstream()) + change = cl.GetChange(cl.GetCommonAncestorWithUpstream(), '') files = [f.LocalPath() for f in change.AffectedFiles()] if not files: print('Cannot lint an empty CL') @@ -4130,7 +4108,7 @@ def CMDpresubmit(parser, args): if cl.GetIssue(): description = cl.FetchDescription() else: - description = cl.GetLocalDescription(base_branch) + description = _create_description_from_log([base_branch]) cl.RunHook( committing=not options.upload, @@ -4965,7 +4943,7 @@ def CMDowners(parser, args): # Default to diffing against the common ancestor of the upstream branch. base_branch = cl.GetCommonAncestorWithUpstream() - change = cl.GetChange(base_branch) + change = cl.GetChange(base_branch, '') affected_files = [f.LocalPath() for f in change.AffectedFiles()] if options.batch: diff --git a/split_cl.py b/split_cl.py index 37190295a..a814eb300 100644 --- a/split_cl.py +++ b/split_cl.py @@ -194,7 +194,7 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, EnsureInGitRepository() cl = changelist() - change = cl.GetChange(cl.GetCommonAncestorWithUpstream()) + change = cl.GetChange(cl.GetCommonAncestorWithUpstream(), '') files = change.AffectedFiles() if not files: diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index ed805b0a7..435e57dd9 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -852,8 +852,6 @@ class TestGitCl(unittest.TestCase): if post_amend_description is None: post_amend_description = description cc = cc or [] - # Determined in `_gerrit_base_calls`. - determined_ancestor_revision = custom_cl_base or 'fake_ancestor_sha' calls = [] @@ -863,12 +861,6 @@ class TestGitCl(unittest.TestCase): # If issue is given, then description is fetched from Gerrit instead. if issue is None: - calls += [ - ((['git', 'log', '--pretty=format:%s\n\n%b', - ((custom_cl_base + '..') if custom_cl_base else - 'fake_ancestor_sha..HEAD')],), - description), - ] if squash: title = 'Initial_upload' else: @@ -881,14 +873,6 @@ class TestGitCl(unittest.TestCase): if not git_footers.get_footer_change_id(description) and not squash: calls += [ (('DownloadGerritHook', False), ''), - # Amending of commit message to get the Change-Id. - ((['git', 'log', '--pretty=format:%s\n\n%b', - determined_ancestor_revision + '..HEAD'],), - description), - ((['git', 'commit', '--amend', '-m', description],), ''), - ((['git', 'log', '--pretty=format:%s\n\n%b', - determined_ancestor_revision + '..HEAD'],), - post_amend_description) ] if squash: if force or not issue: @@ -1192,7 +1176,10 @@ class TestGitCl(unittest.TestCase): lambda path: self._mocked_call(['os.path.isfile', path])).start() mock.patch('git_cl.Changelist.GitSanityChecks', return_value=True).start() mock.patch( - 'git_cl.Changelist.GetLocalDescription', return_value='foo').start() + 'git_cl._create_description_from_log', return_value=description).start() + mock.patch( + 'git_cl.Changelist._AddChangeIdToCommitMessage', + return_value=post_amend_description or description).start() mock.patch( 'git_cl.ask_for_data', lambda prompt: self._mocked_call('ask_for_data', prompt)).start() @@ -2818,15 +2805,6 @@ class ChangelistTest(unittest.TestCase): self.addCleanup(mock.patch.stopall) self.temp_count = 0 - @mock.patch('git_cl.RunGitWithCode') - def testGetLocalDescription(self, _mock): - git_cl.RunGitWithCode.return_value = (0, 'description') - cl = git_cl.Changelist() - self.assertEqual('description', cl.GetLocalDescription('branch')) - self.assertEqual('description', cl.GetLocalDescription('branch')) - git_cl.RunGitWithCode.assert_called_once_with( - ['log', '--pretty=format:%s%n%n%b', 'branch...']) - def testRunHook(self): expected_results = { 'more_cc': ['more@example.com', 'cc@example.com'], @@ -3031,7 +3009,7 @@ class CMDPresubmitTestCase(CMDTestCaseBase): 'git_cl.Changelist.FetchDescription', return_value='fetch description').start() mock.patch( - 'git_cl.Changelist.GetLocalDescription', + 'git_cl._create_description_from_log', return_value='get description').start() mock.patch('git_cl.Changelist.RunHook').start()