Use guard type to replace with_saved_path()

Author: djc

src/cli/self_update/test.rs

@@ -1,7 +1,10 @@
 //! Support for functional tests.
 
 #[cfg(windows)]
-use std::{io, sync::Mutex};
+use std::{
+    io,
+    sync::{LockResult, Mutex, MutexGuard},
+};
 
 #[cfg(windows)]
 use winreg::{
@@ -11,37 +14,40 @@ use winreg::{
 };
 
 #[cfg(windows)]
-pub fn with_saved_path(f: &mut dyn FnMut()) {
-    with_saved_reg_value(&USER_PATH, f)
+pub fn get_path() -> io::Result<Option<RegValue>> {
+    USER_PATH.get()
 }
 
-#[cfg(unix)]
-pub fn with_saved_path(f: &mut dyn FnMut()) {
-    f()
+#[cfg(windows)]
+pub struct RegistryGuard<'a> {
+    _locked: LockResult<MutexGuard<'a, ()>>,
+    id: &'static RegistryValueId,
+    prev: Option<RegValue>,
 }
 
 #[cfg(windows)]
-pub fn get_path() -> io::Result<Option<RegValue>> {
-    USER_PATH.get()
+impl<'a> RegistryGuard<'a> {
+    pub fn new(id: &'static RegistryValueId) -> io::Result<Self> {
+        Ok(Self {
+            _locked: REGISTRY_LOCK.lock(),
+            id,
+            prev: id.get()?,
+        })
+    }
 }
 
 #[cfg(windows)]
-pub fn with_saved_reg_value(id: &RegistryValueId, f: &mut dyn FnMut()) {
-    // Lock protects concurrent mutation of registry
-    static LOCK: Mutex<()> = Mutex::new(());
-    let _g = LOCK.lock();
-
-    // Save and restore the global state here to keep from trashing things.
-    let saved_state = id
-        .get()
-        .expect("Error getting global state: Better abort to avoid trashing it");
-    let _g = scopeguard::guard(saved_state, |p| id.set(p.as_ref()).unwrap());
-
-    f();
+impl<'a> Drop for RegistryGuard<'a> {
+    fn drop(&mut self) {
+        self.id.set(self.prev.as_ref()).unwrap();
+    }
 }
 
 #[cfg(windows)]
-const USER_PATH: RegistryValueId = RegistryValueId {
+static REGISTRY_LOCK: Mutex<()> = Mutex::new(());
+
+#[cfg(windows)]
+pub const USER_PATH: RegistryValueId = RegistryValueId {
     sub_key: "Environment",
     value_name: "PATH",
 };

src/cli/self_update/windows.rs

@@ -816,7 +816,7 @@ mod tests {
     use winreg::{RegKey, RegValue};
 
     use crate::currentprocess::TestProcess;
-    use crate::test::with_saved_path;
+    use crate::test::{RegistryGuard, USER_PATH};
 
     fn wide(str: &str) -> Vec<u16> {
         OsString::from(str).encode_wide().collect()
@@ -856,60 +856,58 @@ mod tests {
     #[test]
     fn windows_path_regkey_type() {
         // per issue #261, setting PATH should use REG_EXPAND_SZ.
-        with_saved_path(&mut || {
-            let root = RegKey::predef(HKEY_CURRENT_USER);
-            let environment = root
-                .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-                .unwrap();
-            environment.delete_value("PATH").unwrap();
-
-            {
-                // Can't compare the Results as Eq isn't derived; thanks error-chain.
-                #![allow(clippy::unit_cmp)]
-                assert_eq!((), super::_apply_new_path(Some(wide("foo"))).unwrap());
-            }
-            let root = RegKey::predef(HKEY_CURRENT_USER);
-            let environment = root
-                .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-                .unwrap();
-            let path = environment.get_raw_value("PATH").unwrap();
-            assert_eq!(path.vtype, RegType::REG_EXPAND_SZ);
-            assert_eq!(super::to_winreg_bytes(wide("foo")), &path.bytes[..]);
-        });
+        let _guard = RegistryGuard::new(&USER_PATH);
+        let root = RegKey::predef(HKEY_CURRENT_USER);
+        let environment = root
+            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
+            .unwrap();
+        environment.delete_value("PATH").unwrap();
+
+        {
+            // Can't compare the Results as Eq isn't derived; thanks error-chain.
+            #![allow(clippy::unit_cmp)]
+            assert_eq!((), super::_apply_new_path(Some(wide("foo"))).unwrap());
+        }
+        let root = RegKey::predef(HKEY_CURRENT_USER);
+        let environment = root
+            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
+            .unwrap();
+        let path = environment.get_raw_value("PATH").unwrap();
+        assert_eq!(path.vtype, RegType::REG_EXPAND_SZ);
+        assert_eq!(super::to_winreg_bytes(wide("foo")), &path.bytes[..]);
     }
 
     #[test]
     fn windows_path_delete_key_when_empty() {
         use std::io;
         // during uninstall the PATH key may end up empty; if so we should
         // delete it.
-        with_saved_path(&mut || {
-            let root = RegKey::predef(HKEY_CURRENT_USER);
-            let environment = root
-                .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-                .unwrap();
-            environment
-                .set_raw_value(
-                    "PATH",
-                    &RegValue {
-                        bytes: super::to_winreg_bytes(wide("foo")),
-                        vtype: RegType::REG_EXPAND_SZ,
-                    },
-                )
-                .unwrap();
-
-            {
-                // Can't compare the Results as Eq isn't derived; thanks error-chain.
-                #![allow(clippy::unit_cmp)]
-                assert_eq!((), super::_apply_new_path(Some(Vec::new())).unwrap());
-            }
-            let reg_value = environment.get_raw_value("PATH");
-            match reg_value {
-                Ok(_) => panic!("key not deleted"),
-                Err(ref e) if e.kind() == io::ErrorKind::NotFound => {}
-                Err(ref e) => panic!("error {e}"),
-            }
-        });
+        let _guard = RegistryGuard::new(&USER_PATH);
+        let root = RegKey::predef(HKEY_CURRENT_USER);
+        let environment = root
+            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
+            .unwrap();
+        environment
+            .set_raw_value(
+                "PATH",
+                &RegValue {
+                    bytes: super::to_winreg_bytes(wide("foo")),
+                    vtype: RegType::REG_EXPAND_SZ,
+                },
+            )
+            .unwrap();
+
+        {
+            // Can't compare the Results as Eq isn't derived; thanks error-chain.
+            #![allow(clippy::unit_cmp)]
+            assert_eq!((), super::_apply_new_path(Some(Vec::new())).unwrap());
+        }
+        let reg_value = environment.get_raw_value("PATH");
+        match reg_value {
+            Ok(_) => panic!("key not deleted"),
+            Err(ref e) if e.kind() == io::ErrorKind::NotFound => {}
+            Err(ref e) => panic!("error {e}"),
+        }
     }
 
     #[test]
@@ -921,22 +919,23 @@ mod tests {
                 .cloned()
                 .collect(),
         );
-        with_saved_path(&mut || {
-            let root = RegKey::predef(HKEY_CURRENT_USER);
-            let environment = root
-                .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-                .unwrap();
-            let reg_value = RegValue {
-                bytes: vec![0x12, 0x34],
-                vtype: RegType::REG_BINARY,
-            };
-            environment.set_raw_value("PATH", &reg_value).unwrap();
-            // Ok(None) signals no change to the PATH setting layer
-            assert_eq!(
-                None,
-                super::_with_path_cargo_home_bin(|_, _| panic!("called"), &tp.process).unwrap()
-            );
-        });
+
+        let _guard = RegistryGuard::new(&USER_PATH);
+        let root = RegKey::predef(HKEY_CURRENT_USER);
+        let environment = root
+            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
+            .unwrap();
+        let reg_value = RegValue {
+            bytes: vec![0x12, 0x34],
+            vtype: RegType::REG_BINARY,
+        };
+        environment.set_raw_value("PATH", &reg_value).unwrap();
+        // Ok(None) signals no change to the PATH setting layer
+        assert_eq!(
+            None,
+            super::_with_path_cargo_home_bin(|_, _| panic!("called"), &tp.process).unwrap()
+        );
+
         assert_eq!(
             r"warn: the registry key HKEY_CURRENT_USER\Environment\PATH is not a string. Not modifying the PATH variable
 ",
@@ -947,15 +946,14 @@ mod tests {
     #[test]
     fn windows_treat_missing_path_as_empty() {
         // during install the PATH key may be missing; treat it as empty
-        with_saved_path(&mut || {
-            let root = RegKey::predef(HKEY_CURRENT_USER);
-            let environment = root
-                .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-                .unwrap();
-            environment.delete_value("PATH").unwrap();
-
-            assert_eq!(Some(Vec::new()), super::get_windows_path_var().unwrap());
-        });
+        let _guard = RegistryGuard::new(&USER_PATH);
+        let root = RegKey::predef(HKEY_CURRENT_USER);
+        let environment = root
+            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
+            .unwrap();
+        environment.delete_value("PATH").unwrap();
+
+        assert_eq!(Some(Vec::new()), super::get_windows_path_var().unwrap());
     }
 
     #[test]

src/env_var.rs

@@ -47,8 +47,10 @@ mod tests {
     use std::ffi::{OsStr, OsString};
 
     use super::*;
+    #[cfg(windows)]
+    use crate::cli::self_update::test::{RegistryGuard, USER_PATH};
     use crate::currentprocess::TestProcess;
-    use crate::test::{with_saved_path, Env};
+    use crate::test::Env;
 
     #[test]
     fn prepend_unique_path() {
@@ -58,43 +60,43 @@ mod tests {
             env::join_paths(["/home/a/.cargo/bin", "/home/b/.cargo/bin"].iter()).unwrap(),
         );
         let tp = TestProcess::with_vars(vars);
-        with_saved_path(&mut || {
-            let mut path_entries = vec![];
-            let mut cmd = Command::new("test");
+        #[cfg(windows)]
+        let _path_guard = RegistryGuard::new(&USER_PATH).unwrap();
+        let mut path_entries = vec![];
+        let mut cmd = Command::new("test");
 
-            let a = OsString::from("/home/a/.cargo/bin");
-            let path_a = PathBuf::from(a);
-            path_entries.push(path_a);
+        let a = OsString::from("/home/a/.cargo/bin");
+        let path_a = PathBuf::from(a);
+        path_entries.push(path_a);
 
-            let _a = OsString::from("/home/a/.cargo/bin");
-            let _path_a = PathBuf::from(_a);
-            path_entries.push(_path_a);
+        let _a = OsString::from("/home/a/.cargo/bin");
+        let _path_a = PathBuf::from(_a);
+        path_entries.push(_path_a);
 
-            let z = OsString::from("/home/z/.cargo/bin");
-            let path_z = PathBuf::from(z);
-            path_entries.push(path_z);
+        let z = OsString::from("/home/z/.cargo/bin");
+        let path_z = PathBuf::from(z);
+        path_entries.push(path_z);
 
-            prepend_path("PATH", path_entries, &mut cmd, &tp.process);
-            let envs: Vec<_> = cmd.get_envs().collect();
+        prepend_path("PATH", path_entries, &mut cmd, &tp.process);
+        let envs: Vec<_> = cmd.get_envs().collect();
 
-            assert_eq!(
-                envs,
-                &[(
-                    OsStr::new("PATH"),
-                    Some(
-                        env::join_paths(
-                            [
-                                "/home/z/.cargo/bin",
-                                "/home/a/.cargo/bin",
-                                "/home/b/.cargo/bin"
-                            ]
-                            .iter()
-                        )
-                        .unwrap()
-                        .as_os_str()
+        assert_eq!(
+            envs,
+            &[(
+                OsStr::new("PATH"),
+                Some(
+                    env::join_paths(
+                        [
+                            "/home/z/.cargo/bin",
+                            "/home/a/.cargo/bin",
+                            "/home/b/.cargo/bin"
+                        ]
+                        .iter()
                     )
-                ),]
-            );
-        });
+                    .unwrap()
+                    .as_os_str()
+                )
+            ),]
+        );
     }
 }

src/test.rs

@@ -15,12 +15,11 @@ use std::process::Command;
 #[cfg(test)]
 use anyhow::Result;
 
-pub use crate::cli::self_update::test::with_saved_path;
 use crate::currentprocess::TestProcess;
 use crate::dist::TargetTriple;
 
 #[cfg(windows)]
-pub use crate::cli::self_update::test::{get_path, with_saved_reg_value, RegistryValueId};
+pub use crate::cli::self_update::test::{get_path, RegistryGuard, RegistryValueId, USER_PATH};
 
 // Things that can have environment variables applied to them.
 pub trait Env {