From 24d9ad69facfe9f73b542e434f47881a5caa1db4 Mon Sep 17 00:00:00 2001 From: Devon Loehr Date: Fri, 10 Jan 2025 12:57:14 -0800 Subject: [PATCH] split_cl: Make description file optional during dry runs I frequently find myself running `git cl split` just to see how the files are going to be split up, without actually intending to submit the resulting CLs immediately. Doing so requires creating a file, putting a dummy description in it, and passing that to the command line, which is unnecessary work. This CL simply allows dry runs to use a dummy description if none is provided, to make checking the tool's output easier. Bug: None Change-Id: I47e06c6e6da26701e07dcae81ab605edac2e2ca6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6163904 Reviewed-by: Andy Perelson Commit-Queue: Devon Loehr Reviewed-by: Brian Ryner --- git_cl.py | 5 +++-- split_cl.py | 16 +++++++++++++--- tests/git_cl_test.py | 29 +++++++++++++++++++++++++++++ tests/split_cl_test.py | 11 +++++++++++ 4 files changed, 56 insertions(+), 5 deletions(-) diff --git a/git_cl.py b/git_cl.py index 0838d596b..d997c20dd 100755 --- a/git_cl.py +++ b/git_cl.py @@ -5679,7 +5679,8 @@ def CMDsplit(parser, args): '--description', dest='description_file', help='A text file containing a CL description in which ' - '$directory will be replaced by each CL\'s directory.') + '$directory will be replaced by each CL\'s directory. ' + 'Mandatory except in dry runs.') parser.add_option('-c', '--comment', dest='comment_file', @@ -5731,7 +5732,7 @@ def CMDsplit(parser, args): help='Topic to specify when uploading') options, _ = parser.parse_args(args) - if not options.description_file: + if not options.description_file and not options.dry_run: parser.error('No --description flag specified.') def WrappedCMDupload(args): diff --git a/split_cl.py b/split_cl.py index de52fd226..23ee5a71a 100644 --- a/split_cl.py +++ b/split_cl.py @@ -218,6 +218,14 @@ def PrintClInfo(cl_index, num_cls, directories, file_paths, description, print() +def LoadDescription(description_file): + if not description_file: + return ('Dummy description for dry run.\n' + 'directory = $directory') + + return gclient_utils.FileRead(description_file) + + 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. @@ -237,8 +245,10 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, Returns: 0 in case of success. 1 in case of error. """ - description = AddUploadedByGitClSplitToDescription( - gclient_utils.FileRead(description_file)) + + description = LoadDescription(description_file, dry_run) + description = AddUploadedByGitClSplitToDescription(description) + comment = gclient_utils.FileRead(comment_file) if comment_file else None try: @@ -262,7 +272,7 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, assert refactor_branch_upstream, \ "Branch %s must have an upstream." % refactor_branch - if not CheckDescriptionBugLink(description): + if not dry_run and not CheckDescriptionBugLink(description): return 0 files_split_by_reviewers = SelectReviewersForFiles( diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 635b2ce93..87d8e1e4d 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -175,6 +175,10 @@ class SystemExitMock(Exception): pass +class ParserErrorMock(Exception): + pass + + class TestGitClBasic(unittest.TestCase): def setUp(self): mock.patch('sys.exit', side_effect=SystemExitMock).start() @@ -5549,6 +5553,31 @@ Change-Id: I25699146b24c7ad8776f17775f489b9d41499595 expected_message) +@unittest.skipIf(gclient_utils.IsEnvCog(), + 'not supported in non-git environment') +class CMDSplitTestCase(CMDTestCaseBase): + + def setUp(self): + super(CMDTestCaseBase, self).setUp() + mock.patch('git_cl.Settings.GetRoot', return_value='root').start() + + @mock.patch("split_cl.SplitCl", return_value=0) + @mock.patch("git_cl.OptionParser.error", side_effect=ParserErrorMock) + def testDescriptionFlagRequired(self, _, mock_split_cl): + # --description-file is mandatory... + self.assertRaises(ParserErrorMock, git_cl.main, ['split']) + self.assertEqual(mock_split_cl.call_count, 0) + + self.assertEqual(git_cl.main(['split', '--description=SomeFile.txt']), + 0) + self.assertEqual(mock_split_cl.call_count, 1) + + # 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) + + if __name__ == '__main__': logging.basicConfig( level=logging.DEBUG if '-v' in sys.argv else logging.ERROR) diff --git a/tests/split_cl_test.py b/tests/split_cl_test.py index f81391b15..ab5cf9f46 100755 --- a/tests/split_cl_test.py +++ b/tests/split_cl_test.py @@ -193,6 +193,17 @@ class SplitClTest(unittest.TestCase): self.assertTrue(split_cl.CheckDescriptionBugLink("Description")) self.assertEqual(mock_ask_for_data.call_count, 1) + @mock.patch("gclient_utils.FileRead", return_value="Description") + def testLoadDescription(self, mock_file_read): + # No description provided, use the dummy: + self.assertTrue(split_cl.LoadDescription(None).startswith("Dummy")) + self.assertEqual(mock_file_read.call_count, 0) + + # Description file provided, load it + self.assertEqual(split_cl.LoadDescription("SomeFile.txt"), + "Description") + self.assertEqual(mock_file_read.call_count, 1) + if __name__ == '__main__': unittest.main()