Use iterative algorithms for mkdir_recursive and rmdir_recursive #12573

lbonn · 2014-02-26T18:48:30Z

As mentioned in #6109, mkdir_recursive doesn't really need to use recursive calls, so here is an iterative version.
The other points of the proposed overhaul (renaming and existing permissions) still need to be resolved.

I also bundled an iterative rmdir_recursive, for the same reason.

Please do not hesitate to provide feedback on style as this is my first code change in rust.

adrientetar · 2014-02-26T21:22:50Z

These functions should be renamed if they are not recursive anymore, the doc comments before them also.

huonw · 2014-02-26T22:10:10Z

@adrientetar: I don't think the names correspond to their implementations, but rather their behaviour (making nested directories, etc.).

lbonn · 2014-02-26T22:39:07Z

Yes, they serve the same purposes as mkdir -p and rm -r, the latter being an alias for rm --recursive.
However, I agree that mkdir_recursive, is somehow confusing.

I voluntarily left the original functions names pending for discussion.
As for the doc comments, they describe the functions' intended behaviours which haven't changed.

alexcrichton · 2014-02-26T23:34:23Z

src/libstd/io/fs.rs

+    let mut curpath = match path.is_absolute() {
+        true => Path::new("/"),
+        false => Path::new("./")
+    };


I've found recently that as soon as you look at a path you've probably forgotten one thing or another. This may need to change to something like path.root_path().unwrap_or(Path::new(".")) to take into account drives on windows.

cc @kballard, you know lots about windows paths.

That's absolutely correct. That takes into account the multitude of possible path prefixes on Windows.

I also wonder if this takes into account symlinks and whatnot.

lbonn · 2014-02-27T01:00:34Z

Here are some corrections for the root problem and is_dir usage.

Still need to be worked on :

spatial complexity
rm a non-directory directly ?
use vec_ng

alexcrichton · 2014-02-27T01:03:11Z

@lbonn, if you want, you can defer the semantics fixes until later. I think that the change to non-recursion is worth merging regardless of changing what exactly the function does.

To that end, I would recommend dealing with the spatial complexity and using vec_ng for now.

kud1ing · 2014-02-27T05:26:02Z

Nice.

lbonn · 2014-02-27T09:18:01Z

This one is more space efficient and uses vec_ng.

However, it calls readdir each time it falls back on a directory, I'm not sure if this is a big deal.
The alternative would be to keep a state of to-be-deleted children for each directory in the stack.

Also it looks like vec! cannot be used inside std itself, because of ::std::vec_ng::Vec in the macro expansion.

huonw · 2014-02-27T10:46:27Z

src/libstd/io/fs.rs

+
+        // delete all regular files in the way and push subdirs
+        // on the stack
+        for child in children.iter() {


If you use .move_iter() here, the .clone() will be unnecessary.

lbonn · 2014-02-27T11:20:05Z

Thanks for the re-read, it should look better.

This turned out to be a good learning exercice :).

alexcrichton · 2014-02-27T18:37:03Z

src/libstd/io/fs.rs

+                match unlink(&child) {
+                    Ok(()) => (),
+                    Err(ref e) if e.kind == io::FileNotFound => (),
+                    Err(e) => return Err(e)


I don't personally understand why io::FileNotFound is ignored. Can you add a comment explaining why?

I guess, because there is no harm done if the file you want to remove is not there in the first place. But i agree: this constellation/reasoning should be documented.

alexcrichton · 2014-02-27T18:39:14Z

I would also like to see some tests for all the new functionality. I'm a little uncomfortable adding in the lstat calls without tests asserting that you're calling lstat.

You'll find a bunch of tests at the end of this module, and if you wrap them in the iotest! macro it will test it for both the native and green implementations.

lbonn · 2014-02-28T22:06:07Z

It turns out the functions were not really tested, except for the somewhat odd recursive_mkdir_slash.

Here are some tests which check for basic functionnality and ensure that symlinks to other directories are not explored during deletion.

alexcrichton · 2014-02-28T23:29:17Z

src/libstd/io/fs.rs

+        let testdir = tmpdir().join("d1/d2");
+        check!(mkdir_recursive(&testdir, io::UserRWX));
+        assert!(testdir.is_dir())
+    })


This test should have tmpdir alive for the entire test. Otherwise it leaves around temporary directories.

