From 2db646c77925eace6723a3a96de57ea29087acb2 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 02:09:56 +0000 Subject: [PATCH] address review: rescan a directory reached from a shallower path, skipping files already collected --- apps/gridcraft-cli/src/check.rs | 51 +++++++++++++++++---------- apps/gridcraft-cli/src/check/tests.rs | 4 +-- apps/gridcraft-cli/tests/it/cli.rs | 20 +++++++++++ 3 files changed, 55 insertions(+), 20 deletions(-) diff --git a/apps/gridcraft-cli/src/check.rs b/apps/gridcraft-cli/src/check.rs index 1a19a1e9b..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,23 +280,28 @@ 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, 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 identity = (m.dev(), m.ino()); - // Already scanned: an alias. 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. - if seen.contains(&identity) { - return true; + // 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); + seen.insert(identity, depth); } } } @@ -329,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..cc8d1228f 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()); diff --git a/apps/gridcraft-cli/tests/it/cli.rs b/apps/gridcraft-cli/tests/it/cli.rs index ab549e341..e59295bc4 100644 --- a/apps/gridcraft-cli/tests/it/cli.rs +++ b/apps/gridcraft-cli/tests/it/cli.rs @@ -223,6 +223,26 @@ fn a_directory_past_the_depth_limit_is_still_scanned_when_named_as_an_input() { 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).