Feature metadata refactor

[phated]

Clean diff against master. /cc @erikkemperman @contra @piranna

Ref #144

[yocontra]

@phated Got a brief summary of the changes inside this? I haven't been keeping up with the symlink/chown issues

[phated]

@contra Goals of these PRs:

• Make sure futimes is only called if there is a difference between on-disk and in-memory and only if we have the correct permissions to actually do it (aka own the file)
• Make sure fchmod is only called if there is a difference in permissions on-disk and in-memory and only if we have the correct permissions to actually do it (aka own the file)
• Use file descriptors everywhere to reduce the overhead of these operations
• Update test for the above
• Still TODO: add lots more tests for the new cases

A lot of stuff is boilerplate that would have happened in node core (like my writeFile helper module) because we need finer grained control over the file descriptor after the content is written.

[erikkemperman]

I haven't been keeping up with the symlink/chown issues

Actually fchown is not in there. Yet? It could write stat.gid/uid to disk as well, if we are owner/root, respectively.

[phated]

@erikkemperman currently trying to fix the features we currently support, we can talk about feature additions after.

[erikkemperman]

Fair enough :-) Just it was cute that chown was mentioned, it does belong with chmod and touch, that way.

[phated]

Working on some tests today

[phated]

@erikkemperman should we use process.umask() anywhere in this new code?

[phated]

Still more to do but made great progress today. Now sleep.

[erikkemperman]

@phated
process.umask() -- Maybe, if the mode on the vinyl is not defined. Currently in writeFile we default to '666' (very metal :-)) but we could do & ~process.umask() like mkdirp does. But I would expect the fs functions to do this for us, anyway?

PS I would always move constants created with a function call (like parseInt(..,8)) out of the critical path -- unless we can be sure v8 will optimize these?

[phated]

@erikkemperman @piranna @contra I've finished the "fileOperations" tests with 100% coverage. Could you review and let me know if anything is missing? I plan to work on "dest" tests next to get coverage on the new stuff and handle some edge cases we aren't testing. Thanks!

[phated]

Coverage is through the roof, finished the fileOperations unit tests (as noted above), added some more dest tests (including some for unowned files). @erikkemperman @piranna @contra Please review everything when you get a chance because I think we are closing in on a complete PR.

Once I get thorough reviews, I'll squash my stuff together.

Anyway, tests are exhausting so I'm headed off for the night.

[phated]

this wasn't a valid option for gulp-sourcemaps so I snuck it in

[erikkemperman]

@phated Unfortunately I am kinda swamped this week -- will have some time to review more thoroughly this weekend if still needed.

Couple of things I notice at a glance:

I was really curious how you would go about unit testing the not-owned file checks. Because I am pretty sure that files you get from git are always owned by whoever ran the git command that put them there. How could it not?

And sure enough, having just fetched in your branch, the not-owned file is owned by me. But that means that those tests might not be doing what they should?

Another thing I notice is that there are a couple should(<spy>.called).be.ok and ...be.not.ok tests in there, which I discovered a couple days back doesn't actually appear to ever make a test fail. See the related PR. So I would guess something like that would have to be changed within this PR as well.

About whether or not to throw errors on failed fchmod or futimes calls... We've talked about that early in this whole affair. @piranna 's implementation just skips silently any call that it determines wouldn't be allowed (ownership). My refactor, and I guess yours on top of that, does the same. But if folks make a point of setting a custom mode, or atime/mtime on the vinyl object, they might want to know about failure to write those attributes.

Would it therefore make sense to have something like a pedantic option which, if set, would either

• throw an error if we determine fchmod or futimes will fail, or
• skip the test for ownership and just attempt the call/s, propagating up any errors off it/them?

[phated]

@erikkemperman thanks for the quick points and I'll wait on any merges until I get a thorough review. I should have time to work on this on the weekend also. I'll address your comments below:

Thanks for checking the ownership stuff. I didn't know that's how git worked because it keeps file modes and it would have been annoying for me to check that. I'll dig into it some more.

I don't believe I have added any lines with should(spies.called) but there are some in the diff so it must be somewhere in our rebases. I will make the changes like your other PR. On a side note, the new tests I've added are using expect() instead of should and I want to do a pass on all the tests and swap it completely (in a separate PR after this).

I don't think it's appropriate to swallow the errors, especially now that we have a lot of checks on the diff and ownership; However, if the vinyl object is a directory and we successfully write it with mode 000, then we aren't going to be writing anything inside it (and if we did, that would error) so I don't see the point in erroring on that open. I guess if they wrote the directory as 000 but then wanted to update the mtime/atime, that would silently succeed but not update the times.

[erikkemperman]

@phated
Well, since the git remote doesn't (shouldn't!) know about my local uid/gid mapping it kinda makes sense. Modes are universal that way.

I don't think it's appropriate to swallow the errors

So instead of having a pedantic option and defaulting to silently skipping futimes and fchmod, you would favour throwing (most) errors instead and possibly a lenient option to get the current behaviour?

I actually think that makes sense for unlikely edge-cases like directory mode '000' too -- it won't error until user tries to violate his own restriction, which is not vfs' problem?

[erikkemperman]

Hm, you were probably talking about the order in which fchmod and futimes are called. For a directory vinyl object with mode 000 the fchmod might work but then the futimes would fail. We would want to do them in reverse order.

But conversely, if the directory already exists and the mode on the vinyl is less restrictive than the one on disk, then futimes might fail if we run it before the fchmod, and the so order we have now is what we want.

Should we be determining whether the requested fchmod makes for a more or a less restrictive subject for futimes, and adjust the call order accordingly?

[phated]

@erikkemperman the mode would be the same (barring any crazy getter usage) because the mkdirp call is passed the stat object from the vinyl object. The only thing that could be different would be the atime/mtime, which wouldn't be called. The only way to make this work would be to check the diff before calling open but if we don't open it, we can't use fstat. I wonder if fs.stat works if permissions are 000

[erikkemperman]

No time to check just now, but does mkdirp also update the mode from file.stat.mode if the directory pre-exists?

[phated]

Wow, a green check next to appveyor...it's the beginning of the end!

[phated]

Rebased and tests are running. Going to review the full diff and write some docs on these changes (and note it doesn't work on Windows) and then ship this.

[phated]

[phated]

Thanks for everyone that helped out on this: @piranna @erikkemperman @contra! This has been shipped as 2.3.2

[yocontra]

@phated Thanks for taking this across the finish line 🌴