Improve broken symlink and other error handling #13
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
These can occur during recursion, but are not necessarily a problem of the ReadDir, so just assume such problematic files are not directories and return them. The caller can then figure out whether they are even needed at all, and an error should be returned. This allows simply ignoring a broken symlink when it would not even be used because of its extension.
There was some code that claimed to ignore symlinks, but: - It would run *after* recursive directory copying, so any symlinks to directories would still be copied (potentially creating a destination directory that is a lot bigger than the original) - It checked for the symlink bit in the result of Stat(), but that resolves the symlink and returns the mode bits of the target, so that code would never fire. This fixes both problems by moving the check for symlinks up, and using Lstat() instead of Stat(), so symlinks are ignored as intended.
Broken symlinks would previously be treated as directories and filtered out, due to the use of IsNotDir (which defaults to false on Stat errors), but it makes more sense to only filter out elements that are certainly directories, so use IsDir (which also defaults to false on Stat errors) and negate its result.
|
|
Codecov Report
@@ Coverage Diff @@
## master #13 +/- ##
==========================================
+ Coverage 66.66% 67.59% +0.92%
==========================================
Files 5 5
Lines 285 287 +2
==========================================
+ Hits 190 194 +4
+ Misses 64 63 -1
+ Partials 31 30 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
5 tasks
per1234
added
type: bug
6 tasks
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 PR makes a few changes that came up when working on (a replacement PR for) arduino/arduino-cli#1201, but which probably are a good idea by themselves. This PR contains four commits:
Stat()errors, but instead just add such problematic files to the returned list and let the caller decide whether these files are really needed at all and an error is appropriate. I needed this for broken symlinks (which return DoesNotExist), but it also seemed useful to ignore permission errors, and let the caller decide.Lstat()method, needed for the next commit.CopyDirTo()really ignore symlinks, as was seemingly intended based on a comment in the code. I noticed this problem when adding a broken symlink to the testdata (needed to test the next commit), and then discovered that a broken symlink madeCopyDirTo()fail, and a correct symlink was actually duplicated, potentially leading to a lot more diskspace. Now symlinks are ignored entirely, at least until properly copying them is implemented.FilterOutDirsincorrectly marking files with stat errors (such as broken symlinks) as directories and filtering them out. I needed this because commit 1. ensures that broken symlinks end up in theReadDirRecursive()output, but inside arduino-cliFilterOutDirs()would again throw them away, IMHO inappropriately.Note also arduino/arduino-cli#1353 (comment), which makes an additional suggestion on improving the
ReadDirRecursiveAPI (adding a callback to control pruning the recursion). If implemented, this might remove the need for commit 1., especially if the callback could also control whether an error is returned on a failed stat call.