From 80d2a7ee7adf1deb47f542c057019dd80f1d4982 Mon Sep 17 00:00:00 2001 From: Sam Hedin Date: Tue, 14 Jul 2020 21:16:50 +0200 Subject: [PATCH] Fix autoenv executing scripts multiple times (#2171) * Fix autoenv executing scripts multiple times Previously, if the user had only specified entry or exitscripts the scripts would execute many times. This should be fixed now * Add tests * Run exitscripts * More tests and fixes to existing tests * Test solution with visited dirs * Track visited directories * Comments and fmt --- .../src/env/directory_specific_environment.rs | 34 +++++++++-- tests/shell/pipeline/commands/internal.rs | 61 +++++++++++++++---- 2 files changed, 79 insertions(+), 16 deletions(-) diff --git a/crates/nu-cli/src/env/directory_specific_environment.rs b/crates/nu-cli/src/env/directory_specific_environment.rs index 2786e65e78..08ccd4396d 100644 --- a/crates/nu-cli/src/env/directory_specific_environment.rs +++ b/crates/nu-cli/src/env/directory_specific_environment.rs @@ -20,6 +20,9 @@ pub struct DirectorySpecificEnvironment { //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_vars: IndexMap>>, + + //We track directories that we have read .nu-env from. This is different from the keys in added_vars since sometimes a file only wants to run scripts. + visited_dirs: IndexSet, exitscripts: IndexMap>, } @@ -42,6 +45,7 @@ impl DirectorySpecificEnvironment { DirectorySpecificEnvironment { last_seen_directory: root_dir, added_vars: IndexMap::new(), + visited_dirs: IndexSet::new(), exitscripts: IndexMap::new(), } } @@ -75,13 +79,15 @@ impl DirectorySpecificEnvironment { return Ok(()); } + //We track which keys we set as we go up the directory hierarchy, so that we don't overwrite a value we set in a subdir. 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.added_vars.contains_key(&dir) && popped { + while popped && !self.visited_dirs.contains(&dir) { let nu_env_file = dir.join(".nu-env"); if nu_env_file.exists() { let nu_env_doc = self.toml_if_trusted(&nu_env_file)?; @@ -92,6 +98,7 @@ impl DirectorySpecificEnvironment { self.maybe_add_key(&mut added_keys, &dir, &env_key, &env_val); } } + self.visited_dirs.insert(dir.clone()); //Add variables that need to evaluate scripts to run, from [scriptvars] section if let Some(sv) = nu_env_doc.scriptvars { @@ -132,13 +139,30 @@ impl DirectorySpecificEnvironment { std::env::remove_var(k); } } - if let Some(es) = self.exitscripts.get(&dir) { - for s in es { - run(s.as_str())?; - } + } + } + + let mut new_visited = IndexSet::new(); + for dir in self.visited_dirs.drain(..) { + if seen_directories.contains(&dir) { + new_visited.insert(dir); + } + } + + //Run exitscripts, can not be done in same loop as new vars as some files can contain only exitscripts + let mut new_exitscripts = IndexMap::new(); + for (dir, scripts) in self.exitscripts.drain(..) { + if seen_directories.contains(&dir) { + new_exitscripts.insert(dir, scripts); + } else { + for s in scripts { + run(s.as_str())?; } } } + + self.visited_dirs = new_visited; + self.exitscripts = new_exitscripts; self.added_vars = new_vars; self.last_seen_directory = current_dir()?; Ok(()) diff --git a/tests/shell/pipeline/commands/internal.rs b/tests/shell/pipeline/commands/internal.rs index 1338e4648e..a5436e1cf8 100644 --- a/tests/shell/pipeline/commands/internal.rs +++ b/tests/shell/pipeline/commands/internal.rs @@ -41,6 +41,7 @@ fn takes_rows_of_nu_value_strings_and_pipes_it_to_stdin_of_external() { fn autoenv() { Playground::setup("autoenv_test", |dirs, sandbox| { sandbox.mkdir("foo/bar"); + sandbox.mkdir("bizz/buzz"); sandbox.mkdir("foob"); let scriptfile = if cfg!(target_os = "windows") { @@ -77,15 +78,53 @@ fn autoenv() { "foo/.nu-env", r#"[env] overwrite_me = "set_in_foo" - fookey = "fooval""#, + fookey = "fooval" "#, ), FileWithContent( "foo/bar/.nu-env", r#"[env] overwrite_me = "set_in_bar""#, ), + FileWithContent( + "bizz/.nu-env", + r#"[scripts] + entryscripts = ["touch hello.txt"] + exitscripts = ["touch bye.txt"]"#, + ), ]); + // Make sure entry scripts are run + let actual = nu!( + cwd: dirs.test(), + r#"cd .. + autoenv trust autoenv_test + cd autoenv_test + ls | where name == "hello.txt" | get name"# + ); + assert!(actual.out.contains("hello.txt")); + + // Make sure entry scripts are run when re-visiting a directory + let actual = nu!( + cwd: dirs.test(), + r#"autoenv trust bizz + cd bizz + rm hello.txt + cd .. + cd bizz + ls | where name == "hello.txt" | get name"# + ); + assert!(actual.out.contains("hello.txt")); + + // Entryscripts should not run after changing to a subdirectory. + let actual = nu!( + cwd: dirs.test(), + r#"autoenv trust bizz + cd bizz + cd buzz + ls | where name == hello.txt | get name"# + ); + assert!(!actual.out.ends_with("hello.txt")); + //Make sure basic keys are set let actual = nu!( cwd: dirs.test(), @@ -145,16 +184,6 @@ fn autoenv() { ); 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(), @@ -165,6 +194,16 @@ fn autoenv() { ); assert!(actual.out.ends_with("fooval")); + //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")); + //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!(