diff --git a/CHANGELOG.md b/CHANGELOG.md index c522d6f23..fd18df250 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ when output goes to a terminal, to prevent terminal escape-sequence injection. Also reject a placeholder as the executable for `--exec-batch`, while still allowing it for `--exec`. +- Fix broken symlinks being incorrectly filtered out by `--min-depth` when following links (`--follow`), because their depth was not computed; see #1017 (@hexbinoct). - Handle invalid working directories gracefully when using `--full-path`, see #1900 (@Xavrir). - Fire the "search pattern contains a path separator" diagnostic for any pattern containing `/`, not just patterns that happen to name an existing directory. Preserves the legacy Windows behaviour that also flags native `\` separators when the pattern resolves to a real directory. See #1873. - Also fire the "search pattern contains a path separator" diagnostic for `--and` patterns, not only the primary positional pattern. `--and` patterns are matched against the file name just like the primary pattern, so a path separator in them silently returned zero results. See #1873. diff --git a/Cargo.toml b/Cargo.toml index c9c847231..3f8f15efa 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,7 +34,7 @@ path = "src/main.rs" aho-corasick = "1.1" nu-ansi-term = "0.50" argmax = "0.4.0" -ignore = "0.4.25" +ignore = "0.4.28" regex = "1.12.2" regex-syntax = "0.8" ctrlc = "3.5" diff --git a/src/dir_entry.rs b/src/dir_entry.rs index c79c3e899..93aa514c0 100644 --- a/src/dir_entry.rs +++ b/src/dir_entry.rs @@ -11,7 +11,9 @@ use crate::filesystem::strip_current_dir; #[derive(Debug)] enum DirEntryInner { Normal(ignore::DirEntry), - BrokenSymlink(PathBuf), + // Broken symlinks reach us as walk errors rather than entries, so we carry + // over the depth the walker recorded on the error. + BrokenSymlink { path: PathBuf, depth: Option }, } #[derive(Debug)] @@ -31,9 +33,9 @@ impl DirEntry { } } - pub fn broken_symlink(path: PathBuf) -> Self { + pub fn broken_symlink(path: PathBuf, depth: Option) -> Self { Self { - inner: DirEntryInner::BrokenSymlink(path), + inner: DirEntryInner::BrokenSymlink { path, depth }, metadata: OnceCell::new(), style: OnceCell::new(), } @@ -42,14 +44,14 @@ impl DirEntry { pub fn path(&self) -> &Path { match &self.inner { DirEntryInner::Normal(e) => e.path(), - DirEntryInner::BrokenSymlink(pathbuf) => pathbuf.as_path(), + DirEntryInner::BrokenSymlink { path, .. } => path.as_path(), } } pub fn into_path(self) -> PathBuf { match self.inner { DirEntryInner::Normal(e) => e.into_path(), - DirEntryInner::BrokenSymlink(p) => p, + DirEntryInner::BrokenSymlink { path, .. } => path, } } @@ -82,7 +84,7 @@ impl DirEntry { pub fn file_type(&self) -> Option { match &self.inner { DirEntryInner::Normal(e) => e.file_type(), - DirEntryInner::BrokenSymlink(_) => self.metadata().map(|m| m.file_type()), + DirEntryInner::BrokenSymlink { .. } => self.metadata().map(|m| m.file_type()), } } @@ -90,7 +92,7 @@ impl DirEntry { self.metadata .get_or_init(|| match &self.inner { DirEntryInner::Normal(e) => e.metadata().ok(), - DirEntryInner::BrokenSymlink(path) => path.symlink_metadata().ok(), + DirEntryInner::BrokenSymlink { path, .. } => path.symlink_metadata().ok(), }) .as_ref() } @@ -98,7 +100,7 @@ impl DirEntry { pub fn depth(&self) -> Option { match &self.inner { DirEntryInner::Normal(e) => Some(e.depth()), - DirEntryInner::BrokenSymlink(_) => None, + DirEntryInner::BrokenSymlink { depth, .. } => *depth, } } @@ -144,7 +146,7 @@ impl Colorable for DirEntry { fn file_name(&self) -> OsString { let name = match &self.inner { DirEntryInner::Normal(e) => e.file_name(), - DirEntryInner::BrokenSymlink(path) => { + DirEntryInner::BrokenSymlink { path, .. } => { // Path::file_name() only works if the last component is Normal, // but we want it for all component types, so we open code it. // Copied from LsColors::style_for_path_with_metadata(). diff --git a/src/walk.rs b/src/walk.rs index 30128a0bc..1005209c5 100644 --- a/src/walk.rs +++ b/src/walk.rs @@ -2,7 +2,7 @@ use std::borrow::Cow; use std::ffi::OsStr; use std::io::{self, Write}; use std::mem; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex, MutexGuard}; use std::thread; @@ -484,24 +484,24 @@ impl WorkerState { } let entry = match entry { Ok(e) => DirEntry::normal(e), - Err(ignore::Error::WithPath { - path, - err: inner_err, - }) if inner_err - .io_error() - .is_some_and(|io_error| io_error.kind() == io::ErrorKind::NotFound) - && path - .symlink_metadata() - .ok() - .is_some_and(|m| m.file_type().is_symlink()) => - { - DirEntry::broken_symlink(path) - } Err(err) => { - return match tx.send(WorkerResult::Error(err)) { - Ok(_) => WalkState::Continue, - Err(_) => WalkState::Quit, - }; + // The depth has to be read off the error before it is + // taken apart, since it is recorded on an inner variant. + let depth = err.depth(); + match err { + ignore::Error::WithPath { + path, + err: inner_err, + } if is_broken_symlink(&path, &inner_err) => { + DirEntry::broken_symlink(path, depth) + } + err => { + return match tx.send(WorkerResult::Error(err)) { + Ok(_) => WalkState::Continue, + Err(_) => WalkState::Quit, + }; + } + } } }; @@ -653,6 +653,21 @@ impl WorkerState { } } +/// Whether a walk error is really a broken symlink rather than a failure worth +/// reporting. +/// +/// A symlink whose target is missing is surfaced by the walker as a NotFound +/// error against the link's own path, so it never arrives as an entry. fd still +/// wants to match and print it (see issue #1017), which means recovering it here. +fn is_broken_symlink(path: &Path, err: &ignore::Error) -> bool { + err.io_error() + .is_some_and(|io_error| io_error.kind() == io::ErrorKind::NotFound) + && path + .symlink_metadata() + .ok() + .is_some_and(|m| m.file_type().is_symlink()) +} + fn search_str_for_entry<'a>( entry_path: &'a std::path::Path, full_path_base: Option<&std::path::Path>, diff --git a/tests/tests.rs b/tests/tests.rs index 0fce5ed35..d6a9e7063 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -1208,6 +1208,207 @@ fn test_min_depth() { ); } +/// Minimum depth with a broken symlink (regression test for #1017) +/// +/// A broken symlink, surfaced while following links, has no depth reported by +/// the walker, so --min-depth used to drop it unconditionally. +#[test] +fn test_min_depth_broken_symlink() { + let mut te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES); + te.create_broken_symlink("one/two/broken_symlink") + .expect("Failed to create broken symlink."); + + // The broken symlink sits at depth 3, so it is kept up to that depth. + te.assert_output( + &[ + "--follow", + "--type", + "symlink", + "--min-depth", + "3", + "broken_symlink", + ], + "one/two/broken_symlink", + ); + + // A --min-depth beyond its actual depth must exclude it. + te.assert_output( + &[ + "--follow", + "--type", + "symlink", + "--min-depth", + "4", + "broken_symlink", + ], + "", + ); +} + +/// Minimum depth with a broken symlink combined with --absolute-path (#1017) +/// +/// With --absolute-path the search root is made absolute before walking, so the +/// broken symlink's depth must still be computed relative to that root rather +/// than from the absolute path's full component count. +#[test] +fn test_min_depth_broken_symlink_absolute_path() { + let (mut te, abs_path) = get_test_env_with_abs_path(DEFAULT_DIRS, DEFAULT_FILES); + te.create_broken_symlink("one/two/broken_symlink") + .expect("Failed to create broken symlink."); + + // The broken symlink sits at depth 3 relative to the (absolute) root. + te.assert_output( + &[ + "--follow", + "--absolute-path", + "--type", + "symlink", + "--min-depth", + "3", + "broken_symlink", + ], + &format!("{abs_path}/one/two/broken_symlink"), + ); + + // A --min-depth beyond its actual depth must exclude it. + te.assert_output( + &[ + "--follow", + "--absolute-path", + "--type", + "symlink", + "--min-depth", + "4", + "broken_symlink", + ], + "", + ); +} + +/// Minimum depth with a broken symlink under overlapping search roots (#1017) +/// +/// When two search roots overlap, the walker visits the same broken symlink once +/// per root, at a different depth each time. Here `one/two/broken_symlink` is at +/// depth 2 under root `one` and at depth 1 under root `one/two`, so --min-depth 2 +/// must keep the entry the walker reached via `one` and drop the one it reached +/// via `one/two`. A depth derived from the entry's path cannot distinguish the +/// two visits, since both carry the same path. +#[test] +fn test_min_depth_broken_symlink_overlapping_roots() { + let mut te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES); + te.create_broken_symlink("one/two/broken_symlink") + .expect("Failed to create broken symlink."); + + // Only the route through `one` reaches depth 2. + te.assert_output( + &[ + "--follow", + "--type", + "symlink", + "--min-depth", + "2", + "broken_symlink", + "one", + "one/two", + ], + "one/two/broken_symlink", + ); + + // Both routes clear --min-depth 1, so it is reported once per root. + te.assert_output( + &[ + "--follow", + "--type", + "symlink", + "--min-depth", + "1", + "broken_symlink", + "one", + "one/two", + ], + "one/two/broken_symlink + one/two/broken_symlink", + ); +} + +/// Maximum depth with a broken symlink (#1017) +/// +/// A broken symlink must be filtered by --max-depth like any other entry. The +/// default environment also exposes it through the followed `symlink` directory +/// (`symlink -> one/two`), so it is reachable at depth 2 as well as depth 3. +#[test] +fn test_max_depth_broken_symlink() { + let mut te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES); + te.create_broken_symlink("one/two/broken_symlink") + .expect("Failed to create broken symlink."); + + // --max-depth 3 keeps both routes to the broken symlink. + te.assert_output( + &[ + "--follow", + "--type", + "symlink", + "--max-depth", + "3", + "broken_symlink", + ], + "one/two/broken_symlink + symlink/broken_symlink", + ); + + // A --max-depth below either route must exclude it. + te.assert_output( + &[ + "--follow", + "--type", + "symlink", + "--max-depth", + "1", + "broken_symlink", + ], + "", + ); +} + +/// Exact depth with a broken symlink (#1017) +/// +/// A broken symlink must be kept only at its exact depth. It is reachable at +/// depth 3 (`one/two/broken_symlink`) and, through the followed `symlink` +/// directory, at depth 2 (`symlink/broken_symlink`). +#[test] +fn test_exact_depth_broken_symlink() { + let mut te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES); + te.create_broken_symlink("one/two/broken_symlink") + .expect("Failed to create broken symlink."); + + // Only the depth-3 route matches --exact-depth 3. + te.assert_output( + &[ + "--follow", + "--type", + "symlink", + "--exact-depth", + "3", + "broken_symlink", + ], + "one/two/broken_symlink", + ); + + // Only the depth-2 route (via the followed symlink) matches --exact-depth 2, + // which confirms the depth is computed relative to the search root. + te.assert_output( + &[ + "--follow", + "--type", + "symlink", + "--exact-depth", + "2", + "broken_symlink", + ], + "symlink/broken_symlink", + ); +} + /// Exact depth (--exact-depth) #[test] fn test_exact_depth() {