Merge pull request #829 from thegedge/fix-multiple-values-for-external-command

Fix bug with multiple input objects to an external command.

src/commands/classified.rs

@@ -225,7 +225,6 @@ impl ExternalCommand {
     ) -> Result<ClassifiedInputStream, ShellError> {
         let stdin = input.stdin;
         let inputs: Vec<Tagged<Value>> = input.objects.into_vec().await;
-        let name_tag = self.name_tag.clone();
 
         trace!(target: "nu::run::external", "-> {}", self.name);
         trace!(target: "nu::run::external", "inputs = {:?}", inputs);
@@ -235,53 +234,47 @@ impl ExternalCommand {
             arg_string.push_str(&arg);
         }
 
+        trace!(target: "nu::run::external", "command = {:?}", self.name);
+
         let mut process;
-
-        process = Exec::cmd(&self.name);
-
-        trace!(target: "nu::run::external", "command = {:?}", process);
-
         if arg_string.contains("$it") {
-            let mut first = true;
-
-            for i in &inputs {
-                if i.as_string().is_err() {
-                    let mut tag = None;
-                    for arg in &self.args {
-                        if arg.item.contains("$it") {
-                            tag = Some(arg.tag());
+            let input_strings = inputs
+                .iter()
+                .map(|i| {
+                    i.as_string().map_err(|_| {
+                        let arg = self.args.iter().find(|arg| arg.item.contains("$it"));
+                        if let Some(arg) = arg {
+                            ShellError::labeled_error(
+                                "External $it needs string data",
+                                "given row instead of string data",
+                                arg.tag(),
+                            )
+                        } else {
+                            ShellError::labeled_error(
+                                "Error: $it needs string data",
+                                "given something else",
+                                self.name_tag.clone(),
+                            )
                         }
-                    }
-                    if let Some(tag) = tag {
-                        return Err(ShellError::labeled_error(
-                            "External $it needs string data",
-                            "given row instead of string data",
-                            tag,
-                        ));
-                    } else {
-                        return Err(ShellError::labeled_error(
-                            "Error: $it needs string data",
-                            "given something else",
-                            name_tag,
-                        ));
-                    }
-                }
-                if !first {
-                    process = process.arg("&&");
-                    process = process.arg(&self.name);
-                } else {
-                    first = false;
-                }
+                    })
+                })
+                .collect::<Result<Vec<String>, ShellError>>()?;
 
-                for arg in &self.args {
+            let commands = input_strings.iter().map(|i| {
+                let args = self.args.iter().filter_map(|arg| {
                     if arg.chars().all(|c| c.is_whitespace()) {
-                        continue;
+                        None
+                    } else {
+                        Some(arg.replace("$it", &i))
                     }
+                });
 
-                    process = process.arg(&arg.replace("$it", &i.as_string()?));
-                }
-            }
+                format!("{} {}", self.name, itertools::join(args, " "))
+            });
+
+            process = Exec::shell(itertools::join(commands, " && "))
         } else {
+            process = Exec::cmd(&self.name);
             for arg in &self.args {
                 let arg_chars: Vec<_> = arg.chars().collect();
                 if arg_chars.len() > 1
@@ -321,6 +314,7 @@ impl ExternalCommand {
 
         trace!(target: "nu::run::external", "next = {:?}", stream_next);
 
+        let name_tag = self.name_tag.clone();
         if let Ok(mut popen) = popen {
             match stream_next {
                 StreamNext::Last => {

tests/commands_test.rs

@@ -164,6 +164,28 @@ fn save_figures_out_intelligently_where_to_write_out_with_metadata() {
     })
 }
 
+#[test]
+fn it_arg_works_with_many_inputs_to_external_command() {
+    Playground::setup("it_arg_works_with_many_inputs", |dirs, sandbox| {
+        sandbox.with_files(vec![
+            FileWithContent("file1", "text"),
+            FileWithContent("file2", " and more text"),
+        ]);
+
+        let (stdout, stderr) = nu_combined!(
+            cwd: dirs.test(), h::pipeline(
+            r#"
+                echo file1 file2
+                | split-row " "
+                | cat $it
+            "#
+        ));
+
+        assert_eq!("text and more text", stdout);
+        assert!(!stderr.contains("No such file or directory"));
+    })
+}
+
 #[test]
 fn save_can_write_out_csv() {
     Playground::setup("save_test_2", |dirs, _| {

tests/helpers/mod.rs

@@ -155,6 +155,60 @@ macro_rules! nu_error {
     }};
 }
 
+#[macro_export]
+macro_rules! nu_combined {
+    (cwd: $cwd:expr, $path:expr, $($part:expr),*) => {{
+        use $crate::helpers::DisplayPath;
+
+        let path = format!($path, $(
+            $part.display_path()
+        ),*);
+
+        nu_combined!($cwd, &path)
+    }};
+
+    (cwd: $cwd:expr, $path:expr) => {{
+        nu_combined!($cwd, $path)
+    }};
+
+    ($cwd:expr, $path:expr) => {{
+        pub use std::error::Error;
+        pub use std::io::prelude::*;
+        pub use std::process::{Command, Stdio};
+
+        let commands = &*format!(
+            "
+                            cd {}
+                            {}
+                            exit",
+            $crate::helpers::in_directory($cwd),
+            $crate::helpers::DisplayPath::display_path(&$path)
+        );
+
+        let mut process = Command::new(helpers::executable_path())
+            .stdin(Stdio::piped())
+            .stdout(Stdio::piped())
+            .stderr(Stdio::piped())
+            .spawn()
+            .expect("couldn't run test");
+
+        let stdin = process.stdin.as_mut().expect("couldn't open stdin");
+        stdin
+            .write_all(commands.as_bytes())
+            .expect("couldn't write to stdin");
+
+        let output = process
+            .wait_with_output()
+            .expect("couldn't read from stdout/stderr");
+
+        let err = String::from_utf8_lossy(&output.stderr).into_owned();
+        let out = String::from_utf8_lossy(&output.stdout).into_owned();
+        let out = out.replace("\r\n", "");
+        let out = out.replace("\n", "");
+        (out, err)
+    }};
+}
+
 pub enum Stub<'a> {
     FileWithContent(&'a str, &'a str),
     FileWithContentToBeTrimmed(&'a str, &'a str),