Review comments

Author: spoorcc

dfetch/project/archivesubproject.py

@@ -109,19 +109,28 @@ def _latest_revision_on_branch(self, branch: str) -> str:
         del branch
         return self.remote
 
-    def _download_and_compute_hash(self, algorithm: str = "sha256") -> IntegrityHash:
+    def _download_and_compute_hash(
+        self, algorithm: str = "sha256", url: str | None = None
+    ) -> IntegrityHash:
         """Download the archive to a temporary file and return its :class:`IntegrityHash`.
 
         The hash is computed during the download stream — no extra file read.
         The temporary file is always cleaned up, even on error.
 
+        Args:
+            algorithm: Hash algorithm to use (``sha256``, ``sha384``, ``sha512``).
+            url: If given, download from this URL instead of ``self._remote_repo``.
+                 Use this to pin to the exact URL stored in the on-disk revision.
+
         Raises:
             RuntimeError: On download failure or unsupported algorithm.
         """
-        fd, tmp_path = tempfile.mkstemp(suffix=_suffix_for_url(self.remote))
+        effective_url = url if url is not None else self.remote
+        remote = ArchiveRemote(effective_url) if url is not None else self._remote_repo
+        fd, tmp_path = tempfile.mkstemp(suffix=_suffix_for_url(effective_url))
         os.close(fd)
         try:
-            hex_digest = self._remote_repo.download(tmp_path, algorithm=algorithm)
+            hex_digest = remote.download(tmp_path, algorithm=algorithm)
             return IntegrityHash(algorithm, hex_digest)
         finally:
             try:
@@ -231,8 +240,9 @@ def freeze_project(self, project: ProjectEntry) -> str | None:
         revision = on_disk.revision
 
         # Already hash-pinned — use the on-disk revision directly.
+        # Otherwise download from the revision URL (not the possibly-updated manifest URL).
         pinned = IntegrityHash.parse(revision) or self._download_and_compute_hash(
-            "sha256"
+            "sha256", url=revision
         )
         new_hash = str(pinned)
         if project.hash == new_hash:

dfetch/util/util.py

