diff --git a/split_cl.py b/split_cl.py index 24e07d991..d7ca2457b 100644 --- a/split_cl.py +++ b/split_cl.py @@ -129,14 +129,42 @@ def CreateBranchForOneCL(prefix: str, files: List[Tuple[str, str]], Return false if the branch already exists. |upstream| is used as upstream for the created branch. """ - existing_branches = set(git.branches(use_limit=False)) + branches_on_disk = set(git.branches(use_limit=False)) branch_name = CreateBranchName(prefix, files) - if branch_name in existing_branches: + if branch_name in branches_on_disk: return False git.run('checkout', '-t', upstream, '-b', branch_name) return True +def ValidateExistingBranches(prefix: str, cl_infos: List[CLInfo]) -> bool: + """ + Check if there are splitting branches left over from a previous run. + We only allow branches to exist if we're resuming a previous upload, + in which case we require that the existing branches are a subset of + the branches we're going to generate. + """ + branches_on_disk = set( + branch for branch in git.branches(use_limit=False) + if branch.startswith(prefix + "_") and branch.endswith("_split")) + + branches_to_be_made = set( + CreateBranchName(prefix, info.files) for info in cl_infos) + + if not branches_on_disk.issubset(branches_to_be_made): + print( + "It seems like you've already run `git cl split` on this branch.\n" + "If you're resuming a previous upload, you must pass in the " + "same splitting as before, using the --from-file option.\n" + "If you're starting a new upload, please clean up existing split " + f"branches (starting with '{prefix}_' and ending with '_split'), " + "and re-run the tool.") + print("The following branches need to be cleaned up:\n") + for branch in branches_on_disk - branches_to_be_made: + print(branch) + return False + return True + def FormatDirectoriesForPrinting(directories, prefix=None): """Formats directory list for printing @@ -172,8 +200,9 @@ def AddUploadedByGitClSplitToDescription(description): def UploadCl(refactor_branch, refactor_branch_upstream, directories, files, - description, comment, reviewers, changelist, cmd_upload, - cq_dry_run, enable_auto_submit, topic, repository_root): + description, saved_splitting_file, comment, reviewers, changelist, + cmd_upload, cq_dry_run, enable_auto_submit, topic, + repository_root): """Uploads a CL with all changes to |files| in |refactor_branch|. Args: @@ -240,8 +269,9 @@ def UploadCl(refactor_branch, refactor_branch_upstream, directories, files, if ret != 0: print('Uploading failed.') print('Note: git cl split has built-in resume capabilities.') - print('Delete ' + git.current_branch() + - ' then run git cl split again to resume uploading.') + print(f'Delete {git.current_branch()} then run\n' + f'git cl split --from-file={saved_splitting_file}\n' + 'to resume uploading.') if comment: changelist().AddComment(FormatDescriptionOrComment( @@ -409,13 +439,17 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, answer = gclient_utils.AskForData( 'Proceed? (y/N, or i to edit interactively): ') if answer.lower() == 'i': - cl_infos = EditSplittingInteractively(cl_infos, files_on_disk=files) + cl_infos, saved_splitting_file = EditSplittingInteractively( + cl_infos, files_on_disk=files) else: # Save even if we're continuing, so the user can safely resume an # aborted upload with the same splitting - SaveSplittingToTempFile(cl_infos) + saved_splitting_file = SaveSplittingToTempFile(cl_infos) if answer.lower() != 'y': return 0 + # Make sure there isn't any clutter left over from a previous run + if not ValidateExistingBranches(refactor_branch, cl_infos): + return 0 cls_per_reviewer = collections.defaultdict(int) for cl_index, cl_info in enumerate(cl_infos, 1): @@ -427,9 +461,10 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, enable_auto_submit, topic) else: UploadCl(refactor_branch, refactor_branch_upstream, - cl_info.directories, cl_info.files, description, comment, - cl_info.reviewers, changelist, cmd_upload, cq_dry_run, - enable_auto_submit, topic, repository_root) + cl_info.directories, cl_info.files, description, + saved_splitting_file, comment, cl_info.reviewers, + changelist, cmd_upload, cq_dry_run, enable_auto_submit, + topic, repository_root) for reviewer in cl_info.reviewers: cls_per_reviewer[reviewer] += 1 @@ -715,4 +750,4 @@ def EditSplittingInteractively( # and edit it if there are any typos SaveSplittingToFile(cl_infos, tmp_file) ValidateSplitting(cl_infos, "the provided splitting", files_on_disk) - return cl_infos + return cl_infos, tmp_file diff --git a/tests/split_cl_test.py b/tests/split_cl_test.py index 3a265b1d2..6b9524f85 100755 --- a/tests/split_cl_test.py +++ b/tests/split_cl_test.py @@ -132,9 +132,10 @@ class SplitClTest(unittest.TestCase): def DoUploadCl(self, directories, files, reviewers, cmd_upload): split_cl.UploadCl("branch_to_upload", "upstream_branch", - directories, files, "description", None, - reviewers, mock.Mock(), cmd_upload, True, True, - "topic", os.path.sep) + directories, files, "description", + "splitting_file.txt", None, reviewers, + mock.Mock(), cmd_upload, True, True, "topic", + os.path.sep) def testUploadCl(self): """Tests commands run by UploadCl.""" @@ -245,7 +246,9 @@ class SplitClTest(unittest.TestCase): self.mock_git_current_branch = self.StartPatcher( "git_common.current_branch", test, - return_value="current_branch") + return_value="branch_to_upload") + self.mock_git_branches = self.StartPatcher("git_common.branches", + test) self.mock_git_upstream = self.StartPatcher( "git_common.upstream", test, return_value="upstream_branch") self.mock_get_reviewers = self.StartPatcher( @@ -285,34 +288,35 @@ class SplitClTest(unittest.TestCase): split_cl.SplitCl(description_file, None, mock.Mock(), mock.Mock(), dry_run, False, False, None, None, None, None) + # Save for re-use + 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"), + ], []) + } + 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") + self.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)) + len(self.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") + self.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() @@ -321,13 +325,47 @@ class SplitClTest(unittest.TestCase): 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") + self.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)) + len(self.files_split_by_reviewers)) split_cl_tester.mock_upload_cl.assert_not_called() + def testValidateExistingBranches(self): + """ + Make sure that we skip existing branches if they match what we intend + to do, and fail if there are existing branches that don't match. + """ + + split_cl_tester = self.SplitClTester(self) + + # If no split branches exist, we should call upload once per CL + split_cl_tester.mock_git_branches.return_value = [ + "branch0", "branch_to_upload" + ] + split_cl_tester.DoSplitCl("SomeFile.txt", False, + self.files_split_by_reviewers, "y") + self.assertEqual(split_cl_tester.mock_upload_cl.call_count, + len(self.files_split_by_reviewers)) + + # TODO(389069356): We should also ensure that if there are existing + # branches that match our current splitting, we skip them when uploading + # Unfortunately, we're not set up to test that, so this will have to + # wait until we've refactored SplitCl and UploadCL to be less + # monolithic + + # If a split branch with a bad name already exists, we should fail + split_cl_tester.mock_upload_cl.reset_mock() + split_cl_tester.mock_git_branches.return_value = [ + "branch0", "branch_to_upload", + "branch_to_upload_123456789_whatever_split" + ] + split_cl_tester.DoSplitCl("SomeFile.txt", False, + self.files_split_by_reviewers, "y") + split_cl_tester.mock_upload_cl.assert_not_called() + + # Tests related to saving to and loading from files # Sample CLInfos for testing CLInfo_1 = split_cl.CLInfo(reviewers=["a@example.com"],