From eb638847b92912d25b7de4a98418bb96f0d43eec Mon Sep 17 00:00:00 2001 From: Scott Worley Date: Mon, 7 Jun 2021 23:17:05 -0700 Subject: [PATCH] Fix bug: Fetch if ancestry check fails --- Changelog | 4 ++++ git_cache.py | 29 +++++++++++++---------------- test_git_cache.py | 11 +++++++++++ 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/Changelog b/Changelog index 3246263..77b3265 100644 --- a/Changelog +++ b/Changelog @@ -1,4 +1,8 @@ ## [Unreleased] +### Changed +- Fixed a bug in the is-fetch-needed logic: Sometimes a fetch is needed to +move the branch heads even if the sought rev is already present (eg: was +fetched via a different branch). ## [1.3.0] - 2021-04-19 diff --git a/git_cache.py b/git_cache.py index aab5b07..4843003 100644 --- a/git_cache.py +++ b/git_cache.py @@ -31,11 +31,17 @@ def git_cachedir(repo: Repo) -> Path: hashlib.sha256(repo.encode()).hexdigest())) -def verify_ancestry(repo: Repo, ref: Ref, rev: Rev) -> None: +def is_ancestor(repo: Repo, ref: Ref, rev: Rev) -> bool: cachedir = git_cachedir(repo) - logging.debug('Verifying rev %s is an ancestor of ref "%s"', rev, ref) - subprocess.run(['git', '-C', cachedir, 'merge-base', '--is-ancestor', - rev, ref], check=True) + logging.debug('Checking if rev %s is an ancestor of ref "%s"', rev, ref) + process = subprocess.run( + ['git', '-C', cachedir, 'merge-base', '--is-ancestor', rev, ref], check=False) + return process.returncode == 0 + + +def verify_ancestry(repo: Repo, ref: Ref, rev: Rev) -> None: + if not is_ancestor(repo, ref, rev): + raise Exception('Rev %s is not an ancestor of ref "%s"' % (rev, ref)) @backoff.on_exception( @@ -68,18 +74,8 @@ def fetch(repo: Repo, ref: Ref) -> Tuple[Path, Rev]: def ensure_rev_available(repo: Repo, ref: Ref, rev: Rev) -> Path: cachedir = git_cachedir(repo) - if os.path.exists(cachedir): - logging.debug('Checking if we already have rev %s', rev) - process = subprocess.run( - ['git', '-C', cachedir, 'cat-file', '-e', rev], check=False) - if process.returncode == 0: - logging.debug('We already have rev %s', rev) - verify_ancestry(repo, ref, rev) - return cachedir - if process.returncode != 1: - raise Exception( - 'Could not test for presence of rev %s. Is cache dir "%s" messed up?' % - (rev, cachedir)) + if os.path.exists(cachedir) and is_ancestor(repo, ref, rev): + return cachedir logging.debug( 'We do not have rev %s. We will fetch ref "%s" and hope it appears.', @@ -87,6 +83,7 @@ def ensure_rev_available(repo: Repo, ref: Ref, rev: Rev) -> Path: fetch(repo, ref) logging.debug('Verifying that fetch retrieved rev %s', rev) subprocess.run(['git', '-C', cachedir, 'cat-file', '-e', rev], check=True) + verify_ancestry(repo, ref, rev) return cachedir diff --git a/test_git_cache.py b/test_git_cache.py index 64151fe..caac6a3 100644 --- a/test_git_cache.py +++ b/test_git_cache.py @@ -159,6 +159,17 @@ class TestGitCache(unittest.TestCase): d = git_cache.ensure_rev_available(self.upstream, 'otherbranch', rev) self.assertEqual(_git(d, 'show', '%s:foofile' % rev), b'foo') + def test_catch_up(self) -> None: + _git(self.upstream, 'checkout', '-b', 'otherbranch') + _commit_file(self.upstream, 'foofile', 'foo', 'Foo') + rev = _git(self.upstream, 'log', '--format=%H', '-n1').strip().decode() + d = git_cache.ensure_rev_available(self.upstream, 'otherbranch', rev) + self.assertEqual(_git(d, 'show', '%s:foofile' % rev), b'foo') + _git(self.upstream, 'checkout', 'master') + _git(self.upstream, 'merge', '--ff-only', 'otherbranch') + d = git_cache.ensure_rev_available(self.upstream, 'master', rev) + self.assertEqual(_git(d, 'show', '%s:foofile' % rev), b'foo') + def test_fetch_after_cache_deleted(self) -> None: d1, rev1 = git_cache.fetch(self.upstream, 'master') shutil.rmtree(d1) -- 2.44.1