@@ -87,12 +87,21 @@ def prune_files_by_pattern(directory: str, patterns: Sequence[str]) -> None:
     License files are never removed even when they match a pattern.
     """
     seen: set[str] = set()
+    paths = []
     for file_or_dir in find_matching_files(directory, patterns):
         resolved = str(file_or_dir.resolve())
         if resolved in seen:
             continue
         seen.add(resolved)
-        if not (file_or_dir.is_file() and is_license_file(file_or_dir.name)):
+        paths.append(file_or_dir)
+
+    # Remove children before parents to avoid FileNotFoundError on already-deleted paths.
+    paths.sort(key=lambda p: len(str(p.resolve())), reverse=True)
+
+    for file_or_dir in paths:
+        if file_or_dir.exists() and not (
+            file_or_dir.is_file() and is_license_file(file_or_dir.name)
+        ):
             safe_rm(file_or_dir)
 
 

features/steps/archive_steps.py

@@ -13,27 +13,9 @@
 from behave import given  # pylint: disable=no-name-in-module
 
 
-def _sha256(path: str) -> str:
-    """Return the SHA-256 hex digest of a file."""
-    h = hashlib.sha256()
-    with open(path, "rb") as f:
-        for chunk in iter(lambda: f.read(8192), b""):
-            h.update(chunk)
-    return h.hexdigest()
-
-
-def _sha384(path: str) -> str:
-    """Return the SHA-384 hex digest of a file."""
-    h = hashlib.sha384()
-    with open(path, "rb") as f:
-        for chunk in iter(lambda: f.read(8192), b""):
-            h.update(chunk)
-    return h.hexdigest()
-
-
-def _sha512(path: str) -> str:
-    """Return the SHA-512 hex digest of a file."""
-    h = hashlib.sha512()
+def _file_digest(path: str, constructor) -> str:
+    """Return the hex digest of *path* using the given hashlib *constructor*."""
+    h = constructor()
     with open(path, "rb") as f:
         for chunk in iter(lambda: f.read(8192), b""):
             h.update(chunk)
@@ -85,12 +67,14 @@ def _create_archive(context, name: str, extension: str) -> None:
 
     if extension == ".tar.gz":
         create_tar_gz(archive_path, name, files)
-    else:
+    elif extension == ".zip":
         create_zip(archive_path, name, files)
+    else:
+        raise ValueError(f"Unsupported archive extension: {extension!r}")
 
-    context.archive_sha256 = _sha256(archive_path)
-    context.archive_sha384 = _sha384(archive_path)
-    context.archive_sha512 = _sha512(archive_path)
+    context.archive_sha256 = _file_digest(archive_path, hashlib.sha256)
+    context.archive_sha384 = _file_digest(archive_path, hashlib.sha384)
+    context.archive_sha512 = _file_digest(archive_path, hashlib.sha512)
     context.archive_url = _archive_url(context, filename)
 
 

tests/test_archive.py

@@ -1,14 +1,18 @@
 """Unit tests for dfetch.vcs.archive and dfetch.project.archivesubproject."""
 
+import hashlib
 import io
 import os
 import tarfile
 import tempfile
 import zipfile
+from unittest.mock import patch
 
 import pytest
 
-from dfetch.project.archivesubproject import _suffix_for_url
+from dfetch.manifest.project import ProjectEntry
+from dfetch.manifest.version import Version
+from dfetch.project.archivesubproject import ArchiveSubProject, _suffix_for_url
 from dfetch.vcs.archive import (
     ARCHIVE_EXTENSIONS,
     ArchiveLocalRepo,
@@ -249,3 +253,115 @@ def test_all_archive_extensions_covered():
     assert len(ARCHIVE_EXTENSIONS) > 0
     for ext in ARCHIVE_EXTENSIONS:
         assert ext.startswith(".")
+
+
+# ---------------------------------------------------------------------------
+# Helpers shared by ArchiveSubProject tests
+# ---------------------------------------------------------------------------
+
+
+def _make_tar_gz(path: str, content: bytes = b"hello") -> None:
+    """Write a minimal .tar.gz archive containing one file to *path*."""
+    with tarfile.open(path, "w:gz") as tf:
+        info = tarfile.TarInfo(name="pkg/README.md")
+        info.size = len(content)
+        tf.addfile(info, io.BytesIO(content))
+
+
+def _sha256_file(path: str) -> str:
+    h = hashlib.sha256()
+    with open(path, "rb") as f:
+        for chunk in iter(lambda: f.read(8192), b""):
+            h.update(chunk)
+    return h.hexdigest()
+
+
+def _file_url(path: str) -> str:
+    return "file:///" + path.lstrip("/")
+
+
+def _make_subproject(url: str) -> ArchiveSubProject:
+    return ArchiveSubProject(
+        ProjectEntry({"name": "pkg", "url": url, "vcs": "archive"})
+    )
+
+
+# ---------------------------------------------------------------------------
+# ArchiveSubProject._download_and_compute_hash – explicit url parameter
+# ---------------------------------------------------------------------------
+
+
+def test_download_and_compute_hash_default_uses_remote_repo():
+    """Without an explicit url the hash is computed from self._remote_repo."""
+    with tempfile.TemporaryDirectory() as tmp:
+        archive = os.path.join(tmp, "pkg.tar.gz")
+        _make_tar_gz(archive)
+        url = _file_url(archive)
+        sp = _make_subproject(url)
+
+        result = sp._download_and_compute_hash("sha256")
+
+        assert result.algorithm == "sha256"
+        assert result.hex_digest == _sha256_file(archive)
+
+
+def test_download_and_compute_hash_explicit_url_overrides_remote_repo():
+    """When *url* is supplied a fresh ArchiveRemote for that URL is used.
+
+    This is the regression guard for the fix: if the manifest URL was changed
+    after fetching, freeze must still hash the *original* archive (the one
+    recorded in the on-disk revision), not the current manifest URL.
+    """
+    with tempfile.TemporaryDirectory() as tmp:
+        archive_a = os.path.join(tmp, "pkg_a.tar.gz")
+        archive_b = os.path.join(tmp, "pkg_b.tar.gz")
+        _make_tar_gz(archive_a, content=b"version A")
+        _make_tar_gz(archive_b, content=b"version B")
+        url_a = _file_url(archive_a)
+        url_b = _file_url(archive_b)
+
+        # SubProject points to archive_b (current manifest URL).
+        sp = _make_subproject(url_b)
+
+        # Passing url=url_a must use archive_a's content.
+        result = sp._download_and_compute_hash("sha256", url=url_a)
+
+        assert result.hex_digest == _sha256_file(archive_a)
+        assert result.hex_digest != _sha256_file(archive_b)
+
+
+# ---------------------------------------------------------------------------
+# ArchiveSubProject.freeze_project – uses on-disk revision URL
+# ---------------------------------------------------------------------------
+
+
+def test_freeze_project_uses_on_disk_url_not_manifest_url():
+    """freeze_project must hash the archive at the on-disk revision URL.
+
+    Scenario: the manifest URL was updated after the last fetch.  Without the
+    fix, freeze would download from the new (current) manifest URL and produce
+    a hash that doesn't match the fetched archive.  With the fix it uses the
+    URL stored in the on-disk revision.
+    """
+    with tempfile.TemporaryDirectory() as tmp:
+        archive_a = os.path.join(tmp, "pkg_a.tar.gz")
+        archive_b = os.path.join(tmp, "pkg_b.tar.gz")
+        _make_tar_gz(archive_a, content=b"original fetch")
+        _make_tar_gz(archive_b, content=b"updated manifest url")
+        url_a = _file_url(archive_a)
+        url_b = _file_url(archive_b)
+
+        # SubProject now points to archive_b (manifest was updated after fetch).
+        sp = _make_subproject(url_b)
+
+        # Simulate on-disk state: was fetched from url_a (no hash-pin at the time).
+        on_disk = Version(revision=url_a)
+        with patch.object(sp, "on_disk_version", return_value=on_disk):
+            project_entry = ProjectEntry(
+                {"name": "pkg", "url": url_b, "vcs": "archive"}
+            )
+            sp.freeze_project(project_entry)
+
+        expected_hash = f"sha256:{_sha256_file(archive_a)}"
+        assert project_entry.hash == expected_hash
+        assert _sha256_file(archive_b) not in project_entry.hash

tests/test_util.py

@@ -5,7 +5,7 @@
 
 import pytest
 
-from dfetch.util.util import copy_src_subset, hash_directory
+from dfetch.util.util import copy_src_subset, hash_directory, prune_files_by_pattern
 
 # ---------------------------------------------------------------------------
 # copy_src_subset – path-traversal protection
@@ -100,3 +100,58 @@ def test_hash_directory_skiplist_excludes_file(tmp_path):
     h_with_skip2 = hash_directory(str(d), ["ignored.txt"])
 
     assert h_with_skip == h_with_skip2
+
+
+# ---------------------------------------------------------------------------
+# prune_files_by_pattern – delete-order safety
+# ---------------------------------------------------------------------------
+
+
+def test_prune_removes_matched_file(tmp_path):
+    (tmp_path / "remove_me.txt").write_text("gone")
+    prune_files_by_pattern(str(tmp_path), ["remove_me.txt"])
+    assert not (tmp_path / "remove_me.txt").exists()
+
+
+def test_prune_parent_and_child_both_matched_no_error(tmp_path):
+    """When a dir and a file inside it both match, removal must not raise.
+
+    Before the fix, removing the parent first left the child path pointing at a
+    non-existent location; the subsequent safe_rm call then raised
+    FileNotFoundError.
+    """
+    src = tmp_path / "src"
+    src.mkdir()
+    (src / "main.c").write_text("int main(){}")
+
+    # "src" matches the directory; "main.c" matches the child inside it.
+    prune_files_by_pattern(str(tmp_path), ["src", "main.c"])
+
+    assert not src.exists()
+
+
+def test_prune_preserves_license_file(tmp_path):
+    """License files must survive even when they match a removal pattern."""
+    (tmp_path / "LICENSE").write_text("MIT")
+    (tmp_path / "delete_me.txt").write_text("gone")
+
+    prune_files_by_pattern(str(tmp_path), ["LICENSE", "delete_me.txt"])
+
+    assert (tmp_path / "LICENSE").exists()
+    assert not (tmp_path / "delete_me.txt").exists()
+
+
+def test_prune_skips_already_removed_paths(tmp_path):
+    """Paths that no longer exist after a parent removal are silently skipped."""
+    parent = tmp_path / "libs"
+    parent.mkdir()
+    child = parent / "lib.a"
+    child.write_text("binary")
+    unrelated = tmp_path / "readme.txt"
+    unrelated.write_text("keep")
+
+    # Both "libs" (directory) and "libs/lib.a" (child) match; no exception expected.
+    prune_files_by_pattern(str(tmp_path), ["libs", "lib.a"])
+
+    assert not parent.exists()
+    assert unrelated.exists()