renameFile now consistently reports an error if the destination is a directory #8
Conversation
This file contains 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
…directory, as specified by documentation. Previously the exceptions raised would be quite inconsistent. For example, given a file 'f' and a directory 'd', on Linux, the simple case worked: Prelude System.Directory> renameFile "f" "d" *** Exception: f: rename: inappropriate type (Is a directory) however: Prelude System.Directory> renameFile "f" "d/" *** Exception: f: rename: inappropriate type (Not a directory) Prelude System.Directory> renameFile "f" "." *** Exception: e: rename: resource busy (Device or resource busy) Prelude System.Directory> renameFile "f" "/tmp" *** Exception: e: rename: unsatisified constraints (Directory not empty) Windows was inconsistent with the documentation even in the general case: Prelude System.Directory> renameFile "f" "d" *** Exception: f: MoveFileEx "f" "d": permission denied (Access is denied.) The additional check should not incur noticeable cost as an extra stat to check for a directory is only performed in case of an IO exception. I am not sure if this is actually the right abstraction level to fix these inconsistencies. Perhaps they should be pushed down to libraries/Win32, but the thing is, the Win32 documentation does not try to specify which errors are raised in which settings, but System.Directory does, and the implementation goes against the documentation, which seems wrong.
|
This looks like a similar issue as we're discussing in #10 and #4, that after an operation we need to check if it's a dir or not to distinguish certain error cases. The subtlety is what to do when that "is it a dir" check itself fails (e.g. because it does not exist, or has a perm error or whatever). We should look around for other instances of this within |
| checkNotDir opath | ||
| doRename `E.catch` renameExcHandler | ||
| where checkNotDir path = do | ||
| isdir <- pathIsDir path `E.catch` ((\ _ -> return False) :: IOException -> IO Bool) |
There was a problem hiding this comment.
You can use catchIOError to avoid specifying the type signature.
On second thought, I'm not sure if It is for 7.4 at least.catchIOError is available in older versions of GHC.
|
Certain race conditions can still cause the bug to reappear, but I'm not sure if there's a way to avoid this problem entirely. |
Rufflewind
added a commit
to Rufflewind/directory
that referenced
this pull request
Mar 4, 2015
Rufflewind
added a commit
to Rufflewind/directory
that referenced
this pull request
Mar 4, 2015
Rufflewind
added a commit
to Rufflewind/directory
that referenced
this pull request
Mar 4, 2015
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I only tested on Windows, could you please run the tests on Unix to make sure they are not broken?
https://ghc.haskell.org/trac/ghc/ticket/8482 is related.