924916 - [OS.File] Provide way to set file last modified time

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, enhancement)

Description

[Nils Maier [:nmaier]]

OS.File should provide a similar API to the nsIFile.lastModifiedTime setter.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

How important/when do you need this feature?

Comment 2

[Nils Maier [:nmaier]]

DownThemAll! currently uses the nsIFile.lastModifiedTime setter (potentially after each download finishes), so it would be really nice to have ASAP.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Under Unix, this can be implemented using utimes or futimes.
Under Windows, this can be implemented using SetFileTime.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

niels, I won't have time to implement this in the near future, but if you are interested, I can mentor you along that bug.

Comment 5

[Nils Maier [:nmaier]]

Attachment: Patch v0, Add setLastAccessDate/setLastModificationDate (unix-only)

Alright, here is what I can up with so far. I only implemented the unix side plus some tests.
Didn't run a try yet, but the tests are successful at least on OSX, in case you already wanna play with it.

Is this a viable approach and somewhat sane API? Or do you want me to change something important there (before I start on Windows).

Also, I'm really unfamiliar with the code, e.g. Types.long vs. ctypes.long and the rest of the ctypes wrappers. 

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> niels

Nils ;)

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 815137 [details] [diff] [review]
Patch v0, Add setLastAccessDate/setLastModificationDate (unix-only)

Review of attachment 815137 [details] [diff] [review]:
-----------------------------------------------------------------

Given that:
1. both Unix and Windows (only) provide a function that changes both the last access and the last modification date;
2. I suspect that most use cases don't care about preserving the one while changing the other;
3. providing an API comparable to what Unix and Windows do would result in code that is both simpler and faster (only one I/O instead of two or four, for what I believe is the common scenario) without making the other scenarios more expensive (still two I/Os to change one without changing the other) ...

I would go for an API that changes both dates at once.

