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

Add --filter argument that filters results based on command exit-code #1545

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

Conversation

braden-godley
Copy link

Based on the RFC at #400.

This PR adds in a new argument, -f / --filter <command> which will filter all the results based on the result of the command. If it returns a non-zero exit code, the result will be filtered out.

This is my first Rust PR, please let me know if there is anything I can do to improve this!

Arg::new("filter")
.action(ArgAction::Append)
.long("filter")
.short('f')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if we should use a short option for this. After all we don't have a short option for --exec or --exec-batch which I think are probably more common than --filter. And we may want to use -f for something else in the future (maybe a --format option? ). It is easier to add this alias later if desired than to remove it, so let's leave it off for now.

.allow_hyphen_values(true)
.value_terminator(";")
.value_name("cmd")
.conflicts_with_all(["exec", "exec_batch", "list_details"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this need to conflict?

Logically, it makes sense to allow filtering items first, and then passing the results that succeed through to exec or exec-batch.

Copy link
Author

Choose a reason for hiding this comment

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

I like this idea, I'll consider how we can make these commands compatible.

.long_help(
"Execute a command in parallel for each search result, filtering out results where the exit code is non-zero. \
There is no guarantee of the order commands are executed in, and the order should not be depended upon. \
All positional arguments following --filter are considered to be arguments to the command - not to fd. \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe mention that you can end with a \;?

@@ -776,6 +776,11 @@ impl clap::FromArgMatches for Exec {
.get_occurrences::<String>("exec_batch")
.map(CommandSet::new_batch)
})
.or_else(|| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be implemented in a way that would allow using it along with exec or exec-batch?

out_perm: &Mutex<()>,
) -> ExitCode {
let mut output_buffer = OutputBuffer::new(out_perm);
let path = format!("{}\n", path.to_str().unwrap());
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we add a newline to the path?

Copy link
Author

Choose a reason for hiding this comment

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

The newline is necessary for each result to be shown on a different line if it passed the filter

cmds: I,
out_perm: &Mutex<()>,
) -> ExitCode {
let mut output_buffer = OutputBuffer::new(out_perm);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Writing to this output buffer skips all the logic in output.rs.

The filter code shouldn't be handling output at all.

Rather, this should be used as one of the filters that we use to exclude files to be processed in walk.rs in the spawn_senders function.

Copy link
Author

Choose a reason for hiding this comment

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

I based this on how the --exec command does the same. Should we change that as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, with exec, the output of the command itself is the desired output, but with filter, the desired output is the path (but possibly with some transformations done on it).

The question here is what should we do with the output from the filter command, if any (I imagine in most cases there won't be any).

Some options are:

  1. Pipe to /dev/null or equivalent
  2. Read, and immediately drop
  3. Collect into buffer, then write to stderr, so that error messages are preserved.
  4. Collect all filter output, then print all of it at the end (probably on stderr).

I do think if we do print the output it should be to stderr.

@@ -11,6 +11,8 @@ struct Outputs {
stdout: Vec<u8>,
stderr: Vec<u8>,
}

/// Used to print the results of commands that run on results in a thread-safe way
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do weweant to print output from the filter command, or just suppress it?

Copy link
Author

Choose a reason for hiding this comment

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

I agree it would make more sense for the filter command to simply reduce the results that are matched before they're output to the user as usual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants