-
Notifications
You must be signed in to change notification settings - Fork 14.1k
fix: clarify that fs::rename on unix accepts targets that don't exist #149267
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
base: main
Are you sure you want to change the base?
Conversation
|
@sftse do you have link to a good source that documents this? |
Co-authored-by: Marijn Schouten <hkBst@users.noreply.github.com>
Co-authored-by: Marijn Schouten <hkBst@users.noreply.github.com>
|
This is the current behavior on Linux fn dir_to_nonexist_dir() {
let dir = Path::new("__testdir");
std::fs::create_dir(dir).unwrap();
std::fs::create_dir(dir.join("foo")).unwrap();
std::fs::rename(dir.join("foo"), dir.join("bar")).unwrap();
assert!(dir.join("bar").is_dir());
std::fs::remove_dir(dir.join("bar")).unwrap();
std::fs::remove_dir(dir).unwrap();
}
|
| /// This function currently corresponds to the `rename` function on Unix | ||
| /// and the `MoveFileExW` or `SetFileInformationByHandle` function on Windows. | ||
| /// | ||
| /// Because of this, the behavior when both `from` and `to` exist differs. On | ||
| /// Unix, if `from` is a directory, `to` must also be an (empty) directory. If | ||
| /// `from` is not a directory, `to` must also be not a directory. The behavior | ||
| /// Unix, if `from` is a directory, `to` must be an empty directory or non-existent. If | ||
| /// `from` is not a directory, `to` must also not be a directory. The behavior | ||
| /// on Windows is the same on Windows 10 1607 and higher if `FileRenameInfoEx` | ||
| /// is supported by the filesystem; otherwise, `from` can be anything, but | ||
| /// `to` must *not* be a directory. |
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.
The top of the paragraph already has the qualifier "when both from and to exist", so I think this is slightly redundant as written. This is a bit dense anyway so it wouldn't be bad to clean up the whole section. How about switching to a list format, something like:
This function currently corresponds to the `rename` function on Unix, and
`MoveFileExW` with a fallback to `SetFileInformationByHandle` on Windows.
The exact behavior differs:
- On Unix, when `from` is a directory and `to` exists, `to` must be an empty directory.
- On Unix, when `from` is not a directory and `to` exists, `to` may not be a directory.
- On Windows 10 version 1607 and above, the behavior is the same as Unix if the
filesystem supports `FileRenameInfoEx`.
- Otherwise on Windows, `from` can be anything but `to` must not be a directory.
- If `to` does not exist, `from` can be anything.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.
Sounds good, although grouping the list by first behavior that is common to (most) platforms and then mentioning the caveats and edge cases might be better?
I can't currently tell what all the behaviors are, so I'll either test them or have to take your word for it.
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 kind of mirrored the ordering that exists, which I think makes enough sense: you read from top to bottom and stop when you have a match. The final item is a fallback.
I can't currently tell what all the behaviors are, so I'll either test them or have to take your word for it.
I don't think I added anything that wasn't there. I did double check the Windows case, and it should be accurate given we use FILE_RENAME_POSIX_SEMANTICS. The posix/unix behavior is specified at https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html and says:
If the old argument names a file that is not a directory and the new argument names a directory, or old names a directory and new names a file that is not a directory, or new names a directory that is not empty, rename() shall fail. Otherwise, if the directory entry named by new exists, it shall be removed and old renamed to new.
So I think what we have matches.
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.
No description provided.