Also, could you please migrate your tests to xpcshell? Our mochitests are bloated and need trimming down, so let's not make this even harder.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +398,5 @@
>      );
>    },
>  
>    /**
> +  * Set the last access date of the file.

Nit: Please match the /** indentation of the rest of the file.

::: toolkit/components/osfile/modules/osfile_async_worker.js
@@ +319,5 @@
>         });
>     },
> +   File_prototype_setLastAccessDate: function setLastAccessDate(fd, date) {
> +     return withFile(fd,
> +       function do_stat() {

|do_stat| is not a very good name. Maybe |do_setLastAccessDate|?
Same thing for the next method.

::: toolkit/components/osfile/modules/osfile_unix_back.jsm
@@ +178,5 @@
> +       // See utimes(3).
> +       Types.timeval = ctypes.StructType("timeval", [
> +         { "tv_sec": ctypes.long },
> +         { "tv_usec": ctypes.long }
> +       ]);

We have no guarantee that tv_sec and tv_usec are the only fields in the structure, or in which order they appear, so you will need to get that information in C (see OSFileConstants.cpp) and use a HollowStructure to build Types.timeval.

@@ +628,5 @@
> +         _timevalbuf[0].tv_usec = 0;
> +         _timevalbuf[1].tv_sec = (modifiedDate / 1000) | 0;
> +         _timevalbuf[1].tv_usec = 0;
> +         return _timevalbuf;
> +       };

This looks like it should live in osfile_unix_front.jsm.

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +167,5 @@
> +      * @param {Date,number} date The date to sanitize.
> +      *
> +      * @return {number} Sanitized, numeric date in milliseconds since epoch.
> +      */
> +     let sanitizeDate = function sanitizeDate(fn, date) {

To match other stuff in OS.File, could you rename this |normalizeDate|?

@@ +189,5 @@
> +      * @throws {TypeError} In case of invalid paramters.
> +      * @throws {OS.File.Error} In case of I/O error.
> +      */
> +     File.prototype.setLastAccessDate = function setLastAccessDate(date) {
> +       date = sanitizeDate("File.prototype.setLastAccessDate",date);

Nit: whitespace after the comma.

@@ +191,5 @@
> +      */
> +     File.prototype.setLastAccessDate = function setLastAccessDate(date) {
> +       date = sanitizeDate("File.prototype.setLastAccessDate",date);
> +       let info = this.stat();
> +       let modDate = info.lastModificationDate.getTime();

Nit: I believe that you swapped |modDate| and |accDate|.

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +517,5 @@
> +    let currentDir = yield OS.File.getCurrentDirectory();
> +    let pathSource = OS.Path.join(currentDir, EXISTING_FILE);
> +    let pathDest = OS.Path.join(OS.Constants.Path.tmpDir,
> +      "osfile async test setlastDates1.tmp");
> +    yield OS.File.copy(pathSource, pathDest);

Rather than copying that file, let's create one with writeAtomic. This will be easier to port to xpcshell anyway.

Comment 8

[Nils Maier [:nmaier]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #7)
> Given that:
> 1. both Unix and Windows (only) provide a function that changes both the
> last access and the last modification date;
> 2. I suspect that most use cases don't care about preserving the one while
> changing the other;
> 3. providing an API comparable to what Unix and Windows do would result in
> code that is both simpler and faster (only one I/O instead of two or four,
> for what I believe is the common scenario) without making the other
> scenarios more expensive (still two I/Os to change one without changing the
> other) ...
> 
> I would go for an API that changes both dates at once.

Agreed. How about just |setDates(lastAccessDate, lastModificationDate)|?

> Also, could you please migrate your tests to xpcshell? Our mochitests are
> bloated and need trimming down, so let's not make this even harder.

No problem. I'll look into that.

Comment 9

[Nils Maier [:nmaier]]

Attachment: Part 1 - Implement OS.File.setDates() for Unix.

Comment 10

[Nils Maier [:nmaier]]

Attachment: Part 2 - Implement OS.File.setDates() for Windows.

Comment 11

[Nils Maier [:nmaier]]

Attachment: Part 3 - Provide async OS.File.setDates() API for main-thread.

Comment 12

[Nils Maier [:nmaier]]

Attachment: Part 4 - xpcshell tests for OS.File.setDates().

Also: https://tbpl.mozilla.org/?tree=Try&rev=001338135252

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 815936 [details] [diff] [review]
Part 1 - Implement OS.File.setDates() for Unix.

Review of attachment 815936 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +171,5 @@
> +      * @param {Date,number=} modificationDate The last modification date. If
> +      * numeric, milliseconds since epoch. If omitted or null, then the current
> +      * date will be used.
> +      *
> +      * @throws {TypeError} In case of invalid paramters.

Nit: typo

@@ +958,5 @@
> +       if (typeof date !== "number" && !date) {
> +         date = Date.now();
> +       }
> +       // Input might be a date or date-like object.
> +       else if (typeof date.getTime === "function") {

Nit: In this file, we write
 } else if

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 815937 [details] [diff] [review]
Part 2 - Implement OS.File.setDates() for Windows.

Review of attachment 815937 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +193,5 @@
> +      * @param {Date,number=} modificationDate The last modification date. If
> +      * numeric, milliseconds since epoch. If omitted or null, then the current
> +      * date will be used.
> +      *
> +      * @throws {TypeError} In case of invalid paramters.

Nit: Same typo.

@@ +525,5 @@
> +       if (typeof date === "number") {
> +         date = new Date(date);
> +       }
> +       else if (!date) {
> +         date = new Date();

Why is this different from the Unix patch?

@@ +529,5 @@
> +         date = new Date();
> +       }
> +       else if (typeof date.getUTCFullYear !== "function") {
> +         throw new TypeError("|date| parameter of " + fn + " must be a " +
> +                             "|Date| instance or number");

Nit: Here, too
 } else if {

@@ +532,5 @@
> +         throw new TypeError("|date| parameter of " + fn + " must be a " +
> +                             "|Date| instance or number");
> +       }
> +       gSystemTime.wYear = date.getUTCFullYear();
> +       gSystemTime.wMonth = date.getUTCMonth() + 1;

That's good, but could you add a comment explaining the +1?

@@ +542,5 @@
> +       let rv = new OS.Shared.Type.FILETIME.implementation();
> +       throw_on_zero("Date_to_FILETIME",
> +                     WinFile.SystemTimeToFileTime(gSystemTimePtr,
> +                                                  rv.address()));
> +       return rv;

In this module, we generally call these values |result| instead of |rv|.

@@ +839,5 @@
> +       let file = File.open(path, FILE_SETDATES_MODE, FILE_SETDATES_OPTIONS);
> +       try {
> +         return file.setDates(accessDate, modificationDate);
> +       }
> +       finally {

} finally {

@@ +850,5 @@
> +       write: true
> +     };
> +     const FILE_SETDATES_OPTIONS = {
> +       winAccess: OS.Constants.Win.GENERIC_WRITE,
> +       // Directories can only be opened with backup semantics(!)

... yeah (sadface)

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 815939 [details] [diff] [review]
Part 4 - xpcshell tests for OS.File.setDates().

Review of attachment 815939 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, once you use add_task instead of Task.spawn

::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_setDates.js
@@ +21,5 @@
> +
> +  try {
> +    // 1. Try to set some well known dates.
> +    // We choose multiples of 2000ms, because the time stamp resolution of
> +    // the underlying OS might not support something more precise.

We've had countless issues with file system not rounding predictably.
If you end up with intermittent failures, I suggest just picking a day-at-noon Date instead of something that precise.

@@ +37,5 @@
> +    {
> +      yield OS.File.setDates(path, accDate);
> +      let stat = yield OS.File.stat(path);
> +      do_check_eq(accDate, stat.lastAccessDate.getTime());
> +      do_check_neq(modDate, stat.lastModificationDate.getTime());

That do_check_neq will probably work but looks a bit unpredictable.

@@ +220,5 @@
> +  do_print("spawning");
> +  Task.spawn(function() {
> +    yield Task.spawn(test_nonproto);
> +    yield Task.spawn(test_proto);
> +    yield Task.spawn(test_dirs);

Could you use |add_task| rather than |yield Task.spawn|?

Comment 16

[Nils Maier [:nmaier]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #14)
> Comment on attachment 815937 [details] [diff] [review]
> Part 2 - Implement OS.File.setDates() for Windows.
> 
> Review of attachment 815937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/modules/osfile_win_front.jsm
> @@ +525,5 @@
> > +       if (typeof date === "number") {
> > +         date = new Date(date);
> > +       }
> > +       else if (!date) {
> > +         date = new Date();
> 
> Why is this different from the Unix patch?

On Windows, we need a |Date| to construct the |SYSTEMTIME| from it, while on Unix we just need a |Number| and feed that directly into |utimes| and/or |futimes|.
I can make both functions more similar, by converting to |Date| on Unix (like on Windows) and returning |.getTime|, if you like...

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Not necessary, thanks for the explanations.

Comment 18

[Nils Maier [:nmaier]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #15)
> Comment on attachment 815939 [details] [diff] [review]
> Part 4 - xpcshell tests for OS.File.setDates().
> 
> Review of attachment 815939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, once you use add_task instead of Task.spawn
> 
> ::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_setDates.js
> @@ +21,5 @@
> > +
> > +  try {
> > +    // 1. Try to set some well known dates.
> > +    // We choose multiples of 2000ms, because the time stamp resolution of
> > +    // the underlying OS might not support something more precise.
> 
> We've had countless issues with file system not rounding predictably.
> If you end up with intermittent failures, I suggest just picking a
> day-at-noon Date instead of something that precise.

For that reason, I did use 2000ms since epoch, etc, so they should be predictable in that they should either always succeed or always fail for a given file system.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

I understand and I agree with your technique. My comment was just a suggestion in case it turns out to be insufficient.

Comment 20

[Nils Maier [:nmaier]]

Attachment: Bug 924916: Part 1 - Implement OS.File.setDates() for Unix. r=yoric

Comment 21

[Nils Maier [:nmaier]]

Attachment: Bug 924916: Part 2 - Implement OS.File.setDates() for Windows. r=yoric

Comment 22

[Nils Maier [:nmaier]]

Attachment: Bug 924916: Part 3 - Provide async OS.File.setDates() API for main-thread. r=yoric

Comment 23

[Nils Maier [:nmaier]]

Attachment: Bug 924916: Part 4 - xpcshell tests for OS.File.setDates().

Did I use add_task() correctly?

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 816636 [details] [diff] [review]
Bug 924916: Part 4 - xpcshell tests for OS.File.setDates().

Review of attachment 816636 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_setDates.js
@@ +14,5 @@
> +  run_next_test();
> +}
> +
> +// Non-prototypical tests, operating on path names.
> +add_task(function test_nonproto() {

Could you take the opportunity and make it the (probably) first xpcshell test to use |function*| instead of |function|?

@@ +17,5 @@
> +// Non-prototypical tests, operating on path names.
> +add_task(function test_nonproto() {
> +  // First, create a file we can mess with.
> +  let path = OS.Path.join(OS.Constants.Path.tmpDir,
> +                              "test_osfile_async_setDates_nonproto.tmp");

Nit: indentation

@@ +32,5 @@
> +      let stat = yield OS.File.stat(path);
> +      do_check_eq(accDate, stat.lastAccessDate.getTime());
> +      do_check_eq(modDate, stat.lastModificationDate.getTime());
> +    }
> +

I don't think that these braces are necessary. Is there a reason to put them?

@@ +66,5 @@
> +        try {
> +          yield OS.File.setDates(path, p, modDate);
> +          do_throw("Invalid access date should have thrown for: " + p);
> +        }
> +        catch (ex) {

Nit: } catch (ex) {

@@ +92,5 @@
> +        }
> +      }
> +    }
> +  }
> +  finally {

Nit:
 } finally {

Comment 25

[Nils Maier [:nmaier]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #24)
> Comment on attachment 816636 [details] [diff] [review]
> Bug 924916: Part 4 - xpcshell tests for OS.File.setDates().
> 
> Review of attachment 816636 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

> @@ +32,5 @@
> > +      let stat = yield OS.File.stat(path);
> > +      do_check_eq(accDate, stat.lastAccessDate.getTime());
> > +      do_check_eq(modDate, stat.lastModificationDate.getTime());
> > +    }
> > +
> 
> I don't think that these braces are necessary. Is there a reason to put them?

Well, this way I can repeatedly write |let stat =|. Also the blocks give a bit more structure.
I can easily change this, if you really want me to...

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Nah, leave them be.

Comment 27

[Nils Maier [:nmaier]]

Attachment: Part 1 - Implement OS.File.setDates() for Unix. r=yoric

Unbitrotted, carrying over r=yoric.

Comment 28

[Nils Maier [:nmaier]]

Attachment: Part 2 - Implement OS.File.setDates() for Windows. r=yoric

Unbitrotted, carrying over r=yoric

Comment 29

[Nils Maier [:nmaier]]

Attachment: Part 3 - Provide async OS.File.setDates() API for main-thread. r=yoric

Unbitrotted, carrying over r=yoric

Comment 30

[Nils Maier [:nmaier]]

Attachment: Part 4 - xpcshell tests for OS.File.setDates(). r=yoric

Unbitrotted, carrying over r=yoric

Comment 31

[Nils Maier [:nmaier]]

Previous green try, FWIW, https://tbpl.mozilla.org/?tree=Try&rev=62ec046931e3

Comment 32

[Ed Morley [:emorley]]

Thank you for unbittrotting & linking to the Try run etc, makes pushing a breeze :-)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1de977085578
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2597b9d35196
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b8c5e232949d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/84ff2ec724a0

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1de977085578
https://hg.mozilla.org/mozilla-central/rev/2597b9d35196
https://hg.mozilla.org/mozilla-central/rev/b8c5e232949d
https://hg.mozilla.org/mozilla-central/rev/84ff2ec724a0