Repository: vitiral/path_abs (language: rust)

Issue:
windows path handling
@zilbuz opened a bug in vitiral/artifact#247. They mentioned that it might be better addressed in `path_abs` and I agree.

Looking at https://docs.microsoft.com/fr-fr/windows/desktop/FileIO/naming-a-file#maxpath, it doesn't look like windows even allows `'/'` to be in the name of a file or a directory, so I don't see any harm in converting them. Honestly I thought we already did this.


Discussion / hints:
@Screwtapello thoughts?
In particular, they suggested some good test cases that we should add to `path_abs` for windows.
For your information, I tried this edit in `PathArc` and it resolved my particular problem in artifact:
```rust
    pub fn join<P: AsRef<Path>>(&self, path: P) -> PathArc {
        let mut buf = PathBuf::new();
        for c in path.as_ref().iter() {
            buf.push(c);
        }
        PathArc::from(self.0.join(buf))
    }
```

Although rereading it, I think it might be better to directly clone `self.0` and push on it instead of constructing a temporary `PathBuf` and then joining.

I went with `PathBuf::push()` instead of replacing `/` with `PATH_SEPARATOR` because the method takes `P: AsRef<Path>` instead of `&str` so there isn't a `replace()` method, and the `Path` iterator did the right thing when passing it `/`-separated relative paths on Windows.

The drawback of this fix is that `PathArc::join()` isn't a direct call to `PathBuf::join()` anymore and does some more potentially expensive operations. If that's a concern it could be beneficial to instead add a new method that either does the same `iter()`/`push()` thing or takes a `&str` and replace `/` with `PATH_SEPARATOR` before joining.
The various path-manipulation routines I've written all tend to iterate over `path.components()` rather than manually concatenating strings at least in part because of this: `path.components()` and `PathBuf::push()` will always do the Right Thing with respect to path separators on the current platform, and they're tested thoroughly by the Rust project itself. Any kind of `#[cfg(windows)]` code we write ourselves won't get tested nearly as thoroughly.

I really need to get around to making PRs for the rest of my trait-rework branch; it uses `path.components()` extensively.
sounds good. Let me know if you need any help!

This is a serious pain point for writing software using user-specified paths. It's good to see how `path_abs` can make a positive impact by abstracting away doing the right thing... especially when doing the right thing is annoying.

Fix patch:
diff --git a/src/arc.rs b/src/arc.rs
index 3ee0944..7e8014b 100644
--- a/src/arc.rs
+++ b/src/arc.rs
@@ -65,7 +65,12 @@ impl PathArc {
     ///
     /// [0]: https://doc.rust-lang.org/std/path/struct.PathBuf.html#method.join
     pub fn join<P: AsRef<Path>>(&self, path: P) -> PathArc {
-        PathArc::from(self.0.join(path))
+        let mut joined_path = self.as_path().to_path_buf();
+        // Issue #34: Convert '/' into '\\' for Windows
+        for c in path.as_ref().components() {
+            joined_path.push(c);
+        }
+        PathArc::from(joined_path)
     }
 
     /// Creates an owned `PathArc` like self but with the given file name.


Test patch:
diff --git a/tests/test_absolute.rs b/tests/test_absolute.rs
index 6013e0b..6a42981 100644
--- a/tests/test_absolute.rs
+++ b/tests/test_absolute.rs
@@ -92,4 +92,47 @@ fn test_absolute() {
         assert_eq!(PathDir::new("/").unwrap(), root);
     }
     assert!(PathDir::new(root.join("..")).is_err());
+
+    if cfg!(windows) {
+        // Test that /-separated and \-separated paths can be joined
+        let ac1 = a.join(r"b/c");
+        assert!(ac1.metadata().is_ok());
+
+        let ac2 = a.join(r"b\c");
+        assert!(ac2.metadata().is_ok());
+    }
+}
+
+/// Check that issue #34 is fixed
+/// 
+/// After calling join(), the metadata are accessed to check that the computed path is valid.
+#[test]
+fn test_forward_and_backward_slashes() {
+    let tmp = tempdir::TempDir::new("ex").unwrap();
+    let tmp = tmp.path();
+    
+    // Create directories:
+    // a/
+    // + b/
+    //   + c/
+    let a = PathDir::create(&tmp.join("a")).unwrap();
+    let b = PathDir::create(&a.join("b")).unwrap();
+    let c = PathDir::create(&b.join("c")).unwrap();
+
+    let a_abs = PathAbs::new(a).unwrap();
+
+    // Join /-separated relative path and check that the metadata are accessible
+    let forward_slash = a_abs.join(r"b/c");
+    assert!(forward_slash.metadata().is_ok());
+    assert_eq!(c, PathDir::new(forward_slash).unwrap());
+    
+    // Join \-separated relative path and check that the metadata are accessible
+    // The following test only make sense on windows because the \ character isn't illegal in a
+    // directory name on linux, so the call to `join(r"b\c")` would just add the single directory 
+    // named "b\c"
+    if cfg!(windows) {
+        let backward_slash = a_abs.join(r"b\c");
+        assert!(backward_slash.metadata().is_ok());
+        assert_eq!(c, PathDir::new(backward_slash).unwrap());
+    }
 }