Skip to content
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.

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

Closes #535 --prune option #546

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ pub fn build_app() -> App<'static, 'static> {
.short("0")
.overrides_with("print0"),
)
.arg(arg("prune").long("prune").hidden_short_help(true))
.arg(arg("depth").long("max-depth").short("d").takes_value(true))
// support --maxdepth as well, for compatibility with rg
.arg(
Expand Down Expand Up @@ -351,6 +352,10 @@ fn usage() -> HashMap<&'static str, Help> {
, "Separate results by the null character"
, "Separate search results by the null character (instead of newlines). Useful for \
piping results to 'xargs'.");
doc!(h, "prune"
, "Do not descend into matched directories"
, "When a directory matches the pattern, fd does not descend into it but still shows \
all matches at the same level as that directory.");
Comment on lines +355 to +358

Choose a reason for hiding this comment

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

To me, this is slightly ambiguous for people who aren't familiar with find's -prune option. In particular, I think it should be more explicitly stated that the matched directories are included in the output. Maybe something like:

"Do not descend into matched directories"
"When a directory matches the pattern, fd does not descend into it. The directory itself is not excluded."

doc!(h, "depth"
, "Set maximum search depth (default: none)"
, "Limit the directory traversal to a given depth. By default, there is no limit \
Expand Down
3 changes: 3 additions & 0 deletions src/internal/opts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ pub struct FdOptions {
/// Whether elements of output should be separated by a null character
pub null_separator: bool,

/// Whether to not descend into matching directories.
pub prune: bool,

/// The maximum search depth, or `None` if no maximum search depth should be set.
///
/// A depth of `1` includes all files under the current directory, a depth of `2` also includes
Expand Down
1 change: 1 addition & 0 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ fn main() {
follow_links: matches.is_present("follow"),
one_file_system: matches.is_present("one-file-system"),
null_separator: matches.is_present("null_separator"),
prune: matches.is_present("prune"),
max_depth: matches
.value_of("depth")
.or_else(|| matches.value_of("rg-depth"))
Expand Down
25 changes: 18 additions & 7 deletions src/walk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,17 @@ fn spawn_senders(
return ignore::WalkState::Continue;
}

// when pruning, don't descend into matching dirs
// note that prune requires pattern
let walk_action = if config.prune
&& pattern.as_str().len() != 0
Copy link
Owner

Choose a reason for hiding this comment

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

I don't really understand why this is necessary?

Copy link
Author

Choose a reason for hiding this comment

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

I added it so that the prune is only evaluated if there is a pattern specified, thinking that the prune option should only be used with a pattern. (and I added a test case for this). But, I suppose it could be allowed and then the expected behavior would be that no directories would be descended into. What do you think/prefer?

Copy link
Owner

Choose a reason for hiding this comment

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

Hm, "not having a pattern" does not only refer to cases where --type is used, right? It could also be a unconstrained search, a --size search or a --changed-{within,before} search.

I agree that --prune maybe doesn't make sense in these cases, but we should take a closer look, I think. We could use the find behavior as a guideline.

If two options in combination do not make sense, we should also think about disallowing that via .conflicts_with.

Copy link
Author

Choose a reason for hiding this comment

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

Hm, "not having a pattern" does not only refer to cases where --type is used, right? It could also be a unconstrained search, a --size search or a --changed-{within,before} search.

Good point! I hadn't considered that...

I agree that --prune maybe doesn't make sense in these cases, but we should take a closer look, I think. We could use the find behavior as a guideline.

Lets see. If we are using find as an example, it seems like the -prune option actually takes an argument on what to prune.
(I found this explanation helpful: https://stackoverflow.com/questions/1489277/how-to-use-prune-option-of-find-in-sh)

Currently this implementation uses the <pattern> as what to also prune. But I suppose we could make --prune take an argument, stating what to prune. In this case, the user has more control over what to prune. for example:

$ fd foo --prune=bar

, will look for matches with foo but not descend into dirs matching bar.

This would eliminate the need to do the check for pattern on line 369.
What do you think?
Yet it almost sounds like the functionality of --exclude <pattern> but specific to directories. hmmmm... are they too similar?

If two options in combination do not make sense, we should also think about disallowing that via .conflicts_with.

That is good to know about!
After reading your comment, and looking at the find documentation
http://man7.org/linux/man-pages/man1/find.1.html

  • The find -prune seems to conflict with -depth, so that could be something to consider, to mark conflicts _with on --max-depth?

  • I'm not convinced we'd need to mark prune as conflicts with --size or --changed-{} , if we added an argument to it as I proposed above. Someone could want to see files of a certain size but omitting some directories, maybe? But if we don't go with adding an argument to prune, marking as conflicts with --size and --changed-{} might be a better way to enforce the need for a to prune on.

Choose a reason for hiding this comment

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

Yet it almost sounds like the functionality of --exclude but specific to directories. hmmmm... are they too similar?

FWIW, the -E option invokes ripgreps gitignore style excludes. To ignore a directory -E foo/ should work. Eg.

[squirt ~/tmp/tests]
$ tree -p
.
├── [drwxr-xr-x]  bar
│   └── [drwxr-xr-x]  lower_dir
│       └── [-rw-r--r--]  bar
└── [drwxr-xr-x]  foo
    └── [drwxr-xr-x]  lower_dir
        └── [-rw-r--r--]  bar

4 directories, 2 files
[squirt ~/tmp/tests]
$ fd -E bar/
foo
foo/lower_dir
foo/lower_dir/bar

&& entry.file_type().map_or(false, |e| e.is_dir())
{
ignore::WalkState::Skip
} else {
ignore::WalkState::Continue
};

// Filter out unwanted extensions.
if let Some(ref exts_regex) = config.extensions {
if let Some(path_str) = entry_path.file_name() {
Expand All @@ -387,10 +398,10 @@ fn spawn_senders(
|| (file_types.empty_only && !fshelper::is_empty(&entry))
|| !(entry_type.is_file() || entry_type.is_dir() || entry_type.is_symlink())
{
return ignore::WalkState::Continue;
return walk_action;
}
} else {
return ignore::WalkState::Continue;
return walk_action;
}
}

Expand All @@ -404,13 +415,13 @@ fn spawn_senders(
.iter()
.any(|sc| !sc.is_within(file_size))
{
return ignore::WalkState::Continue;
return walk_action;
}
} else {
return ignore::WalkState::Continue;
return walk_action;
}
} else {
return ignore::WalkState::Continue;
return walk_action;
}
}

Expand All @@ -426,7 +437,7 @@ fn spawn_senders(
}
}
if !matched {
return ignore::WalkState::Continue;
return walk_action;
}
}

Expand All @@ -436,7 +447,7 @@ fn spawn_senders(
return ignore::WalkState::Quit;
}

ignore::WalkState::Continue
walk_action
})
});
}
74 changes: 74 additions & 0 deletions tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1470,3 +1470,77 @@ fn test_base_directory() {
),
);
}

