[git-cl] Add graceful error handling to "git cl archive".

Adds error handling logic for pre-existing tags (which can occur
if "archive" is CTRL-C aborted midway through) and for undeletable
branches (which can happen if they are currently checked out in a
working dir).

Change-Id: I27b6da9f5860c307f49cbeabb1b0ccf9cfb28eb6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1930023
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Auto-Submit: Kevin Marshall <kmarshall@chromium.org>

git_cl.py

@@ -3727,6 +3727,22 @@ def upload_branch_deps(cl, args):
   return 0
 
 
+def GetArchiveTagForBranch(issue_num, branch_name, existing_tags):
+  """Given a proposed tag name, returns a tag name that is guaranteed to be
+  unique. If 'foo' is proposed but already exists, then 'foo-2' is used,
+  or 'foo-3', and so on."""
+
+  proposed_tag = 'git-cl-archived-%s-%s' % (issue_num, branch_name)
+  for suffix_num in itertools.count(1):
+    if suffix_num == 1:
+      to_check = proposed_tag
+    else:
+      to_check = '%s-%d' % (proposed_tag, suffix_num)
+
+    if to_check not in existing_tags:
+      return to_check
+
+
 @metrics.collector.collect_metrics('git cl archive')
 def CMDarchive(parser, args):
   """Archives and deletes branches associated with closed changelists."""
@@ -3752,6 +3768,10 @@ def CMDarchive(parser, args):
   if not branches:
     return 0
 
+  tags = RunGit(['for-each-ref', '--format=%(refname)',
+                 'refs/tags']).splitlines() or []
+  tags = [t.split('/')[-1] for t in tags]
+
   print('Finding all branches associated with closed issues...')
   changes = [Changelist(branchref=b)
              for b in branches.splitlines()]
@@ -3760,7 +3780,8 @@ def CMDarchive(parser, args):
                              fine_grained=True,
                              max_processes=options.maxjobs)
   proposal = [(cl.GetBranch(),
-               'git-cl-archived-%s-%s' % (cl.GetIssue(), cl.GetBranch()))
+               GetArchiveTagForBranch(cl.GetIssue(), cl.GetBranch(),
+                                      tags))
               for cl, status in statuses
               if status in ('closed', 'rietveld-not-supported')]
   proposal.sort()
@@ -3800,7 +3821,10 @@ def CMDarchive(parser, args):
   for branch, tagname in proposal:
     if not options.notags:
       RunGit(['tag', tagname, branch])
-    RunGit(['branch', '-D', branch])
+
+    if RunGitWithCode(['branch', '-D', branch])[0] != 0:
+      # Clean up the tag if we failed to delete the branch.
+      RunGit(['tag', '-d', tagname])
 
   print('\nJob\'s done!')
 

tests/git_cl_test.py

@@ -2274,6 +2274,7 @@ class TestGitCl(TestCase):
     self.calls = [
       ((['git', 'for-each-ref', '--format=%(refname)', 'refs/heads'],),
        'refs/heads/master\nrefs/heads/foo\nrefs/heads/bar'),
+      ((['git', 'for-each-ref', '--format=%(refname)', 'refs/tags'],), ''),
       ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
       ((['git', 'tag', 'git-cl-archived-456-foo', 'foo'],), ''),
       ((['git', 'branch', '-D', 'foo'],), '')
@@ -2287,11 +2288,33 @@ class TestGitCl(TestCase):
 
     self.assertEqual(0, git_cl.main(['archive', '-f']))
 
+  def test_archive_tag_collision(self):
+    self.mock(git_cl.sys, 'stdout', StringIO())
+
+    self.calls = [
+      ((['git', 'for-each-ref', '--format=%(refname)', 'refs/heads'],),
+       'refs/heads/master\nrefs/heads/foo\nrefs/heads/bar'),
+      ((['git', 'for-each-ref', '--format=%(refname)', 'refs/tags'],),
+       'refs/tags/git-cl-archived-456-foo'),
+      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+      ((['git', 'tag', 'git-cl-archived-456-foo-2', 'foo'],), ''),
+      ((['git', 'branch', '-D', 'foo'],), '')
+    ]
+
+    self.mock(git_cl, 'get_cl_statuses',
+              lambda branches, fine_grained, max_processes:
+              [(MockChangelistWithBranchAndIssue('master', 1), 'open'),
+               (MockChangelistWithBranchAndIssue('foo', 456), 'closed'),
+               (MockChangelistWithBranchAndIssue('bar', 789), 'open')])
+
+    self.assertEqual(0, git_cl.main(['archive', '-f']))
+
   def test_archive_current_branch_fails(self):
     self.mock(git_cl.sys, 'stdout', StringIO())
     self.calls = [
       ((['git', 'for-each-ref', '--format=%(refname)', 'refs/heads'],),
          'refs/heads/master'),
+      ((['git', 'for-each-ref', '--format=%(refname)', 'refs/tags'],), ''),
       ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
     ]
 
@@ -2307,6 +2330,7 @@ class TestGitCl(TestCase):
     self.calls = [
       ((['git', 'for-each-ref', '--format=%(refname)', 'refs/heads'],),
          'refs/heads/master\nrefs/heads/foo\nrefs/heads/bar'),
+      ((['git', 'for-each-ref', '--format=%(refname)', 'refs/tags'],), ''),
       ((['git', 'symbolic-ref', 'HEAD'],), 'master')
     ]
 
@@ -2324,6 +2348,7 @@ class TestGitCl(TestCase):
     self.calls = [
       ((['git', 'for-each-ref', '--format=%(refname)', 'refs/heads'],),
          'refs/heads/master\nrefs/heads/foo\nrefs/heads/bar'),
+      ((['git', 'for-each-ref', '--format=%(refname)', 'refs/tags'],), ''),
       ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
       ((['git', 'branch', '-D', 'foo'],), '')
     ]
@@ -2336,6 +2361,29 @@ class TestGitCl(TestCase):
 
     self.assertEqual(0, git_cl.main(['archive', '-f', '--notags']))
 
+  def test_archive_tag_cleanup_on_branch_deletion_error(self):
+    self.mock(git_cl.sys, 'stdout', StringIO())
+
+    self.calls = [
+      ((['git', 'for-each-ref', '--format=%(refname)', 'refs/heads'],),
+         'refs/heads/master\nrefs/heads/foo\nrefs/heads/bar'),
+      ((['git', 'for-each-ref', '--format=%(refname)', 'refs/tags'],), ''),
+      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+      ((['git', 'tag', 'git-cl-archived-456-foo', 'foo'],),
+        'refs/tags/git-cl-archived-456-foo'),
+      ((['git', 'branch', '-D', 'foo'],), CERR1),
+      ((['git', 'tag', '-d', 'git-cl-archived-456-foo'],),
+       'refs/tags/git-cl-archived-456-foo'),
+    ]
+
+    self.mock(git_cl, 'get_cl_statuses',
+              lambda branches, fine_grained, max_processes:
+              [(MockChangelistWithBranchAndIssue('master', 1), 'open'),
+               (MockChangelistWithBranchAndIssue('foo', 456), 'closed'),
+               (MockChangelistWithBranchAndIssue('bar', 789), 'open')])
+
+    self.assertEqual(0, git_cl.main(['archive', '-f']))
+
   def test_cmd_issue_erase_existing(self):
     out = StringIO()
     self.mock(git_cl.sys, 'stdout', out)