Remove shell with --exec #160
Conversation
src/exec/ticket.rs
Outdated
| command: &'a mut String, | ||
| /// The ticket holds the collection of arguments of a command to be executed. | ||
| pub struct CommandTicket { | ||
| args: Vec<String>, |
There was a problem hiding this comment.
Should probably try to eliminate the Vec allocations between commands by making this a &'a mut Vec<String>, and then implementing Drop for CommandTicket to clear the args buffer.
There was a problem hiding this comment.
I dropped the allocation by removing CommandTicket altogether. There was no need for CommandTicket anymore, as it only collected the arguments which were then immediately used to build and execute the Command. Now the TokenizedCommand builds the Command with the generated arguments directly and uses execute_command (previously CommandTicket::then_execute) to execute it.
|
Thank you very much! I'll add my review soon. Does this change imply that we will have to put |
|
With this change,
I haven't had much time to work on this last week, but I'll try to refine this PR further over the weekend. |
|
I refactored some things to clean up and updated the documentation to reflect the new |
| .multiple(true) | ||
| .min_values(1) | ||
| .allow_hyphen_values(true) | ||
| .value_terminator(";") |
There was a problem hiding this comment.
We should probably keep ; as terminator here for consistency with find but I was shortly wondering if we should maybe change this to something that doesn't need to be escaped on the shell. The \; has always looked really awkward to me.
On the other hand, what ever we would choose as an alternative could not be passed to other programs...
There was a problem hiding this comment.
Yeah, I'm not particularly fond of ; either. I've more than once been puzzled by odd error messages when I forgot to escape it. : would be an alternative that doesn't need escaping (but doesn't convey the same "end of statement" feel).
There was a problem hiding this comment.
Let's keep ; for now and I'll keep this in mind. I was also thinking of :. Would be great if we (clap) could support both.
src/app.rs
Outdated
| '{/.}': places the basename of the input, without the extension"); | ||
| , "Execute a command for each search result" | ||
| , "Execute a command for each search result.\n\ | ||
| All arguments following -exec are are taken to be arguments to the command until the \ |
There was a problem hiding this comment.
Right. There's also a duplicated "are".
| let _ = stdout.lock().write_all(&output.stdout); | ||
| let _ = stderr.lock().write_all(&output.stderr); | ||
| } | ||
| Err(why) => eprintln!("fd: exec error: {}", why), |
There was a problem hiding this comment.
It would be great if we could improve this error message here. I'm not sure what all the possible cases are that could trigger this error, but this message will be shown to the user if the given command is not available:
> fd -x foo
fd: exec error: No such file or directory (os error 2)
I encountered this by accident and was definitely confused until I realized that the command I specified did not exist.
There was a problem hiding this comment.
This is probably out of scope for this issue. While this code is all marked as new, it's actually what is left from the renamed and modified ticket.rs.
There was a problem hiding this comment.
You'll have to construct the error message yourself, by checking the error kind of the error.
if why.kind() == io::ErrorKind::NotFound {
eprintln!("fd: exec error: command not found");
} else {
eprintln!("fd: exec error: {}", why);
}
| let _ = io::copy(&mut pout, &mut stdout.lock()); | ||
| let _ = io::copy(&mut perr, &mut stderr.lock()); | ||
| } | ||
| Err(why) => eprintln!("fd: exec error: {}", why), |
|
@reima Thank you very much (again) - great work! I added just some minor comments. |
|
Maybe also have a benchmark comparison of find versus fd with parallel execution? |
Sounds great! I imagine there are two cases where fd could potentially be faster:
Do you have any particular benchmarks in mind? |
This is a WIP/PoC for removing the intermediary shell when calling a command with
--exec. See #155 for the discussion.