/// pruning (--prune)
#[test]
fn test_prune() {
let dirs = &["foo", "bar/zap/foo"];
let files = &[
"foo/a.foo",
"foo/b.bar",
"bar/zap/c.foo",
"bar/zap/foo/d.foo",
];
let te = TestEnv::new(dirs, files);

// before prune
te.assert_output(
&["foo"],
"bar/zap/foo/d.foo
bar/zap/c.foo
bar/zap/foo
foo/a.foo
foo",
);
// with prune
te.assert_output(
&["--prune", "foo"],
"bar/zap/c.foo
bar/zap/foo
foo",
);
neuronull marked this conversation as resolved.
Show resolved Hide resolved

// --type f
te.assert_output(&["--prune", "--type", "f", "foo"], "bar/zap/c.foo");
// --type d
te.assert_output(
&["--prune", "--type", "d", "foo"],
"bar/zap/foo
foo",
);
// --prune with other options but no pattern
te.assert_output(
&["--prune", "--type", "d"],
"bar/zap/foo
bar/zap
bar
foo",
);

// --max-depth
// before prune
te.assert_output(
&["--max-depth", "3", "foo"],
"bar/zap/foo
bar/zap/c.foo
foo/a.foo
foo",
);
// with prune
te.assert_output(
&["--prune", "--max-depth", "3", "foo"],
"bar/zap/foo
bar/zap/c.foo
foo",
);

// --exclude
// before prune
te.assert_output(
&["foo", "--exclude", "zap"],
"foo
foo/a.foo",
);
// with prune
te.assert_output(&["foo", "--prune", "--exclude", "zap"], "foo");
}