Merge pull request #2735 from thomasqueirozb/printenv_env_compat

env+printenv: use UResult + improve compatibility

src/uu/env/src/env.rs

@@ -1,6 +1,7 @@
 // This file is part of the uutils coreutils package.
 //
 // (c) Jordi Boggiano <j.boggiano@seld.be>
+// (c) Thomas Queiroz <thomasqueirozb@gmail.com>
 //
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
@@ -23,7 +24,7 @@ use std::io::{self, Write};
 use std::iter::Iterator;
 use std::process::Command;
 use uucore::display::Quotable;
-use uucore::error::{UResult, USimpleError};
+use uucore::error::{UResult, USimpleError, UUsageError};
 
 const USAGE: &str = "env [OPTION]... [-] [NAME=VALUE]... [COMMAND [ARG]...]";
 const AFTER_HELP: &str = "\
@@ -50,7 +51,7 @@ fn print_env(null: bool) {
     }
 }
 
-fn parse_name_value_opt<'a>(opts: &mut Options<'a>, opt: &'a str) -> Result<bool, i32> {
+fn parse_name_value_opt<'a>(opts: &mut Options<'a>, opt: &'a str) -> UResult<bool> {
     // is it a NAME=VALUE like opt ?
     if let Some(idx) = opt.find('=') {
         // yes, so push name, value pair
@@ -64,17 +65,12 @@ fn parse_name_value_opt<'a>(opts: &mut Options<'a>, opt: &'a str) -> Result<bool
     }
 }
 
