Feature metadata #144

erikkemperman · 2016-01-14T06:33:05Z

phated · 2016-01-15T20:58:29Z

@erikkemperman it seems that fchmod can't be used on a file that you don't own. Can you add the same check as getTimes to compare the uid and a test to confirm it is working?

erikkemperman · 2016-01-15T21:20:14Z

At a glance, there is no test for the check in getTimes yet, either? I will have some time for this tomorrow.

erikkemperman · 2016-01-17T16:35:29Z

Several changes, including the guard against calling fchmod when we don't own the file. I have no idea how to unit test this though, we're not allowed to chown a file in order to verify this guard works. Ideas?

The last couple of commits are smaller changes I think are improvements, but for the moment I'll leave them unsquashed for easier review.

erikkemperman · 2016-01-17T16:40:40Z

Note, there are of course still those two tests that fail against the version of vinyl with the contents getter/setter changing stat.xtime. Because that change is already in master, I propose to get that sorted out first -- i.e. in the corresponding PR -- before contemplating this one.

phated · 2016-01-18T22:28:13Z

@erikkemperman we don't rely on the master branch of vinyl so I don't know why they would be failing

phated · 2016-01-26T00:50:15Z

@erikkemperman can you revert some of the changes that were added to handle the vinyl changes I've now removed. I'd like to get this in but it got a lot more complicated with those workarounds.

erikkemperman · 2016-01-26T06:36:59Z

@phated I've tried to keep the changes related to the now removed vinyl changes in a separate PR, here. If there's anything in here you'd rather not have I can of course remove those, but I believe these commits are orthogonal to the vinyl stuff.

phated · 2016-01-26T06:44:52Z

@erikkemperman ah, I had thought the last 4-5 commits were addressing things from the vinyl changes.

erikkemperman · 2016-01-26T06:53:30Z

@phated No they're just tweaks on @piranna and my efforts that I kept separate for the moment. I think they are slight improvements, especially the commit that makes the stat post-dest reflect the file on disk, which ought to keep piping onto dest equivalent to having a new src on that file.

Note however that the other PR, which is now mostly pointless until the vinyl issue is resolved, does contain as requested a more or less unrelated commit to address the intermittently failing tests, here.

I didn't have any time the last week or so, but still think the vinyl issue deserves some thought, would love some feedback on that issue.

phated · 2016-01-26T06:55:22Z

@erikkemperman it definitely needs more thought and discussion. However, can you submit a separate PR with the fix to the failing tests? I'd like to get that merged.

phated · 2016-01-26T06:56:01Z

lib/dest/writeContents/index.js

