[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 <dloehr@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>

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,
+             description, saved_splitting_file, comment, reviewers, changelist,
-             cq_dry_run, enable_auto_submit, topic, repository_root):
+             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() +
+        print(f'Delete {git.current_branch()} then run\n'
-              ' then run git cl split again to resume uploading.')
+              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.directories, cl_info.files, description,
-                     cl_info.reviewers, changelist, cmd_upload, cq_dry_run,
+                     saved_splitting_file, comment, cl_info.reviewers,
-                     enable_auto_submit, topic, repository_root)
+                     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

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,
+                              directories, files, "description",
-                              reviewers, mock.Mock(), cmd_upload, True, True,
+                              "splitting_file.txt", None, reviewers,
-                              "topic", os.path.sep)
+                              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)
 
-    def testSplitClConfirm(self):
+    # Save for re-use
-        split_cl_tester = self.SplitClTester(self)
-
     files_split_by_reviewers = {
-            "a@example.com":
+        ("a@example.com", ):
         split_cl.FilesAndOwnersDirectory([
             ("M", "a/b/foo.cc"),
             ("M", "d/e/bar.h"),
         ], []),
-            "b@example.com":
+        ("b@example.com", ):
         split_cl.FilesAndOwnersDirectory([
             ("A", "f/g/baz.py"),
         ], [])
     }
 
+    def testSplitClConfirm(self):
+        split_cl_tester = self.SplitClTester(self)
+
         # 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"],