From ae53af0953026a52b425da948c5c7f6ed70857c3 Mon Sep 17 00:00:00 2001 From: Arnav Kapoor Date: Sat, 4 Jul 2026 00:45:32 +0530 Subject: [PATCH 1/3] iinew fix --- tagbot/action/repo.py | 49 +++++++++---- test/action/test_repo.py | 152 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 188 insertions(+), 13 deletions(-) diff --git a/tagbot/action/repo.py b/tagbot/action/repo.py index edbfdc81..b0e160ce 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 @@ -564,15 +568,21 @@ def _commit_sha_from_registry_pr(self, version: str, tree: str) -> Optional[str] return None commit = self._repo.get_commit(m[1]) # 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 does not match the git commit tree hash. + # For versions registered after the subdir move, we compare the subdirectory + # tree hash. For versions registered before the move, we fall back to the + # root 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 + # Fallback: check if tree matches the root commit tree SHA + # (for versions registered before the subdir restructure) + if commit.commit.tree.sha == tree: + return cast(str, commit.sha) + msg = "Tree SHA of commit from registry PR does not match (subdir or root)" + logger.warning(msg) + return None # Handle regular case (subdir is not set) if commit.commit.tree.sha == tree: return cast(str, commit.sha) @@ -620,20 +630,33 @@ def _commit_sha_of_tree(self, tree: str) -> Optional[str]: 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: + # Build a cache of subdir tree hashes from commits, stored separately + # from the full-tree cache to avoid one overwriting the other. + if self.__subdir_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) + 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 + self.__subdir_tree_to_commit_cache = subdir_cache + + # Check the subdir cache first + result = self.__subdir_tree_to_commit_cache.get(tree) + if result: + return result - return self.__tree_to_commit_cache.get(tree) + # Fallback: check the full tree cache for trees registered before the + # subdir restructure (where the registry stored the root tree SHA). + full_cache = self._build_tree_to_commit_cache() + return full_cache.get(tree) def _subdir_tree_hash( self, commit_sha: str, *, suppress_abort: bool diff --git a/test/action/test_repo.py b/test/action/test_repo.py index 2323fc41..8f3de608 100644 --- a/test/action/test_repo.py +++ b/test/action/test_repo.py @@ -534,6 +534,65 @@ 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 (subdir or root)" + ) + + +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 we fall through to + the root tree check.""" + 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 returns None (suppressed Abort) when subdir doesn't exist + with patch.object(r, "_subdir_tree_hash", return_value=None): + # Falls through to root tree check and matches + assert r._commit_sha_from_registry_pr("v1.0.0", "root_tree") == "sha" + + @patch("tagbot.action.repo.logger") def test_branch_from_registry_pr(logger): """Test extracting branch from registry PR body.""" @@ -608,6 +667,67 @@ def test_commit_sha_of_tree_subdir_fallback_no_match(): 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") + # git log --format=%H returns commit SHAs for subdir cache + r._git.command = Mock(return_value="abc123\ndef456") + with patch.object(r, "_subdir_tree_hash", return_value="dir_tree"): + # Build subdir cache + assert r._commit_sha_of_tree("dir_tree") == "abc123" + # Lookup a tree that isn't in the subdir cache but would be in the + # full cache (simulates a pre-subdir version registered at root level). + # Change mock to return full git log format with tree SHAs. + r._git.command.return_value = ( + "abc123 root_tree_1\ndef456 root_tree_2\nghi789 root_tree_3" + ) + assert r._commit_sha_of_tree("root_tree_2") == "def456" + + +def test_commit_sha_of_tree_subdir_prefers_subdir_cache(): + """The subdir cache is checked first; if the tree exists there, the + full cache fallback is not consulted.""" + r = _repo(subdir="path/to/package") + r._git.command = Mock(return_value="abc123\ndef456") + with patch.object(r, "_subdir_tree_hash", side_effect=["sub_tree", "other"]): + # Build subdir cache and find match + assert r._commit_sha_of_tree("sub_tree") == "abc123" + # Even if the full cache would also match a different tree, + # subsequent lookups go through the subdir cache first + r._git.command.return_value = "abc123 root_tree\ndef456 other" + assert r._commit_sha_of_tree("other") == "def456" + # subdir cache found this — no additional calls to _build_tree_to_commit_cache + assert r._git.command.call_count == 1 + + +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") + r._git.command = Mock(return_value="abc123\ndef456") + + # Build the full cache via _build_tree_to_commit_cache first + r._git.command.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 + r._git.command.return_value = "abc123\ndef456" + with patch.object(r, "_subdir_tree_hash", side_effect=["dir_a", "dir_b"]): + assert r._commit_sha_of_tree("dir_b") == "def456" + # 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 +920,38 @@ 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"} + + @patch("tagbot.action.repo.logger") def test_versions(logger): r = _repo() From 02a1de4badfd0d85889daf5fe37059bf516655fb Mon Sep 17 00:00:00 2001 From: Arnav Kapoor Date: Sat, 4 Jul 2026 00:50:45 +0530 Subject: [PATCH 2/3] format --- tagbot/action/repo.py | 8 ++------ test/action/test_repo.py | 14 +++++++++----- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/tagbot/action/repo.py b/tagbot/action/repo.py index b0e160ce..817f5eb7 100644 --- a/tagbot/action/repo.py +++ b/tagbot/action/repo.py @@ -635,12 +635,8 @@ def _commit_sha_of_tree(self, tree: str) -> Optional[str]: if self.__subdir_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 - ) + 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( diff --git a/test/action/test_repo.py b/test/action/test_repo.py index 8f3de608..883fded5 100644 --- a/test/action/test_repo.py +++ b/test/action/test_repo.py @@ -724,8 +724,14 @@ def test_commit_sha_of_tree_subdir_caches_are_separate(): with patch.object(r, "_subdir_tree_hash", side_effect=["dir_a", "dir_b"]): assert r._commit_sha_of_tree("dir_b") == "def456" # 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"} + 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(): @@ -936,9 +942,7 @@ def test_filter_map_versions_subdir(): # 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" - } + 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 From 5c95a9cdac69831c7c413b16bd016c76a99f9962 Mon Sep 17 00:00:00 2001 From: Ian Butterworth Date: Wed, 8 Jul 2026 13:31:29 -0400 Subject: [PATCH 3/3] Fix review findings for subdir pre-move version resolution - Skip versions whose resolved commit is already tagged under the un-prefixed name, so repos restructured into a subdir don't get duplicate prefixed tags/releases backfilled for pre-move history - Return None from _commit_sha_of_tree when a tree matches both the subdir cache and the root-tree cache (content-preserving move): either cache may point at the wrong commit, so defer to the registry PR, which pins the exact registered commit - Extract _build_subdir_tree_to_commit_cache, derived from the full tree cache: one git log pass populates both caches, per-commit rev-parse calls are deduped by root tree, and git failures degrade gracefully like the sibling builder - Check the root tree (already in hand from the API) before shelling out to rev-parse in _commit_sha_from_registry_pr, and share one root-tree comparison/warning between the subdir and regular paths - Update subdir tests to the single-pass cache format, replace the dead-setup cache-preference test with an ambiguity test, and cover the un-prefixed-tag guard Co-Authored-By: Claude Fable 5 --- tagbot/action/repo.py | 98 ++++++++++++++++++++++++---------------- test/action/test_repo.py | 87 ++++++++++++++++++++--------------- 2 files changed, 110 insertions(+), 75 deletions(-) diff --git a/tagbot/action/repo.py b/tagbot/action/repo.py index 817f5eb7..d4ca9056 100644 --- a/tagbot/action/repo.py +++ b/tagbot/action/repo.py @@ -567,28 +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. - # For versions registered after the subdir move, we compare the subdirectory - # tree hash. For versions registered before the move, we fall back to the - # root 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=True) if subdir_tree_hash and subdir_tree_hash == tree: return cast(str, commit.sha) - # Fallback: check if tree matches the root commit tree SHA - # (for versions registered before the subdir restructure) - if commit.commit.tree.sha == tree: - return cast(str, commit.sha) - msg = "Tree SHA of commit from registry PR does not match (subdir or root)" - 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. @@ -618,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) @@ -629,29 +643,22 @@ 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, stored separately - # from the full-tree cache to avoid one overwriting the other. - if self.__subdir_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.__subdir_tree_to_commit_cache = subdir_cache - - # Check the subdir cache first - result = self.__subdir_tree_to_commit_cache.get(tree) - if result: - return result - - # Fallback: check the full tree cache for trees registered before the - # subdir restructure (where the registry stored the root tree SHA). + # 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( @@ -928,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 883fded5..5fe119da 100644 --- a/test/action/test_repo.py +++ b/test/action/test_repo.py @@ -574,23 +574,27 @@ def test_commit_sha_from_registry_pr_subdir_neither_matches(logger): 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 (subdir or root)" + "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 we fall through to - the root tree check.""" + 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" - # _subdir_tree_hash returns None (suppressed Abort) when subdir doesn't exist + # Root tree matches: resolved without consulting the subdir at all with patch.object(r, "_subdir_tree_hash", return_value=None): - # Falls through to root tree check and matches 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") @@ -644,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 @@ -660,7 +664,7 @@ 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 @@ -675,54 +679,48 @@ def test_commit_sha_of_tree_subdir_falls_back_to_full_cache(): subdirectory tree SHA. """ r = _repo(subdir="path/to/package") - # git log --format=%H returns commit SHAs for subdir cache - r._git.command = Mock(return_value="abc123\ndef456") + 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"): - # Build subdir cache + # Post-move version resolves via the subdir cache assert r._commit_sha_of_tree("dir_tree") == "abc123" - # Lookup a tree that isn't in the subdir cache but would be in the - # full cache (simulates a pre-subdir version registered at root level). - # Change mock to return full git log format with tree SHAs. - r._git.command.return_value = ( - "abc123 root_tree_1\ndef456 root_tree_2\nghi789 root_tree_3" - ) - assert r._commit_sha_of_tree("root_tree_2") == "def456" + # 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_prefers_subdir_cache(): - """The subdir cache is checked first; if the tree exists there, the - full cache fallback is not consulted.""" +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") - r._git.command = Mock(return_value="abc123\ndef456") - with patch.object(r, "_subdir_tree_hash", side_effect=["sub_tree", "other"]): - # Build subdir cache and find match - assert r._commit_sha_of_tree("sub_tree") == "abc123" - # Even if the full cache would also match a different tree, - # subsequent lookups go through the subdir cache first - r._git.command.return_value = "abc123 root_tree\ndef456 other" - assert r._commit_sha_of_tree("other") == "def456" - # subdir cache found this — no additional calls to _build_tree_to_commit_cache - assert r._git.command.call_count == 1 + # 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") - r._git.command = Mock(return_value="abc123\ndef456") # Build the full cache via _build_tree_to_commit_cache first - r._git.command.return_value = "abc123 root_tree\ndef456 other_tree" + 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 - r._git.command.return_value = "abc123\ndef456" + # 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", @@ -956,6 +954,23 @@ def test_filter_map_versions_subdir(): 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()