fix(path): change normalization strategy to not resolve symlinks

helix-core/src/path.rs

@@ -30,30 +30,10 @@ pub fn expand_tilde(path: &Path) -> PathBuf {
     path.to_path_buf()
 }
 
-/// Normalize a path, removing things like `.` and `..`.
-///
-/// CAUTION: This does not resolve symlinks (unlike
-/// [`std::fs::canonicalize`]). This may cause incorrect or surprising
-/// behavior at times. This should be used carefully. Unfortunately,
-/// [`std::fs::canonicalize`] can be hard to use correctly, since it can often
-/// fail, or on Windows returns annoying device paths. This is a problem Cargo
-/// needs to improve on.
-/// Copied from cargo: <https://github.com/rust-lang/cargo/blob/070e459c2d8b79c5b2ac5218064e7603329c92ae/crates/cargo-util/src/paths.rs#L81>
+/// Normalize a path without resolving symlinks
+/// Strategy: https://github.com/helix-editor/helix/pull/7658#issuecomment-1654633705
 pub fn get_normalized_path(path: &Path) -> PathBuf {
-    // normalization strategy is to canonicalize first ancestor path that exists (i.e., canonicalize as much as possible),
-    // then run handrolled normalization on the non-existent remainder
-    let (base, path) = path
-        .ancestors()
-        .find_map(|base| {
-            let canonicalized_base = dunce::canonicalize(base).ok()?;
-            let remainder = path.strip_prefix(base).ok()?.into();
-            Some((canonicalized_base, remainder))
-        })
-        .unwrap_or_else(|| (PathBuf::new(), PathBuf::from(path)));
-
-    if path.as_os_str().is_empty() {
-        return base;
-    }
+    log::debug!("path before normalization: {}", path.to_string_lossy());
 
     let mut components = path.components().peekable();
     let mut ret = if let Some(c @ Component::Prefix(..)) = components.peek().cloned() {
@@ -71,14 +51,39 @@ pub fn get_normalized_path(path: &Path) -> PathBuf {
             }
             Component::CurDir => {}
             Component::ParentDir => {
-                ret.pop();
+                if let Some(head) = ret.components().next_back() {
+                    match head {
+                        Component::Prefix(_) | Component::RootDir => {}
+                        Component::CurDir | Component::ParentDir => unreachable!(),
+                        Component::Normal(_) => {
+                            ret.pop();
+                        }
+                    }
+                }
             }
             Component::Normal(c) => {
-                ret.push(c);
+                let new_path = ret.join(c);
+                // If current and next paths are symlinks, we can't strip prefix because the next path might resolve to a completely different location
+                if ret.is_symlink() && !new_path.is_symlink() {
+                    if let Ok(canon) = dunce::canonicalize(&new_path) {
+                        // If we can canonicalize new path, then it's safe to assume we can also canonicalize the old path
+                        ret.push(
+                            canon
+                                .strip_prefix(dunce::canonicalize(&ret).unwrap())
+                                .unwrap(),
+                        )
+                    } else {
+                        ret.push(c);
+                    }
+                } else {
+                    ret.push(c);
+                }
             }
         }
     }
-    base.join(ret)
+
+    log::debug!("normalized path: {}", ret.to_string_lossy());
+    ret
 }
 
 /// Returns the canonical, absolute form of a path with all intermediate components normalized.
@@ -160,3 +165,27 @@ pub fn get_truncated_path<P: AsRef<Path>>(path: P) -> PathBuf {
     ret.push(file);
     ret
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    #[cfg(unix)]
+    fn test_normalize_path() {
+        let tests = [
+            ("/../../..", "/"),
+            ("/home/blah/./../blah/", "/home/blah/"),
+            ("~/..", ""),
+        ];
+        for (unit, ans) in tests {
+            assert_eq!(
+                get_normalized_path(&PathBuf::from(unit)),
+                PathBuf::from(ans),
+                "expected normalized path from `{}` is `{}`",
+                unit,
+                ans
+            );
+        }
+    }
+}