Add a linter for symbolic links.
AbandonedPublic
Actions

Authored by joshuaspence on Jun 9 2014, 10:52 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T5300: Don't lint symbolic links

Summary

Fixes T5300. Adds an ArcanistSymlinkLinter which checks symbolic links.

Test Plan

Created two symbolic links for testing:

>>> Lint for broken:


   Error  (SYMLINK1) Broken Symlink
    Broken symlinks should not exist.
>>> Lint for external:


   Advice  (SYMLINK2) External Symlink
    Symlinks generally should not point outside of the working copy, for
    portability reasons.

Diff Detail

Repository

rARC Arcanist

Branch

symlink-linter

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 946
Build 946: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence updated this revision to Diff 22545.Jun 9 2014, 10:52 PM

joshuaspence retitled this revision from to Add a linter for symbolic links..

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

joshuaspence added a task: T5300: Don't lint symbolic links.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJun 9 2014, 10:52 PM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

joshuaspence edited the test plan for this revision. (Show Details)Jun 9 2014, 10:52 PM

joshuaspence edited edge metadata.

Harbormaster completed remote builds in B946: Diff 22545.Jun 9 2014, 10:53 PM

joshuaspence added inline comments.Jun 9 2014, 10:55 PM

src/lint/linter/ArcanistSymlinkLinter.php
16	This could be more descriptive...
28	This linter needs to run before other all other linters... otherwise: Exception Some linters failed: - ArcanistGeneratedLinter: FilesystemException: Requested path `/home/joshua/workspace/github.com/phacility/arcanist/broken' is not a file. - ArcanistNoLintLinter: FilesystemException: Requested path `/home/joshua/workspace/github.com/phacility/arcanist/broken' is not a file. - ArcanistTextLinter: FilesystemException: Requested path `/home/joshua/workspace/github.com/phacility/arcanist/broken' is not a file. - ArcanistSpellingLinter: FilesystemException: Requested path `/home/joshua/workspace/github.com/phacility/arcanist/broken' is not a file. - ArcanistMergeConflictLinter: FilesystemException: Requested path `/home/joshua/workspace/github.com/phacility/arcanist/broken' is not a file.
54	I am not sure how portable this is... i.e. will this work on Windows?
59	As above.

Per T5300, the cases I can imagine with this are:

External relative symlinks, like a link in arcanist/ pointed at ../libphutil/src/whatever.php. We don't have any of these in Phabricator (and allow the libraries to be non-adjacent), and this sort of usage is suspicious, but it's more reasonable than putting symlinks to random absolute external files in the working copy.
In SVN, maybe external relative symlinks leading upwards to nothing present in subtrees?
Internal symlinks to externals/submodules which haven't been updated.

I don't know how often this stuff will actually happen, but I think broken symlinks are also rare.

Maybe a more practical attack on this is to add shouldLintSymlinks() and default it to false. Let the FilenameLinter run on symlinks, and then this one can exist in some slightly reduced form without needing all the stopAllLinters() stuff and we can see how bad the false positives are?

src/lint/linter/ArcanistSymlinkLinter.php
31–37	Maybe we should add `shouldLintSymlinks()` to this set of special cases.

Yeah, okay. I'm happy with that.

joshuaspence abandoned this revision.Jun 9 2014, 11:24 PM

epriestley added inline comments.Jun 9 2014, 11:24 PM

src/lint/linter/ArcanistSymlinkLinter.php
54	I think this is portable, but see below.
59	`Filesystem::pathExists()` + `readlink($path)` might be a better test. `$path` (which will be relative?) should theoretically be `$this->getEngine()->getPathOnDisk($path)` (which will produce the correct absolute path?) or something, too, I believe.