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

Unix.utimes with 0.0 #6289

Closed
vicuna opened this Issue Jan 8, 2014 · 6 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link
Collaborator

vicuna commented Jan 8, 2014

Original bug ID: 6289
Reporter: @Chris00
Status: closed (set by @xavierleroy on 2017-02-16T14:18:29Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.01.0
Target version: 4.03.0+dev / +beta1
Fixed in version: 4.03.0+dev / +beta1
Category: standard library
Tags: junior_job
Monitored by: @gasche @hcarty

Bug description

The documentation says "A time of [0.0] is interpreted as the current time". However the implementation makes the test

if (times.actime || times.modtime)

or

  if (tv[0].tv_sec || tv[1].tv_sec)

which has two unfortunate consequences

  • If one time is 0.0 and the other one is not, 0.0 is not interpreted as the current time.
  • If the time is 0.1 or any value in [0,1[ (thus not 0.0), it is interpreted as 0 because it is rounded down before the test.

Also, since the name is Unix.utimes and not Unix.utime, one should maybe prefer the utimes function (with its sub-second resolution) over the utime one when both are available.

Finally, one may also question the benefit of this convention. Maybe a function Unix.utimes_now would be a better replacement (Unix.utimes would thus have no exceptional behavior).

Additional information

See http://roscidus.com/blog/blog/2014/01/07/ocaml-the-bugs-so-far/

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 16, 2014

Comment author: @damiendoligez

This is what I propose:

  1. change the documentation to say that the current time is used for both iff both are 0.0.
  2. change the implementation to use proper tests (== 0.0) instead of implicit cast to integer to bool.
@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 16, 2014

Comment author: @Chris00

That would mitigate the problem indeed. However, since in doing so you change the semantics of the function and you do not speak of using utimes (or utimensat) as the default, another solution would be to deprecate utimes and add utime, say with the following signature:

utime : string -> (float * float) option -> unit

or

utime : ?a: float -> ?m: float -> string -> unit

with the time being the current time if the argument is not set (this requires to call time if only one of the two optional arguments is present but I think the semantics are less easily forgotten).

Does this sound OK or is it too invasive?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 21, 2014

Comment author: @damiendoligez

The problem with utimes (or more exotic functions like utimensat) is, as usual, portability. We try to rely only on POSIX-standard functions, and anything else needs to be auto-detected by the configure script.

If we make a new function, I'd rather go with your first one (an optional pair of floats) because it sticks with the interface of the underlying system call.

Also, I'm not convinced that the change in semantics is significant enough to warrant polluting the library with a deprecated function.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 26, 2014

Comment author: @Chris00

I am personally fine with your solution. I was more worried about others using this feature.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 28, 2014

Comment author: talex

(author of the original blog post)

I'd also prefer the "(float * float) option" API to the optional arguments one, because I'd expect "utime ~a path" to set only the access time, not to set the mtime to the current time.

Just documenting that you can set the mtime to zero by setting the atime to something else would work for my use, though it's a bit ugly.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 6, 2015

Comment author: @xavierleroy

Commit [trunk 0939a59]: use current time only if both arguments are exactly 0.0. Also, use utimes() in preference to utime().

@vicuna vicuna closed this Feb 16, 2017

@vicuna vicuna added this to the 4.03.0 milestone Mar 14, 2019

@vicuna vicuna added the bug label Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.