Add --filter argument that filters results based on command exit-code#1545
Add --filter argument that filters results based on command exit-code#1545braden-godley wants to merge 11 commits intosharkdp:masterfrom
Conversation
| Arg::new("filter") | ||
| .action(ArgAction::Append) | ||
| .long("filter") | ||
| .short('f') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm guessing that this might have changed since this comment was written, but don't we have -x and -X?
| .allow_hyphen_values(true) | ||
| .value_terminator(";") | ||
| .value_name("cmd") | ||
| .conflicts_with_all(["exec", "exec_batch", "list_details"]) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. \ |
There was a problem hiding this comment.
Maybe mention that you can end with a \;?
| .get_occurrences::<String>("exec_batch") | ||
| .map(CommandSet::new_batch) | ||
| }) | ||
| .or_else(|| { |
There was a problem hiding this comment.
Could this be implemented in a way that would allow using it along with exec or exec-batch?
src/exec/command.rs
Outdated
| out_perm: &Mutex<()>, | ||
| ) -> ExitCode { | ||
| let mut output_buffer = OutputBuffer::new(out_perm); | ||
| let path = format!("{}\n", path.to_str().unwrap()); |
There was a problem hiding this comment.
why do we add a newline to the path?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I based this on how the --exec command does the same. Should we change that as well?
There was a problem hiding this comment.
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:
- Pipe to /dev/null or equivalent
- Read, and immediately drop
- Collect into buffer, then write to stderr, so that error messages are preserved.
- 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.
| stderr: Vec<u8>, | ||
| } | ||
|
|
||
| /// Used to print the results of commands that run on results in a thread-safe way |
There was a problem hiding this comment.
Do weweant to print output from the filter command, or just suppress it?
There was a problem hiding this comment.
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.
|
@braden-godley I want to say thanks for working on this! I came here looking for this exact functionality and was delighted to see you'd already put so much thought and effort into it! |
4 participants
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!