Tests for autoenv (and fixes for bugs the tests found) (#2148)

* add test basic_autoenv_vars_are_added

* Tests

* Entry and exit scripts

* Recursive set and overwrite

* Make sure that overwritten vals are restored

* Move tests to autoenv

* Move tests out of cli crate

* Tests help, apparently. Windows has issues

On windows, .nu-env is not applied immediately after running autoenv trust.
You have to cd out of the directory for it to work.

* Sort paths non-lexicographically

* Sibling dir test

* Revert "Sort paths non-lexicographically"

This reverts commit 72e4b856af.

* Rename test

* Change conditions

* Revert "Revert "Sort paths non-lexicographically""

This reverts commit 71606bc62f.

* Set vars as they are discovered

This means that if a parent directory is untrusted,
the variables in its child directories are still set properly.

* format

* Fix cleanup issues too

* Run commands in their separate functions

* Make everything into one large function like all the cool kids

* Refactoring

* fmt

* Debugging windows path issue

* Canonicalize

* Trim whitespace

* On windows, use echo nul instead of touch to create file in test

* Avoid cloning by using drain()

crates/nu-cli/src/commands/autoenv.rs

@@ -22,10 +22,10 @@ impl Trusted {
 }
 pub fn file_is_trusted(nu_env_file: &PathBuf, content: &[u8]) -> Result<bool, ShellError> {
     let contentdigest = Sha256::digest(&content).as_slice().to_vec();
-    let nufile = nu_env_file.to_str().unwrap_or("");
+    let nufile = std::fs::canonicalize(nu_env_file)?;
 
     let trusted = read_trusted()?;
-    Ok(trusted.files.get(nufile) == Some(&contentdigest))
+    Ok(trusted.files.get(&nufile.to_string_lossy().to_string()) == Some(&contentdigest))
 }
 
 pub fn read_trusted() -> Result<Trusted, ShellError> {

crates/nu-cli/src/commands/autoenv_trust.rs

@@ -39,7 +39,7 @@ impl WholeStreamCommand for AutoenvTrust {
                 dir
             }
             _ => {
-                let mut dir = std::env::current_dir()?;
+                let mut dir = fs::canonicalize(std::env::current_dir()?)?;
                 dir.push(".nu-env");
                 dir
             }

crates/nu-cli/src/env/directory_specific_environment.rs

@@ -3,7 +3,6 @@ use commands::autoenv;
 use indexmap::{IndexMap, IndexSet};
 use nu_errors::ShellError;
 use serde::Deserialize;
-use std::cmp::Ordering::Less;
 use std::env::*;
 use std::process::Command;
 
@@ -20,7 +19,7 @@ pub struct DirectorySpecificEnvironment {
     pub last_seen_directory: PathBuf,
     //If an environment var has been added from a .nu in a directory, we track it here so we can remove it when the user leaves the directory.
     //If setting the var overwrote some value, we save the old value in an option so we can restore it later.
-    added_env_vars: IndexMap<PathBuf, IndexMap<EnvKey, Option<EnvVal>>>,
+    added_vars: IndexMap<PathBuf, IndexMap<EnvKey, Option<EnvVal>>>,
     exitscripts: IndexMap<PathBuf, Vec<String>>,
 }
 
@@ -42,15 +41,12 @@ impl DirectorySpecificEnvironment {
         };
         DirectorySpecificEnvironment {
             last_seen_directory: root_dir,
-            added_env_vars: IndexMap::new(),
+            added_vars: IndexMap::new(),
             exitscripts: IndexMap::new(),
         }
     }
 
-    fn toml_if_directory_is_trusted(
+    fn toml_if_trusted(&mut self, nu_env_file: &PathBuf) -> Result<NuEnvDoc, ShellError> {
-        &mut self,
-        nu_env_file: &PathBuf,
-    ) -> Result<NuEnvDoc, ShellError> {
         let content = std::fs::read(&nu_env_file)?;
 
         if autoenv::file_is_trusted(&nu_env_file, &content)? {
@@ -72,134 +68,101 @@ impl DirectorySpecificEnvironment {
                 format!("{:?} is untrusted. Run 'autoenv trust {:?}' to trust it.\nThis needs to be done after each change to the file.", nu_env_file, nu_env_file.parent().unwrap_or_else(|| &Path::new("")))))
     }
 
-    pub fn env_vars_to_add(&mut self) -> Result<IndexMap<EnvKey, EnvVal>, ShellError> {
+    pub fn maintain_autoenv(&mut self) -> Result<(), ShellError> {
         let mut dir = current_dir()?;
-        let mut vars_to_add: IndexMap<EnvKey, EnvVal> = IndexMap::new();
 
-        //If we are in the last seen directory, do nothing
+        if self.last_seen_directory == dir {
-        //If we are in a parent directory to last_seen_directory, just return without applying .nu-env in the parent directory - they were already applied earlier.
+            return Ok(());
-        //parent.cmp(child) = Less
+        }
+
+        let mut added_keys = IndexSet::new();
+        //We note which directories we pass so we can clear unvisited dirs later.
+        let mut seen_directories = IndexSet::new();
+
+        //Add all .nu-envs until we reach a dir which we have already added, or we reached the root.
         let mut popped = true;
-        while self.last_seen_directory.cmp(&dir) == Less && popped {
+        while !self.added_vars.contains_key(&dir) && popped {
             let nu_env_file = dir.join(".nu-env");
             if nu_env_file.exists() {
-                let nu_env_doc = self.toml_if_directory_is_trusted(&nu_env_file)?;
+                let nu_env_doc = self.toml_if_trusted(&nu_env_file)?;
 
                 //add regular variables from the [env section]
                 if let Some(env) = nu_env_doc.env {
                     for (env_key, env_val) in env {
-                        self.add_key_if_appropriate(&mut vars_to_add, &dir, &env_key, &env_val);
+                        self.maybe_add_key(&mut added_keys, &dir, &env_key, &env_val);
                     }
                 }
 
                 //Add variables that need to evaluate scripts to run, from [scriptvars] section
-                if let Some(scriptvars) = nu_env_doc.scriptvars {
+                if let Some(sv) = nu_env_doc.scriptvars {
-                    for (env_key, dir_val_script) in scriptvars {
+                    for (key, script) in sv {
-                        let command = if cfg!(target_os = "windows") {
+                        self.maybe_add_key(
-                            Command::new("cmd")
+                            &mut added_keys,
-                                .args(&["/C", dir_val_script.as_str()])
-                                .output()?
-                        } else {
-                            Command::new("sh").arg("-c").arg(&dir_val_script).output()?
-                        };
-                        if command.stdout.is_empty() {
-                            return Err(ShellError::untagged_runtime_error(format!(
-                                "{:?} in {:?} did not return any output",
-                                dir_val_script, dir
-                            )));
-                        }
-                        let response =
-                            std::str::from_utf8(&command.stdout[..command.stdout.len() - 1])
-                                .or_else(|e| {
-                                    Err(ShellError::untagged_runtime_error(format!(
-                                        "Couldn't parse stdout from command {:?}: {:?}",
-                                        command, e
-                                    )))
-                                })?;
-                        self.add_key_if_appropriate(
-                            &mut vars_to_add,
                             &dir,
-                            &env_key,
+                            &key,
-                            &response.to_string(),
+                            value_from_script(&script)?.as_str(),
                         );
                     }
                 }
 
-                if let Some(entryscripts) = nu_env_doc.entryscripts {
+                if let Some(es) = nu_env_doc.entryscripts {
-                    for script in entryscripts {
+                    for s in es {
-                        if cfg!(target_os = "windows") {
+                        run(s.as_str())?;
-                            Command::new("cmd")
-                                .args(&["/C", script.as_str()])
-                                .output()?;
-                        } else {
-                            Command::new("sh").arg("-c").arg(script).output()?;
-                        }
                     }
                 }
 
-                if let Some(exitscripts) = nu_env_doc.exitscripts {
+                if let Some(es) = nu_env_doc.exitscripts {
-                    self.exitscripts.insert(dir.clone(), exitscripts);
+                    self.exitscripts.insert(dir.clone(), es);
                 }
             }
+            seen_directories.insert(dir.clone());
             popped = dir.pop();
         }
 
-        Ok(vars_to_add)
+        //Time to clear out vars set by directories that we have left.
+        let mut new_vars = IndexMap::new();
+        for (dir, dirmap) in self.added_vars.drain(..) {
+            if seen_directories.contains(&dir) {
+                new_vars.insert(dir, dirmap);
+            } else {
+                for (k, v) in dirmap {
+                    if let Some(v) = v {
+                        std::env::set_var(k, v);
+                    } else {
+                        std::env::remove_var(k);
+                    }
+                }
+                if let Some(es) = self.exitscripts.get(&dir) {
+                    for s in es {
+                        run(s.as_str())?;
+                    }
+                }
+            }
+        }
+        self.added_vars = new_vars;
+        self.last_seen_directory = current_dir()?;
+        Ok(())
     }
 
-    pub fn add_key_if_appropriate(
+    pub fn maybe_add_key(
         &mut self,
-        vars_to_add: &mut IndexMap<EnvKey, EnvVal>,
+        seen_vars: &mut IndexSet<EnvKey>,
         dir: &PathBuf,
-        env_key: &str,
+        key: &str,
-        env_val: &str,
+        val: &str,
     ) {
         //This condition is to make sure variables in parent directories don't overwrite variables set by subdirectories.
-        if !vars_to_add.contains_key(env_key) {
+        if !seen_vars.contains(key) {
-            vars_to_add.insert(env_key.to_string(), OsString::from(env_val));
+            seen_vars.insert(key.to_string());
-            self.added_env_vars
+            self.added_vars
                 .entry(dir.clone())
                 .or_insert(IndexMap::new())
-                .insert(env_key.to_string(), var_os(env_key));
+                .insert(key.to_string(), var_os(key));
+
+            std::env::set_var(key, val);
         }
     }
 
-    pub fn cleanup_after_dir_exit(
-        &mut self,
-    ) -> Result<IndexMap<EnvKey, Option<EnvVal>>, ShellError> {
-        let current_dir = current_dir()?;
-        let mut vars_to_cleanup = IndexMap::new();
-
-        //If we are in the same directory as last_seen, or a subdirectory to it, do nothing
-        //If we are in a subdirectory to last seen, do nothing
-        //If we are in a parent directory to last seen, exit .nu-envs from last seen to parent and restore old vals
-        let mut dir = self.last_seen_directory.clone();
-
-        let mut popped = true;
-        while current_dir.cmp(&dir) == Less && popped {
-            if let Some(vars_added_by_this_directory) = self.added_env_vars.get(&dir) {
-                for (k, v) in vars_added_by_this_directory {
-                    vars_to_cleanup.insert(k.clone(), v.clone());
-                }
-                self.added_env_vars.remove(&dir);
-            }
-
-            if let Some(scripts) = self.exitscripts.get(&dir) {
-                for script in scripts {
-                    if cfg!(target_os = "windows") {
-                        Command::new("cmd")
-                            .args(&["/C", script.as_str()])
-                            .output()?;
-                    } else {
-                        Command::new("sh").arg("-c").arg(script).output()?;
-                    }
-                }
-            }
-            popped = dir.pop();
-        }
-        Ok(vars_to_cleanup)
-    }
-
     // If the user recently ran autoenv untrust on a file, we clear the environment variables it set and make sure to not run any possible exitscripts.
     pub fn clear_recently_untrusted_file(&mut self) -> Result<(), ShellError> {
         // Figure out which file was untrusted
@@ -213,7 +176,7 @@ impl DirectorySpecificEnvironment {
         // We figure out which file(s) the user untrusted by taking the set difference of current trusted files in .config/nu/nu-env.toml and the files tracked by self.added_env_vars
         // If a file is in self.added_env_vars but not in nu-env.toml, it was just untrusted.
         let untrusted_files: IndexSet<PathBuf> = self
-            .added_env_vars
+            .added_vars
             .iter()
             .filter_map(|(path, _)| {
                 if !current_trusted_files.contains(path) {
@@ -224,15 +187,45 @@ impl DirectorySpecificEnvironment {
             .collect();
 
         for path in untrusted_files {
-            if let Some(added_keys) = self.added_env_vars.get(&path) {
+            if let Some(added_keys) = self.added_vars.get(&path) {
                 for (key, _) in added_keys {
                     remove_var(key);
                 }
             }
             self.exitscripts.remove(&path);
-            self.added_env_vars.remove(&path);
+            self.added_vars.remove(&path);
         }
 
         Ok(())
     }
 }
+
+fn run(cmd: &str) -> Result<(), ShellError> {
+    if cfg!(target_os = "windows") {
+        Command::new("cmd").args(&["/C", cmd]).output()?
+    } else {
+        Command::new("sh").arg("-c").arg(&cmd).output()?
+    };
+    Ok(())
+}
+fn value_from_script(cmd: &str) -> Result<String, ShellError> {
+    let command = if cfg!(target_os = "windows") {
+        Command::new("cmd").args(&["/C", cmd]).output()?
+    } else {
+        Command::new("sh").arg("-c").arg(&cmd).output()?
+    };
+    if command.stdout.is_empty() {
+        return Err(ShellError::untagged_runtime_error(format!(
+            "{:?} did not return any output",
+            cmd
+        )));
+    }
+    let response = std::str::from_utf8(&command.stdout[..command.stdout.len()]).or_else(|e| {
+        Err(ShellError::untagged_runtime_error(format!(
+            "Couldn't parse stdout from command {:?}: {:?}",
+            command, e
+        )))
+    })?;
+
+    Ok(response.trim().to_string())
+}

crates/nu-cli/src/env/environment.rs

@@ -61,23 +61,10 @@ impl Environment {
     }
 
     pub fn autoenv(&mut self, reload_trusted: bool) -> Result<(), ShellError> {
-        for (k, v) in self.autoenv.env_vars_to_add()? {
+        self.autoenv.maintain_autoenv()?;
-            set_var(&k, OsString::from(v.to_string_lossy().to_string()));
-        }
-
-        for (k, v) in self.autoenv.cleanup_after_dir_exit()? {
-            if let Some(v) = v {
-                set_var(k, v);
-            } else {
-                remove_var(k);
-            }
-        }
-
         if reload_trusted {
             self.autoenv.clear_recently_untrusted_file()?;
         }
-
-        self.autoenv.last_seen_directory = current_dir()?;
         Ok(())
     }
 

tests/shell/pipeline/commands/internal.rs

@@ -1,4 +1,5 @@
 use nu_test_support::fs::Stub::EmptyFile;
+use nu_test_support::fs::Stub::FileWithContent;
 use nu_test_support::fs::Stub::FileWithContentToBeTrimmed;
 use nu_test_support::nu;
 use nu_test_support::pipeline;
@@ -36,6 +37,148 @@ fn takes_rows_of_nu_value_strings_and_pipes_it_to_stdin_of_external() {
     })
 }
 
+#[test]
+fn autoenv() {
+    Playground::setup("autoenv_test", |dirs, sandbox| {
+        sandbox.mkdir("foo/bar");
+        sandbox.mkdir("foob");
+
+        let scriptfile = if cfg!(target_os = "windows") {
+            FileWithContent(
+                ".nu-env",
+                r#"[env]
+                    testkey = "testvalue"
+
+                    [scriptvars]
+                    myscript = "echo myval"
+
+                    [scripts]
+                    entryscripts = ["echo nul > hello.txt"]
+                    exitscripts = ["echo nul > bye.txt"]"#,
+            )
+        } else {
+            FileWithContent(
+                ".nu-env",
+                r#"[env]
+                    testkey = "testvalue"
+
+                    [scriptvars]
+                    myscript = "echo myval"
+
+                    [scripts]
+                    entryscripts = ["touch hello.txt"]
+                    exitscripts = ["touch bye.txt"]"#,
+            )
+        };
+
+        sandbox.with_files(vec![
+            scriptfile,
+            FileWithContent(
+                "foo/.nu-env",
+                r#"[env]
+                    overwrite_me = "set_in_foo"
+                    fookey = "fooval""#,
+            ),
+            FileWithContent(
+                "foo/bar/.nu-env",
+                r#"[env]
+                    overwrite_me = "set_in_bar""#,
+            ),
+        ]);
+
+        //Make sure basic keys are set
+        let actual = nu!(
+            cwd: dirs.test(),
+            r#"autoenv trust
+               echo $nu.env.testkey"#
+        );
+        assert!(actual.out.ends_with("testvalue"));
+
+        //Backing out of the directory should unset the keys
+        let actual = nu!(
+            cwd: dirs.test(),
+            r#"cd ..
+               echo $nu.env.testkey"#
+        );
+        assert!(!actual.out.ends_with("testvalue"));
+
+        // Make sure script keys are set
+        let actual = nu!(
+            cwd: dirs.test(),
+            r#"echo $nu.env.myscript"#
+        );
+        assert!(actual.out.ends_with("myval"));
+
+        //Going to sibling directory without passing parent should work.
+        let actual = nu!(
+            cwd: dirs.test(),
+            r#"autoenv trust foo
+               cd foob
+               cd ../foo
+               echo $nu.env.fookey
+               cd .."#
+        );
+        assert!(actual.out.ends_with("fooval"));
+
+        //Going to sibling directory should unset keys
+        let actual = nu!(
+            cwd: dirs.test(),
+            r#"cd foo
+               cd ../foob
+               echo $nu.env.fookey
+               cd .."#
+        );
+        assert!(!actual.out.ends_with("fooval"));
+
+        // Make sure entry scripts are run
+        let actual = nu!(
+            cwd: dirs.test(),
+            r#"ls | where name == "hello.txt" | get name"#
+        );
+        assert!(actual.out.contains("hello.txt"));
+
+        // Make sure exit scripts are run
+        let actual = nu!(
+            cwd: dirs.test(),
+            r#"cd ..
+               ls | where name == "bye.txt" | get name"#
+        );
+        assert!(actual.out.contains("bye.txt"));
+
+        //Subdirectories should overwrite the values of parent directories.
+        let actual = nu!(
+            cwd: dirs.test(),
+            r#"autoenv trust foo
+                   cd foo/bar
+                   autoenv trust
+                   echo $nu.env.overwrite_me"#
+        );
+        assert!(actual.out.ends_with("set_in_bar"));
+
+        //Variables set in parent directories should be set even if you directly cd to a subdir
+        let actual = nu!(
+            cwd: dirs.test(),
+            r#"autoenv trust foo
+                   cd foo/bar
+                   autoenv trust
+                   echo $nu.env.fookey"#
+        );
+        assert!(actual.out.ends_with("fooval"));
+
+        //Make sure that overwritten values are restored.
+        //By deleting foo/.nu-env, we make sure that the value is actually restored and not just set again by autoenv when we re-visit foo.
+        let actual = nu!(
+            cwd: dirs.test(),
+            r#"cd foo
+                   cd bar
+                   rm ../.nu-env
+                   cd ..
+                   echo $nu.env.overwrite_me"#
+        );
+        assert!(actual.out.ends_with("set_in_foo"))
+    })
+}
+
 #[test]
 fn proper_it_expansion() {
     Playground::setup("ls_test_1", |dirs, sandbox| {