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 <dloehr@google.com>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Andy Perelson <ajp@google.com>

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)
+        if not dry_run:
-        print('Will split current branch (' + refactor_branch + ') into ' +
+            PrintSummary(files_split_by_reviewers, refactor_branch)
-              str(num_cls) + ' CLs.\n')
+            answer = gclient_utils.AskForData('Proceed? (y/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 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,
+                PrintClInfo(cl_index, len(files_split_by_reviewers),
-                            file_paths, description, reviewer_set, cq_dry_run,
+                            cl_info.owners_directories, file_paths, description,
-                            enable_auto_submit, topic)
+                            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'
 
 

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()