Fix four code review findings

- subproject.py: _is_already_pinned now treats a matching tag as
  sufficient even when on_disk_version carries a resolved revision,
  preventing freeze_project from silently rewriting tag pins to SHAs
- git_types.py: align must_keeps to Sequence[str]|None so both
  CheckoutOptions collection fields share the same type
- git.py: move __all__ to after all imports (was between the
  git_types and patch imports)
- tests/test_util.py: use tmp_path_factory for the outside directory
  in test_glob_within_root_rejects_escaped_paths so pytest manages
  cleanup instead of leaving it on disk on failure

https://claude.ai/code/session_01Doq8oQtBRH4afusvp9Dv4p

Author: claude

dfetch/project/subproject.py

@@ -466,9 +466,11 @@ def freeze_project(self, project: ProjectEntry) -> str | None:
     def _is_already_pinned(
         self, project: ProjectEntry, on_disk_version: Version
     ) -> bool:
-        """Return True if *project* is already pinned to its on-disk version."""
+        """Return True if *project* is already pinned to *on_disk_version*."""
+        if project.version.tag and project.version.tag == on_disk_version.tag:
+            return True
         return (
             project.version.tag == on_disk_version.tag
             and project.version.revision == on_disk_version.revision
-            and (bool(project.version.tag) or self.revision_is_enough())
+            and self.revision_is_enough()
         )

dfetch/vcs/git.py

@@ -22,9 +22,9 @@
     unique_parent_dirs,
 )
 from dfetch.vcs.git_types import CheckoutOptions, Submodule
+from dfetch.vcs.patch import Patch, PatchType
 
 __all__ = ["CheckoutOptions", "GitLocalRepo", "GitRemote", "Submodule"]
-from dfetch.vcs.patch import Patch, PatchType
 
 logger = get_logger(__name__)
 

dfetch/vcs/git_types.py

@@ -24,5 +24,5 @@ class CheckoutOptions:
     remote: str
     version: str
     src: str | None = None
-    must_keeps: list[str] | None = None
+    must_keeps: Sequence[str] | None = None
     ignore: Sequence[str] | None = None

tests/test_util.py

@@ -203,18 +203,17 @@ def test_glob_within_root_returns_safe_paths(tmp_path):
     assert escaped == []
 
 
-def test_glob_within_root_rejects_escaped_paths(tmp_path):
+def test_glob_within_root_rejects_escaped_paths(tmp_path, tmp_path_factory):
     """Paths that resolve outside the root are returned in the escaped list."""
-    outside = tmp_path.parent / "outside_glob_test"
-    outside.mkdir(exist_ok=True)
+    outside = tmp_path_factory.mktemp("outside_glob_test")
     (outside / "secret.txt").write_text("data")
 
     from unittest.mock import patch
 
     with patch(
         "dfetch.util.util.glob.glob", return_value=[str(outside / "secret.txt")]
     ):
-        safe, escaped = glob_within_root("../outside_glob_test/*.txt", tmp_path)
+        safe, escaped = glob_within_root(str(outside / "*.txt"), tmp_path)
 
     assert safe == []
     assert escaped == [str(outside / "secret.txt")]