From b46232e7299ab6da73aa65f1137f881973c5b3e4 Mon Sep 17 00:00:00 2001 From: Joanna Wang Date: Sat, 21 Jan 2023 01:58:46 +0000 Subject: [PATCH] [stacked_changes] Add Changelist._PrepareChange() common func called before creating commits. Bug: b/265929888 Change-Id: I18a0c5ba6757aef91e750b9703079c96b090dc1e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4178920 Commit-Queue: Joanna Wang Reviewed-by: Gavin Mak Reviewed-by: Josip Sokcevic --- git_cl.py | 87 ++++++++++++++++++++++++++++++-- scm.py | 12 +++-- tests/git_cl_test.py | 116 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 209 insertions(+), 6 deletions(-) diff --git a/git_cl.py b/git_cl.py index b1eafde67..e4b3caabf 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1322,9 +1322,14 @@ class Changelist(object): self.issue = None self.patchset = None - def GetAffectedFiles(self, upstream): + def GetAffectedFiles(self, upstream, end_commit=None): + # type: (str, Optional[str]) -> Sequence[str] + """Returns the list of affected files for the given commit range.""" try: - return [f for _, f in scm.GIT.CaptureStatus(settings.GetRoot(), upstream)] + return [ + f for _, f in scm.GIT.CaptureStatus( + settings.GetRoot(), upstream, end_commit=end_commit) + ] except subprocess2.CalledProcessError: DieWithError( ('\nFailed to diff against upstream branch %s\n\n' @@ -1483,7 +1488,9 @@ class Changelist(object): p_py3.wait() def _GetDescriptionForUpload(self, options, git_diff_args, files): - # Get description message for upload. + # type: (optparse.Values, Sequence[str], Sequence[str] + # ) -> ChangeDescription + """Get description message for upload.""" if self.GetIssue(): description = self.FetchDescription() elif options.message: @@ -1562,6 +1569,80 @@ class Changelist(object): return title return user_title or title + def _PrepareChange(self, options, parent, end_commit): + # type: (optparse.Values, str, str) -> + # Tuple[Sequence[str], Sequence[str], ChangeDescription] + """Prepares the change to be uploaded.""" + self.EnsureCanUploadPatchset(options.force) + + files = self.GetAffectedFiles(parent, end_commit=end_commit) + change_desc = self._GetDescriptionForUpload(options, [parent, end_commit], + files) + + watchlist = watchlists.Watchlists(settings.GetRoot()) + self.ExtendCC(watchlist.GetWatchersForPaths(files)) + if not options.bypass_hooks: + hook_results = self.RunHook(committing=False, + may_prompt=not options.force, + verbose=options.verbose, + parallel=options.parallel, + upstream=parent, + description=change_desc.description, + all_files=False) + self.ExtendCC(hook_results['more_cc']) + + # Update the change description and ensure we have a Change Id. + if self.GetIssue(): + if options.edit_description: + change_desc.prompt() + change_detail = self._GetChangeDetail(['CURRENT_REVISION']) + change_id = change_detail['change_id'] + change_desc.ensure_change_id(change_id) + + # TODO(b/265929888): Pull in external changes for the current branch + # only. No clear way to pull in external changes for upstream branches + # yet. Potentially offer a separate command to pull in external changes. + else: # No change issue. First time uploading + if not options.force and not options.message_file: + change_desc.prompt() + + # Check if user added a change_id in the descripiton. + change_ids = git_footers.get_footer_change_id(change_desc.description) + if len(change_ids) == 1: + change_id = change_ids[0] + else: + change_id = GenerateGerritChangeId(change_desc.description) + change_desc.ensure_change_id(change_id) + + if options.preserve_tryjobs: + change_desc.set_preserve_tryjobs() + + SaveDescriptionBackup(change_desc) + + # Add ccs + ccs = [] + # Add default, watchlist, presubmit ccs if this is an existing change + # and CL is not private and auto-ccing has not been disabled. + if self.GetIssue() and not (options.private and options.no_autocc): + ccs = self.GetCCList().split(',') + if len(ccs) > 100: + lsc = ('https://chromium.googlesource.com/chromium/src/+/HEAD/docs/' + 'process/lsc/lsc_workflow.md') + print('WARNING: This will auto-CC %s users.' % len(ccs)) + print('LSC may be more appropriate: %s' % lsc) + print('You can also use the --no-autocc flag to disable auto-CC.') + confirm_or_exit(action='continue') + + # Add ccs from the --cc flag. + if options.cc: + ccs.extend(options.cc) + + ccs = [email.strip() for email in ccs if email.strip()] + if change_desc.get_cced(): + ccs.extend(change_desc.get_cced()) + + return change_desc.get_reviewers(), ccs, change_desc + def CMDUpload(self, options, git_diff_args, orig_args): """Uploads a change to codereview.""" custom_cl_base = None diff --git a/scm.py b/scm.py index 4013a7625..dce9ef789 100644 --- a/scm.py +++ b/scm.py @@ -119,16 +119,22 @@ class GIT(object): return output.strip() if strip_out else output @staticmethod - def CaptureStatus(cwd, upstream_branch): + def CaptureStatus(cwd, upstream_branch, end_commit=None): + # type: (str, str, Optional[str]) -> Sequence[Tuple[str, str]] """Returns git status. Returns an array of (status, file) tuples.""" + if end_commit is None: + end_commit = '' if upstream_branch is None: upstream_branch = GIT.GetUpstreamBranch(cwd) if upstream_branch is None: raise gclient_utils.Error('Cannot determine upstream branch') - command = ['-c', 'core.quotePath=false', 'diff', - '--name-status', '--no-renames', '-r', '%s...' % upstream_branch] + command = [ + '-c', 'core.quotePath=false', 'diff', '--name-status', '--no-renames', + '-r', + '%s...%s' % (upstream_branch, end_commit) + ] status = GIT.Capture(command, cwd) results = [] if status: diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 03aa95e14..0c7e8c40c 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -3555,6 +3555,122 @@ class ChangelistTest(unittest.TestCase): for user_title in ['not empty', 'yes', 'YES']: self.assertEqual(cl._GetTitleForUpload(options), user_title) + @mock.patch('git_cl.Changelist.GetAffectedFiles', return_value=[]) + @mock.patch('git_cl.GenerateGerritChangeId', return_value='1a2b3c') + @mock.patch('git_cl.Changelist.GetIssue', return_value=None) + @mock.patch('git_cl.ChangeDescription.prompt') + @mock.patch('git_cl.Changelist.RunHook') + @mock.patch('git_cl.Changelist._GetDescriptionForUpload') + @mock.patch('git_cl.Changelist.EnsureCanUploadPatchset') + def testPrepareChange_new(self, mockEnsureCanUploadPatchset, + mockGetDescriptionForupload, mockRunHook, + mockPrompt, *_mocks): + options = optparse.Values() + + options.force = False + options.bypass_hooks = False + options.verbose = False + options.parallel = False + options.preserve_tryjobs = False + options.private = False + options.no_autocc = False + options.message_file = None + options.cc = ['chicken@bok.farm'] + parent = '420parent' + latest_tree = '420latest_tree' + + mockRunHook.return_value = {'more_cc': ['cow@moo.farm']} + desc = 'AH!\nCC=cow2@moo.farm\nR=horse@apple.farm' + mockGetDescriptionForupload.return_value = git_cl.ChangeDescription(desc) + + cl = git_cl.Changelist() + reviewers, ccs, change_desc = cl._PrepareChange(options, parent, + latest_tree) + self.assertEqual(reviewers, ['horse@apple.farm']) + self.assertEqual(ccs, ['chicken@bok.farm', 'cow2@moo.farm']) + self.assertEqual(change_desc._description_lines, [ + 'AH!', 'CC=cow2@moo.farm', 'R=horse@apple.farm', '', 'Change-Id: 1a2b3c' + ]) + mockPrompt.assert_called_once() + mockEnsureCanUploadPatchset.assert_called_once() + mockRunHook.assert_called_once_with(committing=False, + may_prompt=True, + verbose=False, + parallel=False, + upstream='420parent', + description=desc, + all_files=False) + + @mock.patch('git_cl.Settings.GetDefaultCCList', return_value=[]) + @mock.patch('git_cl.Changelist.GetAffectedFiles', return_value=[]) + @mock.patch('git_cl.Changelist.GetIssue', return_value='123') + @mock.patch('git_cl.ChangeDescription.prompt') + @mock.patch('gerrit_util.GetChangeDetail') + @mock.patch('git_cl.Changelist.RunHook') + @mock.patch('git_cl.Changelist._GetDescriptionForUpload') + @mock.patch('git_cl.Changelist.EnsureCanUploadPatchset') + def testPrepareChange_existing(self, mockEnsureCanUploadPatchset, + mockGetDescriptionForupload, mockRunHook, + mockGetChangeDetail, mockPrompt, *_mocks): + cl = git_cl.Changelist() + options = optparse.Values() + + options.force = False + options.bypass_hooks = False + options.verbose = False + options.parallel = False + options.edit_description = False + options.preserve_tryjobs = False + options.private = False + options.no_autocc = False + options.cc = ['chicken@bok.farm'] + parent = '420parent' + latest_tree = '420latest_tree' + + mockRunHook.return_value = {'more_cc': ['cow@moo.farm']} + desc = 'AH!\nCC=cow2@moo.farm\nR=horse@apple.farm' + mockGetDescriptionForupload.return_value = git_cl.ChangeDescription(desc) + + # Existing change + gerrit_util.GetChangeDetail.return_value = { + 'change_id': ('123456789'), + 'current_revision': 'sha1_of_current_revision', + } + + reviewers, ccs, change_desc = cl._PrepareChange(options, parent, + latest_tree) + self.assertEqual(reviewers, ['horse@apple.farm']) + self.assertEqual(ccs, ['cow@moo.farm', 'chicken@bok.farm', 'cow2@moo.farm']) + self.assertEqual(change_desc._description_lines, [ + 'AH!', 'CC=cow2@moo.farm', 'R=horse@apple.farm', '', + 'Change-Id: 123456789' + ]) + mockRunHook.assert_called_once_with(committing=False, + may_prompt=True, + verbose=False, + parallel=False, + upstream='420parent', + description=desc, + all_files=False) + mockEnsureCanUploadPatchset.assert_called_once() + + # Test preserve_tryjob + options.preserve_tryjobs = True + # Test edit_description + options.edit_description = True + # Test private + options.private = True + options.no_autocc = True + + reviewers, ccs, change_desc = cl._PrepareChange(options, parent, + latest_tree) + self.assertEqual(ccs, ['chicken@bok.farm', 'cow2@moo.farm']) + mockPrompt.assert_called_once() + self.assertEqual(change_desc._description_lines, [ + 'AH!', 'CC=cow2@moo.farm', 'R=horse@apple.farm', '', + 'Change-Id: 123456789', 'Cq-Do-Not-Cancel-Tryjobs: true' + ]) + class CMDTestCaseBase(unittest.TestCase): _STATUSES = [