fix: clarify that fs::rename on unix accepts targets that don't exist

[sftse]

View all comments

[rustbot]

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[hkBst]

@sftse do you have link to a good source that documents this?

[sftse]

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();
    }

rename

int rename(const char *oldpath, const char *newpath);
...
oldpath can specify a directory. In this case, newpath must
either not exist, or it must specify an empty directory.

[tgross35]

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.

[sftse]

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.

[tgross35]

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.

[tgross35]

Ah, you were probably talking about the "with a fallback" part. For context there see #131072 and #137528.

It might also be worth turning `rename` into a link to that posix page so anyone who needs more details can find it quick.

[tgross35]

This needs followup to the above,
@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[tgross35]

@sftse gentle ping, would you be able to do the update from #149267 (comment)?

[sftse]

Yes, thanks for the ping, I'll get to it.

[Dylan-DPC]

@sftse any updates on this? thanks

[sftse]

@Dylan-DPC sorry for this. If I don't get to it tomorrow feel free to close, supersede or reassign.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[tgross35]

Two small nits then r=me

View changes since this review

[sftse]

I was not able to figure out whether If to does not exist, from can be anything. really applies to all platforms as the wording seems to imply.

Is this the case in Windows pre 1607?

[tgross35]

LGTM, thank you!

@bors r+ rollup

View changes since this review

[rust-bors]

📌 Commit d04b878 has been approved by tgross35

It is now in the queue for this repository.

[tgross35]

I was not able to figure out whether If to does not exist, from can be anything. really applies to all platforms as the wording seems to imply.

Is this the case in Windows pre 1607?

I can't think of any reason it wouldn't apply; it should be able to move either files or directories, and if the destination doesn't exist then it doesn't have any reason to care what the source is.

[sftse]

Ok, thanks for reviewing! Took me a while.