-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Use iterative algorithms for mkdir_recursive and rmdir_recursive #12573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the reason for including this, I'm not entirely convinced that it's necessary at all. I would hope that one of the removers would receive an error in this case.
Can you switch this back to using
try!
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexcrichton Why? The only reasonable action to take if this fails for FileNotFound is to retry the deletion, which means there was no reason to bother returning the error in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have yet to see an explanation of why this should specifically check for only file not found errors. This makes no sense to me and I don't understand why it should keep going for this one kind of error, but bail out on all the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because removing the file is what we're trying to do. If the file was already removed, then someone did our work for us. But that's not a reason to return an error.
In a straight-up
rm()
method that removes only the listed path, returningFileNotFound
makes sense. We wanted to perform a single action, and we couldn't perform that action.But in
rmdir_recursive()
, we're trying to perform all actions necessary to delete a given directory. If we determine that we need to delete a file, and then discover the file was already deleted, returning an error doesn't make sense because we were never told to explicitly delete that file. That was something we told ourselves to do, in order to fulfill the desired action. Since we told ourselves to do it, we also handle the error ourselves.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a normal occurrence, this is not something that should be swept under the rug. Just a few milliseconds earlier I listed the directory and found files A, B, and C. I successfully deleted A, and then B. Turns one someone else deleted C in the meantime.
That is not normal. That should be delivered upstairs to let them deal with it. Just because our job happens to be done for us doesn't mean that we should continue along our merry way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upstairs didn't even know that C existed. Telling them that we didn't find C isn't something they can reasonably deal with. As I said before, the only reasonable action they have to take is to request the deletion of the directory a second time (although more likely they're just going to wrap this in a
try!()
block and end up passing this error even higher up).If the path that was passed to
rmdir_recursive()
ends up returningFileNotFound
when that is ultimately deleted, I can see returning that error (although personally I have no problem treating that as success as well; the path existed when we first calledreaddir()
so this wasn't a case of the caller giving us bad input). But for nested files that the caller did not explicitly mention, I do not see any value in treating that as an error.It's also worth noting that
FileNotFound
doesn't even provide a way to indicate that the file that was missing was not the one that was passed to the function. The only available location to record that info is in theIoError.detail
, but that's an unstructured string, presumably intended to display to the user, rather than something we can use to record this information. If I callrmdir_recursive()
and getFileNotFound
back, I'm certainly going to expect that means the directory didn't exist, and be quite surprised when it turns out to still exist.