-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: executable command #1872
feat: executable command #1872
Conversation
088284e
to
5d9cab7
Compare
3f94464
to
b6008c7
Compare
2d0d616
to
8e3458e
Compare
b6008c7
to
b3b3a6a
Compare
commit-id:12d49314 --- **Stack**: - #1872 - #1871 ⬅⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*
8e3458e
to
059ea3b
Compare
059ea3b
to
424e610
Compare
if !filepath.exists() { | ||
fs::File::create(&filepath)?; |
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.
This is not atomic, as we first ask OS whether the file exists or not and only then attempt creating it. It's prone to a race condition, as another process might have been scheduled by the OS between call to file metadata (exists) and call to create it.
Instead, please do:
let file = OpenOptions::new().create_new(true).open(path.as_std_path());
return match file {
Err(e) => {
if e.kind() == std::io::ErrorKind::AlreadyExists {
continue;
}
Err(e.into())
}
Ok(_) => Ok(filepath),
};
.create_new(true)
ensures new file will be created, or else AlreadyExists
thrown.
If the file has not been created because of some error, other than already exist (e.g. running out of disk space) we can show the error to the user.
Similar approach can be used for directories:
let dir = fs::create_dir(&filepath);
return match dir {
Err(e) => {
if e.kind() == std::io::ErrorKind::AlreadyExists {
continue;
}
Err(e.into())
}
Ok(_) => Ok(filepath),
424e610
to
99baf62
Compare
commit-id:c985abe1
99baf62
to
ebf49ae
Compare
let absolute = |path_buf: PathBuf| { | ||
path_buf | ||
.as_path() | ||
.canonicalize() |
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.
cargo clippy
warns agains using canonicalize()
, should i use fsx::canonicalize_utf8
does it apply in this case? please advise
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.
Yes, you should. fsx
is basically our wrapper over path manipulation functions that handles some edge-cases that we have run into in the past. The clippy lint you see has been added by us here
Line 1 in f1e8abb
disallowed-methods = [ |
Replaced with #1885 |
No description provided.