diff --git a/tagbot/action/repo.py b/tagbot/action/repo.py index 81b9bf31..9c3fd2ec 100644 --- a/tagbot/action/repo.py +++ b/tagbot/action/repo.py @@ -234,6 +234,10 @@ def __init__( 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 + # Separate cache for subdirectory tree SHA → commit SHA mapping. + # Kept separate from __tree_to_commit_cache to allow fallback from + # subdir trees to root-level trees (for pre-subdir-restructure versions). + self.__subdir_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 @@ -563,22 +567,21 @@ def _commit_sha_from_registry_pr(self, version: str, tree: str) -> Optional[str] logger.info("Registry PR body did not match") return None commit = self._repo.get_commit(m[1]) + # The registered tree matches the root commit tree for regular packages, + # and for subdir packages whose version was registered before the package + # moved into the subdirectory. Check it first since the data is already + # in hand from the API. + if commit.commit.tree.sha == tree: + return cast(str, commit.sha) # Handle special case of tagging packages in a repo subdirectory, in which - # case the Julia package tree hash does not match the git commit tree hash + # case the Julia package tree hash matches the subdirectory tree hash + # rather than the git commit tree hash. if self.__subdir: - subdir_tree_hash = self._subdir_tree_hash(commit.sha, suppress_abort=False) - if subdir_tree_hash == tree: + subdir_tree_hash = self._subdir_tree_hash(commit.sha, suppress_abort=True) + if subdir_tree_hash and subdir_tree_hash == tree: return cast(str, commit.sha) - else: - msg = "Subdir tree SHA of commit from registry PR does not match" - logger.warning(msg) - return None - # Handle regular case (subdir is not set) - if commit.commit.tree.sha == tree: - return cast(str, commit.sha) - else: - logger.warning("Tree SHA of commit from registry PR does not match") - return None + logger.warning("Tree SHA of commit from registry PR does not match") + return None def _build_tree_to_commit_cache(self) -> Dict[str, str]: """Build a cache mapping tree SHAs to commit SHAs. @@ -608,6 +611,27 @@ def _build_tree_to_commit_cache(self) -> Dict[str, str]: self.__tree_to_commit_cache = cache return cache + def _build_subdir_tree_to_commit_cache(self) -> Dict[str, str]: + """Build a cache mapping subdirectory tree SHAs to commit SHAs. + + Derived from the full tree→commit cache: commits sharing a root tree + have identical content and therefore identical subdirectory trees, so + one representative commit per root tree covers every distinct subdir + tree without a second pass over the history. + """ + if self.__subdir_tree_to_commit_cache is not None: + return self.__subdir_tree_to_commit_cache + + logger.debug("Building subdir tree→commit cache") + subdir_cache: Dict[str, str] = {} + for commit_sha in self._build_tree_to_commit_cache().values(): + subdir_tree_hash = self._subdir_tree_hash(commit_sha, suppress_abort=True) + if subdir_tree_hash and subdir_tree_hash not in subdir_cache: + subdir_cache[subdir_tree_hash] = commit_sha + logger.debug(f"Subdir tree→commit cache built with {len(subdir_cache)} entries") + self.__subdir_tree_to_commit_cache = subdir_cache + return subdir_cache + def _commit_sha_of_tree(self, tree: str) -> Optional[str]: """Look up the commit SHA of a tree with the given SHA.""" # Fast path: use pre-built tree→commit cache (built from git log) @@ -619,21 +643,23 @@ def _commit_sha_of_tree(self, tree: str) -> Optional[str]: # Tree not found in any commit return None - # 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) + # For subdirectories, check the subdirectory tree hash. + subdir_cache = self._build_subdir_tree_to_commit_cache() + full_cache = self._build_tree_to_commit_cache() + if tree in subdir_cache: + if tree in full_cache: + # Ambiguous: the tree is both the subdirectory tree of + # post-move commits and the root tree of a pre-move commit + # (a content-preserving move into the subdirectory). Either + # cache may point at the wrong commit, so return None and let + # the caller fall back to the registry PR, which pins the + # exact registered commit. + logger.debug(f"Tree {tree} matches both subdir and root trees") + return None + return subdir_cache[tree] + # Fallback: trees registered before the subdir restructure are root + # tree SHAs. + return full_cache.get(tree) def _subdir_tree_hash( self, commit_sha: str, *, suppress_abort: bool @@ -909,6 +935,19 @@ def _filter_map_versions(self, versions: Dict[str, str]) -> Dict[str, str]: f"Skipping {version}: no matching tree or registry PR found" ) continue + + # Versions registered before the package moved into a subdirectory + # may already be tagged under the un-prefixed name (e.g. "v1.0.0" + # from when the package lived at the repo root). Don't create a + # duplicate prefixed tag for the same version on the same commit. + if version_tag != version and self._commit_sha_of_tag(version) == expected: + logger.info( + f"Skipping {version}: commit {expected} is already tagged " + f"as {version}" + ) + skipped_existing += 1 + continue + valid[version] = expected if skipped_existing > 0: diff --git a/test/action/test_repo.py b/test/action/test_repo.py index 4b84c128..c2d229a1 100644 --- a/test/action/test_repo.py +++ b/test/action/test_repo.py @@ -534,6 +534,69 @@ def test_commit_sha_from_registry_pr(logger): assert r._commit_sha_from_registry_pr("v4.5.6", "def") == "sha" +def test_commit_sha_from_registry_pr_subdir_matches_subdir_hash(): + """With subdir set, the subdir tree hash is matched first.""" + r = _repo(subdir="path/to/Foo.jl") + r._registry_pr = Mock(return_value=Mock(body=f"- Commit: {'a' * 32}")) + r._repo.get_commit = Mock() + r._repo.get_commit.return_value.sha = "sha" + r._repo.get_commit.return_value.commit.tree.sha = "root_tree" + # The subdir tree hash matches the expected tree + with patch.object(r, "_subdir_tree_hash", return_value="foo_tree"): + assert r._commit_sha_from_registry_pr("v1.0.0", "foo_tree") == "sha" + r._subdir_tree_hash.assert_called_with("sha", suppress_abort=True) + + +def test_commit_sha_from_registry_pr_subdir_falls_back_to_root_tree(): + """When subdir tree hash doesn't match, fall back to root commit tree SHA. + + This handles versions registered *before* the package was moved into the + subdirectory — the registry stores the root tree SHA at that point. + """ + r = _repo(subdir="path/to/Foo.jl") + r._registry_pr = Mock(return_value=Mock(body=f"- Commit: {'a' * 32}")) + r._repo.get_commit = Mock() + r._repo.get_commit.return_value.sha = "sha" + r._repo.get_commit.return_value.commit.tree.sha = "root_tree" + # Subdir tree hash does NOT match, but root tree does + with patch.object(r, "_subdir_tree_hash", return_value="wrong_hash"): + assert r._commit_sha_from_registry_pr("v1.0.0", "root_tree") == "sha" + + +@patch("tagbot.action.repo.logger") +def test_commit_sha_from_registry_pr_subdir_neither_matches(logger): + """When neither subdir nor root tree hash matches, return None.""" + r = _repo(subdir="path/to/Foo.jl") + r._registry_pr = Mock(return_value=Mock(body=f"- Commit: {'a' * 32}")) + r._repo.get_commit = Mock() + r._repo.get_commit.return_value.sha = "sha" + r._repo.get_commit.return_value.commit.tree.sha = "root_tree" + with patch.object(r, "_subdir_tree_hash", return_value="wrong_hash"): + assert r._commit_sha_from_registry_pr("v1.0.0", "neither_match") is None + logger.warning.assert_called_with( + "Tree SHA of commit from registry PR does not match" + ) + + +def test_commit_sha_from_registry_pr_subdir_no_crash_on_missing_subdir(): + """When the subdir doesn't exist in the commit (e.g. pre-move), + suppress_abort=True prevents an Abort crash and the lookup returns None. + + Pre-move versions whose root tree matches never reach the subdir check: + the root tree is compared first.""" + r = _repo(subdir="path/to/Foo.jl") + r._registry_pr = Mock(return_value=Mock(body=f"- Commit: {'a' * 32}")) + r._repo.get_commit = Mock() + r._repo.get_commit.return_value.sha = "sha" + r._repo.get_commit.return_value.commit.tree.sha = "root_tree" + # Root tree matches: resolved without consulting the subdir at all + with patch.object(r, "_subdir_tree_hash", return_value=None): + assert r._commit_sha_from_registry_pr("v1.0.0", "root_tree") == "sha" + r._subdir_tree_hash.assert_not_called() + # Root tree mismatch + missing subdir (suppressed Abort): returns None + assert r._commit_sha_from_registry_pr("v1.0.0", "no_match") is None + + @patch("tagbot.action.repo.logger") def test_branch_from_registry_pr(logger): """Test extracting branch from registry PR body.""" @@ -585,14 +648,14 @@ def test_commit_sha_of_tree(): def test_commit_sha_of_tree_subdir_fallback(): """Test subdirectory tree→commit cache.""" r = _repo(subdir="path/to/package") - # git log returns commit SHAs - r._git.command = Mock(return_value="abc123\ndef456\nghi789") + # One git log pass provides commit and root tree SHAs for both caches + r._git.command = Mock(return_value="abc123 t1\ndef456 t2\nghi789 t3") # _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" - r._git.command.assert_called_once_with("log", "--all", "--format=%H") + r._git.command.assert_called_once_with("log", "--all", "--format=%H %T") # 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 @@ -601,13 +664,74 @@ def test_commit_sha_of_tree_subdir_fallback(): def test_commit_sha_of_tree_subdir_fallback_no_match(): """Test subdirectory cache returns None when no match found.""" r = _repo(subdir="path/to/package") - r._git.command = Mock(return_value="abc123\ndef456") + r._git.command = Mock(return_value="abc123 t1\ndef456 t2") # 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 +def test_commit_sha_of_tree_subdir_falls_back_to_full_cache(): + """When subdir cache misses, fall back to full tree→commit cache. + + This handles versions registered *before* the package was moved into + a subdirectory — those registrations store the root tree SHA, not the + subdirectory tree SHA. + """ + r = _repo(subdir="path/to/package") + r._git.command = Mock( + return_value="abc123 root_tree_1\ndef456 root_tree_2\nghi789 root_tree_3" + ) + with patch.object(r, "_subdir_tree_hash", return_value="dir_tree"): + # Post-move version resolves via the subdir cache + assert r._commit_sha_of_tree("dir_tree") == "abc123" + # Pre-subdir version registered at root level resolves via the + # full cache, with no additional git log pass + assert r._commit_sha_of_tree("root_tree_2") == "def456" + assert r._git.command.call_count == 1 + + +def test_commit_sha_of_tree_subdir_ambiguous_defers_to_registry_pr(): + """A tree that is both a subdir tree and a root tree (content-preserving + move into the subdirectory) is ambiguous: either cache may point at the + wrong commit, so return None and let the caller use the registry PR, + which pins the exact registered commit.""" + r = _repo(subdir="path/to/package") + # shared_tree is the root tree of abc123 AND the subdir tree of def456 + r._git.command = Mock(return_value="def456 t_move\nabc123 shared_tree") + with patch.object(r, "_subdir_tree_hash", side_effect=["shared_tree", None]): + assert r._commit_sha_of_tree("shared_tree") is None + + +def test_commit_sha_of_tree_subdir_caches_are_separate(): + """The subdir tree cache and the full tree cache are stored separately + and do not interfere. Building one does not invalidate the other.""" + r = _repo(subdir="path/to/package") + + # Build the full cache via _build_tree_to_commit_cache first + r._git.command = Mock(return_value="abc123 root_tree\ndef456 other_tree") + full_cache = r._build_tree_to_commit_cache() + assert full_cache == {"root_tree": "abc123", "other_tree": "def456"} + assert r._Repo__tree_to_commit_cache is not None + # The subdir cache should still be None (not yet built) + assert r._Repo__subdir_tree_to_commit_cache is None + + # Now build the subdir cache via _commit_sha_of_tree; it derives from + # the already-built full cache without another git log pass + with patch.object(r, "_subdir_tree_hash", side_effect=["dir_a", "dir_b"]): + assert r._commit_sha_of_tree("dir_b") == "def456" + assert r._git.command.call_count == 1 + # Both caches exist independently + assert r._Repo__subdir_tree_to_commit_cache == { + "dir_a": "abc123", + "dir_b": "def456", + } + assert r._Repo__tree_to_commit_cache == { + "root_tree": "abc123", + "other_tree": "def456", + } + + def test_commit_sha_of_tag(): r = _repo() # Mock get_git_matching_refs to return tags (used by _build_tags_cache) @@ -800,6 +924,53 @@ def test_filter_map_versions(logger): assert r._filter_map_versions({"5.6.7": "tree5"}) == {"v5.6.7": "pr_sha"} +def test_filter_map_versions_subdir(): + """_filter_map_versions with subdir resolves pre- and post-move versions.""" + r = _repo(subdir="path/to/SubPkgA") + r._build_tags_cache = Mock(return_value={}) + r._commit_sha_from_registry_pr = Mock(return_value=None) + r._commit_sha_of_tree = Mock(return_value=None) + r._project = Mock(return_value="SubPkgA") + + # Both pre-subdir (root tree) and post-subdir (subdir tree) lookups fail + assert not r._filter_map_versions({"1.0.0": "root_tree"}) + r._commit_sha_of_tree.assert_called_with("root_tree") + r._commit_sha_from_registry_pr.assert_called_with("v1.0.0", "root_tree") + + # Post-subdir version found via tree resolver + r._commit_sha_of_tree.return_value = "subdir_sha" + r._commit_sha_from_registry_pr.reset_mock() + assert r._filter_map_versions({"2.0.0": "subdir_tree"}) == {"v2.0.0": "subdir_sha"} + r._commit_sha_from_registry_pr.assert_not_called() + + # Tag exists - skip + r._build_tags_cache.return_value = {"SubPkgA-v3.0.0": "existing"} + assert not r._filter_map_versions({"3.0.0": "tree3"}) + + # Fallback to registry PR works when tree not found + r._build_tags_cache.return_value = {} + r._commit_sha_of_tree.return_value = None + r._commit_sha_from_registry_pr.return_value = "pr_sha" + assert r._filter_map_versions({"5.0.0": "tree5"}) == {"v5.0.0": "pr_sha"} + + +def test_filter_map_versions_subdir_skips_versions_tagged_unprefixed(): + """A pre-subdir-move version already tagged under the un-prefixed name + (from when the package lived at the repo root) is not re-tagged with + the subdir prefix on the same commit.""" + r = _repo(subdir="path/to/SubPkgA") + r._project = Mock(return_value="SubPkgA") + r._commit_sha_of_tree = Mock(return_value="pre_move_sha") + + # Commit already tagged as v1.0.0 → no duplicate SubPkgA-v1.0.0 + r._build_tags_cache = Mock(return_value={"v1.0.0": "pre_move_sha"}) + assert not r._filter_map_versions({"1.0.0": "root_tree"}) + + # Un-prefixed tag exists but points at a different commit → still tagged + r._build_tags_cache.return_value = {"v1.0.0": "unrelated_sha"} + assert r._filter_map_versions({"1.0.0": "root_tree"}) == {"v1.0.0": "pre_move_sha"} + + @patch("tagbot.action.repo.logger") def test_versions(logger): r = _repo()