[git cl split]: Allow users to override reviewer assignment

This CL adds a --reviewers flag, which allows users to specify that CLs
should all be sent to the same (possibly empty) set of reviewers. This
can be useful for LSCs where OO+1 is available.

Bug: 389069356, 40269201
Change-Id: Ic6202b280bb676f75a71c4ccf0c41b32483d5c6e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6298664
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
Commit-Queue: Devon Loehr <dloehr@google.com>

git_cl.py

@@ -5872,18 +5872,36 @@ def CMDsplit(parser, args):
         help='If passed during a dry run, will print out a summary of the '
         'generated splitting, rather than full details. No effect outside of '
         'a dry run.')
+    # If we ever switch from optparse to argparse, we can combine these flags
+    # using nargs='*'
+    parser.add_option(
+        '--reviewers',
+        action='append',
+        default=None,
+        help='If present, all generated CLs will be sent to the specified '
+        'reviewer(s) specified, rather than automatically assigned reviewers.\n'
+        'Multiple reviewers can be specified as '
+        '--reviewers a@b.com --reviewers c@d.com\n')
+    parser.add_option(
+        '--no-reviewers',
+        action='store_true',
+        help='If present, generated CLs will not be assigned reviewers. '
+        'Overrides --reviewers.')
     options, _ = parser.parse_args(args)
 
     if not options.description_file and not options.dry_run:
         parser.error('No --description flag specified.')
 
+    if options.no_reviewers:
+        options.reviewers = []
+
     def WrappedCMDupload(args):
         return CMDupload(OptionParser(), args)
 
     return split_cl.SplitCl(options.description_file, options.comment_file,
                             Changelist, WrappedCMDupload, options.dry_run,
-                            options.summarize, options.cq_dry_run,
-                            options.enable_auto_submit,
+                            options.summarize, options.reviewers,
+                            options.cq_dry_run, options.enable_auto_submit,
                             options.max_depth, options.topic, options.from_file,
                             settings.GetRoot())
 

split_cl.py

@@ -393,8 +393,8 @@ def PrintSummary(cl_infos, refactor_branch):
 
 
 def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run,
