From 36427aab02ce0a0a4ca8bf86a1ba78a4f2ef1402 Mon Sep 17 00:00:00 2001 From: Devon Loehr Date: Wed, 22 Jan 2025 12:05:24 -0800 Subject: [PATCH] split_cl: Always ask for confirmation before uploading `git cl split` has the potential to do a lot of operations at once, which is problematic if it didn't end up splitting things in a way the user approves of. This changes it to print a summary of the splitting, and asking the user to review it briefly before proceeding with the upload. Bug: 389069356, 40642562, 40269201 Change-Id: I90bead0147badbaa3afcce2264d805ae498f101c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6163905 Commit-Queue: Devon Loehr Reviewed-by: Peter Kotwicz Reviewed-by: Andy Perelson --- split_cl.py | 57 ++++++++++++++--------- tests/split_cl_test.py | 100 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 20 deletions(-) diff --git a/split_cl.py b/split_cl.py index fb521a9f0..62de4d119 100644 --- a/split_cl.py +++ b/split_cl.py @@ -230,6 +230,36 @@ def LoadDescription(description_file, dry_run): return gclient_utils.FileRead(description_file) +def PrintSummary(files_split_by_reviewers, refactor_branch): + """Print a brief summary of the splitting so the user + can review it before uploading. + + Args: + files_split_by_reviewers: A dictionary mapping reviewer tuples + to the files and directories assigned to them. + """ + + for reviewers, file_info in files_split_by_reviewers.items(): + print(f'Reviewers: {reviewers}, files: {len(file_info.files)}, ' + f'directories: {file_info.owners_directories}') + + num_cls = len(files_split_by_reviewers) + print(f'\nWill split branch {refactor_branch} into {num_cls} CLs. ' + 'Please quickly review them before proceeding.\n') + if (num_cls > CL_SPLIT_FORCE_LIMIT): + print('Warning: Uploading this many CLs may potentially ' + 'reach the limit of concurrent runs, imposed on you by the ' + 'build infrastructure. Your runs may be throttled as a ' + 'result.\n\nPlease email infra-dev@chromium.org if you ' + 'have any questions. ' + 'The infra team reserves the right to cancel ' + 'your jobs if they are overloading the CQ.\n\n' + '(Alternatively, you can reduce the number of CLs created by ' + 'using the --max-depth option. Pass --dry-run to examine the ' + 'CLs which will be created until you are happy with the ' + 'results.)') + + def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, cq_dry_run, enable_auto_submit, max_depth, topic, repository_root): """"Splits a branch into smaller branches and uploads CLs. @@ -282,22 +312,9 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, files_split_by_reviewers = SelectReviewersForFiles( cl, author, files, max_depth) - num_cls = len(files_split_by_reviewers) - print('Will split current branch (' + refactor_branch + ') into ' + - str(num_cls) + ' CLs.\n') - if not dry_run and num_cls > CL_SPLIT_FORCE_LIMIT: - print('This will generate "%r" CLs. This many CLs may potentially' - ' reach the limit of concurrent runs, imposed on you by the ' - 'build infrastructure. Your runs may be throttled as a ' - 'result.\n\nPlease email infra-dev@chromium.org if you ' - 'have any questions. ' - 'The infra team reserves the right to cancel' - ' your jobs if they are overloading the CQ.\n\n' - '(Alternatively, you can reduce the number of CLs created by' - ' using the --max-depth option. Pass --dry-run to examine the' - ' CLs which will be created until you are happy with the' - ' results.)' % num_cls) - answer = gclient_utils.AskForData('Proceed? (y/n):') + if not dry_run: + PrintSummary(files_split_by_reviewers, refactor_branch) + answer = gclient_utils.AskForData('Proceed? (y/N):') if answer.lower() != 'y': return 0 @@ -308,9 +325,9 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, reviewer_set = set(reviewers) if dry_run: file_paths = [f for _, f in cl_info.files] - PrintClInfo(cl_index, num_cls, cl_info.owners_directories, - file_paths, description, reviewer_set, cq_dry_run, - enable_auto_submit, topic) + PrintClInfo(cl_index, len(files_split_by_reviewers), + cl_info.owners_directories, file_paths, description, + reviewer_set, cq_dry_run, enable_auto_submit, topic) else: UploadCl(refactor_branch, refactor_branch_upstream, cl_info.owners_directories, cl_info.files, description, @@ -352,7 +369,7 @@ def CheckDescriptionBugLink(description): answer = 'y' if not matches: answer = gclient_utils.AskForData( - 'Description does not include a bug link. Proceed? (y/n):') + 'Description does not include a bug link. Proceed? (y/N):') return answer.lower() == 'y' diff --git a/tests/split_cl_test.py b/tests/split_cl_test.py index 923e22b2c..bb838c935 100755 --- a/tests/split_cl_test.py +++ b/tests/split_cl_test.py @@ -12,6 +12,7 @@ import split_cl class SplitClTest(unittest.TestCase): + def testAddUploadedByGitClSplitToDescription(self): description = """Convert use of X to Y in $directory @@ -97,6 +98,7 @@ class SplitClTest(unittest.TestCase): class UploadClTester: """Sets up test environment for testing split_cl.UploadCl()""" + def __init__(self, test): self.mock_git_branches = self.StartPatcher("git_common.branches", test) @@ -214,6 +216,104 @@ class SplitClTest(unittest.TestCase): "Description") self.assertEqual(mock_file_read.call_count, 1) + class SplitClTester: + """Sets up test environment for testing split_cl.SplitCl()""" + + def __init__(self, test): + self.mocks = [] + self.mock_file_read = self.StartPatcher( + "gclient_utils.FileRead", + test, + return_value="Non-dummy description\nBug: 1243") + self.mock_in_git_repo = self.StartPatcher( + "split_cl.EnsureInGitRepository", test) + self.mock_git_status = self.StartPatcher("scm.GIT.CaptureStatus", + test) + self.mock_git_run = self.StartPatcher("git_common.run", test) + self.mock_git_current_branch = self.StartPatcher( + "git_common.current_branch", + test, + return_value="current_branch") + self.mock_git_upstream = self.StartPatcher( + "git_common.upstream", test, return_value="upstream_branch") + self.mock_get_reviewers = self.StartPatcher( + "split_cl.SelectReviewersForFiles", test) + self.mock_ask_for_data = self.StartPatcher( + "gclient_utils.AskForData", test) + self.mock_print_cl_info = self.StartPatcher("split_cl.PrintClInfo", + test) + self.mock_upload_cl = self.StartPatcher("split_cl.UploadCl", test) + # Suppress output for cleaner tests + self.mock_print = self.StartPatcher("builtins.print", test) + + def StartPatcher(self, target, test, **kwargs): + patcher = mock.patch(target, **kwargs) + test.addCleanup(patcher.stop) + m = patcher.start() + self.mocks.append(m) + return m + + def ResetMocks(self): + for m in self.mocks: + m.reset_mock() + + def DoSplitCl(self, description_file, dry_run, files_split_by_reviewers, + proceed_response): + all_files = [v.files for v in files_split_by_reviewers.values()] + all_files_flattened = [ + file for files in all_files for file in files + ] + + self.mock_git_status.return_value = all_files_flattened + self.mock_get_reviewers.return_value = files_split_by_reviewers + self.mock_ask_for_data.return_value = proceed_response + + split_cl.SplitCl(description_file, None, mock.Mock(), None, dry_run, + False, False, None, None, None) + + def testSplitClConfirm(self): + split_cl_tester = self.SplitClTester(self) + + files_split_by_reviewers = { + "a@example.com": + split_cl.FilesAndOwnersDirectory([ + ("M", "a/b/foo.cc"), + ("M", "d/e/bar.h"), + ], []), + "b@example.com": + split_cl.FilesAndOwnersDirectory([ + ("A", "f/g/baz.py"), + ], []) + } + + # Should prompt for confirmation and upload several times + split_cl_tester.DoSplitCl("SomeFile.txt", False, + files_split_by_reviewers, "y") + + split_cl_tester.mock_ask_for_data.assert_called_once() + split_cl_tester.mock_print_cl_info.assert_not_called() + self.assertEqual(split_cl_tester.mock_upload_cl.call_count, + len(files_split_by_reviewers)) + + split_cl_tester.ResetMocks() + # Should prompt for confirmation and not upload + split_cl_tester.DoSplitCl("SomeFile.txt", False, + files_split_by_reviewers, "f") + + split_cl_tester.mock_ask_for_data.assert_called_once() + split_cl_tester.mock_print_cl_info.assert_not_called() + split_cl_tester.mock_upload_cl.assert_not_called() + + split_cl_tester.ResetMocks() + # Dry run: Don't prompt, print info instead of uploading + split_cl_tester.DoSplitCl("SomeFile.txt", True, + files_split_by_reviewers, "f") + + split_cl_tester.mock_ask_for_data.assert_not_called() + self.assertEqual(split_cl_tester.mock_print_cl_info.call_count, + len(files_split_by_reviewers)) + split_cl_tester.mock_upload_cl.assert_not_called() + if __name__ == '__main__': unittest.main()