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
38 changes: 36 additions & 2 deletions src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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?

matches
.get_occurrences::<String>("filter")
.map(CommandSet::new_filter)
})
.transpose()
.map_err(|e| clap::Error::raw(ErrorKind::InvalidValue, e))?;
Ok(Exec { command })
Expand All @@ -797,7 +802,7 @@ impl clap::Args for Exec {
.allow_hyphen_values(true)
.value_terminator(";")
.value_name("cmd")
.conflicts_with("list_details")
.conflicts_with_all(["list_details", "exec_batch"])
.help("Execute a command for each search result")
.long_help(
"Execute a command for each search result in parallel (use --threads=1 for sequential command execution). \
Expand Down Expand Up @@ -851,7 +856,35 @@ impl clap::Args for Exec {
fd -g 'test_*.py' -X vim\n\n \
- Find all *.rs files and count the lines with \"wc -l ...\":\n\n \
fd -e rs -X wc -l\
"
"
),
)
.arg(
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.

.num_args(1..)
.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.

.help("Execute a command to determine whether each result should be filtered")
.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 \;?

It is therefore recommended to place the '-f'/'--filter' option last.\n\
The following placeholders are substituted before the command is executed:\n \
'{}': path (of the current search result)\n \
'{/}': basename\n \
'{//}': parent directory\n \
'{.}': path without file extension\n \
'{/.}': basename without file extension\n \
'{{': literal '{' (for escaping)\n \
'}}': literal '}' (for escaping)\n\n\
If no placeholder is present, an implicit \"{}\" at the end is assumed.\n\n\
"
),
)
}
Expand All @@ -874,3 +907,4 @@ fn ensure_current_directory_exists(current_directory: &Path) -> anyhow::Result<(
))
}
}

35 changes: 35 additions & 0 deletions src/exec/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,41 @@ pub fn execute_commands<I: Iterator<Item = io::Result<Command>>>(
ExitCode::Success
}

pub fn execute_commands_filtering<I: Iterator<Item = io::Result<Command>>>(
path: &std::path::Path,
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.

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

let path: Vec<u8> = path.into();

for result in cmds {
let mut cmd = match result {
Ok(cmd) => cmd,
Err(e) => return handle_cmd_error(None, e),
};

let output = cmd.output();

match output {
Ok(output) => {
if output.status.code() != Some(0) {
return ExitCode::GeneralError;
} else {
output_buffer.push(path.clone(), vec![]);
}
},
Err(why) => {
return handle_cmd_error(Some(&cmd), why);
},
}
}

output_buffer.write();
ExitCode::Success
}

