check, xtask stats: visited directories keyed by (dev, inode); stats reads only regular files (#3287)

* Directory walkers key visited directories by (dev, inode); stats skips non-regular or oversized .rs targets

* address review: record a walked directory only within the depth limit; stats skips symlinked sources and bounds the read

* address review: rescan a directory reached from a shallower path, skipping files already collected

* xtask stats: test the directory walk (links, identity, size limit)

* address review: create the test directories atomically and privately

* xtask stats tests: no unreachable! in the directory helper

* gridcraft check: unit-test that each workbook is listed once whatever path reaches it

apps/gridcraft-cli/src/check.rs

@@ -214,7 +214,7 @@ pub fn main(args: &[String]) -> ExitCode {
             Err(_) => return usage(format!("--max-report needs a whole number, not `{n}`")),
         }
     }
-    let Discovery { files, failures: mut reports } = collect_inputs(&a.positional);
+    let Discovery { files, failures: mut reports, .. } = collect_inputs(&a.positional);
     reports.extend(files.iter().map(|p| {
         let path = p.to_string_lossy().into_owned();
         match gridcraft_engine::io::read_file(&path) {
@@ -233,6 +233,9 @@ pub fn main(args: &[String]) -> ExitCode {
 #[derive(Default)]
 struct Discovery {
     files: Vec<PathBuf>,
+    /// Identity (dev+inode) of each collected file, so a rescan reached by a shallower path doesn't add it twice.
+    #[cfg(unix)]
+    file_ids: std::collections::HashSet<(u64, u64)>,
     failures: Vec<FileReport>,
 }
 
@@ -257,11 +260,11 @@ impl Discovery {
 /// (`~$…`) are skipped.
 fn collect_inputs(inputs: &[String]) -> Discovery {
     let mut out = Discovery::default();
-    let mut seen = std::collections::HashSet::new();
+    let mut seen = std::collections::HashMap::new();
     for i in inputs {
         let p = PathBuf::from(i);
         if p.is_dir() {
-            if !walk(&p, 0, &mut out, &mut seen) {
+            if !walk(&p, 0, false, &mut out, &mut seen) {
                 break;
             }
         } else if !out.add_file(p) {
@@ -277,19 +280,29 @@ fn collect_inputs(inputs: &[String]) -> Discovery {
     out
 }
 
-/// Returns false only when the global file budget stops traversal.
-fn walk(dir: &Path, depth: usize, out: &mut Discovery, seen: &mut std::collections::HashSet<u64>) -> bool {
-    // A directory is visited once by identity (dev+inode on unix): several links to the same
-    // directory are aliases, not exponential branches of the scan. Other platforms keep the
-    // depth cap only.
+/// Returns false only when the global file budget stops traversal. `seen` holds the shallowest
+/// depth each directory was scanned from; `rescan` is set below a directory reached again from a
+/// shallower path (its depth-limited subtree may now fit), where already collected files are skipped.
+#[cfg_attr(not(unix), allow(unused_mut))]
+fn walk(dir: &Path, depth: usize, mut rescan: bool, out: &mut Discovery, seen: &mut std::collections::HashMap<(u64, u64), usize>) -> bool {
+    // A directory is visited once by identity (dev+inode on unix) unless reached from a shallower
+    // path: several links to the same directory are aliases, not exponential branches of the scan
+    // (each directory is scanned at most MAX_DEPTH + 1 times). Other platforms keep the depth cap only.
     #[cfg(unix)]
     {
         use std::os::unix::fs::MetadataExt as _;
-        if let Ok(m) = std::fs::metadata(dir)
-            && let Some(id) = m.dev().checked_mul(1 << 60).and_then(|d| d.checked_add(m.ino()))
-            && !seen.insert(id)
-        {
-            return true;
+        if let Ok(m) = std::fs::metadata(dir) {
+            let identity = (m.dev(), m.ino());
+            // A directory past the depth limit is not scanned, so it is not recorded: asked for
+            // by name later, it is walked from depth 0.
+            match seen.get(&identity) {
+                Some(&shallowest) if shallowest <= depth => return true,
+                Some(_) => rescan = true,
+                None => {}
+            }
+            if depth <= MAX_DEPTH {
+                seen.insert(identity, depth);
+            }
         }
     }
     if depth > MAX_DEPTH {
@@ -324,13 +337,20 @@ fn walk(dir: &Path, depth: usize, out: &mut Discovery, seen: &mut std::collectio
             }
         };
         if metadata.is_dir() {
-            if !walk(&p, depth + 1, out, seen) {
+            if !walk(&p, depth + 1, rescan, out, seen) {
+                return false;
+            }
+        } else if p.extension().and_then(std::ffi::OsStr::to_str).is_some_and(|e| e.eq_ignore_ascii_case("xlsx") || e.eq_ignore_ascii_case("xlsm")) {
+            #[cfg(unix)]
+            {
+                use std::os::unix::fs::MetadataExt as _;
+                if !out.file_ids.insert((metadata.dev(), metadata.ino())) && rescan {
+                    continue;
+                }
+            }
+            if !out.add_file(p) {
                 return false;
             }
-        } else if p.extension().and_then(std::ffi::OsStr::to_str).is_some_and(|e| e.eq_ignore_ascii_case("xlsx") || e.eq_ignore_ascii_case("xlsm"))
-            && !out.add_file(p)
-        {
-            return false;
         }
     }
     true

apps/gridcraft-cli/src/check/tests.rs

@@ -11,8 +11,8 @@ fn discovery_reports_a_directory_read_error() {
     let path = std::env::temp_dir().join(format!("gridcraft-check-not-a-directory-{}", std::process::id()));
     std::fs::write(&path, "not a directory").unwrap();
     let mut discovery = Discovery::default();
-    let mut seen = std::collections::HashSet::new();
-    assert!(walk(&path, 0, &mut discovery, &mut seen));
+    let mut seen = std::collections::HashMap::new();
+    assert!(walk(&path, 0, false, &mut discovery, &mut seen));
     assert!(discovery.files.is_empty());
     assert_eq!(discovery.failures.len(), 1);
     assert_eq!(discovery.failures[0].path, path.to_string_lossy());
@@ -31,6 +31,47 @@ fn discovery_reports_omissions_only_after_the_file_limit() {
     assert!(discovery.failures[0].error.as_ref().unwrap().contains("file limit"));
 }
 
+/// A new directory under the temp dir: `create_dir` fails on a name somebody else made first.
+#[cfg(unix)]
+fn fresh_dir(tag: &str) -> PathBuf {
+    let mut n = 0u32;
+    loop {
+        let dir = std::env::temp_dir().join(format!("gridcraft-check-{tag}-{}-{n}", std::process::id()));
+        match std::fs::create_dir(&dir) {
+            Ok(()) => return dir,
+            Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => n += 1,
+            Err(e) => panic!("{e}"),
+        }
+    }
+}
+
+/// A directory named again by a deeper path, or by a link back to a parent, lists each workbook once.
+#[cfg(unix)]
+#[test]
+fn discovery_lists_each_workbook_once_whatever_path_reaches_it() {
+    let dir = fresh_dir("once");
+    let sub = dir.join("sub");
+    std::fs::create_dir(&sub).unwrap();
+    std::fs::write(dir.join("a.xlsx"), b"x").unwrap();
+    std::fs::write(sub.join("b.xlsx"), b"x").unwrap();
+    std::os::unix::fs::symlink(&dir, sub.join("loop")).unwrap();
+    let names = |d: &Discovery| {
+        let mut v: Vec<String> = d.files.iter().map(|p| p.file_name().unwrap().to_string_lossy().into_owned()).collect();
+        v.sort();
+        v
+    };
+    // The link back to the top is one directory, not another branch of the scan.
+    let found = collect_inputs(&[dir.to_string_lossy().into_owned()]);
+    assert_eq!(names(&found), ["a.xlsx", "b.xlsx"]);
+    // `sub` named after its parent is reached from a shallower depth: scanned again, nothing listed twice.
+    let found = collect_inputs(&[dir.to_string_lossy().into_owned(), sub.to_string_lossy().into_owned()]);
+    assert_eq!(names(&found), ["a.xlsx", "b.xlsx"]);
+    // Named first, the same `sub` is the shallowest path and the parent's scan skips it.
+    let found = collect_inputs(&[sub.to_string_lossy().into_owned(), dir.to_string_lossy().into_owned()]);
+    assert_eq!(names(&found), ["a.xlsx", "b.xlsx"]);
+    let _ = std::fs::remove_dir_all(dir);
+}
+
 fn at(a1: &str) -> CellRef {
     CellRef::parse(a1).unwrap()
 }

apps/gridcraft-cli/tests/it/cli.rs

@@ -205,6 +205,44 @@ fn check_reports_a_depth_limited_scan_while_checking_readable_files() {
     let _ = std::fs::remove_dir_all(dir);
 }
 
+#[cfg(unix)]
+#[test]
+fn a_directory_past_the_depth_limit_is_still_scanned_when_named_as_an_input() {
+    let dir = tmpdir("check-depth-named");
+    // The first folder past the limit (33 levels down): the scan from the top reports it unreached.
+    let mut deep = dir.clone();
+    for _ in 0..33 {
+        deep.push("d");
+    }
+    std::fs::create_dir_all(&deep).unwrap();
+    ok(&["run", "--out", deep.join("deep.xlsx").to_str().unwrap(), "--quiet"]);
+    let out = run(&["check", dir.to_str().unwrap(), deep.to_str().unwrap(), "--json"]);
+    let report: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap();
+    // The first scan reports the subtree it could not reach; the named directory is checked.
+    assert_eq!(report["totals"]["opened"], 1, "{report}");
+    let _ = std::fs::remove_dir_all(dir);
+}
+
+#[cfg(unix)]
+#[test]
+fn a_directory_first_reached_at_the_depth_limit_is_completed_when_named_as_an_input() {
+    let dir = tmpdir("check-depth-shallower");
+    // The 32nd folder is scanned from the top, but its child (and the workbook in it) is past the limit.
+    let mut limit = dir.clone();
+    for _ in 0..32 {
+        limit.push("d");
+    }
+    let child = limit.join("c");
+    std::fs::create_dir_all(&child).unwrap();
+    ok(&["run", "--out", limit.join("at_limit.xlsx").to_str().unwrap(), "--quiet"]);
+    ok(&["run", "--out", child.join("below.xlsx").to_str().unwrap(), "--quiet"]);
+    let out = run(&["check", dir.to_str().unwrap(), limit.to_str().unwrap(), "--json"]);
+    let report: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap();
+    // Named from depth 0 the subtree is read: the workbook below the limit is checked, the one at it only once.
+    assert_eq!(report["totals"]["opened"], 2, "{report}");
+    let _ = std::fs::remove_dir_all(dir);
+}
+
 /// A script line whose parameters are under a key `run` does not read used to run the command
 /// with no parameters at all, and the run still exited 0: three mistaken lines made Sheet2,
 /// Sheet3 and Sheet4 instead of the named sheets, and nothing said so (#2256).

xtask/src/stats.rs

@@ -13,7 +13,10 @@ struct Counts {
     proptests: usize,
 }
 
-fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet<u64>, budget: &mut usize) {
+/// Larger files (or devices behind a symlink) are not source files and are not read.
+const MAX_SOURCE_BYTES: u64 = 16 << 20;
+
+fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet<(u64, u64)>, budget: &mut usize) {
     if *budget == 0 {
         // Hard stop for pathological trees: two directory symlinks pointing at each other
         // would otherwise be walked exponentially.
@@ -22,9 +25,9 @@ fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet<u64>, b
     // Directory identities (dev+inode on unix) make a symlink to a visited directory a
     // no-op, not another exponential branch of the scan.
     #[cfg(unix)]
-    let id = std::fs::metadata(dir).ok().and_then(|m| m.dev().checked_mul(1 << 60).and_then(|d| d.checked_add(m.ino())));
+    let id = std::fs::metadata(dir).ok().map(|m| (m.dev(), m.ino()));
     #[cfg(not(unix))]
-    let id: Option<u64> = None;
+    let id: Option<(u64, u64)> = None;
     if let Some(id) = id
         && !seen.insert(id)
     {
@@ -45,7 +48,9 @@ fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet<u64>, b
                 walk(&p, c, seen, budget);
             }
         } else if name.ends_with(".rs")
-            && let Ok(s) = std::fs::read_to_string(&p)
+            // The entry's own metadata does not follow a symlink: only regular files are sources.
+            && e.metadata().is_ok_and(|m| m.is_file() && m.len() <= MAX_SOURCE_BYTES)
+            && let Some(s) = read_source(&p)
         {
             c.files += 1;
             c.lines += s.lines().count();
@@ -56,6 +61,15 @@ fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet<u64>, b
     }
 }
 
+/// A source file's text, read up to [`MAX_SOURCE_BYTES`]: one that grew or was replaced since its
+/// size was checked is skipped, not read whole.
+fn read_source(p: &Path) -> Option<String> {
+    use std::io::Read as _;
+    let mut s = String::new();
+    std::fs::File::open(p).ok()?.take(MAX_SOURCE_BYTES + 1).read_to_string(&mut s).ok()?;
+    (s.len() as u64 <= MAX_SOURCE_BYTES).then_some(s)
+}
+
 /// `#[test]` attributes and `proptest!` blocks (each block may hold many
 /// property tests, and macro-generated tests are only counted once — use
 /// `--exact` for harness-accurate numbers).
@@ -129,11 +143,61 @@ pub fn run(_root: &Path, exact: bool) -> Result<(), String> {
 
 #[cfg(test)]
 mod tests {
-    use super::count_tests;
+    use super::{Counts, MAX_SOURCE_BYTES, count_tests, read_source, walk};
+
+    /// A new, private directory: `create_dir` fails if the name exists (even as a link), so a
+    /// directory somebody else made in advance is never used.
+    fn private_dir(tag: &str) -> std::path::PathBuf {
+        let mut n = 0u32;
+        loop {
+            let dir = std::env::temp_dir().join(format!("xtask-stats-{tag}-{}-{n}", std::process::id()));
+            let mut b = std::fs::DirBuilder::new();
+            #[cfg(unix)]
+            std::os::unix::fs::DirBuilderExt::mode(&mut b, 0o700);
+            match b.create(&dir) {
+                Ok(()) => return dir,
+                Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {}
+                Err(e) => panic!("{e}"),
+            }
+            n += 1;
+        }
+    }
 
     #[test]
     fn counts_attributes_and_proptest_blocks() {
         let src = "#[test]\nfn a() {}\n// #[test] commented\n    #[test]\nfn b() {}\nproptest! {\n}\n";
         assert_eq!(count_tests(src), (2, 1));
     }
+
+    #[test]
+    fn a_source_past_the_size_limit_is_not_read() {
+        let dir = private_dir("size");
+        let (small, big) = (dir.join("a.rs"), dir.join("b.rs"));
+        std::fs::write(&small, "fn a() {}\n").unwrap();
+        let file = std::fs::File::create(&big).unwrap();
+        file.set_len(MAX_SOURCE_BYTES + 1).unwrap(); // sparse: nothing is written
+        drop(file);
+        assert_eq!(read_source(&small).as_deref(), Some("fn a() {}\n"));
+        assert_eq!(read_source(&big), None);
+        let _ = std::fs::remove_dir_all(dir);
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn walk_counts_regular_sources_once_and_skips_links_and_big_files() {
+        let dir = private_dir("walk");
+        let sub = dir.join("sub");
+        std::fs::create_dir(&sub).unwrap();
+        std::fs::write(dir.join("a.rs"), "#[test]\nfn a() {}\n").unwrap();
+        std::fs::write(sub.join("b.rs"), "fn b() {}\n").unwrap();
+        // A directory link back to the root is one identity, a file link is not a regular file,
+        // and a source over the size limit is not read.
+        std::os::unix::fs::symlink(&dir, sub.join("loop")).unwrap();
+        std::os::unix::fs::symlink(dir.join("a.rs"), sub.join("link.rs")).unwrap();
+        std::fs::File::create(dir.join("big.rs")).unwrap().set_len(MAX_SOURCE_BYTES + 1).unwrap();
+        let mut c = Counts::default();
+        walk(&dir, &mut c, &mut std::collections::HashSet::new(), &mut 100);
+        assert_eq!((c.files, c.tests), (2, 1));
+        let _ = std::fs::remove_dir_all(dir);
+    }
 }