-fn parse_program_opt<'a>(opts: &mut Options<'a>, opt: &'a str) -> Result<(), i32> {
+fn parse_program_opt<'a>(opts: &mut Options<'a>, opt: &'a str) -> UResult<()> {
     if opts.null {
-        eprintln!(
+        Err(UUsageError::new(
-            "{}: cannot specify --null (-0) with command",
+            125,
-            uucore::util_name()
+            "cannot specify --null (-0) with command".to_string(),
-        );
+        ))
-        eprintln!(
-            "Type \"{} --help\" for detailed information",
-            uucore::execution_phrase()
-        );
-        Err(1)
     } else {
         opts.program.push(opt);
         Ok(())
@@ -93,10 +89,8 @@ fn load_config_file(opts: &mut Options) -> UResult<()> {
             Ini::load_from_file(file)
         };
 
-        let conf = conf.map_err(|error| {
+        let conf =
-            show_error!("{}: {}", file.maybe_quote(), error);
+            conf.map_err(|e| USimpleError::new(1, format!("{}: {}", file.maybe_quote(), e)))?;
-            1
-        })?;
 
         for (_, prop) in &conf {
             // ignore all INI section lines (treat them as comments)
@@ -138,7 +132,7 @@ pub fn uu_app() -> App<'static, 'static> {
             .long("ignore-environment")
             .help("start with an empty environment"))
         .arg(Arg::with_name("chdir")
-            .short("c")
+            .short("C") // GNU env compatibility
             .long("chdir")
             .takes_value(true)
             .number_of_values(1)
@@ -236,6 +230,14 @@ fn run_env(args: impl uucore::Args) -> UResult<()> {
         }
     }
 
+    // GNU env tests this behavior
+    if opts.program.is_empty() && running_directory.is_some() {
+        return Err(UUsageError::new(
+            125,
+            "must specify command with --chdir (-C)".to_string(),
+        ));
+    }
+
     // NOTE: we manually set and unset the env vars below rather than using Command::env() to more
     //       easily handle the case where no command is given
 
@@ -251,6 +253,13 @@ fn run_env(args: impl uucore::Args) -> UResult<()> {
 
     // unset specified env vars
     for name in &opts.unsets {
+        if name.is_empty() || name.contains(0 as char) || name.contains('=') {
+            return Err(USimpleError::new(
+                125,
+                format!("cannot unset {}: Invalid argument", name.quote()),
+            ));
+        }
+
         env::remove_var(name);
     }
 

src/uu/printenv/src/printenv.rs

@@ -9,6 +9,7 @@
 
 use clap::{crate_version, App, Arg};
 use std::env;
+use uucore::error::UResult;
 
 static ABOUT: &str = "Display the values of the specified environment VARIABLE(s), or (with no VARIABLE) display name and value pairs for them all.";
 
@@ -20,7 +21,8 @@ fn usage() -> String {
     format!("{0} [VARIABLE]... [OPTION]...", uucore::execution_phrase())
 }
 
-pub fn uumain(args: impl uucore::Args) -> i32 {
+#[uucore_procs::gen_uumain]
+pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let usage = usage();
 
     let matches = uu_app().usage(&usage[..]).get_matches_from(args);
@@ -40,15 +42,23 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
         for (env_var, value) in env::vars() {
             print!("{}={}{}", env_var, value, separator);
         }
-        return 0;
+        return Ok(());
     }
 
+    let mut not_found = false;
     for env_var in variables {
         if let Ok(var) = env::var(env_var) {
             print!("{}{}", var, separator);
+        } else {
+            not_found = true;
         }
     }
-    0
+
+    if not_found {
+        Err(1.into())
+    } else {
+        Ok(())
+    }
 }
 
 pub fn uu_app() -> App<'static, 'static> {

tests/by-util/test_env.rs

@@ -1,7 +1,4 @@
-// spell-checker:ignore (words) bamf chdir
+// spell-checker:ignore (words) bamf chdir rlimit prlimit COMSPEC
-
-#[cfg(not(windows))]
-use std::fs;
 
 use crate::common::util::*;
 use std::env;
@@ -80,6 +77,20 @@ fn test_combined_file_set_unset() {
     );
 }
 
+#[test]
+fn test_unset_invalid_variables() {
+    use uucore::display::Quotable;
+
+    // Cannot test input with \0 in it, since output will also contain \0. rlimit::prlimit fails
+    // with this error: Error { kind: InvalidInput, message: "nul byte found in provided data" }
+    for var in &["", "a=b"] {
+        new_ucmd!().arg("-u").arg(var).run().stderr_only(format!(
+            "env: cannot unset {}: Invalid argument",
+            var.quote()
+        ));
+    }
+}
+
 #[test]
 fn test_single_name_value_pair() {
     let out = new_ucmd!().arg("FOO=bar").run();
@@ -163,7 +174,7 @@ fn test_fail_null_with_program() {
 fn test_change_directory() {
     let scene = TestScenario::new(util_name!());
     let temporary_directory = tempdir().unwrap();
-    let temporary_path = fs::canonicalize(temporary_directory.path()).unwrap();
+    let temporary_path = std::fs::canonicalize(temporary_directory.path()).unwrap();
     assert_ne!(env::current_dir().unwrap(), temporary_path);
 
     // command to print out current working directory
@@ -179,27 +190,36 @@ fn test_change_directory() {
     assert_eq!(out.trim(), temporary_path.as_os_str())
 }
 
-// no way to consistently get "current working directory", `cd` doesn't work @ CI
-// instead, we test that the unique temporary directory appears somewhere in the printed variables
 #[cfg(windows)]
 #[test]
 fn test_change_directory() {
     let scene = TestScenario::new(util_name!());
     let temporary_directory = tempdir().unwrap();
-    let temporary_path = temporary_directory.path();
 
-    assert_ne!(env::current_dir().unwrap(), temporary_path);
+    let temporary_path = temporary_directory.path();
+    let temporary_path = temporary_path
+        .strip_prefix(r"\\?\")
+        .unwrap_or(temporary_path);
+
+    let env_cd = env::current_dir().unwrap();
+    let env_cd = env_cd.strip_prefix(r"\\?\").unwrap_or(&env_cd);
+
+    assert_ne!(env_cd, temporary_path);
+
+    // COMSPEC is a variable that contains the full path to cmd.exe
+    let cmd_path = env::var("COMSPEC").unwrap();
+
+    // command to print out current working directory
+    let pwd = [&*cmd_path, "/C", "cd"];
 
     let out = scene
         .ucmd()
         .arg("--chdir")
         .arg(&temporary_path)
+        .args(&pwd)
         .succeeds()
         .stdout_move_str();
-
+    assert_eq!(out.trim(), temporary_path.as_os_str())
-    assert!(!out
-        .lines()
-        .any(|line| line.ends_with(temporary_path.file_name().unwrap().to_str().unwrap())));
 }
 
 #[test]