pub fn handle_cmd_error(cmd: Option<&Command>, err: io::Error) -> ExitCode {
match (cmd, err) {
(Some(cmd), err) if err.kind() == io::ErrorKind::NotFound => {
Expand Down
32 changes: 32 additions & 0 deletions src/exec/job.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,38 @@ pub fn job(
ret
}

pub fn filter_job(
results: impl IntoIterator<Item = WorkerResult>,
cmd: &CommandSet,
out_perm: &Mutex<()>,
config: &Config,
) -> ExitCode {
let mut ret = ExitCode::Success;
for result in results {
// Obtain the next result from the receiver, else if the channel
// has closed, exit from the loop
let dir_entry = match result {
WorkerResult::Entry(dir_entry) => dir_entry,
WorkerResult::Error(err) => {
if config.show_filesystem_errors {
print_error(err.to_string());
}
continue;
}
};

// Generate a command, execute it and store its exit code.
let code = cmd.execute_filter(
dir_entry.stripped_path(config),
config.path_separator.as_deref(),
out_perm,
);
ret = merge_exitcodes([ret, code]);
}
// Returns error in case of any error.
ret
}

pub fn batch(
results: impl IntoIterator<Item = WorkerResult>,
cmd: &CommandSet,
Expand Down
38 changes: 36 additions & 2 deletions src/exec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ use argmax::Command;

use crate::exit_codes::{merge_exitcodes, ExitCode};

use self::command::{execute_commands, handle_cmd_error};
use self::command::{execute_commands, execute_commands_filtering, handle_cmd_error};
use self::input::{basename, dirname, remove_extension};
pub use self::job::{batch, job};
pub use self::job::{batch, job, filter_job};
use self::token::{tokenize, Token};

/// Execution mode of the command
Expand All @@ -28,6 +28,8 @@ pub enum ExecutionMode {
OneByOne,
/// Command is run for a batch of results at once
Batch,
/// Command is executed for each search result to determine if it should be filtered
FilterResults,
}

#[derive(Debug, Clone, PartialEq)]
Expand Down Expand Up @@ -76,6 +78,25 @@ impl CommandSet {
})
}

pub fn new_filter<I, T, S>(input: I) -> Result<CommandSet>
where
I: IntoIterator<Item = T>,
T: IntoIterator<Item = S>,
S: AsRef<str>,
{
Ok(CommandSet {
mode: ExecutionMode::FilterResults,
commands: input
.into_iter()
.map(CommandTemplate::new)
.collect::<Result<_>>()?,
})
}

pub fn get_mode(&self) -> ExecutionMode {
self.mode
}

pub fn in_batch_mode(&self) -> bool {
self.mode == ExecutionMode::Batch
}
Expand All @@ -94,6 +115,19 @@ impl CommandSet {
execute_commands(commands, out_perm, buffer_output)
}

pub fn execute_filter(
&self,
input: &Path,
path_separator: Option<&str>,
out_perm: &Mutex<()>,
) -> ExitCode {
let commands = self
.commands
.iter()
.map(|c| c.generate(input, path_separator));
execute_commands_filtering(input, commands, out_perm)
}

pub fn execute_batch<I>(&self, paths: I, limit: usize, path_separator: Option<&str>) -> ExitCode
where
I: Iterator<Item = PathBuf>,
Expand Down
1 change: 1 addition & 0 deletions src/exit_codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ impl ExitCode {
}
}

/// If any of the exit codes was an error, this returns a GeneralError
pub fn merge_exitcodes(results: impl IntoIterator<Item = ExitCode>) -> ExitCode {
if results.into_iter().any(ExitCode::is_error) {
return ExitCode::GeneralError;
Expand Down
52 changes: 30 additions & 22 deletions src/walk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -408,28 +408,36 @@ impl WorkerState {

// This will be set to `Some` if the `--exec` argument was supplied.
if let Some(ref cmd) = config.command {
if cmd.in_batch_mode() {
exec::batch(rx.into_iter().flatten(), cmd, config)
} else {
let out_perm = Mutex::new(());

thread::scope(|scope| {
// Each spawned job will store its thread handle in here.
let threads = config.threads;
let mut handles = Vec::with_capacity(threads);
for _ in 0..threads {
let rx = rx.clone();

// Spawn a job thread that will listen for and execute inputs.
let handle = scope
.spawn(|| exec::job(rx.into_iter().flatten(), cmd, &out_perm, config));

// Push the handle of the spawned thread into the vector for later joining.
handles.push(handle);
}
let exit_codes = handles.into_iter().map(|handle| handle.join().unwrap());
merge_exitcodes(exit_codes)
})
match cmd.get_mode() {
exec::ExecutionMode::Batch => {
exec::batch(rx.into_iter().flatten(), cmd, config)
},
exec::ExecutionMode::OneByOne | exec::ExecutionMode::FilterResults => {
let out_perm = Mutex::new(());

thread::scope(|scope| {
// Each spawned job will store its thread handle in here.
let threads = config.threads;
let mut handles = Vec::with_capacity(threads);
for _ in 0..threads {
let rx = rx.clone();

// Spawn a job thread that will listen for and execute inputs.
let handle = if let exec::ExecutionMode::OneByOne = cmd.get_mode() {
scope
.spawn(|| exec::job(rx.into_iter().flatten(), cmd, &out_perm, config))
} else {
scope
.spawn(|| exec::filter_job(rx.into_iter().flatten(), cmd, &out_perm, config))
};

// Push the handle of the spawned thread into the vector for later joining.
handles.push(handle);
}
let exit_codes = handles.into_iter().map(|handle| handle.join().unwrap());
merge_exitcodes(exit_codes)
})
},
}
} else {
let stdout = io::stdout().lock();
Expand Down