Allow ln destination to be an existing directory, fixes #832 by loilo · Pull Request #833 · shelljs/shelljs · GitHub

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

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.

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

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.

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

Check.

Cleanup isn't necessary

Removed that line.

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

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

Done.

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.

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

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.

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.

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.

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.

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

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. 😅)

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.

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

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')).

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?

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?

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.

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

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.

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

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

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

@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

+                const result = shell.ln('-s', 'file1', './');
+                t.truthy(shell.error());
+                t.is(result.code, 1);
+                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);

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

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

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

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

                 var isAbsolute = (path.resolve(source) === sourcePath);
                 dest = path.resolve(process.cwd(), String(dest));
+                if (fs.existsSync(dest) && common.statFollowLinks(dest).isDirectory(dest)) {

                   var resolvedSourcePath = isAbsolute ? sourcePath : path.resolve(process.cwd(), path.dirname(dest), source);
-                  if (!fs.existsSync(resolvedSourcePath)) {
+                  var resolvedSourceExists;

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

Allow ln destination to be an existing directory, fixes #832 by loilo · Pull Request #833 · shelljs/shelljs · GitHub

Uh oh!

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

Allow ln destination to be an existing directory, fixes #832#833loilo wants to merge 7 commits intoshelljs:mainshelljs/shelljs:mainfrom loilo:ln-directory-destloilo/shelljs:ln-directory-destCopy head branch name to clipboard

Conversation

loilo commented Apr 5, 2018

Uh oh!

codecov-io commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nfischer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loilo Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loilo Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfischer left a comment

Choose a reason for hiding this comment

Uh oh!

loilo commented Apr 5, 2018

Uh oh!

loilo commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nfischer commented Apr 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loilo commented Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nfischer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Allow ln destination to be an existing directory, fixes #832#833
loilo wants to merge 7 commits into
shelljs:mainfrom
loilo:ln-directory-dest

codecov-io commented Apr 5, 2018 •
edited
Loading

loilo Apr 5, 2018 •
edited
Loading

loilo Apr 5, 2018 •
edited
Loading

loilo commented Apr 5, 2018 •
edited
Loading

loilo commented Apr 6, 2018 •
edited
Loading

loilo commented Jul 12, 2018 •
edited
Loading