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.

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

fix ~ touch: fix and test for windows #1319

Merged
merged 3 commits into from
Apr 8, 2019
Merged

Conversation

rivy
Copy link
Member

@rivy rivy commented Nov 26, 2018

  • fix windows compilation
  • enable testing windows
  • re-categorize as "generic"

Copy link
Collaborator

@Arcterus Arcterus left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Really sorry it took so long to get back to you!

@@ -18,7 +18,9 @@ extern crate uucore;

use filetime::*;
use std::fs::{self, File};
use std::io::{self, Error};
#[cfg(not(windows))]
use std::io::{self};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can just be use std::io;

@@ -52,7 +55,7 @@ macro_rules! to_timeval {
}

#[cfg(unix)]
fn set_symlink_times(p: &str, atime: FileTime, mtime: FileTime) -> io::Result<()> {
fn set_symlink_file_times(p: &str, atime: FileTime, mtime: FileTime) -> io::Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this function as actually necessary even on Unix. set_symlink_file_times() in filetime seems to say it does the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

🛠 * both fixed

@rivy
Copy link
Member Author

rivy commented Apr 5, 2019

@Arcterus , I'll review and update the code this weekend. Thanks for getting back to it.

Copy link
Collaborator

@Arcterus Arcterus left a comment

Choose a reason for hiding this comment

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

A couple more changes (should be good to go after this).

@@ -42,6 +44,7 @@ macro_rules! local_tm_to_filetime(
})
);

#[cfg(unix)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because the other function is now gone, this macro actually isn't used anywhere now, so it can be deleted too.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

use std::io::{self, Error};
#[cfg(not(windows))]
use std::io;
use std::io::{Error};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I missed this one. Can you remove the braces around Error?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@rivy
Copy link
Member Author

rivy commented Apr 7, 2019

I squashed the code change and dead-code removal into a single commit.
And, I think I found and fixed all the code nits.
Local testing passes for windows and *nix.
Let me know if this passes muster.

@Arcterus Arcterus merged commit cefbe6c into uutils:master Apr 8, 2019
@Arcterus
Copy link
Collaborator

Arcterus commented Apr 8, 2019

Looks good, thanks!

@rivy rivy deleted the fix.touch branch December 8, 2019 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants