Adapt tests to vinyl stats changes

[erikkemperman]

I had to adapt the tests to changes I submitted to vinyl here.

[phated]

@erikkemperman what happened with this?

[erikkemperman]

@phated Oh dear, I'm sorry. Sure made a hash of it!

I had an earlier attempt at the futimes rebase in a different branch, with a name much like this one. So I cleaned up, realized my mistake, then seemed to remember that extant PRs survive a deleted branch just fine. Anyway, I've restored it once again and promise not to do any more crazy late-night cleaning.

[erikkemperman]

Huh. That's odd. The exact same code passed Travis before, here.

And just now I noticed that Travis mailed me about a failure on an earlier commit on top of piranna's effort, with the very same error, after a completely unrelated change...

So it looks like the following test is failing intermittently: "dest stream should not modify file mtime and atime when not provided on the vinyl stat".

Will need some time to look into that...

[phated]

@erikkemperman that is just a rounding problem with setMilliseconds(0) in the tests. I noticed that some of those weren't removed when you changed the utimes call to futimes to get sub-second precision. I'd be fine with you updating those tests in this PR.

[erikkemperman]

I thought I had gotten rid of the last of those in this PR already.

But now I see there were two more test cases which fail if they don't start and end within the same second. I think this must have been happening since before the utimes->futimes change even, albeit rarely?

Anyway, fixed now.

[phated]

why was this test removed?

[erikkemperman]

There's another test here which does for just mtime what this one does for both atime and mtime.

Now the a file's stat.atime changes when its contents are read, and this is reflected on disk, there is no point in providing an expected fixed atime to result from a subsequent statSync on that file.

And so I figured we'd only need the mtime-only version of this test.

But I am increasingly uncomfortable that we've got some of this stuff the wrong way around.

[phated]

what do you mean by "But I am increasingly uncomfortable that we've got some of this stuff the wrong way around."?

[erikkemperman]

$ touch in out

$ ls -lu in out #atime
-rw-r--r--  1 erik  staff  0 Jan 15 22:25 in
-rw-r--r--  1 erik  staff  0 Jan 15 22:25 out

$ ls -lc in out #mtime
-rw-r--r--  1 erik  staff  0 Jan 15 22:25 in
-rw-r--r--  1 erik  staff  0 Jan 15 22:25 out

$ #wait a minute
$ cat in > out

$ ls -lu in out #atime
-rw-r--r--  1 erik  staff  0 Jan 15 22:26 in
-rw-r--r--  1 erik  staff  0 Jan 15 22:25 out

$ ls -lc in out #mtime
-rw-r--r--  1 erik  staff  0 Jan 15 22:25 in
-rw-r--r--  1 erik  staff  0 Jan 15 22:26 out

Currently, the dest code updates the atime on the vinyl file because it gets its contents. While the src code updates the mtime on the vinyl file because it is setting its contents, and so will always appear to be modified.

That seems wrong externally (the out file will have the wrong atime) and internally (plugins will get a later mtime than the sourced file).

Furthermore, questions of what we're allowed to do privilege-wise are different if we are creating or overwriting the destination, which is a distinction which is implicit, at best, as things now stand.

[phated]

Ah. Yeah, those things seem very wrong :P

[erikkemperman]

Ah. Yeah, those things seem very wrong :P

I thought so. And yet all of this passes the tests...

Effectively, I guess, the current behaviour of the getter and setter of contents is correct from the pov of a plugin, but we must kind of invert things at the terminals src and dest?

When src sets file.contents this represents an access of the physical source file it reflects. So the setter of contents, as called by src, should update atime. The mtime should be the same as the source file.

When dest gets file.contents this represents a modification of the file on disk. So the getter of contents, as called by dest, should update mtime. The atime of the physical destination file should be the same as the vinyl.

Does that make any sense?

[phated]

@erikkemperman I'm going to back out the vinyl changes and reopen your PR there.

[erikkemperman]

So... As a temporary band-aid, I've modified this PR to cancel the effects of file.contents getter/setter that are now in vinyl/master when the accessing party is src or dest. This addresses some of the weirdness I mentioned above.

I say temporary band-aid because I still think the merged vinyl patch should be improved on, and hopefully such that it doesn't have side-effects to cancel.

[phated]

This is so far out of date now that I'm just going to close it. Definitely something we need to look into though.

[erikkemperman]

Sorry, forgot about this one. The changes in vinyl that this patch addressed were reverted, so it was not only out of date but useless as well.