From c994f00deea05fc327fb272f3bef8d626343ee72 Mon Sep 17 00:00:00 2001 From: Devon Loehr Date: Fri, 21 Feb 2025 12:18:45 -0800 Subject: [PATCH] [git cl split] Codify and enforce auto-resumption `git cl split` claims to have built-in auto-resume capabilities, but the nondeterminism in its splitting algorithm means they don't work in practice. Now that we have the ability to load splittings from files, we can ensure the user is using the same splitting as before, and resume safely. This CL does two things: - It checks that any existing split branches match the splitting we're about to upload, and complains if they don't match. This relies on the fact that our branch names are unique and deterministic. - It changes the auto-resume message to mention that --from-file is required, and includes the relevant filename. This requires tracking that filename from earlier in the program. Bug: 389069356 Change-Id: Ic1d8964e96193ca93e05a9a39e286b84ffb61b06 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6280953 Commit-Queue: Devon Loehr Reviewed-by: Josip Sokcevic --- split_cl.py | 59 ++++++++++++++++++++++++------- tests/split_cl_test.py | 80 +++++++++++++++++++++++++++++++----------- 2 files changed, 106 insertions(+), 33 deletions(-) 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"],