Fix relative symlinks by freitagbr · Pull Request #282 · shelljs/shelljs

freitagbr · 2016-01-11T05:25:38Z

Fixes #100

freitagbr · 2016-01-11T05:28:38Z

I based this PR on Schoonology's fix (PR #235), removing the warning that is displayed when a symbolic link is created.

nfischer · 2016-01-11T05:30:49Z

@freitagbr Could you check that this works under node v0.10? It seems to be failing on the CI.

nfischer · 2016-01-11T05:32:36Z

Also, would you be able to squash the commits? See here

freitagbr · 2016-01-11T05:43:53Z

The Node.js v0.10 path module does not have the isAbsolute method (it was added in Node v0.11). That is what is causing the build to fail.

nfischer · 2016-01-11T06:02:25Z

The usual method we use is to check for its existence, and if it doesn't exist, use a workaround. Would that be feasible? That way we can use isAbsolute when it's available, and fall back to the alternative when necessary.

freitagbr · 2016-01-11T06:13:40Z

As long as the source string does not start with a /, it is absolute. Simply checking source.indexOf('/') !== 0 should do the same thing, correct?

nfischer · 2016-01-11T06:16:10Z

Will that play well with Windows? I think that should be sufficient for *nix though (unless ~ is supported)

freitagbr · 2016-01-12T10:18:01Z

This should work as intended, though ~ is still not supported. It is trivial to add support, and I think it would be worthwhile. Thoughts?

nfischer · 2016-01-12T20:46:35Z

I'm working on a PR for the tilde (currently unsupported), but there are some other big changes I want to get merged first. So don't add support for it just yet. Once we do, we'll have to add an "is absolute path" method that handles the tilde

nfischer · 2016-01-24T00:42:18Z

test/ln.js

nfischer · 2016-01-24T01:03:08Z

Could you also add some invalid test cases toward the top?

# symlinks
mkdir -p tmp/
touch tmp/file1.txt
ln -sf tmp/file1.txt tmp/file2.txt
# make sure there was an error

mkdir -p tmp/
touch file1.txt
ln -sf file1.txt tmp/file2.txt
# make sure there was an error

# hardlinks
mkdir -p tmp/
touch tmp/file1.txt
ln -f file1.txt tmp/file2.txt
# make sure there was an error

ariporad · 2016-01-24T22:21:09Z

@freitagbr: Any chance I could ask you to add the small changes? I'd really like to include this in the next release. Thanks!

freitagbr · 2016-01-24T22:33:21Z

@ariporad Will do! I'll have this done by the end of the day.

nfischer · 2016-01-24T22:41:29Z

👍 @freitagbr just ping this thread when you have the tests merged in and I'll review

ariporad · 2016-01-24T22:42:55Z

@freitagbr: Thanks, but no rush!

Ari

On Sun, Jan 24, 2016 at 2:41 PM, Nate Fischer notifications@github.com
wrote:

[image: 👍] @freitagbr https://github.com/freitagbr just ping this
thread when you have the tests merged in and I'll review

—
Reply to this email directly or view it on GitHub
#282 (comment).

When creating a symlink with a relative path, e.g. `ln('-s', '../baz.txt', 'foo/bar/link')`, the link will be created releative to the destination directory, not to the current working directory of the node process. Also added tests for functionality.

nfischer · 2016-01-25T07:23:05Z

@freitagbr then my only other comment is the invalid test cases (#282 (comment)). Once those are in, this'll be good!

freitagbr · 2016-01-25T09:56:59Z

src/ln.js

In testing, I found that the source file was not checked properly for hard links. This correctly checks if the source exists for hard links.

nfischer · 2016-01-25T19:03:04Z

@freitagbr nice work!

LGTM

ariporad · 2016-01-26T01:09:38Z

👍

Fix relative symlinks

nfischer added fix Bug/defect, or a fix for such a problem medium priority labels Jan 11, 2016

nfischer reviewed Jan 24, 2016
View reviewed changes

test/ln.js

Copy link
Copy Markdown

Member

nfischer Jan 24, 2016

Choose a reason for hiding this comment

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

👍

ariporad assigned nfischer Jan 24, 2016

ariporad added this to the v0.6.0 milestone Jan 24, 2016

This was referenced Jan 24, 2016

ln: Treat parameters consistently with native ln. #235

Closed

ln: add possibility creating relative links #234

Closed

freitagbr added 2 commits January 24, 2016 22:35

Fix relative symlinks, add tests

bceb066

When creating a symlink with a relative path, e.g. `ln('-s', '../baz.txt', 'foo/bar/link')`, the link will be created releative to the destination directory, not to the current working directory of the node process. Also added tests for functionality.

Use OS-agnostic filepath verification

fd27d85

Check that source exists for hard links

39ab73f

freitagbr reviewed Jan 25, 2016
View reviewed changes

Add invalid tests for ln

70ec630

nfischer added a commit that referenced this pull request Jan 26, 2016

Merge pull request #282 from freitagbr/fix-relative-symlinks

24c2120

Fix relative symlinks

nfischer merged commit 24c2120 into shelljs:master Jan 26, 2016

freitagbr deleted the fix-relative-symlinks branch January 26, 2016 04:50

nfischer mentioned this pull request Feb 19, 2016

test(ln): add tests for linking to cwd #364

Merged

Conversation

freitagbr commented Jan 11, 2016

Uh oh!

freitagbr commented Jan 11, 2016

Uh oh!

nfischer commented Jan 11, 2016

Uh oh!

nfischer commented Jan 11, 2016

Uh oh!

freitagbr commented Jan 11, 2016

Uh oh!

nfischer commented Jan 11, 2016

Uh oh!

freitagbr commented Jan 11, 2016

Uh oh!

nfischer commented Jan 11, 2016

Uh oh!

freitagbr commented Jan 12, 2016

Uh oh!

nfischer commented Jan 12, 2016

Uh oh!

nfischer Jan 24, 2016

Choose a reason for hiding this comment

Uh oh!

nfischer commented Jan 24, 2016

Uh oh!

ariporad commented Jan 24, 2016

Uh oh!

freitagbr commented Jan 24, 2016

Uh oh!

nfischer commented Jan 24, 2016

Uh oh!

ariporad commented Jan 24, 2016

Uh oh!

nfischer commented Jan 25, 2016

Uh oh!

freitagbr Jan 25, 2016

Choose a reason for hiding this comment

Uh oh!

nfischer commented Jan 25, 2016

Uh oh!

ariporad commented Jan 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants