From 0e60ecd30efa9a469b4bcc93006c253861a26164 Mon Sep 17 00:00:00 2001 From: Kevin Marshall Date: Wed, 4 Dec 2019 17:44:13 +0000 Subject: [PATCH] [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 Reviewed-by: Edward Lesmes Auto-Submit: Kevin Marshall --- git_cl.py | 28 ++++++++++++++++++++++++-- tests/git_cl_test.py | 48 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/git_cl.py b/git_cl.py index 505a3e87b7..d41f4831fe 100755 --- a/git_cl.py +++ b/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!') diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 9ed8a1fcad..150369cec8 100755 --- a/tests/git_cl_test.py +++ b/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)