Skip to content

refactor(walk): extract entry filtering and normalization logic #2043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
parneetsingh022 wants to merge 9 commits into
base: master
Choose a base branch
from
Open

refactor(walk): extract entry filtering and normalization logic #2043

Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5022954
refactor(walk): extract entry filtering and normalization logic
parneetsingh022 Jun 29, 2026
2849149
fixed fmt and clippy issues
parneetsingh022 Jun 29, 2026
6b68d13
Merge branch 'master' into refactor/walk-entry-filtering
parneetsingh022 Jul 2, 2026
6ead00f
Resolve merge conflicts
parneetsingh022 Jul 2, 2026
03fd037
Rerun CI
parneetsingh022 Jul 2, 2026
cd161f4
refactor: address PR review feedback
parneetsingh022 Jul 7, 2026
7d778c7
fix: clippy
parneetsingh022 Jul 7, 2026
a79e718
added doc-string for is_below_min_depth functin
parneetsingh022 Jul 7, 2026
a191c8e
refactor: updated evaluate and evaluate_raw to return Result in filter
parneetsingh022 Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
365 changes: 242 additions & 123 deletions src/walk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,238 @@ impl<'a, W: Write> ReceiverBuffer<'a, W> {
}
}

struct EntryFilter<'a> {
config: &'a Config,
patterns: &'a [Regex],
}

