cp: add support for --attributes-only and setting timestamps, links and xattrs #1066
Conversation
Matt8898
commented
Aug 7, 2017
•
Matt8898
commented
- Add windows support
- Implement --preserve
- Implement preserving Links and xattrs
Matt8898
changed the title
Matt8898
changed the title
src/cp/cp.rs
Outdated
| let dest_path = CString::new(dest.as_os_str().to_str().unwrap()).unwrap(); | ||
| unsafe { | ||
| let mut stat: libc::stat = mem::zeroed(); | ||
| libc::stat(src_path.as_ptr(), &mut stat); |
There was a problem hiding this comment.
You should be able to use the filetime crate to get and set the times; that should be more portable, and work on Windows and Redox.
src/cp/cp.rs
Outdated
| modtime: stat.st_mtime | ||
| }); | ||
| } | ||
| let metadata = fs::metadata(source).unwrap(); |
There was a problem hiding this comment.
Probably use ? instead of .unwrap().
Matt8898
changed the title
src/cp/cp.rs
Outdated
| extern crate clap; | ||
| extern crate walkdir; | ||
| extern crate filetime; | ||
| use filetime::FileTime; |
There was a problem hiding this comment.
Please separate the extern crate and use statements into separate sections (with the use section below the extern crate one).
src/cp/cp.rs
Outdated
| preserve_hard_links = true; | ||
| } | ||
| } | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
Add a newline before #[cfg(unix)] please.
src/cp/cp.rs
Outdated
| } else { | ||
| let mut found_hard_link = false; | ||
| if preserve_hard_links { | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
I'd rather this #[cfg(unix)] section and the #[cfg(windows)] section below be split off into separate functions.
src/cp/cp.rs
Outdated
|
|
||
| let file_index = ((*file_info).nFileIndexHigh, (*file_info).nFileIndexLow); | ||
| for hard_link in &hard_links { | ||
| if (hard_link.1).0 == file_index.0 && (hard_link.1).1 == file_index.1 { |
There was a problem hiding this comment.
Are the parentheses here necessary?
There was a problem hiding this comment.
If you mean the parentheses for the tuples then yes, they are necessary
There was a problem hiding this comment.
I'll fix the other ones
src/cp/cp.rs
Outdated
| found_hard_link = true; | ||
| } | ||
| } | ||
| if (((*file_info).nNumberOfLinks) > 1u32) && !found_hard_link { |
There was a problem hiding this comment.
I'd remove the outer parentheses here.
src/cp/cp.rs
Outdated
| if non_fatal_errors { | ||
| Err(Error::NotAllFilesCopied) | ||
| return Err(Error::NotAllFilesCopied) |
There was a problem hiding this comment.
The return here is not necessary.
src/cp/cp.rs
Outdated
| } else { | ||
| Ok(()) | ||
| return Ok(()) |
src/cp/cp.rs
Outdated
| copy_file(path.as_path(), local_to_target.as_path(), options)?; | ||
| if preserve_hard_links { | ||
| #[cfg(unix)] | ||
| unsafe { |
There was a problem hiding this comment.
Both this #[cfg(unix)] section and the #[cfg(windows)] section below seem to share code with the sections in copy(). Maybe try to move some of the code into common functions?
|
No problem, I updated it. |
|
@Arcterus Is anything still blocking this from being merged? |
|
No, I just have been too busy with school lately. Sorry that it's taken me so long to look at this. |
|
No problem. |
