Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Preserve times for fs::copy on Unix. #32067

Closed
wants to merge 2 commits into from
Closed

Preserve times for fs::copy on Unix. #32067

wants to merge 2 commits into from

Conversation

pitdicker
Copy link
Contributor

This adds set_times to File so fs::copy can restore the access, modified and created times.
Also permissions are now set during file creation, as a partly fix for #26933.

I did not test the code for setting the creation time, as I do not have BSD.
It probably works as long as the creation time is earlier than what it is currently set to.
If we want to make a method like this public, we probably should add a way to keep some times the same, or set them to the current time.

There are still some things left for #26933, like copying attributes, extended attributes and ACLs.

@pitdicker
Copy link
Contributor Author

r? @alexcrichton

@pitdicker
Preserve times for fs::copy on Unix.
30bc501 
This adds `set_times` to `File` so `fs::copy` can restore
the access, modified and created times.
Also permissions are now set during file creation, as
a partly fix for #26933.
alexcrichton
alexcrichton reviewed Mar 6, 2016
View reviewed changes
src/libstd/sys/unix/fs.rs
if !from.is_file() {
use fs::OpenOptions;
let mut from_opts = OpenOptions::new();
let mut reader = try!(from_opts.read(true).open(&from));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be the same as File::open?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I don't remember why I changed that...
If I don't find the reason I will change it back.

@alexcrichton
Copy link
Member

I'm curious if other versions of this function in other libraries also copy times by default? I wonder if this should perhaps be an opt-in behavior?

@pitdicker
Copy link
Contributor Author

I would have to look, but I think it is nice to imitate cp as close as possible on Unix

@pitdicker
Fix test
81a75da 
Before, copy2 accidentally worked because `!is_file()`
returned false when the file does not exists. But `fs::copy`
assumed it was a dir, and gives `InvalidInput`.
So `InvalidInput` was not caused by the nul for the destination
path, what we are testing for.
@alexcrichton alexcrichton self-assigned this Mar 6, 2016
@alexcrichton
Copy link
Member

Ah ok, if cp does it, and Windows already does it that sounds like precedent enough to me!

@nodakai
Copy link
Contributor

nodakai commented Mar 23, 2016

cp(1) copies the original timestamp only if told

$ cp /etc/protocols .
$ ls -l protocols
-rw-r--r-- 1 nodakai nodakai 6.4K Mar 23 20:15 protocols
$ cp -a /etc/protocols .
$ ls -l protocols
-rw-r--r-- 1 nodakai nodakai 6.4K Jan 12  2010 protocols

This is probably for avoiding possible performance hit caused by inode access.

@bors
Copy link
Collaborator

bors commented Mar 23, 2016

☔ The latest upstream changes (presumably #32390) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with a rebase!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pitdicker @alexcrichton @nodakai @bors