-            summarize, cq_dry_run, enable_auto_submit, max_depth, topic,
-            from_file, repository_root):
+            summarize, reviewers_override, cq_dry_run, enable_auto_submit,
+            max_depth, topic, from_file, repository_root):
     """"Splits a branch into smaller branches and uploads CLs.
 
     Args:
@@ -403,6 +403,8 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run,
         changelist: The Changelist class.
         cmd_upload: The function associated with the git cl upload command.
         dry_run: Whether this is a dry run (no branches or CLs created).
+        reviewers_override: Either None or a (possibly empty) list of reviewers
+            all CLs should be sent to.
         cq_dry_run: If CL uploads should also do a cq dry run.
         enable_auto_submit: If CL uploads should also enable auto submit.
         max_depth: The maximum directory depth to search for OWNERS files. A
@@ -448,6 +450,12 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run,
         cl_infos = CLInfoFromFilesAndOwnersDirectoriesDict(
             files_split_by_reviewers)
 
+    # Note that we do this override even if the list is empty (indicating that
+    # the user requested CLs not be assigned to any reviewers).
+    if reviewers_override != None:
+        for info in cl_infos:
+            info.reviewers = set(reviewers_override)
+
     if not dry_run:
         PrintSummary(cl_infos, refactor_branch)
         answer = gclient_utils.AskForData(

tests/git_cl_test.py

@@ -5719,11 +5719,36 @@ class CMDSplitTestCase(CMDTestCaseBase):
                          0)
         self.assertEqual(mock_split_cl.call_count, 1)
 
-        # Unless we're doing a dry run
+        # ...unless we're doing a dry run
         mock_split_cl.reset_mock()
         self.assertEqual(git_cl.main(['split', '-n']), 0)
         self.assertEqual(mock_split_cl.call_count, 1)
 
+    @mock.patch("split_cl.SplitCl", return_value=0)
+    @mock.patch("git_cl.OptionParser.error", side_effect=ParserErrorMock)
+    def testReviewerParsing(self, _, mock_split_cl):
+        """Make sure we correctly parse various combinations of --reviewers"""
+
+        # Helper function to pull out the reviewers arg and compare it
+        def testOneSetOfFlags(flags, expected):
+            self.assertEqual(git_cl.main(['split', '-n'] + flags), 0)
+            mock_split_cl.assert_called_once()
+            # It's unfortunate that there's no better way to get the argument
+            # than to hardcode its number, unless we switch to using keyword
+            # arguments everywhere or pass the options in directly.
+            reviewers_arg = mock_split_cl.call_args.args[6]
+            self.assertEqual(reviewers_arg, expected)
+            mock_split_cl.reset_mock()
+
+        # If no --reviewers flag is passed, we should get None
+        testOneSetOfFlags([], None)
+        # If --reviewers flag is passed, we should get a list of args
+        testOneSetOfFlags(['--reviewers', 'a@b.com'], ['a@b.com'])
+        testOneSetOfFlags(['--reviewers', 'a@b.com', '--reviewers', 'c@d.com'],
+                          ['a@b.com', 'c@d.com'])
+        # If --no-reviewers flag is passed, we should always get an empty list
+        testOneSetOfFlags(['--no-reviewers'], [])
+        testOneSetOfFlags(['--reviewers', 'a@b.com', '--no-reviewers'], [])
 
 if __name__ == '__main__':
     logging.basicConfig(

tests/split_cl_test.py

@@ -295,7 +295,8 @@ class SplitClTest(unittest.TestCase):
                 m.reset_mock()
 
         def DoSplitCl(self, description_file, dry_run, summarize,
-                      files_split_by_reviewers, proceed_response):
+                      reviewers_override, 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
@@ -306,8 +307,8 @@ class SplitClTest(unittest.TestCase):
             self.mock_ask_for_data.return_value = proceed_response
 
             split_cl.SplitCl(description_file, None, mock.Mock(), mock.Mock(),
-                             dry_run, summarize, False, False, None, None, None,
-                             None)
+                             dry_run, summarize, reviewers_override, False,
+                             False, None, None, None, None)
 
     # Save for re-use
     files_split_by_reviewers = {
@@ -326,7 +327,7 @@ class SplitClTest(unittest.TestCase):
         split_cl_tester = self.SplitClTester(self)
 
         # Should prompt for confirmation and upload several times
-        split_cl_tester.DoSplitCl("SomeFile.txt", False, False,
+        split_cl_tester.DoSplitCl("SomeFile.txt", False, False, None,
                                   self.files_split_by_reviewers, "y")
 
         split_cl_tester.mock_ask_for_data.assert_called_once()
@@ -336,7 +337,7 @@ class SplitClTest(unittest.TestCase):
 
         split_cl_tester.ResetMocks()
         # Should prompt for confirmation and not upload
-        split_cl_tester.DoSplitCl("SomeFile.txt", False, False,
+        split_cl_tester.DoSplitCl("SomeFile.txt", False, False, None,
                                   self.files_split_by_reviewers, "f")
 
         split_cl_tester.mock_ask_for_data.assert_called_once()
@@ -346,7 +347,7 @@ class SplitClTest(unittest.TestCase):
         split_cl_tester.ResetMocks()
 
         # Dry runs: Don't prompt, print info instead of uploading
-        split_cl_tester.DoSplitCl("SomeFile.txt", True, False,
+        split_cl_tester.DoSplitCl("SomeFile.txt", True, False, None,
                                   self.files_split_by_reviewers, "f")
 
         split_cl_tester.mock_ask_for_data.assert_not_called()
@@ -357,7 +358,7 @@ class SplitClTest(unittest.TestCase):
 
         split_cl_tester.ResetMocks()
         # Summarize is true: Don't prompt, emit a summary
-        split_cl_tester.DoSplitCl("SomeFile.txt", True, True,
+        split_cl_tester.DoSplitCl("SomeFile.txt", True, True, None,
                                   self.files_split_by_reviewers, "f")
 
         split_cl_tester.mock_ask_for_data.assert_not_called()
@@ -365,6 +366,23 @@ class SplitClTest(unittest.TestCase):
         split_cl_tester.mock_print_summary.assert_called_once()
         split_cl_tester.mock_upload_cl.assert_not_called()
 
+    def testReviewerOverride(self):
+        split_cl_tester = self.SplitClTester(self)
+
+        def testOneOverride(reviewers_lst):
+            split_cl_tester.DoSplitCl("SomeFile.txt", False, False,
+                                      reviewers_lst,
+                                      self.files_split_by_reviewers, "y")
+
+            for call in split_cl_tester.mock_upload_cl.call_args_list:
+                self.assertEqual(call.args[7], set(reviewers_lst))
+
+            split_cl_tester.ResetMocks()
+
+        # The 'None' case gets ample testing everywhere else
+        testOneOverride([])
+        testOneOverride(['a@b.com', 'c@d.com'])
+
     def testValidateExistingBranches(self):
         """
         Make sure that we skip existing branches if they match what we intend
@@ -377,7 +395,7 @@ class SplitClTest(unittest.TestCase):
         split_cl_tester.mock_git_branches.return_value = [
             "branch0", "branch_to_upload"
         ]
-        split_cl_tester.DoSplitCl("SomeFile.txt", False, False,
+        split_cl_tester.DoSplitCl("SomeFile.txt", False, False, None,
                                   self.files_split_by_reviewers, "y")
         self.assertEqual(split_cl_tester.mock_upload_cl.call_count,
                          len(self.files_split_by_reviewers))
@@ -394,7 +412,7 @@ class SplitClTest(unittest.TestCase):
             "branch0", "branch_to_upload",
             "branch_to_upload_123456789_whatever_split"
         ]
-        split_cl_tester.DoSplitCl("SomeFile.txt", False, False,
+        split_cl_tester.DoSplitCl("SomeFile.txt", False, False, None,
                                   self.files_split_by_reviewers, "y")
         split_cl_tester.mock_upload_cl.assert_not_called()