Repository: sharkdp/fd (language: rust)

Issue:
fd does not expose exit status from --exec result
It would be helpful for error handling to be able to opt into `fd` returning a non-zero status when a utility invoked with `--exec` returns a non-zero status.

`find` does this when the arguments of `--exec` contain `{} +`




Discussion / hints:
Thank you for your feedback.

Sounds like a reasonable request to me, but we currently don't have the `{} +` variant of `--exec` in `fd` (related: #274). Also partially related: #303.
If you have any design suggestions I could put together a PR.

I also took another look at what I was asking for and what I wanted.
I misread the `--ok` documentation, it does not work in the way I described and have removed it from post.
Also `{} +` is two things, non-zero status code on exec failure and aggregating arguments.

Would it be better to ignore how posix find defines `--exec` and split those two concepts? It seems like it was designed that way to work with find's expression model.


Ok, so there are several things that we need to clarify/decide here:

- Do we want to support the `{} +` variant of `find`s `-exec`?

  Yes, I think so. I kind of like the `--exec-once` idea in #274.

- What should the exit code be in the case where we use `--exec`?

  So `find` only uses a non-zero exit code in case of problems *while traversing the directory tree* (like in a normal execution of `fd`/`find`).

  Additionally using non-zero exit codes in the case where one of the child processes fails would somehow overload that error path.

  However, there could be a chance to do things better than `find` here (see https://apple.stackexchange.com/questions/49042/how-do-i-make-find-fail-if-exec-fails). Maybe we could use different exit codes for the two failure modes (`1` in case of directory traversal error, `2` in case of `--exec` failures).

- What should be the exit-code for uses of `--exec-once`?

  I don't really see a reason why this should be handled differently from the `--exec` code. Maybe someone can find a reason why `find` seems to do this?
`{} +` can still execute multiple times when there are too many characters in the line (8192 for cmd, 32767 for powershell, `ARG_MAX` otherwise). Maybe it should be something else, like `--exec-batch`?

I could not find an explicit reasoning behind the exit status change in the [rational for adding `{} +`](https://collaboration.opengroup.org/external/pasc.org/interpretations/unofficial/db/p1003.2/pasc-1003.2-210.html)

It might have been because `{} +` was meant to workaround a problem with piping weird filenames to `xargs` and `xargs` raises the status code.



> `{} +` can still execute multiple times when there are too many characters in the line (8192 for cmd, 32767 for powershell, `ARG_MAX` otherwise). Maybe it should be something else, like `--exec-batch`?

Exactly - see [my comment](https://github.com/sharkdp/fd/issues/274#issuecomment-427431073) in #274. As for the name, I kind of like `--open-with`.
Meaningful exit status is the biggest fail in the typical UNIX `find` utility. Not to mention the limitations of `-exec`, the limitations of `xargs`, and the exit status of the overall `find ... | xargs` composite command. I would consider replacing `find` in my scripts with another tool, but only if this were addressed by that tool. As much as I dislike find, it is still easier to communicate to my fellow developers a dependency on (GNU) findutils and the known ball of awful that comes with it, than to add a new layer to the stack that doesn't even address find's basic problems.

Ultimately, the fragility of `find` workflows is one reason I am moving away from make and shell scripts in general, and instead using library calls to accomplish my tasks. It's just more reliable to use mage, rake, invoke, tinyrick, and so on compared to constantly fixing corner cases.
@mcandre So what exactly are the big "fails" that you see in `find`s exit status handling? What could we learn from these mistakes and how exactly do you think `fd` should behave?

(@all note that we have recently added `--exec-batch` to `fd`)

Fix patch:
diff --git a/Cargo.lock b/Cargo.lock
index 717798c2c..12eeb074c 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1,3 +1,5 @@
+# This file is automatically @generated by Cargo.
+# It is not intended for manual editing.
 [[package]]
 name = "aho-corasick"
 version = "0.6.9"
diff --git a/src/exec/command.rs b/src/exec/command.rs
index f02b5f494..f6d79d057 100644
--- a/src/exec/command.rs
+++ b/src/exec/command.rs
@@ -11,8 +11,10 @@ use std::io::Write;
 use std::process::Command;
 use std::sync::Mutex;
 
+use crate::exit_codes::ExitCode;
+
 /// Executes a command.
-pub fn execute_command(mut cmd: Command, out_perm: &Mutex<()>) {
+pub fn execute_command(mut cmd: Command, out_perm: &Mutex<()>) -> ExitCode {
     // Spawn the supplied command.
     let output = cmd.output();
 
@@ -28,12 +30,20 @@ pub fn execute_command(mut cmd: Command, out_perm: &Mutex<()>) {
 
             let _ = stdout.lock().write_all(&output.stdout);
             let _ = stderr.lock().write_all(&output.stderr);
+
+            if output.status.code() == Some(0) {
+                ExitCode::Success
+            } else {
+                ExitCode::GeneralError
+            }
         }
         Err(ref why) if why.kind() == io::ErrorKind::NotFound => {
             print_error!("Command not found: {:?}", cmd);
+            ExitCode::GeneralError
         }
         Err(why) => {
             print_error!("Problem while executing command: {}", why);
+            ExitCode::GeneralError
         }
     }
 }
diff --git a/src/exec/job.rs b/src/exec/job.rs
index 56d6966c2..270723844 100644
--- a/src/exec/job.rs
+++ b/src/exec/job.rs
@@ -7,6 +7,7 @@
 // according to those terms.
 
 use super::CommandTemplate;
+use crate::exit_codes::ExitCode;
 use crate::walk::WorkerResult;
 use std::path::PathBuf;
 use std::sync::mpsc::Receiver;
@@ -45,7 +46,11 @@ pub fn job(
     }
 }
 
-pub fn batch(rx: Receiver<WorkerResult>, cmd: &CommandTemplate, show_filesystem_errors: bool) {
+pub fn batch(
+    rx: Receiver<WorkerResult>,
+    cmd: &CommandTemplate,
+    show_filesystem_errors: bool,
+) -> ExitCode {
     let paths = rx.iter().filter_map(|value| match value {
         WorkerResult::Entry(val) => Some(val),
         WorkerResult::Error(err) => {
@@ -55,5 +60,5 @@ pub fn batch(rx: Receiver<WorkerResult>, cmd: &CommandTemplate, show_filesystem_
             None
         }
     });
-    cmd.generate_and_execute_batch(paths);
+    cmd.generate_and_execute_batch(paths)
 }
diff --git a/src/exec/mod.rs b/src/exec/mod.rs
index 8f335e448..369033101 100644
--- a/src/exec/mod.rs
+++ b/src/exec/mod.rs
@@ -20,6 +20,8 @@ use std::sync::{Arc, Mutex};
 use lazy_static::lazy_static;
 use regex::Regex;
 
+use crate::exit_codes::ExitCode;
+
 use self::command::execute_command;
 use self::input::{basename, dirname, remove_extension};
 pub use self::job::{batch, job};
@@ -152,14 +154,14 @@ impl CommandTemplate {
             cmd.arg(arg.generate(&input).as_ref());
         }
 
-        execute_command(cmd, &out_perm)
+        execute_command(cmd, &out_perm);
     }
 
     pub fn in_batch_mode(&self) -> bool {
         self.mode == ExecutionMode::Batch
     }
 
-    pub fn generate_and_execute_batch<I>(&self, paths: I)
+    pub fn generate_and_execute_batch<I>(&self, paths: I) -> ExitCode
     where
         I: Iterator<Item = PathBuf>,
     {
@@ -185,7 +187,9 @@ impl CommandTemplate {
         }
 
         if has_path {
-            execute_command(cmd, &Mutex::new(()));
+            execute_command(cmd, &Mutex::new(()))
+        } else {
+            ExitCode::Success
         }
     }
 }
diff --git a/src/exit_codes.rs b/src/exit_codes.rs
index 225c5bcc7..026562707 100644
--- a/src/exit_codes.rs
+++ b/src/exit_codes.rs
@@ -1,4 +1,5 @@
 pub enum ExitCode {
+    Success,
     GeneralError,
     KilledBySigint,
 }
@@ -6,6 +7,7 @@ pub enum ExitCode {
 impl Into<i32> for ExitCode {
     fn into(self) -> i32 {
         match self {
+            ExitCode::Success => 0,
             ExitCode::GeneralError => 1,
             ExitCode::KilledBySigint => 130,
         }
diff --git a/src/main.rs b/src/main.rs
index d24eb9605..eb6b995af 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -19,6 +19,7 @@ mod walk;
 use std::env;
 use std::error::Error;
 use std::path::{Path, PathBuf};
+use std::process;
 use std::sync::Arc;
 use std::time;
 
@@ -250,7 +251,10 @@ fn main() {
         .dot_matches_new_line(true)
         .build()
     {
-        Ok(re) => walk::scan(&dir_vec, Arc::new(re), Arc::new(config)),
+        Ok(re) => {
+            let exit_code = walk::scan(&dir_vec, Arc::new(re), Arc::new(config));
+            process::exit(exit_code.into());
+        }
         Err(err) => {
             print_error_and_exit!(
                 "{}\nHint: You can use the '--fixed-strings' option to search for a \
diff --git a/src/walk.rs b/src/walk.rs
index 487064355..f2b472280 100644
--- a/src/walk.rs
+++ b/src/walk.rs
@@ -47,7 +47,7 @@ pub enum WorkerResult {
 /// If the `--exec` argument was supplied, this will create a thread pool for executing
 /// jobs in parallel from a given command line and the discovered paths. Otherwise, each
 /// path will simply be written to standard output.
-pub fn scan(path_vec: &[PathBuf], pattern: Arc<Regex>, config: Arc<FdOptions>) {
+pub fn scan(path_vec: &[PathBuf], pattern: Arc<Regex>, config: Arc<FdOptions>) -> ExitCode {
     let mut path_iter = path_vec.iter();
     let first_path_buf = path_iter
         .next()
@@ -122,18 +122,20 @@ pub fn scan(path_vec: &[PathBuf], pattern: Arc<Regex>, config: Arc<FdOptions>) {
     spawn_senders(&config, &wants_to_quit, pattern, parallel_walker, tx);
 
     // Wait for the receiver thread to print out all results.
-    receiver_thread.join().unwrap();
+    let exit_code = receiver_thread.join().unwrap();
 
     if wants_to_quit.load(Ordering::Relaxed) {
         process::exit(ExitCode::KilledBySigint.into());
     }
+
+    exit_code
 }
 
 fn spawn_receiver(
     config: &Arc<FdOptions>,
     wants_to_quit: &Arc<AtomicBool>,
     rx: Receiver<WorkerResult>,
-) -> thread::JoinHandle<()> {
+) -> thread::JoinHandle<ExitCode> {
     let config = Arc::clone(config);
     let wants_to_quit = Arc::clone(wants_to_quit);
 
@@ -144,7 +146,7 @@ fn spawn_receiver(
         // 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, cmd, show_filesystem_errors);
+                exec::batch(rx, cmd, show_filesystem_errors)
             } else {
                 let shared_rx = Arc::new(Mutex::new(rx));
 
@@ -169,6 +171,8 @@ fn spawn_receiver(
                 for h in handles {
                     h.join().unwrap();
                 }
+
+                ExitCode::Success
             }
         } else {
             let start = time::Instant::now();
@@ -233,6 +237,8 @@ fn spawn_receiver(
                     output::print_entry(&mut stdout, &value, &config, &wants_to_quit);
                 }
             }
+
+            ExitCode::Success
         }
     })
 }


Test patch: