Fix symlink loop detection to allow shared symlink targets#299
Open
psss wants to merge 2 commits into
Open
Conversation
The previous implementation used a single shared list of visited symlink targets across the entire tree. This caused sibling symlinks pointing to the same directory to be incorrectly skipped as loops, because the second symlink found its target already recorded by the first one. Replace the shared list with a per-branch set of ancestor real paths. Each child node gets its own copy of the set, so sibling branches cannot interfere with each other. A symlink is now only skipped when its target resolves to an ancestor directory of the current walk path, which is the actual condition for a cycle. Add tests for both the shared symlink target scenario and for the symlink loop detection to prevent future regressions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
@petr-janda, this should fix the problem with directory symlinks. |
LecrisUT
reviewed
May 13, 2026
Comment on lines
+586
to
+587
| common = os.path.join(directory, 'common') | ||
| os.mkdir(common) |
| def test_symlink_shared_target(self): | ||
| """ | ||
| Multiple symlinks pointing to the same directory should all be followed | ||
| """ |
Member
There was a problem hiding this comment.
Could you embed the output of tree in here. If I follow this correctly it is:
$ tree -al .
.
├── common
│ └── main.fmf
├── .fmf
│ └── version
├── one
│ ├── main.fmf
│ └── plan -> ../common [recursive, not followed]
└── two
├── main.fmf
└── plan -> ../common [recursive, not followed]
Yes it does. With this patch I can symlink a directory with plans to multiple directories. Together with inheritance it eases my life. Thank you. |
Member
Author
|
Great, thanks for verifying! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The previous implementation used a single shared list of visited symlink targets across the entire tree. This caused sibling symlinks pointing to the same directory to be incorrectly skipped as loops, because the second symlink found its target already recorded by the first one.
Replace the shared list with a per-branch set of ancestor real paths. Each child node gets its own copy of the set, so sibling branches cannot interfere with each other. A symlink is now only skipped when its target resolves to an ancestor directory of the current walk path, which is the actual condition for a cycle.
Add tests for both the shared symlink target scenario and for the symlink loop detection to prevent future regressions.
Assisted-by: Claude Opus 4.6
Checklist