auto merge of #6107 : catamorphism/rust/mkdir_recursive, r=brson

r? @brson This hopefully addresses your concerns about the termination condition, and adds more tests. With a bonus documentation commit.

src/libcore/os.rs

@@ -643,20 +643,22 @@ pub fn make_dir(p: &Path, mode: c_int) -> bool {
 /// Returns true iff creation
 /// succeeded. Also creates all intermediate subdirectories
 /// if they don't already exist, giving all of them the same mode.
+
+// tjc: if directory exists but with different permissions,
+// should we return false?
 pub fn mkdir_recursive(p: &Path, mode: c_int) -> bool {
     if path_is_dir(p) {
         return true;
     }
-    let parent = p.dir_path();
-    debug!("mkdir_recursive: parent = %s",
-           parent.to_str());
-    if parent.to_str() == ~"."
-        || parent.to_str() == ~"/" { // !!!
+    else if p.components.is_empty() {
+        return false;
+    }
+    else if p.components.len() == 1 {
         // No parent directories to create
-        path_is_dir(&parent) && make_dir(p, mode)
+        path_is_dir(p) || make_dir(p, mode)
     }
     else {
-        mkdir_recursive(&parent, mode) && make_dir(p, mode)
+        mkdir_recursive(&p.pop(), mode) && make_dir(p, mode)
     }
 }
 
@@ -1267,6 +1269,8 @@ mod tests {
     use run;
     use str;
     use vec;
+    use libc::consts::os::posix88::{S_IRUSR, S_IWUSR, S_IXUSR};
+
 
     #[test]
     pub fn last_os_error() {
@@ -1490,16 +1494,16 @@ mod tests {
     }
 
     #[test]
-    fn recursive_mkdir_ok() {
-        use libc::consts::os::posix88::{S_IRUSR, S_IWUSR, S_IXUSR};
+    fn recursive_mkdir_slash() {
+        let path = Path("/");
+        assert!(os::mkdir_recursive(&path,  (S_IRUSR | S_IWUSR | S_IXUSR) as i32));
+    }
 
-        let root = os::tmpdir();
-        let path = "xy/z/zy";
-        let nested = root.push(path);
-        assert!(os::mkdir_recursive(&nested,  (S_IRUSR | S_IWUSR | S_IXUSR) as i32));
-        assert!(os::path_is_dir(&root.push("xy")));
-        assert!(os::path_is_dir(&root.push("xy/z")));
-        assert!(os::path_is_dir(&nested));
+    #[test]
+    fn recursive_mkdir_empty() {
+        let path = Path("");
+        assert!(!os::mkdir_recursive(&path, (S_IRUSR | S_IWUSR | S_IXUSR) as i32));
     }
 
+    // More recursive_mkdir tests are in std::tempfile
 }

src/libcore/path.rs

@@ -49,31 +49,71 @@ pub fn PosixPath(s: &str) -> PosixPath {
 }
 
 pub trait GenericPath {
+    /// Converts a string to a Path
     fn from_str(&str) -> Self;
 
+    /// Returns the directory component of `self`, as a string
     fn dirname(&self) -> ~str;
+    /// Returns the file component of `self`, as a string option.
+    /// Returns None if `self` names a directory.
     fn filename(&self) -> Option<~str>;
+    /// Returns the stem of the file component of `self`, as a string option.
+    /// The stem is the slice of a filename starting at 0 and ending just before
+    /// the last '.' in the name.
+    /// Returns None if `self` names a directory.
     fn filestem(&self) -> Option<~str>;
+    /// Returns the type of the file component of `self`, as a string option.
+    /// The file type is the slice of a filename starting just after the last
+    /// '.' in the name and ending at the last index in the filename.
+    /// Returns None if `self` names a directory.
     fn filetype(&self) -> Option<~str>;
 
+    /// Returns a new path consisting of `self` with the parent directory component replaced
+    /// with the given string.
     fn with_dirname(&self, (&str)) -> Self;
+    /// Returns a new path consisting of `self` with the file component replaced
+    /// with the given string.
     fn with_filename(&self, (&str)) -> Self;
+    /// Returns a new path consisting of `self` with the file stem replaced
+    /// with the given string.
     fn with_filestem(&self, (&str)) -> Self;
+    /// Returns a new path consisting of `self` with the file type replaced
+    /// with the given string.
     fn with_filetype(&self, (&str)) -> Self;
 
+    /// Returns the directory component of `self`, as a new path.
+    /// If `self` has no parent, returns `self`.
     fn dir_path(&self) -> Self;
+    /// Returns the file component of `self`, as a new path.
+    /// If `self` names a directory, returns the empty path.
     fn file_path(&self) -> Self;
 
+    /// Returns a new Path whose parent directory is `self` and whose
+    /// file component is the given string.
     fn push(&self, (&str)) -> Self;
+    /// Returns a new Path consisting of the given path, made relative to `self`.
     fn push_rel(&self, (&Self)) -> Self;
+    /// Returns a new Path consisting of the path given by the given vector
+    /// of strings, relative to `self`.
     fn push_many(&self, (&[~str])) -> Self;
+    /// Identical to `dir_path` except in the case where `self` has only one
+    /// component. In this case, `pop` returns the empty path.
     fn pop(&self) -> Self;
 
+    /// The same as `push_rel`, except that the directory argument must not
+    /// contain directory separators in any of its components.
     fn unsafe_join(&self, (&Self)) -> Self;
+    /// On Unix, always returns false. On Windows, returns true iff `self`'s
+    /// file stem is one of: `con` `aux` `com1` `com2` `com3` `com4`
+    /// `lpt1` `lpt2` `lpt3` `prn` `nul`
     fn is_restricted(&self) -> bool;
 
+    /// Returns a new path that names the same file as `self`, without containing
+    /// any '.', '..', or empty components. On Windows, uppercases the drive letter
+    /// as well.
     fn normalize(&self) -> Self;
 
+    /// Returns `true` if `self` is an absolute path.
     fn is_absolute(&self) -> bool;
 }
 

src/libstd/tempfile.rs

@@ -23,9 +23,62 @@ pub fn mkdtemp(tmpdir: &Path, suffix: &str) -> Option<Path> {
     None
 }
 
-#[test]
-fn test_mkdtemp() {
-    let p = mkdtemp(&Path("."), "foobar").unwrap();
-    os::remove_dir(&p);
-    assert!(str::ends_with(p.to_str(), "foobar"));
-}
+#[cfg(test)]
+mod tests {
+    use tempfile::mkdtemp;
+    use tempfile;
+
+    #[test]
+    fn test_mkdtemp() {
+        let p = mkdtemp(&Path("."), "foobar").unwrap();
+        os::remove_dir(&p);
+        assert!(str::ends_with(p.to_str(), "foobar"));
+    }
+
+    // Ideally these would be in core::os but then core would need
+    // to depend on std
+    #[test]
+    fn recursive_mkdir_rel() {
+        use core::libc::consts::os::posix88::{S_IRUSR, S_IWUSR, S_IXUSR};
+        use core::os;
+
+        let root = mkdtemp(&os::tmpdir(), "temp").expect("recursive_mkdir_rel");
+        os::change_dir(&root);
+        let path = Path("frob");
+        assert!(os::mkdir_recursive(&path,  (S_IRUSR | S_IWUSR | S_IXUSR) as i32));
+        assert!(os::path_is_dir(&path));
+        assert!(os::mkdir_recursive(&path,  (S_IRUSR | S_IWUSR | S_IXUSR) as i32));
+        assert!(os::path_is_dir(&path));
+    }
+
+    #[test]
+    fn recursive_mkdir_dot() {
+        use core::libc::consts::os::posix88::{S_IRUSR, S_IWUSR, S_IXUSR};
+        use core::os;
+
+        let dot = Path(".");
+        assert!(os::mkdir_recursive(&dot,  (S_IRUSR | S_IWUSR | S_IXUSR) as i32));
+        let dotdot = Path("..");
+        assert!(os::mkdir_recursive(&dotdot,  (S_IRUSR | S_IWUSR | S_IXUSR) as i32));
+    }
+
+    #[test]
+    fn recursive_mkdir_rel_2() {
+        use core::libc::consts::os::posix88::{S_IRUSR, S_IWUSR, S_IXUSR};
+        use core::os;
+
+        let root = mkdtemp(&os::tmpdir(), "temp").expect("recursive_mkdir_rel_2");
+        os::change_dir(&root);
+        let path = Path("./frob/baz");
+        debug!("...Making: %s in cwd %s", path.to_str(), os::getcwd().to_str());
+        assert!(os::mkdir_recursive(&path, (S_IRUSR | S_IWUSR | S_IXUSR) as i32));
+        assert!(os::path_is_dir(&path));
+        assert!(os::path_is_dir(&path.pop()));
+        let path2 = Path("quux/blat");
+        debug!("Making: %s in cwd %s", path2.to_str(), os::getcwd().to_str());
+        assert!(os::mkdir_recursive(&path2, (S_IRUSR | S_IWUSR | S_IXUSR) as i32));
+        assert!(os::path_is_dir(&path2));
+        assert!(os::path_is_dir(&path2.pop()));
+    }
+
+}