Fix bug: Fetch if ancestry check fails

diff --git a/Changelog b/Changelog
index 3246263874844312a79dbe233d5988fc70932021..77b3265866879edc13e1961c9cfd2b36da31982b 100644 (file)
--- 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 aab5b070fdb15509f4fd74cd182db5ecfc541280..48430033e7afa805d307cf208b46c52a8e23bd0f 100644 (file)
--- 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 64151fe75b45766af8efa2a1bb9cce742907fbaa..caac6a33289432744717167dc5f0da9c7f397da1 100644 (file)
--- 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)