Skip to content

Compute depth for broken symlinks so --min-depth keeps them#2039

Open
hexbinoct wants to merge 3 commits into
sharkdp:masterfrom
hexbinoct:fix/min-depth-broken-symlink
Open

Compute depth for broken symlinks so --min-depth keeps them#2039
hexbinoct wants to merge 3 commits into
sharkdp:masterfrom
hexbinoct:fix/min-depth-broken-symlink

Conversation

@hexbinoct

Copy link
Copy Markdown

Fixes #1017.

Problem

When following links, a broken symlink is reported by the walker as an error instead of a normal entry, so fd wraps it in DirEntry::broken_symlink. That variant carried no depth, and DirEntry::depth() returned None for it. The --min-depth filter in walk.rs treats a missing depth as below the minimum, so a broken symlink was filtered out for any --min-depth value, including --min-depth 0. The original report shows it:

fd link foo -L                 # prints foo/bar/baz/link
fd link foo -L --min-depth=1   # prints nothing (bug)
fd link foo -L --min-depth=0   # prints nothing (bug)

Fix

I record the depth when the broken symlink entry is created. Following the suggestion in the issue thread, the depth is the number of path components below the search root, so I count the components of the path and subtract the components of the matching root (the longest search root that the path starts with). DirEntry::depth() now returns that value, so broken symlinks are filtered by depth like every other entry. If no root matches, it falls back to the full component count, which keeps the entry visible rather than silently dropping it.

Tests

I added test_min_depth_broken_symlink: a broken symlink at depth 3 is kept by --min-depth 3 and excluded by --min-depth 4. I checked that it fails on the current code and passes with the fix. The full cargo test suite passes, and clippy and rustfmt are clean. Verified locally on Windows. I also added a CHANGELOG entry.

Note on tooling

I used an AI assistant (Claude) to help write this change. I have reviewed it, I understand how it works, and I am happy to answer questions or make changes.

When following links, a broken symlink is surfaced by the walker as an
error that carries no depth, so fd stored it with an unknown depth and
the --min-depth filter dropped it for any minimum value. Record the
depth when the entry is created by counting the path components relative
to the matching search root, which is what the issue discussion
suggested. Broken symlinks are now filtered by depth like any other
entry.

Fixes sharkdp#1017

@tmccombs tmccombs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this doesn't work properly if the --absolute-path option is used. Probably because we are comparing the absolute path to a relative path, and not getting a match.

In particular, my test case looked like:

❯ tree
.
├── a
│   ├── b
│   │   ├── c
│   │   │   ├── d
│   │   │   │   └── foo -> /noexist
│   │   │   └── foo -> /noexist
│   │   ├── foo -> /noexist
│   │   └── two -> /tmp/tmp.od15gRvlEL/a/two
│   ├── foo -> /noexist
│   └── two
│       └── c
│           └── foo -> /noexist
├── foof -> /noexist
└── l -> a/b

9 directories, 6 files
❯ fd -L foo l -a
/tmp/tmp.od15gRvlEL/a/b/c/d/foo
/tmp/tmp.od15gRvlEL/a/b/c/foo
/tmp/tmp.od15gRvlEL/a/b/foo
/tmp/tmp.od15gRvlEL/a/b/two/c/foo

Notice that /tmp/tmp.od15gRvlEL/a/b/foo is printed even though it's relative path would be l/foo which isn't deep enough for the --min-path condition.

Comment thread src/dir_entry.rs Outdated
BrokenSymlink(PathBuf),
// Broken symlinks are surfaced by the walker as errors that carry no depth,
// so we record the depth (relative to the search root) at creation time.
BrokenSymlink(PathBuf, usize),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be best to change this to be a struct-like variant, to make it more clear what the usize value is.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. It is now a struct-like variant with named path and depth fields, so the recorded depth is clear at every match and construction site.

Comment thread src/walk.rs Outdated
Comment on lines +673 to +674
/// fall back to the full component count, which keeps the entry visible rather
/// than silently dropping it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will show it for --min-depth, but will probably hide it if --max-depth or --exact-depth is used. Which, I'm not sure what we should do in that case.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this and it actually behaves the same as a normal entry. --max-depth (and the upper bound of --exact-depth) is enforced by the walker itself through its own internal depth, and --min-depth (and the lower bound of --exact-depth) is the only one applied here using the computed depth. So broken symlinks are kept and dropped at the same depths as real files for all three flags.

To confirm, I placed a real file next to each broken symlink and verified that --min-depth, --max-depth and --exact-depth select the same set for both, in relative and --absolute-path modes, for depths 0 to 4. I also added regression tests test_max_depth_broken_symlink and test_exact_depth_broken_symlink that exercise both the real directory route and the followed symlink route.

Comment thread src/walk.rs Outdated
/// than silently dropping it.
fn depth_relative_to_roots(path: &Path, roots: &[PathBuf]) -> usize {
let components = path.components().count();
let root_components = roots

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we would only compare to the root we are currently searching through, but that would probably require making a PR in the ignore crate (which might be worth doing) to expose the depth there.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that the fully correct source of truth is the depth the walker already tracks, and exposing it in the ignore crate would be the clean fix. For now I pick the deepest matching root, which gives the correct result for nested search roots. If you would like, I can open an issue or PR on ignore to expose the depth on these errors, and then switch this over once that lands. Happy to go that route if you prefer it over the path based computation here.

The depth for a broken symlink was derived by stripping the search root
from its path and counting the remaining components. Compare the path
and the roots in absolute form before stripping, so the match holds
whether or not --absolute-path has already made the roots absolute, and
pick the deepest matching root. This avoids an absolute path failing to
match a relative root and falling back to an inflated depth.

Add a regression test that exercises the broken symlink case together
with --absolute-path.
@hexbinoct

Copy link
Copy Markdown
Author

Thanks for catching this. You're right that comparing an absolute entry path against a relative root would miss, so I changed the depth computation to put both the path and the roots into absolute form before stripping, and to pick the deepest matching root. That removes the relative vs absolute mismatch.

I also tried to reproduce the original symptom on the current code, to be sure I was fixing the right thing. With your tree the depths come out correct even before the change, because --absolute-path runs the root through normpath::normalize (which is canonicalize on Unix) before the walk, so the root and the entry paths are both absolute and share a prefix:

$ fd -L foo l -a --min-depth 2
/tmp/.../a/b/c/d/foo
/tmp/.../a/b/c/foo
/tmp/.../a/b/two/c/foo      # a/b/foo at depth 1 is correctly dropped

To double check, I put a real file next to each broken symlink and confirmed --min-depth 0..4 and --exact-depth select the same set for both, in relative and -a modes. The new version keeps that property and also handles the case where a root could still be relative while the path is absolute.

If you have a setup where the root stays relative under -a, could you share the exact command and tree? I'd like to pin it down with a test. I added one for --absolute-path with --min-depth in the meantime.

Address review feedback. Make DirEntryInner::BrokenSymlink a struct-like
variant with named path and depth fields, so the recorded depth is clear
at every use site. Add regression tests that a broken symlink is filtered
by --max-depth and --exact-depth the same way a normal entry is, both
through the real directory tree and through a followed symlink.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--min-depth is broken for broken symlinks

2 participants