Gives FileSet a single directory walk, all_file_paths(), which byte_chunks, hash() and hash_files() now use. Also fixes a bug in decomposed_fspaths.

fileformats/core/fileset.py

@@ -1071,16 +1071,54 @@ def formats_by_name(cls) -> ty.Dict[str, ty.Set[ty.Type["FileSet"]]]:
             }
         return cls._formats_by_name
 
-    @property
-    def all_file_paths(self) -> ty.Iterable[Path]:
-        """Paths of all files within the fileset"""
+    def all_file_paths(
+        self,
+        ignore_hidden_files: bool = False,
+        ignore_hidden_dirs: bool = False,
+    ) -> ty.List[Path]:
+        """Paths of all files within the fileset, including those nested within
+        directories
+
+        Parameters
+        ----------
+        ignore_hidden_files : bool
+            whether to ignore hidden files within nested directories (i.e. those
+            starting with '.')
+        ignore_hidden_dirs : bool
+            whether to ignore hidden directories within nested directories (i.e. those
+            starting with '.'), along with everything beneath them
+
+        Returns
+        -------
+        file_paths : list[Path]
+            the paths of the files within the fileset, in sorted order. Files that are
+            both listed explicitly and nested within a listed directory are only
+            included once
+
+        Raises
+        ------
+        FileNotFoundError
+            if a symbolic link within a nested directory is broken
+        """
+        file_paths: ty.Set[Path] = set()
         for fspath in self.fspaths:
             if fspath.is_file():
-                yield fspath
+                file_paths.add(fspath)
             else:
