win, fs: detect if symlink target is a directory by bzoz · Pull Request #23724 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation45 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

bzoz

On Windows creating a symlink to a directory will not work unless extra 'dir' parameter is passed. This adds a check if link target is a directory, and if so automatically use 'dir' when creating the symlink.

Ref: #23691

Checklist

@bzoz bzoz added fs

Issues and PRs related to the fs subsystem / file system.

windows

Issues and PRs related to the Windows platform.

semver-major

PRs that contain breaking changes and should be released in the next major version.

labels

Oct 18, 2018

@nodejs-github-bot

@bzoz

refack

refack previously requested changes Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I left some comments

@refack

P.S. I'm not sure this is semver-major if we only change a case that failed into a case that works.

@bzoz

refack

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes LGTM.
But I forget to mentions that async calls need common.mustCall

}
function async(target, path) {
fs.symlink(target, path, (err) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fs.symlink(target, path, (err) => {
fs.symlink(target, path, common.mustCall((err) => {

@bzoz

@refack

lint:

05:47:27 not ok 15 - /usr/home/iojs/build/workspace/node-test-linter/test/parallel/test-fs-symlink-dir.js
05:47:27   ---
05:47:27   message: '''linkTarget'' is never reassigned. Use ''const'' instead.'
05:47:27   severity: error
05:47:27   data:
05:47:27     line: 37
05:47:27     column: 10
05:47:27     ruleId: prefer-const
05:47:27   messages:
05:47:27     - message: '''linkPath'' is never reassigned. Use ''const'' instead.'
05:47:27       severity: error
05:47:27       data:
05:47:27         line: 39
05:47:27         column: 12
05:47:27         ruleId: prefer-const
05:47:27   ...

Also test fails when run in worker.

refack

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with lint removed

@refack

/CC @nodejs/release is this semver-major or minor?

@bzoz

refack

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a good solution

@refack

@refack refack added author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

and removed author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

labels

Oct 25, 2018

@bzoz

@bzoz

@refack: If a user creates a symlink to a folder, removes the folder and creates a file with the same name as the folder, currently the symlink will work, and the user will be able to access the file. With this change, the symlink will be broken. It's an obscure corner case, but still, this is why I think its semver-major.

@refack

@nodejs/TSC PTAL (bug fix with some semver-major edge cases)

@bzoz

jasnell

@bzoz

The git node thingy complains that I need 2 TSC approvals for semver-major change.. so, ping?

@rvagg

OK, so a few questions:

  1. What happens if you supply type='file' for a directory and vice versa on Windows? (background I don't understand)
  2. With this change, what's the point of the type argument now? Is that just useful for junction? Or does the answer to the question above suggest other novel uses?
  3. Should the docs be updated to reflect this new behaviour? They currently say the default is 'file' which it clearly isn't after this change and may not even be before this change (although again, I'm missing background on how Windows does symlinks).

@bzoz

  1. type='file' for a target that is a directory:
    • fs.readdir or cd/dir in the shell will not work
    • if there is a subfolder in the driectory both fs.readdir('link/subdir') and cd link\subdir/dir link/subdir in the shell will work
      type='dir' to a target that is a file: nothing works
  2. junction and to override the detection. If the target does not exists, 'file' will be used by default, if one knows that the link will point to a directory, the type argument can be used
  3. Yeah, it should. I'm on it.

@rvagg

OK, that blows my mind a little bit. But I think the take-away is that the flexibility might be handy for obscure cases but doing best-guess for the user is going to be optimal. Sounds good then.

@bzoz

Updated with doc update, PTAL

rvagg

will automatically be normalized to absolute path.
to the completion callback. The `type` argument is only available on Windows
and ignored on other platforms. It can be set to `'dir'`, `'file'`, or
`'junction'`. If the `type` argument is not set Node will autodetect `target`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comma between "set" and "Node"

to the completion callback. The `type` argument is only available on Windows
and ignored on other platforms. It can be set to `'dir'`, `'file'`, or
`'junction'`. If the `type` argument is not set Node will autodetect `target`
type and use `'file'` or `'dir'`. If the `target` does not exits, `'file'` will

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"exits" -> "exist"

mcollina

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a a passing CITGM

fs.readdirSync(path);
}
function async(target, path) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would prefer this function not to be named as language keyword.

@bzoz

@bzoz

@bzoz

On Windows creating a symlink to a directory will not work unless extra 'dir' parameter is passed. This adds a check if link target is a directory, and if so automatically use 'dir' when creating symlink.

Ref: nodejs#23691

Trott

// errors consistent between platforms if invalid path is
// provided.
absoluteTarget = pathModule.resolve(path, '..', target);
} catch (err) { } // eslint-disable-line no-unused-vars

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to use this instead?:

} catch (err) { } // eslint-disable-line no-unused-vars
} catch { }

Then you don't need to disable the ESLint rule. The usual reason to avoid that catch syntax would be to simplify backporting to 8.x and 6.x but this is a semver-major so that's not an issue here.

Trott

if (statSync(absoluteTarget).isDirectory()) {
type = 'dir';
}
} catch (err) { } // eslint-disable-line no-unused-vars

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} catch (err) { } // eslint-disable-line no-unused-vars
} catch { }

@bzoz

@Trott

Trott pushed a commit to Trott/io.js that referenced this pull request

Nov 29, 2018

@bzoz @Trott

On Windows creating a symlink to a directory will not work unless extra 'dir' parameter is passed. This adds a check if link target is a directory, and if so automatically use 'dir' when creating symlink.

PR-URL: nodejs#23724 Refs: nodejs#23691 Reviewed-By: Refael Ackermann refack@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Rod Vagg rod@vagg.org Reviewed-By: Matteo Collina matteo.collina@gmail.com

refack pushed a commit to refack/node that referenced this pull request

Jan 14, 2019

@bzoz @refack

On Windows creating a symlink to a directory will not work unless extra 'dir' parameter is passed. This adds a check if link target is a directory, and if so automatically use 'dir' when creating symlink.

PR-URL: nodejs#23724 Refs: nodejs#23691 Reviewed-By: Refael Ackermann refack@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Rod Vagg rod@vagg.org Reviewed-By: Matteo Collina matteo.collina@gmail.com

BethGriggs added a commit that referenced this pull request

Apr 22, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

This was referenced

Apr 23, 2019

Reviewers

@Trott Trott Trott left review comments

@mcollina mcollina mcollina approved these changes

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@rvagg rvagg rvagg approved these changes

Labels

fs

Issues and PRs related to the fs subsystem / file system.

semver-major

PRs that contain breaking changes and should be released in the next major version.

windows

Issues and PRs related to the Windows platform.