Fix slow tag checking and tree lookups for non-PR registries (#457)

Co-authored-by: Claude <noreply@anthropic.com>

Author: IanButterworth

tagbot/action/repo.py

@@ -209,6 +209,8 @@ def __init__(
         self.__commit_datetimes: Dict[str, datetime] = {}
         # Cache for existing tags to avoid per-version API calls
         self.__existing_tags_cache: Optional[Dict[str, str]] = None
+        # Cache for tree SHA → commit SHA mapping (for non-PR registries)
+        self.__tree_to_commit_cache: Optional[Dict[str, str]] = None
         # Track manual intervention issue URL for error reporting
         self._manual_intervention_issue_url: Optional[str] = None
 
@@ -515,51 +517,60 @@ def _commit_sha_from_registry_pr(self, version: str, tree: str) -> Optional[str]
             logger.warning("Tree SHA of commit from registry PR does not match")
             return None
 
-    def _commit_sha_of_tree_from_branch(
-        self, branch: str, tree: str, since: Optional[datetime] = None
-    ) -> Optional[str]:
-        """Look up the commit SHA of a tree with the given SHA on one branch."""
-        kwargs: Dict[str, object] = {"sha": branch}
-        if since is not None:
-            kwargs["since"] = since
-        for commit in self._repo.get_commits(**kwargs):
-            if self.__subdir:
-                subdir_tree_hash = self._subdir_tree_hash(
-                    commit.sha, suppress_abort=True
-                )
-                if subdir_tree_hash == tree:
-                    return cast(str, commit.sha)
-            else:
-                if commit.commit.tree.sha == tree:
-                    return cast(str, commit.sha)
-        return None
+    def _build_tree_to_commit_cache(self) -> Dict[str, str]:
+        """Build a cache mapping tree SHAs to commit SHAs.
+
+        Uses git log to get all commit:tree pairs in one command,
+        enabling O(1) lookups instead of iterating through commits.
+        """
+        if self.__tree_to_commit_cache is not None:
+            return self.__tree_to_commit_cache
+
+        logger.debug("Building tree→commit cache")
+        cache: Dict[str, str] = {}
+        try:
+            # Get all commit:tree pairs in one git command
+            output = self._git.command("log", "--all", "--format=%H %T")
+            for line in output.splitlines():
+                parts = line.split()
+                if len(parts) == 2:
+                    commit_sha, tree_sha = parts
+                    # Only keep first occurrence (most recent commit for that tree)
+                    if tree_sha not in cache:
+                        cache[tree_sha] = commit_sha
+            logger.debug(f"Tree→commit cache built with {len(cache)} entries")
+        except Exception as e:
+            logger.warning(f"Failed to build tree→commit cache: {e}")
+
+        self.__tree_to_commit_cache = cache
+        return cache
 
     def _commit_sha_of_tree(self, tree: str) -> Optional[str]:
         """Look up the commit SHA of a tree with the given SHA."""
-        sha = self._commit_sha_of_tree_from_branch(self._release_branch, tree)
-        if sha:
-            return sha
-        for branch in self._repo.get_branches():
-            if branch.name == self._release_branch:
-                continue
-            sha = self._commit_sha_of_tree_from_branch(branch.name, tree)
-            if sha:
-                return sha
-        # For a valid tree SHA, the only time that we reach here is when a release
-        # has been made long after the commit was made, which is reasonably rare.
-        # Fall back to cloning the repo in that case.
-        if self.__subdir:
-            # For subdirectories, we need to check the subdirectory tree hash.
-            # Limit to 10000 commits to avoid performance issues on large repos.
-            for line in self._git.command(
-                "log", "--all", "--format=%H", "-n", "10000"
-            ).splitlines():
-                subdir_tree_hash = self._subdir_tree_hash(line, suppress_abort=True)
-                if subdir_tree_hash == tree:
-                    return line
+        # Fast path: use pre-built tree→commit cache (built from git log)
+        # This is O(1) vs O(branches * commits) for the API-based approach
+        if not self.__subdir:
+            tree_cache = self._build_tree_to_commit_cache()
+            if tree in tree_cache:
+                return tree_cache[tree]
+            # Tree not found in any commit
             return None
-        else:
-            return self._git.commit_sha_of_tree(tree)
+
+        # For subdirectories, we need to check the subdirectory tree hash.
+        # Build a cache of subdir tree hashes from commits.
+        if self.__tree_to_commit_cache is None:
+            logger.debug("Building subdir tree→commit cache")
+            subdir_cache: Dict[str, str] = {}
+            for line in self._git.command("log", "--all", "--format=%H").splitlines():
+                subdir_tree_hash = self._subdir_tree_hash(line, suppress_abort=True)
+                if subdir_tree_hash and subdir_tree_hash not in subdir_cache:
+                    subdir_cache[subdir_tree_hash] = line
+            logger.debug(
+                f"Subdir tree→commit cache built with {len(subdir_cache)} entries"
+            )
+            self.__tree_to_commit_cache = subdir_cache
+
+        return self.__tree_to_commit_cache.get(tree)
 
     def _subdir_tree_hash(
         self, commit_sha: str, *, suppress_abort: bool
@@ -748,11 +759,9 @@ def version_with_latest_commit(self, versions: Dict[str, str]) -> Optional[str]:
 
     def _filter_map_versions(self, versions: Dict[str, str]) -> Dict[str, str]:
         """Filter out versions and convert tree SHA to commit SHA."""
-        # Pre-build caches to avoid repeated API calls
+        # Pre-build tags cache to check existing tags quickly
         self._build_tags_cache()
-        # Pre-build PR cache for versions that need commit SHA lookup
-        if not self._clone_registry:
-            self._build_registry_prs_cache()
+        # Note: PR cache is built lazily only when needed (first registry PR lookup)
 
         valid = {}
         skipped_existing = 0
@@ -761,22 +770,25 @@ def _filter_map_versions(self, versions: Dict[str, str]) -> Dict[str, str]:
             version_tag = self._get_version_tag(version)
 
             # Fast path: check if tag already exists using cached tags
-            existing_sha = self._commit_sha_of_tag(version_tag)
-            if existing_sha:
+            # Just check existence, don't resolve annotated tags (saves API calls)
+            tags_cache = self._build_tags_cache()
+            if version_tag in tags_cache:
                 # Tag exists - we skip without full validation for performance.
-                # Previously we'd verify the tag points to the expected commit,
-                # but that requires expensive PR/tree lookups for each version.
-                logger.info(f"Tag {version_tag} already exists")
                 skipped_existing += 1
                 continue
 
             # Tag doesn't exist - need to find expected commit SHA
+            # Try registry PR first (fast - uses cache)
             expected = self._commit_sha_from_registry_pr(version, tree)
             if not expected:
+                # Fall back to tree lookup (slower but more thorough)
+                logger.debug(
+                    f"No registry PR for {version}, falling back to tree lookup"
+                )
                 expected = self._commit_sha_of_tree(tree)
             if not expected:
-                logger.warning(
-                    f"No matching commit was found for version {version} ({tree})"
+                logger.debug(
+                    f"Skipping {version}: no registry PR or matching tree found"
                 )
                 continue
             valid[version] = expected

test/action/test_repo.py

@@ -475,102 +475,43 @@ def test_commit_sha_from_registry_pr(logger):
     assert r._commit_sha_from_registry_pr("v4.5.6", "def") == "sha"
 
 
-def test_commit_sha_of_tree_from_branch():
-    r = _repo()
-    r._repo.get_commits = Mock(return_value=[Mock(sha="abc"), Mock(sha="sha")])
-    r._repo.get_commits.return_value[1].commit.tree.sha = "tree"
-    assert r._commit_sha_of_tree_from_branch("master", "tree") == "sha"
-    r._repo.get_commits.assert_called_with(sha="master")
-    r._repo.get_commits.return_value.pop()
-    assert r._commit_sha_of_tree_from_branch("master", "tree") is None
-
-
-@patch("tagbot.action.repo.logger")
-def test_commit_sha_of_tree_from_branch_subdir(logger):
-    r = _repo(subdir="path/to/package")
-    commits = [Mock(sha="abc"), Mock(sha="sha")]
-    r._repo.get_commits = Mock(return_value=commits)
-    r._git.command = Mock(side_effect=["other", "tree_hash"])
-
-    assert r._commit_sha_of_tree_from_branch("master", "tree_hash") == "sha"
-
-    r._repo.get_commits.assert_called_with(sha="master")
-    r._git.command.assert_has_calls(
-        [
-            call("rev-parse", "abc:path/to/package"),
-            call("rev-parse", "sha:path/to/package"),
-        ]
-    )
-    logger.debug.assert_not_called()
-
-
-@patch("tagbot.action.repo.logger")
-def test_commit_sha_of_tree_from_branch_subdir_rev_parse_failure(logger):
-    r = _repo(subdir="path/to/package")
-    commits = [Mock(sha="abc"), Mock(sha="sha")]
-    r._repo.get_commits = Mock(return_value=commits)
-    r._git.command = Mock(side_effect=[Abort("missing"), "tree_hash"])
-
-    assert r._commit_sha_of_tree_from_branch("master", "tree_hash") == "sha"
-
-    r._repo.get_commits.assert_called_with(sha="master")
-    logger.debug.assert_called_with(
-        "rev-parse failed while inspecting %s", "abc:path/to/package"
-    )
-    r._git.command.assert_has_calls(
-        [
-            call("rev-parse", "abc:path/to/package"),
-            call("rev-parse", "sha:path/to/package"),
-        ]
-    )
-
-
 def test_commit_sha_of_tree():
+    """Test tree→commit lookup using git log cache."""
     r = _repo()
-    r._repo = Mock(default_branch="master")
-    branches = r._repo.get_branches.return_value = [Mock(), Mock()]
-    branches[0].name = "foo"
-    branches[1].name = "master"
-    r._commit_sha_of_tree_from_branch = Mock(side_effect=["sha1", None, "sha2"])
-    assert r._commit_sha_of_tree("tree") == "sha1"
-    r._repo.get_branches.assert_not_called()
-    r._commit_sha_of_tree_from_branch.assert_called_once_with("master", "tree")
-    assert r._commit_sha_of_tree("tree") == "sha2"
-    r._commit_sha_of_tree_from_branch.assert_called_with("foo", "tree")
-    r._commit_sha_of_tree_from_branch.side_effect = None
-    r._commit_sha_of_tree_from_branch.return_value = None
-    r._git.commit_sha_of_tree = Mock(side_effect=["sha", None])
-    assert r._commit_sha_of_tree("tree") == "sha"
-    assert r._commit_sha_of_tree("tree") is None
+    # Mock git command to return commit:tree pairs
+    r._git.command = Mock(return_value="sha1 tree1\nsha2 tree2\nsha3 tree3")
+    # First lookup builds cache and finds match
+    assert r._commit_sha_of_tree("tree1") == "sha1"
+    r._git.command.assert_called_once_with("log", "--all", "--format=%H %T")
+    # Second lookup uses cache (no additional git command)
+    assert r._commit_sha_of_tree("tree2") == "sha2"
+    assert r._git.command.call_count == 1  # Still just one call
+    # Non-existent tree returns None
+    assert r._commit_sha_of_tree("nonexistent") is None
 
 
 def test_commit_sha_of_tree_subdir_fallback():
-    """Test subdirectory fallback when branch lookups fail."""
+    """Test subdirectory tree→commit cache."""
     r = _repo(subdir="path/to/package")
-    r._repo = Mock(default_branch="master")
-    branches = r._repo.get_branches.return_value = [Mock()]
-    branches[0].name = "master"
-    # Branch lookups return None (fail)
-    r._commit_sha_of_tree_from_branch = Mock(return_value=None)
     # git log returns commit SHAs
     r._git.command = Mock(return_value="abc123\ndef456\nghi789")
-    # _subdir_tree_hash called via helper, simulate finding match on second commit
-    with patch.object(r, "_subdir_tree_hash", side_effect=[None, "tree_hash", "other"]):
+    # _subdir_tree_hash called for each commit, match on second
+    with patch.object(
+        r, "_subdir_tree_hash", side_effect=["other", "tree_hash", "another"]
+    ):
         assert r._commit_sha_of_tree("tree_hash") == "def456"
-        # Verify it iterated through commits
-        assert r._subdir_tree_hash.call_count == 2
+        r._git.command.assert_called_once_with("log", "--all", "--format=%H")
+        # Cache is built, so subsequent lookups don't call git again
+        assert r._commit_sha_of_tree("other") == "abc123"
+        assert r._git.command.call_count == 1
 
 
 def test_commit_sha_of_tree_subdir_fallback_no_match():
-    """Test subdirectory fallback returns None when no match found."""
+    """Test subdirectory cache returns None when no match found."""
     r = _repo(subdir="path/to/package")
-    r._repo = Mock(default_branch="master")
-    branches = r._repo.get_branches.return_value = [Mock()]
-    branches[0].name = "master"
-    r._commit_sha_of_tree_from_branch = Mock(return_value=None)
     r._git.command = Mock(return_value="abc123\ndef456")
-    # No matches found
-    with patch.object(r, "_subdir_tree_hash", return_value=None):
+    # No matching subdir tree hash
+    with patch.object(r, "_subdir_tree_hash", return_value="other_tree"):
         assert r._commit_sha_of_tree("tree_hash") is None
         assert r._subdir_tree_hash.call_count == 2
 
@@ -740,21 +681,28 @@ def test_filter_map_versions(logger):
     r = _repo()
     # Mock the caches to avoid real API calls
     r._build_tags_cache = Mock(return_value={})
-    r._build_registry_prs_cache = Mock(return_value={})
     r._commit_sha_from_registry_pr = Mock(return_value=None)
     r._commit_sha_of_tree = Mock(return_value=None)
+    # No registry PR or tree found - should skip
     assert not r._filter_map_versions({"1.2.3": "tree1"})
-    logger.warning.assert_called_with(
-        "No matching commit was found for version v1.2.3 (tree1)"
+    logger.debug.assert_called_with(
+        "Skipping v1.2.3: no registry PR or matching tree found"
     )
-    r._commit_sha_of_tree.return_value = "sha"
-    r._commit_sha_of_tag = Mock(return_value="sha")
-    # Tag exists - skip it (no validation of commit SHA for performance)
-    assert not r._filter_map_versions({"2.3.4": "tree2"})
-    logger.info.assert_called_with("Tag v2.3.4 already exists")
-    # Tag doesn't exist - should be included
-    r._commit_sha_of_tag.return_value = None
+    # Tree lookup fallback should be called when PR not found
+    r._commit_sha_of_tree.assert_called_with("tree1")
+    # Registry PR found - should include (no tree lookup needed)
+    r._commit_sha_from_registry_pr.return_value = "sha"
+    r._commit_sha_of_tree.reset_mock()
     assert r._filter_map_versions({"4.5.6": "tree4"}) == {"v4.5.6": "sha"}
+    r._commit_sha_of_tree.assert_not_called()
+    # Tag exists - skip it silently (no per-version logging for performance)
+    r._build_tags_cache.return_value = {"v2.3.4": "existing_sha"}
+    assert not r._filter_map_versions({"2.3.4": "tree2"})
+    # Tree fallback works when PR not found
+    r._build_tags_cache.return_value = {}
+    r._commit_sha_from_registry_pr.return_value = None
+    r._commit_sha_of_tree.return_value = "tree_sha"
+    assert r._filter_map_versions({"5.6.7": "tree5"}) == {"v5.6.7": "tree_sha"}
 
 
 @patch("tagbot.action.repo.logger")