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

os: allow Chtimes with time.Time{} to avoid setting time #32558

Closed
rsc opened this issue Jun 11, 2019 · 11 comments
Closed

os: allow Chtimes with time.Time{} to avoid setting time #32558

rsc opened this issue Jun 11, 2019 · 11 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Jun 11, 2019

os.Chtimes(f, a, m) sets the access and modification times of the file f to a, m.
The underlying call can usually say "leave this one unset" to set just one of the two.
We could support that by defining that time.Time{} means "don't set this one".
We probably should do that.

Thoughts?

Related to but different from #31880.

@beoran
Copy link

beoran commented Jun 12, 2019

While this would be an improvement, it might have some issues with backwards compatibility.

Therefore I would propose, as I did in the related issue to have a new function os.Chfiletimes that takes os.FileTime in stead of time.Time for the last two arguments, where os.FileTime could be something like type FileTime struct { time.Time ; Now bool ; Ignore bool }.

@rsc rsc changed the title proposal: os: allow Chtimes with time.Time to avoid setting time proposal: os: allow Chtimes with time.Time{} to avoid setting time Jun 25, 2019
@rsc
Copy link
Contributor Author

rsc commented Jun 25, 2019

Need to understand #31880 before we can reasonably decide something here - maybe more is needed and we can make one change instead of two. But #31880 is blocked on knowing more about the Windows-induced constraints.

@rsc
Copy link
Contributor Author

rsc commented Aug 27, 2019

Re @beoran's comment, we confirmed that Chtimes with the zero time right now ends up in 1754 on a Linux file system. So if we adopted the zero time convention then use on older versions would not recognize or reject the zero time and would end up in 1754 instead.

@rsc
Copy link
Contributor Author

rsc commented Aug 27, 2019

Given that it looks like #31880 may be declined, we'll probably want to decide this next. It seems like the main question is whether anyone needs this functionality. Does anyone need this?

(The backwards compatibility issue can be solved by just waiting a couple releases before starting to use a change.)

@ianlancetaylor
Copy link
Contributor

I looked at 150 uses of os.Chtimes in Google code. I found 20 cases where it appeared that it would be convenient to leave one of the times unchanged.

Coincidentally I found 21 cases that set one or both times to "now".

@bradfitz
Copy link
Contributor

bradfitz commented Nov 7, 2019

I've mentioned a half dozen times to @ianlancetaylor and @griesemer that I thought we already did this and I've tried to find old CLs (that were never submitted?) or old threads on this, but all along it seems I was remembering https://golang.org/pkg/os/#Chown instead which says:

A uid or gid of -1 means to not change that value

The fact that Chown already does this pattern and I assumed we did the same for Chtimes seems like a vote in favor of this proposal.

@rsc
Copy link
Contributor Author

rsc commented Nov 13, 2019

Matching os.Chown's approach for ignoring part of the operation seems like a decent argument for doing the same in os.Chtimes.

This seems like a likely accept in the absence of any objections or arguments against it.

Leaving open for a week for any final comments.

@rsc
Copy link
Contributor Author

rsc commented Nov 20, 2019

No change in consensus. Accepted.

@rsc rsc modified the milestones: Proposal, Go1.15 Nov 20, 2019
@rsc rsc changed the title proposal: os: allow Chtimes with time.Time{} to avoid setting time os: allow Chtimes with time.Time{} to avoid setting time Nov 20, 2019
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 20, 2019
@gopherbot
Copy link

Change https://golang.org/cl/219638 mentions this issue: os: skip change of the zero value in Chtimes for Posix

@ianlancetaylor ianlancetaylor modified the milestones: Go1.15, Backlog, Go1.16 May 19, 2020
@odeke-em
Copy link
Member

We are still waiting for responses from the Dragonfly BSD team, and the CL still needs some more work. However, thank you for the patience everyone, and happy holidays!

@odeke-em odeke-em modified the milestones: Go1.16, Go1.17 Dec 25, 2020
@ianlancetaylor ianlancetaylor modified the milestones: Go1.17, Backlog Apr 23, 2021
@gopherbot
Copy link

Change https://go.dev/cl/498600 mentions this issue: doc/go1.21: mention os package changes

gopherbot pushed a commit that referenced this issue May 26, 2023
Also mention WTF-8 support in the syscall package.

For #32558
For #58977
For #59971

Change-Id: Id1627889b5e498add498748d9bfc69fb58030b35
Reviewed-on: https://go-review.googlesource.com/c/go/+/498600
Reviewed-by: Eli Bendersky <eliben@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
None yet
Development

No branches or pull requests

7 participants