-
Notifications
You must be signed in to change notification settings - Fork 638
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
Add API for setting last accessed and last modified file times #2735
Conversation
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.
Great start, I think we need to change a couple of minor things though to fit in with how syscalls are done in the rest of the module.
switch errno { | ||
case .permissionDenied, .notPermitted: | ||
code = .permissionDenied | ||
message = "Not permited to change last access or last data modification times for \(path)." |
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.
message = "Not permited to change last access or last data modification times for \(path)." | |
message = "Not permitted to change last access or last data modification times for \(path)." |
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 one didn't get fixed
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.
A few small things need fixing otherwise this looks great!
/// > Important: Times are only considered valid if their nanoseconds components are one of the following: | ||
/// > - `UTIME_NOW` (you can use ``FileInfo/Timespec/now`` to get a Timespec set to this value), | ||
/// > - `UTIME_OMIT` (you can use ``FileInfo/Timespec/omit`` to get a Timespec set to this value),, | ||
/// > - Greater than zero and no larger than 1000 million |
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.
Should we remove this line as we clamp the value?
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.
I thought it was better to explain that we will clamp/update the value to the closest valid value instead. Otherwise if people are willingly using an invalid value they could be confused as to why the times are being set to the "wrong" thing. Let me know if you don't agree.
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.
Oh yeah I'm happy with that.
Sources/NIOFileSystem/FileInfo.swift
Outdated
|
||
/// A timespec where the seconds are set to zero and the nanoseconds set to `UTIME_OMIT`. | ||
/// In syscalls such as `futimens`, this means the time component set to this value will be ignored. | ||
public static var omit = Self.init( |
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.
🚨 public static var
alert! 🚨
This should either be a let
or a computed property. Same goes for now
.
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.
Also let's avoid Self.init(...)
and just use Self(...)
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.
Oh god, why did I use var. Good catch.
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.
Easily done 🙂
switch errno { | ||
case .permissionDenied, .notPermitted: | ||
code = .permissionDenied | ||
message = "Not permited to change last access or last data modification times for \(path)." |
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 one didn't get fixed
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.
Looks great, thanks Gus!
API breakage is expected, we're adding requirements to a protocol, but that's okay NIOFileSystem isn't stable API yet. |
/// If **either** time is `nil`, the current value will not be changed. | ||
/// If **both** times are `nil`, then both times will be set to the current time. |
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.
IMO "clamp/update the value to the closest valid value" is good/fine, but the API design still bothers me.
I would have preferred these 2 lines to have been obvious in the API design.
Perhaps using an (structified) enum as the single parameter that the function accepts. Or anything better. Maybe splitting up the responsibility into 2/3 different functions.
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.
I think it's clear from setTimes(lastAccess: t)
that lastDataModification
isn't modified.
It's fair to say though that setting both to nil
isn't an obvious API: setTimes(lastAccess: nil, lastDataModification: nil)
but that's why we also have the touch()
wrapper which many developers are familiar with.
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.
If we had separate methods for each, we'd unavoidably have to make two separate sys calls, which is expensive. We'd also not be setting the times to the exact same value, as the current time will have changed if we use the UTIME_NOW
timespec
.
However, I've added two new convenience methods that default one of the parameters to nil
to have nicer API: setLastAccessTime(to:)
and setLastDataModificationTime(to:)
.
@swift-server-bot test this please |
* commit 'fc79798d5a150d61361a27ce0c51169b889e23de': NIOSendableBox: allow off-loop initialisation iff Value is Sendable (apple#2753) Throw an appropriate error from the writer when the channel closed (apple#2744) put snippet code inside @available function (apple#2750) fix link to NIOFileSystem from NIO index page (apple#2747) convert the NIOFileSystem example code to a Snippet (apple#2746) Silence warning about missing include in macOS builds (apple#2741) Correctly mark 304 as not having a response body (apple#2737) Update availability guard (apple#2739) Add API for setting last accessed and last modified file times (apple#2735) Add a fallback path if renameat2 fails (apple#2733) Release file handles back to caller on failure to take ownership (apple#2715) Add a version of 'write' for 'ByteBuffer' (apple#2730) Imrprove rename error (apple#2731) Remove storage indirection for FileSystemError (apple#2726) testSimpleMPTCP should not fail for ENOPROTOOPT (apple#2725) Fix race in TCPThroughputBenchmark (apple#2724)
This PR adds API to set a file's last accessed and last modified times, as well as a
touch()
shorthand that updates both to the current time.Motivation:
Feature request: #2712
Modifications:
This PR adds API to set a file's last accessed and last modified times, as well as a
touch()
shorthand that updates both to the current time.Result:
Files' last accessed and last modified times can be updated.