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

loilo · 2018-04-05T09:51:57Z

This allows symlinking destinations to be existing directories. Symlinks will be placed inside those, as described in #832.

codecov-io · 2018-04-05T10:17:10Z

Codecov Report

Merging #833 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #833      +/-   ##
==========================================
+ Coverage   95.49%   95.53%   +0.03%     
==========================================
  Files          34       34              
  Lines        1266     1276      +10     
==========================================
+ Hits         1209     1219      +10     
  Misses         57       57

Impacted Files	Coverage Δ
src/ln.js	`97.61% <100%> (+0.74%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9035b27...c57f7ff. Read the comment docs.

nfischer

Looks pretty good, a few things to fix. Thanks for fixing this up, we've received an unusually high number of bug reports related to ln() recently (let me know if you're interested in fixing those too)...

nfischer · 2018-04-05T22:52:38Z

test/ln.js

+  const result = shell.ln('-s', 'file1', './');
+  t.truthy(shell.error());
+  t.is(result.code, 1);
+  shell.cd('..');


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

~~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.~~

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

~~Removed line 56.~~

Re-added line 56, see comment below.

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.

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

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

nfischer · 2018-04-05T22:55:01Z

test/ln.js

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


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

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. 🙈

nfischer · 2018-04-05T22:56:15Z

test/ln.js

+    fs.readFileSync('external/node_script.js').toString(),
+    fs.readFileSync('node_script.js').toString()
+  );
+  shell.rm('node_script.js');


Cleanup isn't necessary

Removed that line.

nfischer · 2018-04-05T22:57:40Z

src/ln.js

@@ -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)) {


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

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

nfischer · 2018-04-05T23:03:52Z

src/ln.js

@@ -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)) {
+    dest = path.resolve(dest, path.basename(sourcePath));


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.

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.

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

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

nfischer

Looks pretty good, a few things to fix. Thanks for fixing this up, we've received an unusually high number of bug reports related to ln() recently (let me know if you're interested in fixing those too)...

loilo · 2018-04-05T23:23:48Z

I'm going to take a look at your comments tomorrow, it's already night time over here. Thanks for the initial feedback. 🙂

Regaring the other ln() issues: I'd be glad to go for those if they're similarly low-hanging fruit as this one was. I'll take a look tomorrow.

loilo · 2018-04-05T23:45:00Z

Okay, I've adjusted the the things you mentioned. Let me know if there's anything else I can help with. 🙂

EDIT: Seems that, for some reason, the temporary test folders are not cleaned up correctly. AppVeyor tries to lint them. Let's see if I can find the cause.

EDIT 2: Weirdly, this was actually caused by removing the working directory cleanup line. I've re-added it to make tests work again.

It somehow caused temporary test folders not to get cleaned up correctly.

nfischer · 2018-04-06T00:44:34Z

Regaring the other ln() issues: I'd be glad to go for those if they're similarly low-hanging fruit as this one was. I'll take a look tomorrow.

I think #829 is low-hanging.

nfischer · 2018-04-06T01:02:27Z

test/ln.js

+  const result = shell.ln('-s', 'file1', './');
+  t.truthy(shell.error());
+  t.is(result.code, 1);
+  shell.cd('..');


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.

nfischer · 2018-04-06T01:04:30Z

src/ln.js

@@ -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)) {
+    dest = path.resolve(dest, path.basename(sourcePath));


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

loilo · 2018-04-06T08:24:53Z

Added a commit which should fix #829, possibly also #830. I can't say for sure since both issues don't provide concrete examples.

This commit now allows you to use dead symlinks

as destination (overriding them with -f).
as source.

It's not covered by the mentioned issues, but I stumbled upon it while fixing those: You can create a symlink pointing to another (dead) symlink via ln -s dead-source-symlink dest-symlink.

ln() didn't do that until now, basically for the same reason as in 1. (existsSync vs lstatSync).

(This thing is kind of turning into a "general fixing of some ln() issues" branch, I hope you're okay with that. 😅)

nfischer

his thing is kind of turning into a "general fixing of some ln() issues" branch, I hope you're okay with that.

My preference is to split into multiple PRs. We squash PRs to a single commit, and it's nice to have commits focused on a single root issue.

But I won't reject the PR if it's too much trouble to split. I left feedback on the new changes regardless, it's up to you if you want to cherry pick to a new branch.

nfischer · 2018-04-06T21:28:20Z

test/ln.js

+  t.is(result.code, 0);
+  t.falsy(result.stderr);
+  t.falsy(shell.error());
+  fs.lstatSync('link-to-dead-link');


Can we turn this into a real assertion? This would be a good use of common.isBrokenLink().

nfischer · 2018-04-06T21:30:09Z

test/ln.js

@@ -161,6 +190,18 @@ test('-sf option', t => {
  });
 });

+test('Override dead destination links with -sf', t => {
+  shell.cd(t.context.tmp);


optional: you could assert the precondition (that this link is in fact a dead link). I'd be fine with t.falsy(fs.existsSync('badlink')).

nfischer · 2018-04-06T21:42:11Z

src/ln.js

@@ -47,7 +59,16 @@ function _ln(options, source, dest) {
    var isWindows = process.platform === 'win32';
    var linkType = isWindows ? 'file' : null;
    var resolvedSourcePath = isAbsolute ? sourcePath : path.resolve(process.cwd(), path.dirname(dest), source);
-    if (!fs.existsSync(resolvedSourcePath)) {
+
+    var resolvedSourceExists;


How about we move this to common.isBrokenLink(nameOfPossibleLink)? This is something I wanted for #835 anyway (and it makes more sense in common than utils). Some ideas for the behavior:

isBrokenLink(pointsToMissingFile) === true isBrokenLink(realRegularFile) // throws an exception isBrokenLink(someDirectory) // throws an exception isBrokenLink(doesNotExist) // throws an exception isBrokenLink(pointsToFileOrDirectory) === false isBrokenLink(pointsToAnotherBrokenLink) === true // `ls` highlights this just like other broken links

WDYT?

nfischer · 2018-04-06T21:47:31Z

src/ln.js

+      resolvedSourceExists = false;
+    }
+
+    if (!resolvedSourceExists) {


Hmm, was this behavior ever correct with ln -s?

$ ls this-does-not-exist ls: cannot access 'this-does-not-exist': No such file or directory $ rm bad-link $ ln -s this-does-not-exist bad-link $ ls bad-link # it's highlighted link a broken link bad-link

I think, long term, the correct logic is if (!resolvedSourceExists && !options.symlink). I wonder if this would be too big of a change (@freitagbr , what do you think?).

@loilo which bug does this address?

loilo · 2018-04-08T12:05:17Z

EDIT: Removed this comment. That wasn't supposed to be posted here, I wanted to create a Gist that helped me remember how to split a pull request — sorry.

nfischer · 2018-04-12T23:21:14Z

@loilo just checking in. I think this is basically good if we revert c57f7ff (to limit the fix to #832). You can cherry-pick that commit to a new branch and create a new PR.

loilo · 2018-04-13T05:44:28Z

I think so too, yes. Didn't have the time to handle this in the last days, I'll take a look around the weekend.

nfischer · 2018-07-12T01:27:06Z

@loilo are you interested in continuing this? Otherwise, I'll take this over and merge the fix for #832.

loilo · 2018-07-12T09:53:43Z

Oh dear, I totally forgot this. 😅
I'll try to take a look later today. 🙈

loilo · 2018-07-12T22:00:02Z

So, I looked into it and if you don't mind, I'd like to give that task back to you. I'm currently on vacation with my family and couldn't re-wrap my head around the codebase as quickly as I'd have needed to.

Otherwise I could set a reminder for after my vacation in 1-2 weeks. 🙂

nfischer · 2018-07-13T04:50:34Z

@loilo no worries, I'll just cherry pick your commits for #832. Feel free to revisit a fix for #829 when you're back.

loilo added 2 commits April 5, 2018 11:49

Allow ln destination to be an existing directory

3fe1a67

Fix coding style

4008596

nfischer requested changes Apr 5, 2018

View reviewed changes

loilo added 2 commits April 6, 2018 01:41

Implement first review feedback

50e6ca8

Remove incorrect parameter from stat.isDirectory()

582414d

Re-add the working directory cleanup in test

9392c55

It somehow caused temporary test folders not to get cleaned up correctly.

nfischer mentioned this pull request Apr 6, 2018

Robustly clean up and restore state after tests #834

Open

nfischer requested changes Apr 6, 2018

View reviewed changes

nfischer mentioned this pull request Apr 6, 2018

Improve tests for ln -s with relative source arguments #835

Open

loilo added 2 commits April 6, 2018 09:22

cd to the original CWD after each test

946ab48

Allow dead symlinks for source/destination, fixes shelljs#829

c57f7ff

nfischer requested changes Apr 6, 2018

View reviewed changes

nfischer added this to the v0.8.x milestone Jul 12, 2018

nfischer self-assigned this Jul 13, 2018

nfischer added bash compat Compatibility issues with bash or POSIX behavior fix Bug/defect, or a fix for such a problem labels Jul 13, 2018

nfischer removed this from the v0.8.x milestone Apr 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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

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

Are you sure you want to change the base?

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

Conversation

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment