From 6da17751ca4e3b90834ca763f448ddc39b32651b Mon Sep 17 00:00:00 2001 From: Mike Frysinger Date: Wed, 11 Sep 2019 18:43:17 -0400 Subject: [PATCH] prune: handle branches that track missing branches Series of steps: * Create a local "b1" branch with `repo start b1` that tracks a remote branch (totally fine) * Manually create a local "b2" branch with `git branch --track b1 b2` that tracks the local "b1" (uh-oh...) * Delete the local "b1" branch manually or via `repo prune` (....) * Try to process the "b2" branch with `repo prune` Since b2 tracks a branch that no longer exists, everything blows up at this point as we try to probe the non-existent ref. Instead, we should flag this as unknown and leave it up to the user to resolve. This probably could come up if a local branch was tracking a remote branch that was deleted from the server, and users ran something like `repo sync --prune` which cleaned up the remote refs. Bug: https://crbug.com/gerrit/11485 Change-Id: I6b6b6041943944b8efa6e2ad0b8b10f13a75a5c2 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/236793 Reviewed-by: David Pursehouse Reviewed-by: Kirtika Ruchandani Reviewed-by: Mike Frysinger Tested-by: Mike Frysinger --- project.py | 39 ++++++++++++++++++----- subcmds/prune.py | 13 +++++--- tests/test_project.py | 74 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 12 deletions(-) diff --git a/project.py b/project.py index 5ffa542..7811d6b 100755 --- a/project.py +++ b/project.py @@ -134,6 +134,7 @@ class DownloadedChange(object): class ReviewableBranch(object): _commit_cache = None + _base_exists = None def __init__(self, project, branch, base): self.project = project @@ -147,14 +148,19 @@ class ReviewableBranch(object): @property def commits(self): if self._commit_cache is None: - self._commit_cache = self.project.bare_git.rev_list('--abbrev=8', - '--abbrev-commit', - '--pretty=oneline', - '--reverse', - '--date-order', - not_rev(self.base), - R_HEADS + self.name, - '--') + args = ('--abbrev=8', '--abbrev-commit', '--pretty=oneline', '--reverse', + '--date-order', not_rev(self.base), R_HEADS + self.name, '--') + try: + self._commit_cache = self.project.bare_git.rev_list(*args) + except GitError: + # We weren't able to probe the commits for this branch. Was it tracking + # a branch that no longer exists? If so, return no commits. Otherwise, + # rethrow the error as we don't know what's going on. + if self.base_exists: + raise + + self._commit_cache = [] + return self._commit_cache @property @@ -173,6 +179,23 @@ class ReviewableBranch(object): R_HEADS + self.name, '--') + @property + def base_exists(self): + """Whether the branch we're tracking exists. + + Normally it should, but sometimes branches we track can get deleted. + """ + if self._base_exists is None: + try: + self.project.bare_git.rev_parse('--verify', not_rev(self.base)) + # If we're still here, the base branch exists. + self._base_exists = True + except GitError: + # If we failed to verify, the base branch doesn't exist. + self._base_exists = False + + return self._base_exists + def UploadForReview(self, people, auto_topic=False, draft=False, diff --git a/subcmds/prune.py b/subcmds/prune.py index dc8b8c9..ff2fba1 100644 --- a/subcmds/prune.py +++ b/subcmds/prune.py @@ -51,11 +51,16 @@ class Prune(PagedCommand): out.project('project %s/' % project.relpath) out.nl() - commits = branch.commits - date = branch.date - print('%s %-33s (%2d commit%s, %s)' % ( + print('%s %-33s ' % ( branch.name == project.CurrentBranch and '*' or ' ', - branch.name, + branch.name), end='') + + if not branch.base_exists: + print('(ignoring: tracking branch is gone: %s)' % (branch.base,)) + else: + commits = branch.commits + date = branch.date + print('(%2d commit%s, %s)' % ( len(commits), len(commits) != 1 and 's' or ' ', date)) diff --git a/tests/test_project.py b/tests/test_project.py index 9b289e1..77126df 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -18,11 +18,30 @@ from __future__ import print_function +import contextlib +import os +import shutil +import subprocess +import tempfile import unittest +import git_config import project +@contextlib.contextmanager +def TempGitTree(): + """Create a new empty git checkout for testing.""" + # TODO(vapier): Convert this to tempfile.TemporaryDirectory once we drop + # Python 2 support entirely. + try: + tempdir = tempfile.mkdtemp(prefix='repo-tests') + subprocess.check_call(['git', 'init'], cwd=tempdir) + yield tempdir + finally: + shutil.rmtree(tempdir) + + class RepoHookShebang(unittest.TestCase): """Check shebang parsing in RepoHook.""" @@ -60,3 +79,58 @@ class RepoHookShebang(unittest.TestCase): for shebang, interp in DATA: self.assertEqual(project.RepoHook._ExtractInterpFromShebang(shebang), interp) + + +class FakeProject(object): + """A fake for Project for basic functionality.""" + + def __init__(self, worktree): + self.worktree = worktree + self.gitdir = os.path.join(worktree, '.git') + self.name = 'fakeproject' + self.work_git = project.Project._GitGetByExec( + self, bare=False, gitdir=self.gitdir) + self.bare_git = project.Project._GitGetByExec( + self, bare=True, gitdir=self.gitdir) + self.config = git_config.GitConfig.ForRepository(gitdir=self.gitdir) + + +class ReviewableBranchTests(unittest.TestCase): + """Check ReviewableBranch behavior.""" + + def test_smoke(self): + """A quick run through everything.""" + with TempGitTree() as tempdir: + fakeproj = FakeProject(tempdir) + + # Generate some commits. + with open(os.path.join(tempdir, 'readme'), 'w') as fp: + fp.write('txt') + fakeproj.work_git.add('readme') + fakeproj.work_git.commit('-mAdd file') + fakeproj.work_git.checkout('-b', 'work') + fakeproj.work_git.rm('-f', 'readme') + fakeproj.work_git.commit('-mDel file') + + # Start off with the normal details. + rb = project.ReviewableBranch( + fakeproj, fakeproj.config.GetBranch('work'), 'master') + self.assertEqual('work', rb.name) + self.assertEqual(1, len(rb.commits)) + self.assertIn('Del file', rb.commits[0]) + d = rb.unabbrev_commits + self.assertEqual(1, len(d)) + short, long = next(iter(d.items())) + self.assertTrue(long.startswith(short)) + self.assertTrue(rb.base_exists) + # Hard to assert anything useful about this. + self.assertTrue(rb.date) + + # Now delete the tracking branch! + fakeproj.work_git.branch('-D', 'master') + rb = project.ReviewableBranch( + fakeproj, fakeproj.config.GetBranch('work'), 'master') + self.assertEqual(0, len(rb.commits)) + self.assertFalse(rb.base_exists) + # Hard to assert anything useful about this. + self.assertTrue(rb.date)