Repository: vitiral/path_abs (language: rust)

Issue:
Remove `arc::handle_prefix()`
This helper was added to address a problem with handling Windows paths, but [after investigation](https://github.com/vitiral/path_abs/pull/20#issuecomment-409582524) the problem seems to have gone away. [As suggested](https://github.com/vitiral/path_abs/pull/20#issuecomment-409584548), the code will be cleaner and simpler without that function.


Discussion / hints:
Cool, I'll try to take a look at this in the weekend.

Are you interested in being a co-maintainer of path_abs? I would still want
to review any code submitted by you but you can do PR approvals for others.

>

I'm working on this right now, actually! I just figured I should have an issue number to mention in the commit that makes the change.

Sure, I'd love to help. :)
Cool, you've been added as a collaborator.

I actually mistakenly thought this was the PR in the email. Carry on :smile: 
A couple of days ago I discovered the problem that `arc::handle_prefix()` was intended to solve had *not* gone away, and I figured out how to reproduce it reliably. The following table is one I posted to rust-lang/rust#49342 but with a few more interesting test-cases I thought of in the mean time. As before, the intersection of each column and row represents the result of `std::env::set_current_dir(column); Path::new(row).canonicalize()` where "-" means "returned an error":

| | `\\?\C:\` | `C:\` | `\\vboxsvr\Shared\` |
| --- | --- | --- | --- |
| `\\vboxsvr\Shared\` | `\\?\UNC\vboxsvr\Shared\` | `\\?\UNC\vboxsvr\Shared\` | `\\?\UNC\vboxsvr\Shared\` |
| `\\?\C:\Users` | `\\?\C:\Users` | `\\?\C:\Users` | `\\?\C:\Users` |
| `C:\Users` | `\\?\C:\Users` | `\\?\C:\Users` | `\\?\C:\Users` |
| `C:Users` | - | `\\?\C:\Users` | - |
| `\Users` | - | `\\?\C:\Users` | `\\?\UNC\vboxsvr\Shared\Users` |
| `Users` | `\\?\C:\Users` | `\\?\C:\Users` | `\\?\UNC\vboxsvr\Shared\Users` |
| `C:` | `\\?\C:\Users\IEUser` | `\\?\C:\` | `\\?\C:\Users\IEUser` |
| `\` | - | `\\?\C:\` | `\\?\UNC\vboxsvr\Shared\` |

When the current directory is a regular (`C:\`) or UNC (`\\vboxsvr\Shared`) path, you can canonicalize just about anything, so we don't have to do anything special to make canonicalization work for those cases.

The path `C:Users` fails to canonicalize for both the extended-length (`\\?\C:\`) and UNC paths, which makes sense. As the `C:` row reveals, in those cases the current directory on C: is "\Users\IEUser" which does *not* contain a Users directory, so `.canonicalize()` is correct to return an error in those situations. We don't have to do anything special to make canonicalization work for those cases (beyond the work we already have to do to support canonicalizing non-existent paths).

The paths `\Users` and `\` fail to canonicalize when the current directory uses extended-length syntax. Luckily, we can easily implement this ourselves:

  - take the prefix from the current working directory (say, `C:`)
  - add a root component (`C:\`)
  - canonicalize that (`\\?\C:\`)
  - add the remainder of the input path to that head (`\\?\C:\Users`)

So, in every situation we can produce a canonical path head for an arbitrary input path, even on Windows, and even if the current working directory uses extended-length path syntax.

I'm gonna try to re-implement `PathArc::absolute()` like that, and we'll see how far I get.
Sounds great, awesome work 👍

Not strictly related to solving this issue, but [this Raymond Chen blog post](https://blogs.msdn.microsoft.com/oldnewthing/20100506-00/?p=14133/) explains a whole bunch of the weirdness I've encountered with relative path resolution on windows. The short version is: unlike Unix, where the shell is a thin wrapper over the kernel's idea of a "process", Windows' `cmd.exe` lies all the time.

Fix patch:
diff --git a/src/arc.rs b/src/arc.rs
index 7a4f154..13600bb 100644
--- a/src/arc.rs
+++ b/src/arc.rs
@@ -12,7 +12,7 @@ use std::fs;
 use std::io;
 use std::env;
 use std::ffi::OsStr;
-use std::path::{Component, Components, Prefix};
+use std::path::{Component, PrefixComponent};
 use std_prelude::*;
 
 use super::{Error, Result};
@@ -34,6 +34,34 @@ pub fn current_dir(resolving: &PathArc) -> Result<PathArc> {
     Ok(PathArc::from(cwd))
 }
 
+/// Converts any PrefixComponent into verbatim ("extended-length") form.
+fn make_verbatim_prefix(prefix: &PrefixComponent) -> Result<PathBuf> {
+    let path_prefix = Path::new(prefix.as_os_str());
+
+    if prefix.kind().is_verbatim() {
+        // This prefix already uses the extended-length
+        // syntax, so we can use it as-is.
+        Ok(path_prefix.to_path_buf())
+    } else {
+        // This prefix needs canonicalization.
+        let res = path_prefix
+            .canonicalize()
+            .map_err(|e|
+                Error::new(e, "canonicalizing", PathArc::new(path_prefix))
+            )?;
+        Ok(res)
+    }
+}
+
+/// Pops the last component from path, returning an error for a root path.
+fn pop_or_error(path: &mut PathBuf) -> ::std::result::Result<(), io::Error> {
+    if path.pop() {
+        Ok(())
+    } else {
+        Err(io::Error::new(io::ErrorKind::NotFound, ".. consumed root"))
+    }
+}
+
 #[derive(Clone, Eq, Hash, PartialEq, PartialOrd, Ord)]
 /// A `PathBuf` that is atomically reference counted and reimplements the `PathBuf`
 /// methods to display the action and path when there is an error.
@@ -183,47 +211,83 @@ impl PathArc {
     /// [`canonicalize`]: struct.PathAbs.html#method.canonicalize
     /// [`current_dir`]: fn.current_dir.html
     pub fn absolute(&self) -> Result<PathAbs> {
-        let mut components = self.components();
-        let mut stack: Vec<OsString> = Vec::new();
-
-        macro_rules! pop_stack { [] => {{
-            if let None = stack.pop() {
-                return Err(Error::new(
-                    io::Error::new(io::ErrorKind::NotFound, ".. consumed root"),
-                    "resolving absolute",
-                    self.clone(),
-                ));
+        let mut res = PathBuf::new();
+
+        fn maybe_init_res(res: &mut PathBuf, resolvee: &PathArc) -> Result<()> {
+            if !res.as_os_str().is_empty() {
+                // res has already been initialized, let's leave it alone.
+                return Ok(());
             }
-        }}}
 
-        handle_prefix(self, &mut stack, &mut components, false)?;
+            // res has not been initialized, let's initialize it to the
+            // canonicalized current directory.
+            let cwd = env::current_dir().map_err(|e| {
+                Error::new(
+                    e,
+                    "getting current_dir while resolving absolute",
+                    resolvee.clone(),
+                )
+            })?;
+            *res = cwd.canonicalize().map_err(|e| {
+                Error::new(e, "canonicalizing", PathArc::new(&cwd))
+            })?;
+
+            Ok(())
+        };
+
+        for each in self.components() {
+            match each {
+                Component::Prefix(p) => {
+                    // We don't care what's already in res, we can entirely
+                    // replace it..
+                    res = make_verbatim_prefix(&p)?;
+                }
 
-        for component in components {
-            match component {
-                Component::CurDir => { /* ignore, probably impossible */ }
-                Component::Prefix(_) => unreachable!(),
                 Component::RootDir => {
-                    if cfg!(unix) {
-                        unreachable!("root is already handled on unix");
+                    if cfg!(windows) {
+                        // In an ideal world, we would say
+                        //
+                        //  res = std::fs::canonicalize(each)?;
+                        //
+                        // ...to get a properly canonicalized path.
+                        // Unfortunately, Windows cannot canonicalize `\` if
+                        // the current directory happens to use extended-length
+                        // syntax (like `\\?\C:\Windows`), so we'll have to do
+                        // it manually: initialize `res` with the current
+                        // working directory (whatever it is), and truncate it
+                        // to its prefix by pushing `\`.
+                        maybe_init_res(&mut res, self)?;
+                        res.push(each);
+                    } else {
+                        // On other platforms, a root path component is always
+                        // absolute so we can replace whatever's in res.
+                        res = Path::new(&each).to_path_buf();
                     }
-                    // This is actually possible on windows because root is distinct
-                    // from prefix (?)
-                    stack.push(to_os(component));
                 }
-                Component::ParentDir => pop_stack!(),
-                Component::Normal(_) => stack.push(to_os(component)),
-            }
-        }
 
-        if stack.is_empty() {
-            return Err(Error::new(
-                io::Error::new(io::ErrorKind::NotFound, "resolving resulted in empty path"),
-                "resolving absolute",
-                self.clone(),
-            ));
+                // This does nothing and can be ignored.
+                Component::CurDir => (),
+
+                Component::ParentDir => {
+                    // A parent component is always relative to some existing
+                    // path.
+                    maybe_init_res(&mut res, self)?;
+                    pop_or_error(&mut res)
+                        .map_err(|e| {
+                            Error::new(e, "resolving absolute", self.clone())
+                        })?;
+                }
+
+                Component::Normal(c) => {
+                    // A normal component is always relative to some existing
+                    // path.
+                    maybe_init_res(&mut res, self)?;
+                    res.push(c);
+                }
+            }
         }
 
-        Ok(PathAbs(PathArc(Arc::new(PathBuf::from_iter(stack)))))
+        Ok(PathAbs(PathArc(Arc::new(res))))
     }
 }
 
@@ -310,120 +374,6 @@ impl Into<PathBuf> for PathArc {
     }
 }
 
-fn to_os(c: Component) -> OsString {
-    c.as_os_str().to_os_string()
-}
-
-/// Handle the prefix in the components.
-///
-/// Pretty much 100% of this logic is because windows is evil. You can't call `canonicalize` on `\`
-/// since it depends on the current directory. You also can't call it when it would be a noop, i.e.
-/// for `\\?\C:`.
-fn handle_prefix(
-    resolving: &PathArc,
-    stack: &mut Vec<OsString>,
-    components: &mut Components,
-    recursing: bool,
-) -> Result<()> {
-    macro_rules! pop_stack { [] => {{
-        if let None = stack.pop() {
-            return Err(Error::new(
-                io::Error::new(io::ErrorKind::NotFound, ".. consumed root"),
-                "resolving absolute",
-                resolving.clone(),
-            ));
-        }
-    }}}
-    loop {
-        // The whole reason we're here is because we haven't added anything to the stack yet.
-        assert_eq!(stack.len(), 0, "{:?}", stack);
-
-        let component = match components.next() {
-            None => break,
-            Some(c) => c,
-        };
-
-        match component {
-            Component::CurDir => {
-                assert!(!recursing);
-
-                // ignore
-                continue;
-            }
-            Component::Prefix(prefix) => {
-                assert!(!cfg!(unix), "Component::Prefix in unix");
-                match prefix.kind() {
-                    Prefix::Disk(_) | Prefix::UNC(_, _) => {
-                        // Make the prefix a more "standard" form
-                        let c = PathArc::new(component.as_os_str()).canonicalize()?;
-                        stack.extend(c.components().map(to_os));
-                    }
-                    _ => {
-                        // Already in the "most standardized" form
-                        // TODO: some more testing to make sure that canoninicalize()
-                        // cannot be called on these forms would be good
-                        stack.push(to_os(component));
-                    }
-                }
-            }
-            Component::RootDir => {
-                if cfg!(windows) {
-                    assert!(!recursing); // windows should not give `\` in CWD
-                                         // https://stackoverflow.com/questions/151860
-                                         // > In Windows [root is] relative to what drive your current working
-                                         // > directory is at the time.
-                                         //
-                                         // So, we need to push the "drive" first.
-
-                    let cwd = current_dir(resolving)?;
-                    handle_prefix(resolving, stack, &mut cwd.components(), true)?;
-                    {
-                        // Double check that we aren't being dumb. `current_dir`
-                        // should have always started with some kind of prefix.
-
-                        // TODO: not sure why, but this assertion actually can fail and
-                        // does in the tests.
-                        // assert_eq!(1, stack.len(), "{:?}", stack);
-
-                        let first = Path::new(&stack[0]).components().next().unwrap();
-                        if let Component::Prefix(prefix) = first {
-                            if let Prefix::DeviceNS(_) = prefix.kind() {
-                            } else if !prefix.kind().is_verbatim() {
-                                panic!(
-                                    "First item kind is neither verbatim nor DeviceNs: {:?}",
-                                    stack
-                                )
-                            }
-                        } else {
-                            panic!("First item is not a Prefix on windows: {:?}", stack)
-                        }
-                    }
-                }
-                // Always push the "root" component.
-                stack.push(to_os(component));
-            }
-            Component::ParentDir | Component::Normal(_) => {
-                assert!(!recursing);
-
-                // First item is either a ParentDir or Normal, in either
-                // case we need to get current_dir
-                let cwd = current_dir(resolving)?;
-                let mut cwd_components = cwd.components();
-                handle_prefix(resolving, stack, &mut cwd_components, true)?;
-                stack.extend(cwd_components.map(to_os));
-
-                match component {
-                    Component::ParentDir => pop_stack!(),
-                    Component::Normal(_) => stack.push(to_os(component)),
-                    _ => unreachable!(),
-                }
-            }
-        }
-        break;
-    }
-    Ok(())
-}
-
 #[test]
 fn test_prefix_windows() {
     fn f<P: AsRef<Path>>(p: P) -> Result<PathAbs> {


Test patch:
diff --git a/tests/absolute_helpers/mod.rs b/tests/absolute_helpers/mod.rs
index aed0a52..eb95430 100644
--- a/tests/absolute_helpers/mod.rs
+++ b/tests/absolute_helpers/mod.rs
@@ -134,7 +134,7 @@ mod unix {
         assert_eq!(err.io_error().kind(), io::ErrorKind::NotFound);
         assert_eq!(
             err.io_error().to_string(),
-            "resolving resulted in empty path"
+            ".. consumed root"
         );
         assert_eq!(err.action(), "resolving absolute");
         assert_eq!(err.path(), path::Path::new("/foo/../.."));
@@ -163,15 +163,15 @@ mod windows {
     #[test]
     fn absolute_path_cannot_go_above_root() {
         ::setup();
-        // TODO: This is broken, and only really works if the current
-        // directory on C: is \. Otherwise, handle_prefix() canonicalizes
-        // "C:" to get "C:\Users\st" then appends "\", "foo", "..", ".."
-        // and gets back to "C:\Users\st".
-        let actual = PathArc::new(r"C:\foo\..\..").absolute().unwrap();
-        let expected = PathArc::new(path::Path::new(r"C:").canonicalize().unwrap())
-            .absolute()
-            .unwrap();
-        assert_eq!(actual, expected);
+        let err = PathArc::new(r"C:\foo\..\..").absolute().unwrap_err();
+
+        assert_eq!(err.io_error().kind(), io::ErrorKind::NotFound);
+        assert_eq!(
+            err.io_error().to_string(),
+            ".. consumed root"
+        );
+        assert_eq!(err.action(), "resolving absolute");
+        assert_eq!(err.path(), path::Path::new(r"C:\foo\..\.."));
     }
 
     #[test]