fix Windows-specific path normalization using GetLongPathNameW (#277)

Fixes #186

Author: karthiknadig

Cargo.lock

@@ -6,6 +6,7 @@ license = "MIT"
 
 [target.'cfg(target_os = "windows")'.dependencies]
 msvc_spectre_libs = { version = "0.1.1", features = ["error"] }
+windows-sys = { version = "0.59", features = ["Win32_Storage_FileSystem", "Win32_Foundation"] }
 
 [dependencies]
 log = "0.4.21"

crates/pet-fs/Cargo.toml

@@ -6,9 +6,12 @@ use std::{
     path::{Path, PathBuf},
 };
 
-// Similar to fs::canonicalize, but ignores UNC paths and returns the path as is (for windows).
-// Usefulfor windows to ensure we have the paths in the right casing.
+// Normalizes the case of a path on Windows without resolving junctions/symlinks.
+// Uses GetLongPathNameW which normalizes case but preserves junction paths.
 // For unix, this is a noop.
+// Note: On Windows, case normalization only works for existing paths. For non-existent
+// paths, the function falls back to the absolute path without case normalization.
+// See: https://github.com/microsoft/python-environment-tools/issues/186
 pub fn norm_case<P: AsRef<Path>>(path: P) -> PathBuf {
     // On unix do not use canonicalize, results in weird issues with homebrew paths
     // Even readlink does the same thing
@@ -18,25 +21,96 @@ pub fn norm_case<P: AsRef<Path>>(path: P) -> PathBuf {
     return path.as_ref().to_path_buf();
 
     #[cfg(windows)]
-    use std::fs;
+    {
+        // First, convert to absolute path if relative, without resolving symlinks/junctions
+        let absolute_path = if path.as_ref().is_absolute() {
+            path.as_ref().to_path_buf()
+        } else if let Ok(abs) = std::env::current_dir() {
+            abs.join(path.as_ref())
+        } else {
+            path.as_ref().to_path_buf()
+        };
 
-    #[cfg(windows)]
-    if let Ok(resolved) = fs::canonicalize(&path) {
-        if cfg!(unix) {
-            return resolved;
-        }
-        // Windows specific handling, https://github.com/rust-lang/rust/issues/42869
-        let has_unc_prefix = path.as_ref().to_string_lossy().starts_with(r"\\?\");
-        if resolved.to_string_lossy().starts_with(r"\\?\") && !has_unc_prefix {
-            // If the resolved path has a UNC prefix, but the original path did not,
-            // we need to remove the UNC prefix.
-            PathBuf::from(resolved.to_string_lossy().trim_start_matches(r"\\?\"))
+        // Use GetLongPathNameW to normalize case without resolving junctions.
+        // If normalization fails, fall back to the computed absolute path to keep behavior consistent.
+        normalize_case_windows(&absolute_path).unwrap_or(absolute_path)
+    }
+}
+
+/// Windows-specific path case normalization using GetLongPathNameW.
+/// This normalizes the case of path components but does NOT resolve junctions or symlinks.
+/// Note: GetLongPathNameW requires the path to exist on the filesystem to normalize it.
+/// For non-existent paths, it will fail and this function returns None.
+/// Also note: Converting paths to strings via to_string_lossy() may lose information
+/// for paths with invalid UTF-8 sequences (replaced with U+FFFD), though Windows paths
+/// are typically valid Unicode.
+#[cfg(windows)]
+fn normalize_case_windows(path: &Path) -> Option<PathBuf> {
+    use std::ffi::OsString;
+    use std::os::windows::ffi::{OsStrExt, OsStringExt};
+    use windows_sys::Win32::Storage::FileSystem::GetLongPathNameW;
+
+    // Check if original path has UNC prefix before normalization
+    let original_path_str = path.to_string_lossy();
+    let original_has_unc = original_path_str.starts_with(r"\\?\");
+
+    // Normalize forward slashes to backslashes (canonicalize did this)
+    let path_str = original_path_str.replace('/', "\\");
+    let normalized_path = PathBuf::from(&path_str);
+
+    // Convert path to wide string (UTF-16) with null terminator
+    let wide_path: Vec<u16> = normalized_path
+        .as_os_str()
+        .encode_wide()
+        .chain(std::iter::once(0))
+        .collect();
+
+    // First call to get required buffer size
+    let required_len = unsafe { GetLongPathNameW(wide_path.as_ptr(), std::ptr::null_mut(), 0) };
+
+    if required_len == 0 {
+        // GetLongPathNameW failed (path likely doesn't exist), return None
+        return None;
+    }
+
+    // Allocate buffer and get the normalized path
+    let mut buffer: Vec<u16> = vec![0; required_len as usize];
+    let actual_len =
+        unsafe { GetLongPathNameW(wide_path.as_ptr(), buffer.as_mut_ptr(), required_len) };
+
+    if actual_len == 0 || actual_len > required_len {
+        // Call failed or buffer too small
+        return None;
+    }
+
+    // Truncate buffer to actual length (excluding null terminator)
+    buffer.truncate(actual_len as usize);
+
+    // Convert back to PathBuf
+    let os_string = OsString::from_wide(&buffer);
+    let mut result_str = os_string.to_string_lossy().to_string();
+
+    // Remove UNC prefix if original path didn't have it
+    // GetLongPathNameW may add \\?\ prefix in some cases
+    if result_str.starts_with(r"\\?\") && !original_has_unc {
+        result_str = result_str.trim_start_matches(r"\\?\").to_string();
+    }
+
+    // Strip trailing path separators to match canonicalize behavior,
+    // but avoid stripping them from root paths (drive roots, UNC roots, network paths).
+    // We use Path::parent() to detect root paths robustly.
+    let mut current_path = PathBuf::from(&result_str);
+    while current_path.parent().is_some() {
+        let s = current_path.to_string_lossy();
+        if s.ends_with('\\') || s.ends_with('/') {
+            result_str.pop();
+            current_path = PathBuf::from(&result_str);
         } else {
-            resolved
+            break;
         }
-    } else {
-        path.as_ref().to_path_buf()
     }
+
+    Some(PathBuf::from(result_str))
 }
 
 // Resolves symlinks to the real file.
@@ -107,3 +181,213 @@ fn get_user_home() -> Option<PathBuf> {
         Err(_) => None,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    #[cfg(unix)]
+    fn test_norm_case_returns_path_for_nonexistent_unix() {
+        // On Unix, norm_case returns the path unchanged (noop)
+        let nonexistent = PathBuf::from("/this/path/does/not/exist/anywhere");
+        let result = norm_case(&nonexistent);
+        assert_eq!(result, nonexistent);
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn test_norm_case_returns_absolute_for_nonexistent_windows() {
+        // On Windows, norm_case returns an absolute path even for non-existent paths
+        // (falls back to absolute_path when GetLongPathNameW fails)
+        let nonexistent = PathBuf::from("C:\\this\\path\\does\\not\\exist\\anywhere");
+        let result = norm_case(&nonexistent);
+        assert!(result.is_absolute(), "Result should be absolute path");
+        // The path should be preserved (just made absolute if it wasn't)
+        assert!(
+            result
+                .to_string_lossy()
+                .to_lowercase()
+                .contains("does\\not\\exist"),
+            "Path components should be preserved"
+        );
+    }
+
+    #[test]
+    fn test_norm_case_existing_path() {
+        // norm_case should work on existing paths
+        let temp_dir = std::env::temp_dir();
+        let result = norm_case(&temp_dir);
+        // On unix, should return unchanged; on Windows, should normalize case
+        assert!(result.exists());
+    }
+
+    #[test]
+    #[cfg(unix)]
+    fn test_norm_case_unix_noop() {
+        // On unix, norm_case should return the path unchanged
+        let path = PathBuf::from("/Some/Path/With/Mixed/Case");
+        let result = norm_case(&path);
+        assert_eq!(result, path);
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn test_norm_case_windows_case_normalization() {
+        // On Windows, norm_case should normalize the case of existing paths
+        // Use the Windows directory which always exists
+        let path = PathBuf::from("c:\\windows\\system32");
+        let result = norm_case(&path);
+        // The result should have proper casing (C:\Windows\System32)
+        assert!(result.to_string_lossy().contains("Windows"));
+        assert!(result.to_string_lossy().contains("System32"));
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn test_norm_case_windows_preserves_junction() {
+        // This is the key test for issue #186:
+        // norm_case should NOT resolve junctions to their target
+        use std::fs;
+        use std::process::Command;
+
+        let temp_dir = std::env::temp_dir();
+        let target_dir = temp_dir.join("pet_test_junction_target");
+        let junction_dir = temp_dir.join("pet_test_junction_link");
+
+        // Clean up any existing test directories
+        let _ = fs::remove_dir_all(&target_dir);
+        let _ = fs::remove_dir_all(&junction_dir);
+
+        // Create target directory
+        fs::create_dir_all(&target_dir).expect("Failed to create target directory");
+
+        // Create a junction using mklink /J (requires no special privileges)
+        let output = Command::new("cmd")
+            .args([
+                "/C",
+                "mklink",
+                "/J",
+                &junction_dir.to_string_lossy(),
+                &target_dir.to_string_lossy(),
+            ])
+            .output()
+            .expect("Failed to create junction");
+
+        if !output.status.success() {
+            // Clean up and skip test if junction creation failed
+            let _ = fs::remove_dir_all(&target_dir);
+            eprintln!(
+                "Skipping junction test - failed to create junction: {}",
+                String::from_utf8_lossy(&output.stderr)
+            );
+            return;
+        }
+
+        // Verify junction was created
+        assert!(junction_dir.exists(), "Junction should exist");
+
+        // The key assertion: norm_case should return the junction path, NOT the target path
+        let result = norm_case(&junction_dir);
+
+        // The result should still be the junction path, not resolved to target
+        // Compare the path names (case-insensitive on Windows)
+        assert!(
+            result
+                .to_string_lossy()
+                .to_lowercase()
+                .contains("pet_test_junction_link"),
+            "norm_case should preserve junction path, got: {:?}",
+            result
+        );
+        assert!(
+            !result
+                .to_string_lossy()
+                .to_lowercase()
+                .contains("pet_test_junction_target"),
+            "norm_case should NOT resolve to target path, got: {:?}",
+            result
+        );
+
+        // Clean up
+        // Remove junction first (using rmdir, not remove_dir_all, to not follow the junction)
+        let _ = Command::new("cmd")
+            .args(["/C", "rmdir", &junction_dir.to_string_lossy()])
+            .output();
+        let _ = fs::remove_dir_all(&target_dir);
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn test_norm_case_windows_relative_path() {
+        // Test that relative paths are converted to absolute
+        let relative = PathBuf::from(".");
+        let result = norm_case(&relative);
+        assert!(result.is_absolute(), "Result should be absolute path");
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn test_norm_case_windows_no_unc_prefix_added() {
+        // Ensure we don't add UNC prefix to paths that didn't have it
+        let path = PathBuf::from("C:\\Windows");
+        let result = norm_case(&path);
+        assert!(
+            !result.to_string_lossy().starts_with(r"\\?\"),
+            "Should not add UNC prefix"
+        );
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn test_norm_case_windows_strips_trailing_slash() {
+        // norm_case should strip trailing slashes to match canonicalize behavior
+        let path_with_slash = PathBuf::from("C:\\Windows\\");
+        let result = norm_case(&path_with_slash);
+        assert!(
+            !result.to_string_lossy().ends_with('\\'),
+            "Should strip trailing backslash, got: {:?}",
+            result
+        );
+
+        // But root paths like C:\ should keep their slash
+        let root_path = PathBuf::from("C:\\");
+        let root_result = norm_case(&root_path);
+        assert!(
+            root_result.to_string_lossy().ends_with('\\'),
+            "Root path should keep trailing backslash, got: {:?}",
+            root_result
+        );
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn test_norm_case_windows_normalizes_slashes() {
+        // norm_case should convert forward slashes to backslashes (like canonicalize did)
+        let path_with_forward_slashes = PathBuf::from("C:/Windows/System32");
+        let result = norm_case(&path_with_forward_slashes);
+        assert!(
+            !result.to_string_lossy().contains('/'),
+            "Should convert forward slashes to backslashes, got: {:?}",
+            result
+        );
+        assert!(
+            result.to_string_lossy().contains('\\'),
+            "Should have backslashes, got: {:?}",
+            result
+        );
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn test_norm_case_windows_preserves_unc_prefix() {
+        // If the original path has a UNC prefix, it should be preserved
+        let unc_path = PathBuf::from(r"\\?\C:\Windows");
+        let result = norm_case(&unc_path);
+        assert!(
+            result.to_string_lossy().starts_with(r"\\?\"),
+            "Should preserve UNC prefix when present in original, got: {:?}",
+            result
+        );
+    }
+}