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.

Sign up for GitHub

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

Jump to bottom

Allow ln destination to be an existing directory, fixes #832 #833

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Allow ln destination to be an existing directory, fixes #832 #833

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3fe1a67
Allow ln destination to be an existing directory
loilo Apr 5, 2018
4008596
Fix coding style
loilo Apr 5, 2018
50e6ca8
Implement first review feedback
loilo Apr 5, 2018
582414d
Remove incorrect parameter from stat.isDirectory()
loilo Apr 5, 2018
9392c55
Re-add the working directory cleanup in test
loilo Apr 5, 2018
946ab48
cd to the original CWD after each test
loilo Apr 6, 2018
c57f7ff
Allow dead symlinks for source/destination, fixes #829
loilo Apr 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/ln.js
View file
Open in desktop
Expand Up @@ -35,6 +35,10 @@ function _ln(options, source, dest) {
var isAbsolute = (path.resolve(source) === sourcePath);
dest = path.resolve(process.cwd(), String(dest));

if (fs.existsSync(dest) && common.statFollowLinks(dest).isDirectory(dest)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think isDirectory() accepts arguments: https://nodejs.org/api/fs.html#fs_stats_isdirectory

Copy link
Author

Choose a reason for hiding this comment

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

Oh, right. That was definitely not on purpose. 😅

Copy link
Author

Choose a reason for hiding this comment

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

Done.

dest = path.resolve(dest, path.basename(sourcePath));
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be path.join(). The docs for path.resolve() claim it produces an absolute path. ln -s actually cares if it's passed an absolute path vs. relative path, however, and path.join() preserves that.

Copy link
Author

Choose a reason for hiding this comment

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

However, that shouldn't matter here, should it? It has been path.resolved three lines earlier anyway.

So given that we are guaranteed that dest already is an absolute path, path.join and path.resolve should return the exact same result and I decided to go for the one that was used in the code around it.

Copy link
Author

Choose a reason for hiding this comment

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

Done. I've replaced resolve with join. Shouldn't change anything, but it doesn't hurt either. 😁

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you're right. It's ok to resolve the destination, but not OK to resolve the source. Either join or resolve is fine.

}

if (fs.existsSync(dest)) {
if (!options.force) {
common.error('Destination file exists', { continue: true });
Expand Down
21 changes: 21 additions & 0 deletions test/ln.js
View file
Open in desktop
Expand Up @@ -48,6 +48,14 @@ test('destination already exists', t => {
t.is(result.code, 1);
});

test('destination already exists inside directory', t => {
shell.cd(t.context.tmp);
const result = shell.ln('-s', 'file1', './');
t.truthy(shell.error());
t.is(result.code, 1);
shell.cd('..');
Copy link
Member

Choose a reason for hiding this comment

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

This line isn't necessary. We restore state inside beforeEach().

Copy link
Author

@loilo loilo Apr 5, 2018 •
edited

Choose a reason for hiding this comment

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

Are you sure that's working correctly? If I omit restoring the original working directory, the following test will fail, afaict due to non-existing file paths.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, nevermind. I get the described behaviour when I remove the shell.cd('..') at line 158, not here.

Copy link
Author

@loilo loilo Apr 5, 2018 •
edited

Choose a reason for hiding this comment

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

Removed line 56.

Re-added line 56, see comment below.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is a bug. As a safer workaround, could you edit these lines:

shelljs/test/ln.js

Lines 18 to 20 in 9035b27

test.afterEach.always(t => {
shell.rm('-rf', t.context.tmp);
});

to include process.chdir(CWD) before the rm() call? I've filed #834 to provide a better, long-term solution.

The problem with cd('..') is that our code might throw exceptions, causing the cd('..') to not be executed (and so state would never be restored). Also, I think ava stops running a test case as soon as the first assertion fails, which is another case where cd('..') would be skipped.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I've already had that case where an unrelated test would fail just because the previous one couldn't cd('..') anymore.

Copy link
Author

Choose a reason for hiding this comment

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

I have added chdiring to the CWD after each test and also removed all now-gratuitous cd('..') lines in the file.

});

test('non-existent source', t => {
const result = shell.ln(`${t.context.tmp}/noexist`, `${t.context.tmp}/linkfile1`);
t.truthy(shell.error());
Expand Down Expand Up @@ -137,6 +145,19 @@ test('To current directory', t => {
shell.cd('..');
});

test('Inside existing directory', t => {
shell.cd(t.context.tmp);
const result = shell.ln('-s', 'external/node_script.js', './');
t.is(result.code, 0);
Copy link
Member

Choose a reason for hiding this comment

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

nit: also assert .stderr and shell.error() are empty

Copy link
Author

Choose a reason for hiding this comment

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

Going to add those. It's not been done anywhere in the ln() tests, and I mostly got inspiration from the other tests around there. 🙈

Copy link
Author

Choose a reason for hiding this comment

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

Check.

t.truthy(fs.existsSync('node_script.js'));
t.is(
fs.readFileSync('external/node_script.js').toString(),
fs.readFileSync('node_script.js').toString()
);
shell.rm('node_script.js');
Copy link
Member

Choose a reason for hiding this comment

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

Cleanup isn't necessary

Copy link
Author

Choose a reason for hiding this comment

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

Removed that line.

shell.cd('..');
});

test('-f option', t => {
const result = shell.ln('-f', `${t.context.tmp}/file1.js`, `${t.context.tmp}/file2.js`);
t.is(result.code, 0);
Expand Down