Skip to content

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

Merged
ariporad merged 1 commit intomasterfrom
test-ln-s-dir
Feb 19, 2016
Merged

test(ln): add tests for linking to cwd#364
ariporad merged 1 commit intomasterfrom
test-ln-s-dir

Conversation

@nfischer
Copy link
Copy Markdown
Member

Add tests for symlinking to ./ (this closes #363 if the behavior here is correct). If this behavior is not posix behavior, we should fix this instead.

assert(fs.existsSync('testfile.txt'));
assert(fs.existsSync('dest/testfile.txt'));
assert(fs.existsSync('dir1/insideDir.txt'));
assert(!fs.existsSync('dest/insideDir.txt'));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This file should exist right? If it doesn't then the dest folder wasn't symlinked properly from the dir1 folder.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think it shouldn't. Symbolic relative links are relative to the destination, hard links are relative to the cwd.

insideDir.txt is inside dir1, not its parent directory, and dest points at dir1's parent

Try out this behavior with UNIX Ln and I think it should behave the same

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah, you're right. Not what I would have expected. Thanks for following up on this.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For posterity, this is the test I ran manually:

➜  ~  docker run -it --rm ubuntu
root@784ff6f8e5e4:/# cd tmp
root@784ff6f8e5e4:/tmp# mkdir dir1
root@784ff6f8e5e4:/tmp# cd dir1/
root@784ff6f8e5e4:/tmp/dir1# ln -s ./ ../dest
root@784ff6f8e5e4:/tmp/dir1# touch insideDir.txt
root@784ff6f8e5e4:/tmp/dir1# cd ../
root@784ff6f8e5e4:/tmp# ls
dest  dir1
root@784ff6f8e5e4:/tmp# ls dest
dest  dir1
root@784ff6f8e5e4:/tmp# cd dest
root@784ff6f8e5e4:/tmp/dest# ls
dest  dir1
root@784ff6f8e5e4:/tmp/dest# exit
exit

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's a very confusing semantics. Hopefully the path.resolve() trick is good enough to fix things. I personally usually use full paths with GNU ln To get around the weirdness

@nfischer
Copy link
Copy Markdown
Member Author

@ariporad this is ready for review

@ariporad
Copy link
Copy Markdown
Contributor

@nfischer: Umm... I don't think I really understand this well enough to properly review it. It seems very confusing. I'll do some more research, and get back to you.

@nfischer
Copy link
Copy Markdown
Member Author

@ariporad this is related to how ln handles relative symbolic links (see #282). I think POSIX has somewhat confusing behavior, but at least it's consistent. This was a change we introduced in v0.6, I'm just adding a couple extra tests to make sure we don't get a regression.

@ariporad
Copy link
Copy Markdown
Contributor

@nfischer: Ok.

@ariporad
Copy link
Copy Markdown
Contributor

LGTM!

ariporad added a commit that referenced this pull request Feb 19, 2016
test(ln): add tests for linking to cwd
@ariporad ariporad merged commit 56eb0c5 into master Feb 19, 2016
@ariporad ariporad deleted the test-ln-s-dir branch February 19, 2016 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ln('-sf', './', '<destination>') is not linking the right folder

3 participants