Fix how excluded files are determined everywhere

Author: saecki

src/check.rs

@@ -19,6 +19,7 @@ mod path;
 mod readme;
 
 pub use diagnostics::{Diagnostics, Result, TryExt};
+pub use manifest::Exclude;
 
 pub async fn all_checks(
     package_spec: Option<&PackageSpec>,

src/check/files.rs

@@ -1,4 +1,3 @@
-use std::collections::HashSet;
 use std::path::Path;
 
 use codespan_reporting::diagnostic::{Diagnostic, Label};
@@ -16,35 +15,17 @@ pub fn check(
     readme: &Option<Readme>,
 ) {
     let exclude = &manifest.package.exclude;
-    let thumbnail_path = manifest.thumbnail();
-
-    // Manually keep track of excluded directories, to figure out if nested
-    // files are ignored. This is done, so we can generate diagnostics for
-    // excluded files.
-    let mut excluded_dirs = HashSet::new();
 
     for ch in WalkDir::new(package_dir).into_iter().flatten() {
         let Ok(metadata) = ch.metadata() else {
             continue;
         };
-
-        let file_path = PackagePath::from_full(package_dir, ch.path());
-
-        if metadata.is_dir() {
-            // If the parent directory is ignored, all children are ignored too.
-            if parent_is_excluded(&excluded_dirs, file_path)
-                || exclude.matched(file_path.relative(), true).is_ignore()
-            {
-                excluded_dirs.insert(ch.into_path());
-            }
+        if !metadata.is_file() {
             continue;
         }
 
-        // The thumbnail is always excluded.
-        let is_thumbnail = thumbnail_path.is_some_and(|t| t.val == file_path);
-        let excluded = is_thumbnail
-            || parent_is_excluded(&excluded_dirs, file_path)
-            || exclude.matched(file_path.relative(), false).is_ignore();
+        let file_path = PackagePath::from_full(package_dir, ch.path());
+        let excluded = exclude.matches_file(&file_path);
 
         forbid_font_files(diags, file_path);
         exclude_large_files(diags, file_path, excluded, metadata.len());
@@ -53,16 +34,6 @@ pub fn check(
     }
 }
 
-fn parent_is_excluded(
-    excluded_dirs: &HashSet<std::path::PathBuf>,
-    file_path: PackagePath<&Path>,
-) -> bool {
-    file_path
-        .full()
-        .parent()
-        .is_some_and(|parent| excluded_dirs.contains(parent))
-}
-
 fn exclude_large_files(
     diags: &mut Diagnostics,
     path: PackagePath<&Path>,

src/check/manifest.rs

@@ -1,3 +1,4 @@
+use std::collections::HashSet;
 use std::path::PathBuf;
 use std::{ops::Range, path::Path, str::FromStr};
 
@@ -27,22 +28,16 @@ pub struct Worlds {
 #[derive(Debug, Clone)]
 pub struct Manifest {
     pub package: Spanned<Package>,
+    #[allow(unused)]
     pub template: Option<Spanned<Template>>,
 }
 
-impl Manifest {
-    pub fn thumbnail(&self) -> Option<Spanned<PackagePath<&Path>>> {
-        let thumbnail = self.template.as_ref()?.thumbnail.as_ref()?;
-        Some(thumbnail.as_ref().map(PackagePath::as_path))
-    }
-}
-
 #[derive(Debug, Clone)]
 pub struct Package {
     pub entrypoint: Spanned<PackagePath>,
     pub name: Option<Spanned<String>>,
     pub version: Option<Spanned<PackageVersion>>,
-    pub exclude: Spanned<Override>,
+    pub exclude: Spanned<Exclude>,
 }
 
 #[derive(Debug, Clone)]
@@ -89,7 +84,8 @@ pub async fn check(
     let entrypoint = entrypoint.map(|e| PackagePath::from_relative(package_dir, e));
     let name = check_name(diags, package, package_spec);
     let version = check_version(diags, package, package_spec);
-    let exclude = check_exclude(diags, package, package_dir)?;
+    let template = check_template(diags, &manifest, package_dir);
+    let exclude = check_exclude(diags, package_dir, package, &template)?;
 
     check_compiler_version(diags, package);
     check_universe_fields(diags, package);
@@ -108,9 +104,8 @@ pub async fn check(
         exclude,
     });
 
-    let template = check_template(diags, &manifest, package_dir);
     if let Some(template) = &template {
-        check_thumbnail(diags, &package.exclude, template);
+        check_thumbnail(diags, template);
         dont_exclude_template_files(diags, package_dir, &package.exclude, template);
     }
 
@@ -285,10 +280,10 @@ fn check_compiler_version(diags: &mut Diagnostics, package: Spanned<&Table>) ->
     Some(())
 }
 
-fn dont_over_exclude(diags: &mut Diagnostics, exclude: &Spanned<Override>) -> Result<()> {
+fn dont_over_exclude(diags: &mut Diagnostics, exclude: &Spanned<Exclude>) -> Result<()> {
     let warning = Diagnostic::warning().with_label(Label::primary(manifest_id(), exclude.span()));
 
-    if exclude.matched("LICENSE", false).is_ignore() {
+    if exclude.matches_relative_file("LICENSE") {
         diags.emit(
             warning
                 .clone()
@@ -297,7 +292,7 @@ fn dont_over_exclude(diags: &mut Diagnostics, exclude: &Spanned<Override>) -> Re
         );
     }
 
-    if exclude.matched("README.md", false).is_ignore() {
+    if exclude.matches_relative_file("README.md") {
         diags.emit(
             warning
                 .with_code("exclude/readme")
@@ -489,11 +484,12 @@ async fn check_repo(diags: &mut Diagnostics, package: Spanned<&Table>) {
 /// a lot of false positives in other diagnostics.
 fn check_exclude(
     diags: &mut Diagnostics,
-    package: Spanned<&Table>,
     package_dir: &Path,
-) -> Result<Spanned<Override>> {
+    package: Spanned<&Table>,
+    template: &Option<Spanned<Template>>,
+) -> Result<Spanned<Exclude>> {
     let Some(exclude) = package.get_spanned("exclude") else {
-        return Ok(Spanned::new(Override::empty(), package.span()));
+        return Ok(Spanned::new(Exclude::empty(), package.span()));
     };
 
     let exclude = exclude.try_map(Item::as_array).error(
@@ -526,10 +522,98 @@ fn check_exclude(
         exclude_globs.add(&format!("!{exclusion_str}")).ok();
     }
 
+    let thumbnail_path = template.as_ref().and_then(|t| t.thumbnail.as_ref());
+    if let Some(thumbnail_path) = thumbnail_path {
+        // Check if the thumbnail is excluded.
+        if let Ok(exclude) = exclude_globs.build()
+            && exclude.matched(thumbnail_path.full(), false).is_ignore()
+        {
+            diags.emit(
+                Diagnostic::error()
+                    .with_label(Label::primary(manifest_id(), thumbnail_path.span()))
+                    .with_code("manifest/template/thumbnail/exclude")
+                    .with_message("The template thumbnail is automatically excluded"),
+            );
+        }
+
+        exclude_globs
+            .add(&format!("!{}", thumbnail_path.relative().display()))
+            .ok();
+    }
+
     let exclude_globs = exclude_globs
         .build()
         .error("manifest/exclude/invalid", "Invalid exclude globs")?;
-    Ok(exclude.map(|_| exclude_globs))
+
+    Ok(exclude.map(|_| Exclude::index(exclude_globs)))
+}
+
+#[derive(Debug, Clone)]
+pub struct Exclude {
+    globs: Override,
+    /// Manually keep track of excluded directories, to figure out if nested
+    /// files are ignored. This has to be done to mimic the behavior of
+    /// [`ignore::Walk`], which won't enter a directory if it is ignored.
+    /// The bare [`Override::matched] function doesn't check if a parent
+    /// directory is excluded.
+    excluded_dirs: HashSet<PathBuf>,
+}
+
+impl Exclude {
+    pub fn empty() -> Self {
+        Self {
+            globs: Override::empty(),
+            excluded_dirs: HashSet::new(),
+        }
+    }
+
+    pub fn index(globs: Override) -> Self {
+        let mut excluded_dirs = HashSet::new();
+
+        for ch in WalkDir::new(globs.path()).into_iter().flatten() {
+            let Ok(metadata) = ch.metadata() else {
+                continue;
+            };
+            if !metadata.is_dir() {
+                continue;
+            }
+
+            let relative_path = ch
+                .path()
+                .strip_prefix(globs.path())
+                .expect("Path should be inside the package dir");
+            let parent_is_excluded = relative_path
+                .parent()
+                .is_some_and(|parent| excluded_dirs.contains(parent));
+
+            // If the parent directory is ignored, all children are ignored too.
+            if parent_is_excluded || globs.matched(relative_path, true).is_ignore() {
+                excluded_dirs.insert(relative_path.to_path_buf());
+            }
+        }
+
+        Self {
+            globs,
+            excluded_dirs,
+        }
+    }
+
+    /// Whether the package file path is excluded.
+    pub fn matches_file<T: AsRef<Path>>(&self, path: &PackagePath<T>) -> bool {
+        self.matches_relative_file(path.relative())
+    }
+
+    /// Whether the package relative file path is excluded.
+    pub fn matches_relative_file(&self, relative_path: impl AsRef<Path>) -> bool {
+        let relative_path = relative_path.as_ref();
+        if self.globs.matched(relative_path, false).is_ignore() {
+            return true;
+        }
+
+        relative_path
+            .parent()
+            .is_some_and(|parent| self.excluded_dirs.contains(parent))
+    }
 }
 
 fn check_template(
@@ -558,7 +642,7 @@ fn check_template(
         let template_dir = path.as_ref()?;
         Some(
             entrypoint
-                .map(|entrypoint| path::relative_to(template_dir.full(), entrypoint))
+                .map(|entrypoint| path::join_to(template_dir.full(), entrypoint))
                 .map(|path| PackagePath::from_full(package_dir, path)),
         )
     });
@@ -607,7 +691,7 @@ fn world_for_template(
 fn dont_exclude_template_files(
     diags: &mut Diagnostics,
     package_dir: &Path,
-    exclude: &Override,
+    exclude: &Exclude,
     template: &Spanned<Template>,
 ) {
     let Some(template_path) = &template.path else {
@@ -635,10 +719,7 @@ fn dont_exclude_template_files(
         }
 
         // For other files, check that they are indeed not excluded.
-        if exclude
-            .matched(entry.path(), entry.metadata().is_ok_and(|m| m.is_dir()))
-            .is_ignore()
-        {
+        if entry.metadata().is_ok_and(|m| m.is_file()) && exclude.matches_file(&entry_path) {
             diags.emit(
                 Diagnostic::error()
                     .with_code("exclude/template")
@@ -649,7 +730,7 @@ fn dont_exclude_template_files(
     }
 }
 
-fn check_thumbnail(diags: &mut Diagnostics, exclude: &Override, template: &Spanned<Template>) {
+fn check_thumbnail(diags: &mut Diagnostics, template: &Spanned<Template>) {
     let Some(thumbnail_path) = &template.thumbnail else {
         return;
     };
@@ -675,15 +756,6 @@ fn check_thumbnail(diags: &mut Diagnostics, exclude: &Override, template: &Spann
         )
     }
 
-    if exclude.matched(thumbnail_path.full(), false).is_ignore() {
-        diags.emit(
-            Diagnostic::error()
-                .with_label(Label::primary(manifest_id(), thumbnail_path.span()))
-                .with_code("manifest/template/thumbnail/exclude")
-                .with_message("The template thumbnail is automatically excluded"),
-        );
-    }
-
     if let Some(template_path) = &template.path
         && thumbnail_path.full().starts_with(template_path.full())
     {

src/check/path.rs

@@ -15,17 +15,10 @@ impl PackagePath<PathBuf> {
     /// Create a new package path from the package directory and a relative path
     /// within the directory.
     pub fn from_relative<T: AsRef<Path>>(package_dir: &Path, relative_path: T) -> Self {
-        let full_path = relative_to(package_dir, relative_path.as_ref());
+        let full_path = join_to(package_dir, relative_path.as_ref());
         let offset = package_dir.components().count();
         Self { offset, full_path }
     }
-
-    pub fn as_path(&self) -> PackagePath<&Path> {
-        PackagePath {
-            offset: self.offset,
-            full_path: self.full_path.as_path(),
-        }
-    }
 }
 
 impl<T: AsRef<Path>> PackagePath<T> {
@@ -76,7 +69,7 @@ impl<T: AsRef<Path>> PackagePath<T> {
 /// Strips any any leading root components (`/` or `\`) of the `path` before
 /// joining it to the `root` path. Absolute paths would otherwise replace the
 /// complete path when `join`ed with a parent path.
-pub fn relative_to(root: &Path, path: impl AsRef<Path>) -> PathBuf {
+pub fn join_to(root: &Path, path: impl AsRef<Path>) -> PathBuf {
     let components = path
         .as_ref()
         .components()

src/cli.rs

@@ -2,10 +2,10 @@ use std::path::PathBuf;
 use std::process::exit;
 
 use codespan_reporting::{diagnostic::Diagnostic, term};
-use ignore::overrides::Override;
 use tracing::error;
 use typst::syntax::{FileId, Source, package::PackageSpec};
 
+use crate::check::Exclude;
 use crate::{check::all_checks, package::PackageExt, world::SystemWorld};
 
 pub async fn main(spec_or_path: String, json_output: bool) {
@@ -59,7 +59,7 @@ pub fn print_diagnostics(
     // We should be able to print diagnostics even on excluded files. If we
     // don't remove the exclusion, it will fail to read and display the file
     // contents.
-    world.exclude(Override::empty());
+    world.exclude(Exclude::empty());
 
     for diagnostic in warnings.iter().chain(errors) {
         if json {