Fix removing from PATH on windows when last item

When the cargo bin is the last item in the PATH on windows, a trailing ;
was left behind, which leave an empty segment in the PATH.

Also refactor into unit tests, for faster testing.

This has arguably the same coverage: we know that windows path setting
works in general due to the presence of at least one end to end test;
and it is implemented by calling this one code path; this pure function
in the code path can therefore be unit tested to examine it's corner
case behaviour rather than testing end to end every possible
interaction.

$ time ...

running 1 test
test cli::self_update::windows::tests::windows_uninstall_removes_semicolon_from_path ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 4 filtered out

real    0m0.041s
user    0m0.000s
sys     0m0.030s

$ time ...

running 1 test
test windows_uninstall_removes_semicolon_from_path ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 56 filtered out

real    0m13.450s
user    0m0.000s
sys     0m0.015s

Author: rbtcollins

src/cli/self_update/windows.rs

@@ -222,43 +222,59 @@ fn get_windows_path_var() -> Result<Option<String>> {
     }
 }
 
-pub fn do_remove_from_path(methods: &[PathUpdateMethod]) -> Result<()> {
-    assert!(methods.len() == 1 && methods[0] == PathUpdateMethod::Windows);
+// Returns None if the existing old_path does not need changing
+fn _remove_from_path(old_path: &str, path_str: &str) -> Option<String> {
+    let idx = old_path.find(&path_str)?;
+    // If there's a trailing semicolon (likely, since we probably added one
+    // during install), include that in the substring to remove. We don't search
+    // for that to find the string, because if its the last string in the path,
+    // there may not be.
+    let mut len = path_str.len();
+    if old_path.as_bytes().get(idx + path_str.len()) == Some(&b';') {
+        len += 1;
+    }
 
-    use std::ptr;
-    use winapi::shared::minwindef::*;
-    use winapi::um::winuser::{
-        SendMessageTimeoutA, HWND_BROADCAST, SMTO_ABORTIFHUNG, WM_SETTINGCHANGE,
-    };
-    use winreg::enums::{RegType, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE};
-    use winreg::{RegKey, RegValue};
+    let mut new_path = old_path[..idx].to_string();
+    new_path.push_str(&old_path[idx + len..]);
+    // Don't leave a trailing ; though, we don't want an empty string in the
+    // path.
+    if new_path.ends_with(';') {
+        new_path.pop();
+    }
+    Some(new_path)
+}
 
+fn _path_without_cargo_home_bin() -> Result<Option<String>> {
     let old_path = if let Some(s) = get_windows_path_var()? {
         s
     } else {
         // Non-unicode path
-        return Ok(());
+        return Ok(None);
     };
 
     let path_str = utils::cargo_home()?
         .join("bin")
         .to_string_lossy()
         .into_owned();
-    let idx = if let Some(i) = old_path.find(&path_str) {
-        i
-    } else {
-        return Ok(());
-    };
 
-    // If there's a trailing semicolon (likely, since we added one during install),
-    // include that in the substring to remove.
-    let mut len = path_str.len();
-    if old_path.as_bytes().get(idx + path_str.len()) == Some(&b';') {
-        len += 1;
-    }
+    Ok(_remove_from_path(&old_path, &path_str))
+}
 
-    let mut new_path = old_path[..idx].to_string();
-    new_path.push_str(&old_path[idx + len..]);
+pub fn do_remove_from_path(methods: &[PathUpdateMethod]) -> Result<()> {
+    assert!(methods.len() == 1 && methods[0] == PathUpdateMethod::Windows);
+
+    use std::ptr;
+    use winapi::shared::minwindef::*;
+    use winapi::um::winuser::{
+        SendMessageTimeoutA, HWND_BROADCAST, SMTO_ABORTIFHUNG, WM_SETTINGCHANGE,
+    };
+    use winreg::enums::{RegType, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE};
+    use winreg::{RegKey, RegValue};
+
+    let new_path = match _path_without_cargo_home_bin()? {
+        Some(new_path) => new_path,
+        None => return Ok(()), // No need to set the path
+    };
 
     let root = RegKey::predef(HKEY_CURRENT_USER);
     let environment = root
@@ -430,3 +446,30 @@ pub fn get_add_path_methods() -> Vec<PathUpdateMethod> {
 pub fn get_remove_path_methods() -> Result<Vec<PathUpdateMethod>> {
     Ok(vec![PathUpdateMethod::Windows])
 }
+
+#[cfg(test)]
+mod tests {
+    #[test]
+    fn windows_uninstall_removes_semicolon_from_path_prefix() {
+        assert_eq!(
+            "foo",
+            super::_remove_from_path(
+                r"c:\users\example\.cargo\bin;foo",
+                r"c:\users\example\.cargo\bin"
+            )
+            .unwrap()
+        )
+    }
+
+    #[test]
+    fn windows_uninstall_removes_semicolon_from_path_suffix() {
+        assert_eq!(
+            "foo",
+            super::_remove_from_path(
+                r"foo;c:\users\example\.cargo\bin",
+                r"c:\users\example\.cargo\bin"
+            )
+            .unwrap()
+        )
+    }
+}

tests/cli-self-upd.rs

@@ -1091,42 +1091,6 @@ fn windows_handle_empty_path_registry_key() {
     });
 }
 
-#[test]
-#[cfg(windows)]
-fn windows_uninstall_removes_semicolon_from_path() {
-    use winreg::enums::{RegType, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE};
-    use winreg::RegKey;
-
-    setup(&|config| {
-        let root = RegKey::predef(HKEY_CURRENT_USER);
-        let environment = root
-            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-            .unwrap();
-
-        // This time set the value of PATH and make sure it's restored exactly after uninstall,
-        // not leaving behind any semi-colons
-        environment.set_value("PATH", &"foo").unwrap();
-
-        expect_ok(config, &["rustup-init", "-y"]);
-
-        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!(path.vtype == RegType::REG_EXPAND_SZ);
-
-        expect_ok(config, &["rustup", "self", "uninstall", "-y"]);
-
-        let root = RegKey::predef(HKEY_CURRENT_USER);
-        let environment = root
-            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-            .unwrap();
-        let path: String = environment.get_value("PATH").unwrap();
-        assert!(path == "foo");
-    });
-}
-
 #[test]
 #[cfg(windows)]
 fn install_doesnt_mess_with_a_non_unicode_path() {