Ah I see. This is pretty subtle because the test still passes.

alexcrichton · 2014-03-06T17:35:31Z

This failed on windows, likely because lstat is not implemented on windows.

lbonn · 2014-03-07T00:13:06Z

Yes, it makes sense :).

I'll try to set up a build on my windows machine and work on lstat soon.

alexcrichton · 2014-03-07T01:49:36Z

It's ok to ignore the tests on windows for now though. I had difficulty figuring out how to implement lstat on windows, so it's fine if we don't get it for now.

as requested in rust-lang#6109

For now, the windows version uses stat, just as before. We should switch back to lstat as soon as rust-lang#12795 is closed.

The rmdir test is blocked by rust-lang#12795 on windows.

As mentioned in #6109, ```mkdir_recursive``` doesn't really need to use recursive calls, so here is an iterative version. The other points of the proposed overhaul (renaming and existing permissions) still need to be resolved. I also bundled an iterative ```rmdir_recursive```, for the same reason. Please do not hesitate to provide feedback on style as this is my first code change in rust.

internal: Split flyimport into its 3 applicable contexts

…ns-in-module-name-repetitions, r=Jarcho [`module_name_repetition`] Recognize common prepositions Fixes rust-lang#12544 changelog: [`module_name_repetition`]: don't report an item name if it consists only of a prefix from `allowed-prefixes` list and a module name (e.g. `AsFoo` in module `foo`). Prefixes allowed by default: [`to`, `from`, `into`, `as`, `try_into`, `try_from`]

alexcrichton reviewed Feb 26, 2014
View reviewed changes

huonw reviewed Feb 27, 2014
View reviewed changes

alexcrichton reviewed Feb 27, 2014
View reviewed changes

alexcrichton reviewed Feb 28, 2014
View reviewed changes

lbonn mentioned this pull request Mar 9, 2014

lstat implementation on windows #12795

Closed

fs: use an iterative algorithm for 'mkdir_recursive'

0fcd5d5

as requested in rust-lang#6109

lbonn added 2 commits March 10, 2014 19:27

fs: use an iterative algorithm for 'rmdir_recursive'

2d754b4

For now, the windows version uses stat, just as before. We should switch back to lstat as soon as rust-lang#12795 is closed.

fs: units tests for mkdir_recusive and rmdir_recursive

164b7c2

The rmdir test is blocked by rust-lang#12795 on windows.

bors merged commit 164b7c2 into rust-lang:master Mar 13, 2014

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022

Auto merge of rust-lang#12573 - Veykril:completion, r=Veykril

69f3096

internal: Split flyimport into its 3 applicable contexts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use iterative algorithms for mkdir_recursive and rmdir_recursive #12573

Use iterative algorithms for mkdir_recursive and rmdir_recursive #12573

lbonn commented Feb 26, 2014

adrientetar commented Feb 26, 2014

huonw commented Feb 26, 2014

lbonn commented Feb 26, 2014

alexcrichton Feb 26, 2014

lilyball Feb 26, 2014

lilyball Feb 26, 2014

lbonn commented Feb 27, 2014

alexcrichton commented Feb 27, 2014

kud1ing commented Feb 27, 2014

lbonn commented Feb 27, 2014

huonw Feb 27, 2014

lbonn commented Feb 27, 2014

alexcrichton Feb 27, 2014

kud1ing Feb 28, 2014

alexcrichton commented Feb 27, 2014

lbonn commented Feb 28, 2014

alexcrichton Feb 28, 2014

lbonn Mar 1, 2014

alexcrichton commented Mar 6, 2014

lbonn commented Mar 7, 2014

alexcrichton commented Mar 7, 2014

Use iterative algorithms for mkdir_recursive and rmdir_recursive #12573

Use iterative algorithms for mkdir_recursive and rmdir_recursive #12573

Conversation

lbonn commented Feb 26, 2014

adrientetar commented Feb 26, 2014

huonw commented Feb 26, 2014

lbonn commented Feb 26, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbonn commented Feb 27, 2014

alexcrichton commented Feb 27, 2014

kud1ing commented Feb 27, 2014

lbonn commented Feb 27, 2014

Choose a reason for hiding this comment

lbonn commented Feb 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 27, 2014

lbonn commented Feb 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Mar 6, 2014

lbonn commented Mar 7, 2014

alexcrichton commented Mar 7, 2014