diff --git a/git_cl.py b/git_cl.py index 28fcb5f16..c16fca686 100755 --- a/git_cl.py +++ b/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()) diff --git a/split_cl.py b/split_cl.py index 5dbc6378d..d77912a1d 100644 --- a/split_cl.py +++ b/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( diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 80bb25652..611bff259 100755 --- a/tests/git_cl_test.py +++ b/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( diff --git a/tests/split_cl_test.py b/tests/split_cl_test.py index 001d44b7e..24af2c744 100755 --- a/tests/split_cl_test.py +++ b/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()