+      // Check if mode needs to be updated
+      var modeDiff = 0;
+      if (typeof file.stat.mode === 'number') {
+        modeDiff = (file.stat.mode ^ stat.mode) & MASK_MODE;


what is the ^ operator? I've never encountered it before

nvm, found it at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Bitwise_Operators#Bitwise_XOR

Heh, I guess I betrayed my C origins there. It's a nice operator to get bit-difference.

erikkemperman · 2016-01-26T06:58:40Z

Sure thing, will do that when I get on campus in an hour or so.

As to the commits here, I kept them separate for easier review but should probably squash at some point.

phated · 2016-01-26T07:01:47Z

lib/dest/writeContents/index.js

 var writeDir = require('./writeDir');
 var writeStream = require('./writeStream');
 var writeBuffer = require('./writeBuffer');
 var writeSymbolicLink = require('./writeSymbolicLink');

+// TODO include sticky/setuid/setgid, i.e. 7777?
+var MASK_MODE = parseInt('0777', 8);


@erikkemperman what would happen if this was just '777' like we did at https://github.com/gulpjs/vinyl-fs/blob/master/test/dest.js#L38

see #103 and #134

@phated Not sure what you mean... If you meant dropping the zero, so it would leave parseInt('777', 8) -- that would do the exact same thing, since it is the same number...

I added the leading zero to emphasize we are now actively ignoring any sticky/setuid/setgid bits that users might have set on stat.mode. Notice that this does not set those bits on extant files on disk to 0 -- all this mask does is ignore any bits on stat.mode that are not rwx flags in building the argument to fchmod. And the TODO comment questioned if we wanted to allow users to set those other bits as well.

Either way, it is separate from the issue you linked, although if we were to allow these bits to be set by the user, and reflected on disk by dest, then there should be a test for it which doesn't use the realMode function of course (awkward description of what it does, acually).

erikkemperman · 2016-01-26T08:11:07Z

~~@piranna~~ @phated The fix for the intermittently failing tests would have to be altered again if this PR were merged. So I would propose getting this out the way first and then addressing those?

Let me know if I should remove any commits from this PR, or rebase/squash.

EDIT: Sorry @piranna, auto-complete got me. I meant to address @phated.

piranna · 2016-01-26T08:41:34Z

EDIT: Sorry @piranna, auto-complete got me. I meant to address @phated.

Ok, don't worry :-)

phated · 2016-01-28T18:58:41Z

@erikkemperman feel free to squash, I am working on some cleanup against this branch currently but can stash and apply after the rebase.

…ta in dest

erikkemperman · 2016-01-28T19:11:53Z

@phated Done! Thanks for being patient, this whole thing took a good while. Once merged I will adapt and submit the fix for intermittently failing atime/mtime tests.

phated · 2016-01-28T19:19:20Z

@erikkemperman one more thing, we are writing a lot of functions that take the file descriptor and the vinyl file object - what do you think about attaching the file descriptor to the vinyl object while we are passing it through the vinyl-fs machinery and then removing it when done? This would be very similar to how node's fs.WriteStream works

erikkemperman · 2016-01-28T19:31:30Z

@phated I inlined some of those functions, like times and mode, so they would have some of their operands in scope and not have to be passed everything. But that won't work across module boundaries obviously.

So you're suggesting to have the various writeXXX modules do file.fd = ... before calling the written callback, and then to have each of them do delete file.fd in their close callback?

I guess I don't see why not, but then I also don't really mind a separate argument in what is after all an internal contract.

erikkemperman · 2016-01-28T19:40:25Z

@phated Would it be a completely ludicrous idea to de-couple the opening/closing of filedescriptors, i.e. have a separate utility which handles this stuff? That might do lazy closing or even predictive opening? I imagine it is pretty common to have intermediate (but persistent) files, and such cases might benefit from subsequent pipes discovering that its file still has a handle.

phated · 2016-02-26T01:19:54Z

Finished by #151 and published as 2.3.2 - Thanks for all your work on this @erikkemperman!

erikkemperman force-pushed the feature-metadata branch from 2d3d7f5 to cba680e Compare January 14, 2016 07:24

erikkemperman force-pushed the feature-metadata branch 2 times, most recently from 91d6e19 to 35cff30 Compare January 17, 2016 16:29

erikkemperman force-pushed the feature-metadata branch from 35cff30 to 9c80274 Compare January 17, 2016 16:37

erikkemperman force-pushed the feature-metadata branch 5 times, most recently from a297ca5 to 61cef07 Compare January 18, 2016 12:09

phated reviewed Jan 26, 2016
View reviewed changes

piranna and others added 2 commits January 28, 2016 20:02

Use filedescriptors where possible, refactor handling of stats metada…

35e9b3a

…ta in dest

Make the tests pass again, some more refactoring of metadata handling

90a1189

erikkemperman force-pushed the feature-metadata branch from 61cef07 to 90a1189 Compare January 28, 2016 19:06

phated mentioned this pull request Jan 28, 2016

Feature metadata refactor #151

Merged

phated merged commit 90a1189 into gulpjs:master Feb 26, 2016

erikkemperman deleted the feature-metadata branch May 12, 2016 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature metadata #144

Feature metadata #144

erikkemperman commented Jan 14, 2016

phated commented Jan 15, 2016

erikkemperman commented Jan 15, 2016

erikkemperman commented Jan 17, 2016

erikkemperman commented Jan 17, 2016

phated commented Jan 18, 2016

phated commented Jan 26, 2016

erikkemperman commented Jan 26, 2016

phated commented Jan 26, 2016

erikkemperman commented Jan 26, 2016

phated commented Jan 26, 2016

phated Jan 26, 2016

phated Jan 26, 2016

erikkemperman Jan 26, 2016

erikkemperman commented Jan 26, 2016

phated Jan 26, 2016

phated Jan 26, 2016

erikkemperman Jan 26, 2016

erikkemperman commented Jan 26, 2016

piranna commented Jan 26, 2016

phated commented Jan 28, 2016

erikkemperman commented Jan 28, 2016

phated commented Jan 28, 2016

erikkemperman commented Jan 28, 2016

erikkemperman commented Jan 28, 2016

phated commented Feb 26, 2016

Feature metadata #144

Feature metadata #144

Conversation

erikkemperman commented Jan 14, 2016

phated commented Jan 15, 2016

erikkemperman commented Jan 15, 2016

erikkemperman commented Jan 17, 2016

erikkemperman commented Jan 17, 2016

phated commented Jan 18, 2016

phated commented Jan 26, 2016

erikkemperman commented Jan 26, 2016

phated commented Jan 26, 2016

erikkemperman commented Jan 26, 2016

phated commented Jan 26, 2016

phated Jan 26, 2016

Choose a reason for hiding this comment

phated Jan 26, 2016

Choose a reason for hiding this comment

erikkemperman Jan 26, 2016

Choose a reason for hiding this comment

erikkemperman commented Jan 26, 2016

phated Jan 26, 2016

Choose a reason for hiding this comment

phated Jan 26, 2016

Choose a reason for hiding this comment

erikkemperman Jan 26, 2016

Choose a reason for hiding this comment

erikkemperman commented Jan 26, 2016

piranna commented Jan 26, 2016

phated commented Jan 28, 2016

erikkemperman commented Jan 28, 2016

phated commented Jan 28, 2016

erikkemperman commented Jan 28, 2016

erikkemperman commented Jan 28, 2016

phated commented Feb 26, 2016