feat(scanner): add Scanner.FollowSymlinks option #4061
Merged
Conversation
This file contains hidden or 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
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 4c93d48 in 1 minute and 1 seconds. Click for details.
- Reviewed
309lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. scanner/walk_dir_tree.go:114
- Draft comment:
Remove extra whitespace in the structured log key (' path' → 'path') for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. scanner/walk_dir_tree.go:285
- Draft comment:
Pass the context to log.Warn in isDirReadable for consistent structured logging. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_0e2bBxRK70OhwRQx
You can customize 
… methods Signed-off-by: Deluan <deluan@navidrome.org>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2bff15a in 1 minute and 58 seconds. Click for details.
- Reviewed
89lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. scanner/walk_dir_tree_test.go:38
- Draft comment:
The test now uses the Data field in MapFS to define the symlink target instead of a separate symlinks map. This simplifies the mock but relies on m.FS being a fstest.MapFS. Consider adding a comment or type‐check to clarify this assumption. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment identifies a real potential issue - the code assumes m.FS is a MapFS but this isn't obvious. However, this is a test file and the mock is only used for testing. The code already has a comment explaining what mockMusicFS is for. The suggested inline comment would make the code more cluttered without adding much value. The comment raises a valid point about making assumptions explicit. Test code should still be maintainable and clear. While documentation is good, the existing type comment on mockMusicFS already explains its purpose. An additional inline comment would be redundant. Delete the comment. The code's purpose is already documented by the type comment, and adding more inline comments would add clutter without significant benefit.
2. scanner/walk_dir_tree_test.go:300
- Draft comment:
The Open method in mockMusicFS now always follows symlinks unconditionally. Ensure that the higher-level scanning logic (e.g. isDirOrSymlinkToDir) correctly respects the FollowSymlinks configuration so that in disabled mode these symlinks are skipped. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the author to ensure that the higher-level logic respects a configuration setting. It is not making a specific code suggestion or pointing out a specific issue with the code. It violates the rule against asking the author to ensure behavior is intended.
3. scanner/walk_dir_tree_test.go:293
- Draft comment:
Removal of the symlinks map in mockMusicFS cleans up the implementation. Verify that all tests (and production code, if applicable) now rely solely on the Data field for symlink targets, ensuring consistent behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_JttcPHSpTr2ILISE
You can customize
…cleanup Signed-off-by: Deluan <deluan@navidrome.org>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 600d1ad in 1 minute and 28 seconds. Click for details.
- Reviewed
253lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. scanner/walk_dir_tree_test.go:55
- Draft comment:
Clarify that the helper getFolders intentionally includes the '.' and 'root' entries (hence the +2 in the length check). A short comment would help future maintainers understand this. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment points out something that could be confusing - why there's a +2 in the length check. However, there's already a comment on line 76 that explains this: "// +2 for.androot". Adding another comment in getFolders() would be redundant. The existing comment is in a more appropriate place - right next to where the +2 is actually used. Maybe having the explanation in both places would make the code more maintainable since someone reading getFolders() might miss the comment in the test case? The explanation is already clearly documented where it matters - at the point where the +2 is used. Adding redundant documentation would violate DRY principles and could lead to maintenance issues if one comment is updated but not the other. The comment should be deleted since the behavior is already documented appropriately where the +2 is used.
2. scanner/walk_dir_tree_test.go:94
- Draft comment:
Consider defining repeated magic strings like 'root/e/symlink' as constants for better maintainability, especially since file names are used in multiple places. - Reason this comment was not posted:
Comment was on unchanged code.
3. scanner/walk_dir_tree_test.go:71
- Draft comment:
The table-driven tests are clear, but consider adding additional cases (e.g. for broken or circular symlinks) to further strengthen the test coverage of the new FollowSymlinks option. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_UrMhkqGWAbKHHcsC
You can customize
3 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.
Fixes #4060
This PR adds a new configuration option
Scanner.FollowSymlinks(default: true) to control whether the filesystem scanner should follow symbolic links when scanning directories.Changes:
Scanner.FollowSymlinks(default: true)isDirOrSymlinkToDirto respect this optionDescribeTablefor better maintainabilityThe option can be configured in
navidrome.toml:Or via environment variable: