diff --git a/apps/gridcraft-cli/src/check.rs b/apps/gridcraft-cli/src/check.rs index 5fc37b14c..cfd583baf 100644 --- a/apps/gridcraft-cli/src/check.rs +++ b/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, + /// 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, } @@ -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) -> 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 diff --git a/apps/gridcraft-cli/src/check/tests.rs b/apps/gridcraft-cli/src/check/tests.rs index 57baa2198..d85cb3a3a 100644 --- a/apps/gridcraft-cli/src/check/tests.rs +++ b/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 = 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() } diff --git a/apps/gridcraft-cli/tests/it/cli.rs b/apps/gridcraft-cli/tests/it/cli.rs index dde018ca8..e59295bc4 100644 --- a/apps/gridcraft-cli/tests/it/cli.rs +++ b/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). diff --git a/xtask/src/stats.rs b/xtask/src/stats.rs index 462f1c7de..269649b17 100644 --- a/xtask/src/stats.rs +++ b/xtask/src/stats.rs @@ -13,7 +13,10 @@ struct Counts { proptests: usize, } -fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet, 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, 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 = 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, 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, 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 { + 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); + } }