-                for dpath, _, file_paths in os.walk(fspath):
-                    for file_path in file_paths:
-                        yield Path(dpath) / file_path
+                for dpath, dirnames, filenames in os.walk(fspath):
+                    if ignore_hidden_dirs:
+                        # Prune in-place so the walk doesn't descend into them
+                        dirnames[:] = [d for d in dirnames if not d.startswith(".")]
+                    for filename in filenames:
+                        if ignore_hidden_files and filename.startswith("."):
+                            continue
+                        file_path = Path(dpath) / filename
+                        if not file_path.exists():
+                            raise FileNotFoundError(
+                                f"Broken symbolic link '{file_path}' found within {self}"
+                            )
+                        file_paths.add(file_path)
+        return sorted(file_paths)
 
     def byte_chunks(
         self,
@@ -1093,6 +1131,8 @@ def byte_chunks(
         """Yields relative paths for all files within the file-set along with iterators
         over their byte-contents. To be used when generating hashes for the file set.
 
+        Files are yielded once each, in the sorted order of `all_file_paths`.
+
         Parameters
         ----------
         mtime : bool, optional
@@ -1168,42 +1208,15 @@ def chunk_file(fspath: Path) -> ty.Iterator[bytes]:
 
             def chunk_file(fspath: Path) -> ty.Iterator[bytes]:
                 """Yields the contents of the file in byte chunks"""
-                if not fspath.is_file():
-                    assert fspath.is_symlink()  # broken symlink
-                    yield b"\x00"
-                else:
-                    with open(fspath, "rb") as fp:
-                        for chunk in iter(functools.partial(fp.read, chunk_len), b""):
-                            yield chunk
+                with open(fspath, "rb") as fp:
+                    for chunk in iter(functools.partial(fp.read, chunk_len), b""):
+                        yield chunk
 
-        def chunk_dir(fspath: Path) -> ty.Iterator[ty.Tuple[str, ty.Iterator[bytes]]]:
-            fspath = Path(fspath)
-            for dpath_str, _, filenames in sorted(os.walk(fspath)):
-                # Sort in-place to guarantee order.
-                filenames.sort()
-                dpath = Path(dpath_str)
-                if (
-                    ignore_hidden_dirs
-                    and dpath.name.startswith(".")
-                    and dpath != fspath
-                ):
-                    continue
-                for filename in filenames:
-                    if ignore_hidden_files and filename.startswith("."):
-                        continue
-                    yield (
-                        relative_path_str(dpath / filename),
-                        chunk_file(dpath / filename),
-                    )
-
-        for key, fspath in sorted(
-            ((relative_path_str(p), p) for p in self.fspaths),
-            key=itemgetter(0),
+        for fspath in self.all_file_paths(
+            ignore_hidden_files=ignore_hidden_files,
+            ignore_hidden_dirs=ignore_hidden_dirs,
         ):
-            if fspath.is_dir():
-                yield from chunk_dir(fspath)
-            else:
-                yield (key, chunk_file(fspath))
+            yield (relative_path_str(fspath), chunk_file(fspath))
 
     def hash(
         self,
@@ -1511,9 +1524,10 @@ def is_subpath(path: Path, others: ty.AbstractSet[Path]) -> bool:
         decomposed_by: dict[Path, tuple[FileSet, tuple[Path, str, str]]] = {}
         for fileset in [self] + self.nested_filesets():
             if isinstance(fileset, File):
-                # Skip files that are nested within other paths (e.g. directories) of
-                # the file-set, as they will be handled along with their parent
-                if is_subpath(fileset.fspath, self.fspaths):
+                # Skip files that are nested within other included paths (e.g.
+                # directories) of the file-set, as they will be handled along with
+                # their parent
+                if is_subpath(fileset.fspath, to_include):
                     implicit.discard(fileset.fspath)
                     continue
                 # Skip files that aren't to be included (e.g. not required)

fileformats/core/tests/test_utils.py

@@ -510,6 +510,51 @@ def header(self):
     assert decomposed == [(work_dir, "mario_dir", "")]
 
 
+def test_decompose_fspaths_required_file_in_unrequired_directory(work_dir):
+    class NestedMario(FileSet):
+        @validated_property
+        def header(self):
+            return Mario(next(p for p in self.fspaths if p.is_file()))
+
+    sub_dir = work_dir / "sub"
+    sub_dir.mkdir()
+    Path.touch(sub_dir / "header.mario")
+    Path.touch(sub_dir / "other.mario")
+
+    fileset = NestedMario([sub_dir, sub_dir / "header.mario"])
+
+    # The directory isn't required, so the required file nested within it shouldn't
+    # be dropped in favour of it
+    assert fileset.decomposed_fspaths(required_only=True) == [
+        (sub_dir, "header", ".mario")
+    ]
+    assert fileset.decomposed_fspaths(required_only=False) == [(work_dir, "sub", "")]
+
+
+def test_copy_trimmed_required_file_in_unrequired_directory(work_dir):
+    class NestedMario(FileSet):
+        @validated_property
+        def header(self):
+            return Mario(next(p for p in self.fspaths if p.is_file()))
+
+    src_dir = work_dir / "src"
+    sub_dir = src_dir / "sub"
+    sub_dir.mkdir(parents=True)
+    (sub_dir / "header.mario").write_text("header")
+    (sub_dir / "other.mario").write_text("other")
+    dest_dir = work_dir / "dest"
+    dest_dir.mkdir()
+
+    fileset = NestedMario([sub_dir, sub_dir / "header.mario"])
+    copied = fileset.copy(dest_dir, trim=True)
+
+    assert [p.relative_to(dest_dir) for p in copied.fspaths] == [
+        Path("sub") / "header.mario"
+    ]
+    assert (dest_dir / "sub" / "header.mario").read_text() == "header"
+    assert not (dest_dir / "sub" / "other.mario").exists()
+
+
 def test_copy_with_subpaths(work_dir):
     src_dir = work_dir / "src"
     src_dir.mkdir()
@@ -612,12 +657,117 @@ def test_hash_mtime(tmp_path: Path):
 def test_hash_files(fsobject: FsObject, work_dir: Path, dest_dir: Path):
     file_hashes = fsobject.hash_files(relative_to=work_dir)
     assert sorted(Path(p) for p in file_hashes) == sorted(
-        p.relative_to(work_dir) for p in fsobject.all_file_paths
+        p.relative_to(work_dir) for p in fsobject.all_file_paths()
     )
     cpy = fsobject.copy(dest_dir)
     assert cpy.hash_files() == fsobject.hash_files()
 
 
+def test_all_file_paths(work_dir: Path):
+    sub_dir = work_dir / "sub"
+    deep_dir = sub_dir / "deep"
+    deep_dir.mkdir(parents=True)
+    Path.touch(sub_dir / "b.mario")
+    Path.touch(sub_dir / "a.mario")
+    Path.touch(deep_dir / "c.mario")
+    Path.touch(work_dir / "sibling.mario")
+
+    # "a.mario" is both listed explicitly and nested within the listed directory
+    fileset = FileSet([sub_dir, sub_dir / "a.mario", work_dir / "sibling.mario"])
+    all_file_paths = fileset.all_file_paths()
+
+    assert [p.relative_to(work_dir) for p in all_file_paths] == [
+        Path("sibling.mario"),
+        Path("sub") / "a.mario",
+        Path("sub") / "b.mario",
+        Path("sub") / "deep" / "c.mario",
+    ]
+
+
+def test_all_file_paths_ignore_hidden(work_dir: Path):
+    sub_dir = work_dir / "sub"
+    hidden_dir = sub_dir / ".hidden"
+    below_hidden_dir = hidden_dir / "below"
+    below_hidden_dir.mkdir(parents=True)
+    Path.touch(sub_dir / "a.mario")
+    Path.touch(sub_dir / ".b.mario")
+    Path.touch(hidden_dir / "c.mario")
+    Path.touch(below_hidden_dir / "d.mario")
+    Path.touch(work_dir / ".listed.mario")
+
+    # Explicitly listed paths are always included, even if they are hidden
+    fileset = FileSet([sub_dir, work_dir / ".listed.mario"])
+
+    def rel_paths(**kwargs: bool) -> ty.List[str]:
+        return [
+            p.relative_to(work_dir).as_posix() for p in fileset.all_file_paths(**kwargs)
+        ]
+
+    assert rel_paths() == [
+        ".listed.mario",
+        "sub/.b.mario",
+        "sub/.hidden/below/d.mario",
+        "sub/.hidden/c.mario",
+        "sub/a.mario",
+    ]
+    assert rel_paths(ignore_hidden_files=True) == [
+        ".listed.mario",
+        "sub/.hidden/below/d.mario",
+        "sub/.hidden/c.mario",
+        "sub/a.mario",
+    ]
+    assert rel_paths(ignore_hidden_dirs=True) == [
+        ".listed.mario",
+        "sub/.b.mario",
+        "sub/a.mario",
+    ]
+    assert rel_paths(ignore_hidden_files=True, ignore_hidden_dirs=True) == [
+        ".listed.mario",
+        "sub/a.mario",
+    ]
+
+
+def test_all_file_paths_broken_symlink(work_dir: Path):
+    sub_dir = work_dir / "sub"
+    sub_dir.mkdir()
+    Path.touch(sub_dir / "a.mario")
+    (sub_dir / "broken.mario").symlink_to(work_dir / "missing.mario")
+
+    fileset = FileSet(sub_dir)
+
+    with pytest.raises(FileNotFoundError, match="Broken symbolic link"):
+        fileset.all_file_paths()
+    with pytest.raises(FileNotFoundError, match="Broken symbolic link"):
+        fileset.hash_files()
+
+
+def test_byte_chunks_matches_all_file_paths(work_dir: Path):
+    sub_dir = work_dir / "sub"
+    below_hidden_dir = sub_dir / ".hidden" / "below"
+    below_hidden_dir.mkdir(parents=True)
+    (sub_dir / "deep").mkdir()
+    Path.touch(sub_dir / "z.mario")
+    Path.touch(sub_dir / "deep" / "c.mario")
+    Path.touch(below_hidden_dir / "d.mario")
+
+    # "z.mario" is both listed explicitly and nested within the listed directory
+    fileset = FileSet([sub_dir, sub_dir / "z.mario"])
+
+    for ignore_hidden_dirs in (False, True):
+        # Keys use the native path separator, so compare them as paths
+        keys = [
+            Path(k)
+            for k, _ in fileset.byte_chunks(
+                relative_to=work_dir, ignore_hidden_dirs=ignore_hidden_dirs
+            )
+        ]
+        assert keys == [
+            p.relative_to(work_dir)
+            for p in fileset.all_file_paths(ignore_hidden_dirs=ignore_hidden_dirs)
+        ]
+    assert keys == [Path("sub") / "deep" / "c.mario", Path("sub") / "z.mario"]
+
+
 def test_hash_files_keys(tmp_path: Path):
     one = Foo.sample(tmp_path, stem="foo-one")
     two = Foo.sample(tmp_path, stem="foo-two")