WIP: follow symlink fix #14
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The use of relative paths to dirs in symlinks can cause walk() to fail.
Factor out the repeated walk() code, for the current vs children nodes, into a single helper function. Reduces the amount of redundant code in the walk() function.
Commit 3464826 added attempt to construct a path to the referent of a symlink, which was desired to make following symlinks work on Windows. However, paths can be constructed (easily done w/ multiple symlinks) that cause this approach to fail, at least for Unix. Since Stat() will traverse the paths with an ending symlink (including any intermediate symlinks in the path), manipulating the path is unneeded on Unix. For now, remove Readlink() and path construction and just rely on Stat(), going back to the original behavior. The TestWalkSymbolicRelativeLinkChain has an example directory tree that fails on Unix before this commit, which was constructed with the following shell commands: mkdir a b touch z ln -s ../b a/x ln -s ../z b/y Note that Go has an EvalSymlinks() func which is designed to return a direct pathname without the symlinks, and which could be useful for fixing the problems that originally caused this (now removed) code to be added. It has a cost, in that it does an Lstat() on every component of a path, to find the symlinks. Separating the helper func into os specific files could at least restrict it's use to Windows only.
Commit 3464826 added some symlink content reading (via Readlink()) and pathname manipulation to purportedly allow symlinks to work on Windows (but which caused problems with certain symlink containing paths on Unix). The Go EvalSymlinks() func appears to have been designed to help with this kind of situation, so call it on paths that we know end with symlinks, and may point to a directory.
The Windows symlinkDirHelper() func needs to call filepath.EvalSymlinks() due to issues with calling Stat() on symlinks (see the following issues for discussion): golang/go#19870 golang/go#19922
Remove the os-specific EvalSymlinks() call from symlinkDirHelper() and put in their own per-os build files as a wrapped func. This reduces code duplication, which will be important as additional changes are made to the symlink directory helper func.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a WIP attempt to address issue #13. It moves some of the common code in walk() into a helper func, which is then put into os specific build files. The approach it takes is to remove the code that was introduced to fix a windows problem with Stat() symlinks on Windows (but which caused problems with proper path handling on Unix). On Unix it simply does the Stat() on the symlink, and on Windows is first uses EvalSymlinks() to turn the path into one without symlinks, before Stat()ing.
This needs further testing, particularly on Windows, so if anyone could help with that and provide feedback, please let me know.
Factoring out the common code has an additional benefit of making the walk() func shorter and a bit more straightforward, although the current helper adds a little complication in having to return both an error and a bool (to choose between return and continue by the helper users).
The first commit is a test which will fail if the remaining commits are not present, so testing the failing behavior can be done by checking out just the test commit.