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

Fixes #420 - Setting a mode for a new file, when opening it. #476

Merged
merged 12 commits into from Nov 22, 2018

Conversation

Projects
None yet
4 participants
@steaward
Copy link

commented Sep 24, 2018

This fixes issue (#420)

  1. a mode has been added to the fs.open_file function definition

  2. only when a mode argument is passed: the mode is then associated with the new Node.
    if no argument, a mode value is still set the default file option of 0o644.

  3. a test has been created to ensure that if a mode is passed in as an argument for the fs.open function, the mode exists as it should.

Would still be nice to...

  • test to ensure that mode's do not change when opening existing files, and passsing in a mode options.
  • thinking of more ways to break this code would be helpful too.
@humphd

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

The mode is somewhat complicated by the fact that it includes two things: bits for the node's type (file, directory, link, ...); and also for permissions (e.g., 644, 777, etc). So if you only want the permissions, you have to mask them out.

Stephen Ward added some commits Sep 24, 2018

Stephen Ward
added the addition of an optional mode when opening a file, removed m…
…y test because it was broken. Seemingly passes all the other tests still. STILL TO DO: add a proper test which makes sure the mode is set.
Stephen Ward

@steaward steaward changed the title WIP: Issue #420 - Setting a mode for a new file, when opening it. Fix #420 - Setting a mode for a new file, when opening it. Sep 25, 2018

});
});


This comment has been minimized.

Copy link
@ThomasNolte

ThomasNolte Sep 26, 2018

Contributor

nit: remove white extra white spaces between functions. line (109, 126)

@@ -533,7 +533,10 @@ function remove_directory(context, path, callback) {
find_node(context, parentPath, read_parent_directory_data);
}

function open_file(context, path, flags, callback) {
function open_file(context, path, flags, mode, callback) {
if (typeof mode == 'function'){

This comment has been minimized.

Copy link
@ThomasNolte

ThomasNolte Sep 26, 2018

Contributor

use === when comparing these values. the reason for this is in JavaScript the == is not as strict with comparing values and does not ensure an exact match.

The code in this source file has many examples of this same if statement for reference see line 1578.

read more about == vs === here

@@ -1623,8 +1627,15 @@ function open(fs, context, path, flags, mode, callback) {
* callback = makeCallback(callback);
* }
*/

callback = arguments[arguments.length - 1];
if (arguments.length == 5){

This comment has been minimized.

Copy link
@ThomasNolte

ThomasNolte Sep 26, 2018

Contributor

I believe this if statement is setup backwards, it is currently allowing all values except 5 to be valid.

Going off the mkdir function on line 1669 there if statement should be:
if(arguments.length < 6)
{handle incorrect number of arguments here}
else()
{validate mode here}

@@ -3278,12 +3278,14 @@
"balanced-match": {

This comment has been minimized.

Copy link
@ThomasNolte

ThomasNolte Sep 26, 2018

Contributor

The package-lock.json file should not be included in your pull request.

mode = 0o644;
}
else {
//need to test this validateAndMakeMode

This comment has been minimized.

Copy link
@ThomasNolte

ThomasNolte Sep 26, 2018

Contributor

nit: can remove this comment

@steaward steaward changed the title Fix #420 - Setting a mode for a new file, when opening it. Fixes #420 - Setting a mode for a new file, when opening it. Sep 27, 2018

@humphd
Copy link
Contributor

left a comment

This is looking pretty close. @ThomasNolte has done an excellent review before I got to this, so please take a look at all his comments. I've added some more in addition. Please update your PR and flag me for review again.

I'm glad you followed through on this, and decided to fix this case. I've always hated that we didn't bother to do the right thing with mode.

Finally, please revert your changes to package-lock.json via git checkout master package-lock.json

Show resolved Hide resolved src/filesystem/implementation.js
@@ -645,6 +648,7 @@ function open_file(context, path, flags, callback) {
}
fileNode = result;
fileNode.nlinks += 1;
if(mode){Node.setMode(mode, fileNode);}

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor

Split this onto new lines please:

if(mode) {
  Node.setMode(mode, fileNode);
}

Stephen Ward added some commits Oct 10, 2018

@humphd
Copy link
Contributor

left a comment

This is looking really close. Left some more feedback.

Show resolved Hide resolved tests/spec/fs.open.spec.js
Show resolved Hide resolved src/filesystem/implementation.js Outdated
Stephen Ward
re-added the newline, as well as another test case to make sure mode …
…is still set to default value when a new file is opened
@steaward

This comment has been minimized.

Copy link
Author

commented Oct 11, 2018

@humphd a successful test case has been added to test the default mode value.

as we learned in class today, should I squash all my commits?

@humphd

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

Wait to rebase/squash this until it's 100% done.

Can you also remove the comment at

/**
* NOTE: we support the same signature as node with a `mode` arg,
* but ignore it. We need to add it. Here is what node.js does:
* function open(path, flags, mode, callback) {
* path = getPathFromURL(path);
* validatePath(path);
* const flagsNumber = stringToFlags(flags);
* if (arguments.length < 4) {
* callback = makeCallback(mode);
* mode = 0o666;
* } else {
* mode = validateAndMaskMode(mode, 'mode', 0o666);
* callback = makeCallback(callback);
* }
*/

@steaward

This comment has been minimized.

Copy link
Author

commented Oct 15, 2018

@humphd is there anything else I can do to make this PR a success?

@humphd

humphd approved these changes Oct 15, 2018

Copy link
Contributor

left a comment

Looks good to me. I'll get @modeswitch to do a final review on it.

@humphd humphd requested a review from modeswitch Oct 15, 2018

@modeswitch

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

Sorry for the huge delay, I've started looking at this.

@modeswitch modeswitch merged commit d1afe97 into filerjs:master Nov 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.