impl<'a> EntryFilter<'a> {
pub fn new(config: &'a Config, patterns: &'a [Regex]) -> Self {
Self { config, patterns }
}

/// Fast-path checks that operate on the raw ignore::DirEntry
fn evaluate_before_normalization(&self, entry: &ignore::DirEntry) -> Option<WalkState> {
Comment thread
parneetsingh022 marked this conversation as resolved.
Outdated
if let Some(state) = self.check_ignore_file(entry) {
return Some(state);
}
if let Some(state) = self.check_root_dir(entry) {
return Some(state);
}
None
}

/// Evaluates a normalized entry against all configured constraints to determine its walk state.
fn evaluate(&self, entry: &DirEntry) -> Option<WalkState> {

@tmccombs tmccombs Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be worth using a macro like

macro_rules! entry_filter {
  ($e:expr) => {
    let result = $e;
    if result.is_some() {
      return result;
    }
  }
}

Then each of these checks becomes entry_filter!(self.check_min_depth(entry)).

Or alternatively, have this return Result<(), WalkState>, and then this becomes

self.check_min_depth(entry)?;
self.check_patterns(entry)?;
//...

@parneetsingh022 parneetsingh022 Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal functions now return a bool, I guess this might be better implementation than using a macro (which would have been effective on previous implementation).

if self.is_below_min_depth(entry)
    || self.fails_pattern_filter(entry)
    || self.fails_extension_filter(entry)
    || self.matches_ignored_file_type(entry)
    || fails_owner_constraints
    || self.fails_size_constraints(entry)
    || self.fails_modification_time_constraints(entry)
{
    return Err(WalkState::Continue);
}

if let Some(state) = self.check_min_depth(entry) {
return Some(state);
}
if let Some(state) = self.check_patterns(entry) {
return Some(state);
}
if let Some(state) = self.check_extensions(entry) {
return Some(state);
}
if let Some(state) = self.check_file_types(entry) {
return Some(state);
}
if let Some(state) = self.check_owner(entry) {
return Some(state);
}
if let Some(state) = self.check_size(entry) {
return Some(state);
}
if let Some(state) = self.check_modification_time(entry) {
return Some(state);
}
None
}

fn check_ignore_file(&self, entry: &ignore::DirEntry) -> Option<WalkState> {
Comment thread
parneetsingh022 marked this conversation as resolved.
Outdated
// If the entry is a directory that contains a
// "ignore contain" file, we want to skip this
// directory.
// Check the filetype first to avoid unnecessary
// syscalls.
if entry.file_type().is_some_and(|t| t.is_dir()) {
let entry_path = entry.path();
if self
.config
.ignore_contain
.iter()
.any(|ic| entry_path.join(ic).exists())
{
return Some(WalkState::Skip);
}
}

None
}

fn check_root_dir(&self, entry: &ignore::DirEntry) -> Option<WalkState> {
Comment thread
parneetsingh022 marked this conversation as resolved.
Outdated
if entry.depth() == 0 {
// Skip the root directory entry.
return Some(WalkState::Continue);
}

None
}

fn check_min_depth(&self, entry: &DirEntry) -> Option<WalkState> {

@tmccombs tmccombs Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these functions are actually just checking a condition, and return WalkState::Continue. I wonder if i instead of these returning Option, it woudl be simpler to just have them return bool, and do the return Some(WalkState::Continue)` in the evaluate method

@parneetsingh022 parneetsingh022 Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you are right. There is no point in returning Option<WalkState> on every function. I've refactored the code to return bool, and now only evaluate and evaluate_raw functions use Result<(), WalkState>.

let min_depth = self.config.min_depth?;

if entry.depth().is_none_or(|depth| depth < min_depth) {
return Some(WalkState::Continue);
}

None
}

fn check_patterns(&self, entry: &DirEntry) -> Option<WalkState> {
let entry_path = entry.path();

let search_str = search_str_for_entry(entry_path, self.config.full_path_base.as_deref());

if !self
.patterns
.iter()
.all(|pat| pat.is_match(&filesystem::osstr_to_bytes(search_str.as_ref())))
{
return Some(WalkState::Continue);
}

None
}

fn check_extensions(&self, entry: &DirEntry) -> Option<WalkState> {
Comment thread
parneetsingh022 marked this conversation as resolved.
Outdated
let entry_path = entry.path();
if let Some(ref exts_regex) = self.config.extensions {
if let Some(path_str) = entry_path.file_name() {
if !exts_regex.is_match(&filesystem::osstr_to_bytes(path_str)) {
return Some(WalkState::Continue);
}
} else {
return Some(WalkState::Continue);
}
}
None
}

fn check_file_types(&self, entry: &DirEntry) -> Option<WalkState> {
if let Some(ref file_types) = self.config.file_types
&& file_types.should_ignore(entry)
{
return Some(WalkState::Continue);
}

None
}

fn check_size(&self, entry: &DirEntry) -> Option<WalkState> {
let entry_path = entry.path();
if !self.config.size_constraints.is_empty() {
if entry_path.is_file() {
if let Some(metadata) = entry.metadata() {
let file_size = metadata.len();
if self
.config
.size_constraints
.iter()
.any(|sc| !sc.is_within(file_size))
{
return Some(WalkState::Continue);
}
} else {
return Some(WalkState::Continue);
}
} else {
return Some(WalkState::Continue);
}
}

None
}

fn check_modification_time(&self, entry: &DirEntry) -> Option<WalkState> {
if !self.config.time_constraints.is_empty() {
let mut matched = false;
if let Some(metadata) = entry.metadata()
&& let Ok(modified) = metadata.modified()
{
matched = self
.config
.time_constraints
.iter()
.all(|tf| tf.applies_to(&modified));
}
if !matched {
return Some(WalkState::Continue);
}
}

None
}

#[cfg(unix)]
fn check_owner(&self, entry: &DirEntry) -> Option<WalkState> {
Comment thread
parneetsingh022 marked this conversation as resolved.
Outdated
if let Some(ref owner_constraint) = self.config.owner_constraint {
if let Some(metadata) = entry.metadata() {
if !owner_constraint.matches(metadata) {
return Some(WalkState::Continue);
}
} else {
return Some(WalkState::Continue);
}
}

None
}

#[cfg(not(unix))]
#[inline]

@tmccombs tmccombs Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would hope this inline annotation isn't necessary

@tmccombs tmccombs Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it might be better to just not have this defined for non-unix, and have a #[cfg(unix)] on the call as well.

@parneetsingh022 parneetsingh022 Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that makes sense. I removed the extra non-Unix helper implementation.

Instead, the owner constraint check is now only called on Unix, and non-Unix builds just set fails_owner_constraints to false inside evaluate:

#[cfg(unix)]
let fails_owner_constraints = self.fails_owner_constraints(entry);

#[cfg(not(unix))]
let fails_owner_constraints = false;

fn check_owner(&self, _entry: &DirEntry) -> Option<WalkState> {
None
}
}

/// Converts an `ignore` walker entry into a `DirEntry`.
///
/// Normal entries are wrapped directly. Broken symlinks are recovered from
/// `NotFound` errors and returned as `DirEntry::broken_symlink`. All other
/// errors are forwarded to the worker result channel, returning `Continue` if
/// the error was sent and `Quit` if the channel is closed.
fn normalize_walk_entry(
entry: Result<ignore::DirEntry, ignore::Error>,
tx: &mut BatchSender,
) -> Result<DirEntry, WalkState> {
match entry {
Ok(e) => Ok(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()) =>
{
Ok(DirEntry::broken_symlink(path))
}

Err(err) => {
let result = tx.send(WorkerResult::Error(err));

match result {
Ok(_) => Err(WalkState::Continue),
Err(_) => Err(WalkState::Quit),
}
}
}
}

/// State shared by the sender and receiver threads.
struct WorkerState {
/// The search patterns.
Expand Down Expand Up @@ -445,6 +677,7 @@ impl WorkerState {
let patterns = &self.patterns;
let config = &self.config;
let quit_flag = self.quit_flag.as_ref();
let filter = EntryFilter::new(config, patterns);

let mut limit = 0x100;
if let Some(cmd) = &config.command
Expand All @@ -461,133 +694,19 @@ impl WorkerState {
return WalkState::Quit;
}

if let Ok(e) = &entry {
// If the entry is a directory that contains a
// "ignore contain" file", we want to skip this
// directory.
// Check the filetype first to avoid unnecessary
// syscalls.
if e.file_type().is_some_and(|t| t.is_dir()) {
let entry_path = e.path();
if config
.ignore_contain
.iter()
.any(|ic| entry_path.join(ic).exists())
{
return WalkState::Skip;
}
}
if e.depth() == 0 {
// Skip the root directory entry.
return WalkState::Continue;
}
}
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,
};
}
};

if let Some(min_depth) = config.min_depth
&& entry.depth().is_none_or(|d| d < min_depth)
{
return WalkState::Continue;
}

// Check the name first, since it doesn't require metadata
let entry_path = entry.path();

let search_str = search_str_for_entry(entry_path, config.full_path_base.as_deref());

if !patterns
.iter()
.all(|pat| pat.is_match(&filesystem::osstr_to_bytes(search_str.as_ref())))
{
return WalkState::Continue;
}

// Filter out unwanted extensions.
if let Some(ref exts_regex) = config.extensions {
if let Some(path_str) = entry_path.file_name() {
if !exts_regex.is_match(&filesystem::osstr_to_bytes(path_str)) {
return WalkState::Continue;
}
} else {
return WalkState::Continue;
}
}

// Filter out unwanted file types.
if let Some(ref file_types) = config.file_types
&& file_types.should_ignore(&entry)
if let Ok(e) = &entry
&& let Some(state) = filter.evaluate_before_normalization(e)
{
return WalkState::Continue;
return state;
}

#[cfg(unix)]
{
if let Some(ref owner_constraint) = config.owner_constraint {
if let Some(metadata) = entry.metadata() {
if !owner_constraint.matches(metadata) {
return WalkState::Continue;
}
} else {
return WalkState::Continue;
}
}
}

// Filter out unwanted sizes if it is a file and we have been given size constraints.
if !config.size_constraints.is_empty() {
if entry_path.is_file() {
if let Some(metadata) = entry.metadata() {
let file_size = metadata.len();
if config
.size_constraints
.iter()
.any(|sc| !sc.is_within(file_size))
{
return WalkState::Continue;
}
} else {
return WalkState::Continue;
}
} else {
return WalkState::Continue;
}
}
let entry = match normalize_walk_entry(entry, &mut tx) {
Ok(entry) => entry,
Err(state) => return state,
};

// Filter out unwanted modification times
if !config.time_constraints.is_empty() {
let mut matched = false;
if let Some(metadata) = entry.metadata()
&& let Ok(modified) = metadata.modified()
{
matched = config
.time_constraints
.iter()
.all(|tf| tf.applies_to(&modified));
}
if !matched {
return WalkState::Continue;
}
if let Some(state) = filter.evaluate(&entry) {
return state;
}

if config.is_printing()
Expand Down