Ignore symlinked directories in source ignores#20091
Conversation
Confidence Score: 5/5The change is narrowly scoped: it adds a thin wrapper around dunce::canonicalize that only diverges for negated (@source not) entries, leaving non-negated source resolution unchanged. Both callsites are straightforward. The lexical normalizer is simple and well-documented (including the known limitation with relative .. paths, which cannot reach these callsites in practice). The guard condition switch from resolved_path.is_dir() to combined_path.is_dir() is safe because Rust's Path::is_dir() already follows symlinks, so non-negated paths produce the same answer as before. The new test exercises the exact failure mode described in the linked issue and also covers the case of ignoring the real directory alongside the symlink. No files require special attention. Reviews (4): Last reviewed commit: "test(oxide): document lexical path norma..." | Re-trigger Greptile |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReplace later dunce::canonicalize calls in PublicSourceEntry::optimize with a new resolve_path(path, preserve_symlinks) helper that either canonicalizes or, when preserving symlinks, validates via metadata and returns a lexically normalized path (removing 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/oxide/tests/scanner.rs (1)
1773-1780: ⚡ Quick winIsolate the symlink regression in its own assertion.
This setup proves the combined
./symlink+./directory_aignore case, but not the symlink-path behavior by itself. Splitting this into a symlink-only scenario would make the regression coverage much sharper and easier to debug if it breaks again.Suggested test shape
- let mut scanner = Scanner::new(vec![ - public_source_entry_from_pattern(dir.clone(), "`@source` './'"), - public_source_entry_from_pattern(dir.clone(), "`@source` not './symlink'"), - public_source_entry_from_pattern(dir.clone(), "`@source` not './directory_a'"), - ]); - let candidates = scanner.scan(); - - assert!(candidates.is_empty()); + let ScanResult { files, .. } = scan_with_globs( + &[("directory_a/a.html", "content-['directory_a/a.html']")], + vec!["`@source` './'", "`@source` not './symlink'"], + ); + assert_eq!(files, vec!["directory_a/a.html"]); + + let mut scanner = Scanner::new(vec![ + public_source_entry_from_pattern(dir.clone(), "`@source` './'"), + public_source_entry_from_pattern(dir.clone(), "`@source` not './symlink'"), + public_source_entry_from_pattern(dir.clone(), "`@source` not './directory_a'"), + ]); + assert!(scanner.scan().is_empty());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/oxide/tests/scanner.rs` around lines 1773 - 1780, The current test combines ignores for "./symlink" and "./directory_a" which masks whether the symlink handling is broken; create a separate assertion that tests the symlink-only case by constructing a Scanner with only public_source_entry_from_pattern(dir.clone(), "`@source` not './symlink'"), call Scanner::new(...).scan(), and assert that candidates.is_empty() (or the expected result) for that symlink-only input before (or after) the existing combined test; update the test to keep the original combined case too so both symlink-only and combined-ignore behavior are covered (referencing Scanner::new, public_source_entry_from_pattern, scan, and candidates in your changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/oxide/tests/scanner.rs`:
- Around line 1773-1780: The current test combines ignores for "./symlink" and
"./directory_a" which masks whether the symlink handling is broken; create a
separate assertion that tests the symlink-only case by constructing a Scanner
with only public_source_entry_from_pattern(dir.clone(), "`@source` not
'./symlink'"), call Scanner::new(...).scan(), and assert that
candidates.is_empty() (or the expected result) for that symlink-only input
before (or after) the existing combined test; update the test to keep the
original combined case too so both symlink-only and combined-ignore behavior are
covered (referencing Scanner::new, public_source_entry_from_pattern, scan, and
candidates in your changes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d82a1133-5b16-438e-8f4a-fe61e10e5a88
📒 Files selected for processing (2)
crates/oxide/src/scanner/sources.rscrates/oxide/tests/scanner.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/oxide/tests/scanner.rs`:
- Around line 1773-1790: The test currently assumes a symlink was created but
ignores creation errors; ensure the test fails if symlink setup fails by
verifying the symlink immediately after setup and panicking on failure. After
creating the symlink (the same path referenced by dir and the
public_source_entry_from_pattern patterns) add an explicit check using symlink
metadata (e.g., std::fs::symlink_metadata) and assert that the metadata exists
and file_type().is_symlink() (or unwrap the Result from the symlink creation
helper) before constructing Scanner::new and calling scanner.scan(); this makes
the test reliably fail when symlink creation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 546f0c1d-1e26-48bf-acad-b5344e772c9e
📒 Files selected for processing (1)
crates/oxide/tests/scanner.rs
yudin-s
force-pushed
the
fix/source-not-symlink
branch
from
4de744d to
e344530
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/oxide/src/scanner/sources.rs (1)
183-197: 💤 Low valueDocument the lexical-only contract of
normalize_path_lexically.This helper is correct for the current call sites because
self.baseis canonicalized first (Line 105), so every path reaching here is absolute. However, the routine has two non-obvious behaviors worth pinning down with a doc comment so it isn't reused later with surprising results:
- It is purely lexical, so for inputs containing
..segments after a symlinked component (e.g../symlink_to_x/..), the result diverges from the physical resolution — by design, since we're preserving symlinks, but worth stating.- On a relative input, a leading
..is silently dropped becausePathBuf::pop()on an empty buffer is a no-op, so../foonormalizes tofoo. Safe today (all callers pass absolute paths) but a footgun for future reuse.📝 Suggested doc comment
+/// Lexically normalizes `path` by dropping `.` components and collapsing `..` +/// segments via `PathBuf::pop`, without touching the filesystem. +/// +/// Intended for absolute inputs only: a leading `..` on a relative path will +/// be silently discarded because `pop()` on an empty buffer is a no-op. +/// Because this is lexical, results diverge from the physical resolution when +/// `..` follows a symlinked component — that divergence is intentional when +/// callers want to preserve symlink path shapes. fn normalize_path_lexically(path: &Path) -> PathBuf {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/oxide/src/scanner/sources.rs` around lines 183 - 197, Add a doc comment to the normalize_path_lexically function that states it performs purely lexical normalization (preserves symlinks so sequences like ./symlink_to_x/.. will not resolve to the symlink target) and that on relative inputs leading `..` segments are dropped because PathBuf::pop is a no-op on an empty buffer (e.g., `../foo` becomes `foo`); also note the current callers pass absolute, canonicalized paths (see usage around self.base canonicalization) so this behavior is safe, and warn future callers to canonicalize or avoid relying on physical resolution if they need symlink-aware semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/oxide/src/scanner/sources.rs`:
- Around line 183-197: Add a doc comment to the normalize_path_lexically
function that states it performs purely lexical normalization (preserves
symlinks so sequences like ./symlink_to_x/.. will not resolve to the symlink
target) and that on relative inputs leading `..` segments are dropped because
PathBuf::pop is a no-op on an empty buffer (e.g., `../foo` becomes `foo`); also
note the current callers pass absolute, canonicalized paths (see usage around
self.base canonicalization) so this behavior is safe, and warn future callers to
canonicalize or avoid relying on physical resolution if they need symlink-aware
semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4393c434-b5eb-42a3-b2b2-e983dc1f2f92
📒 Files selected for processing (2)
crates/oxide/src/scanner/sources.rscrates/oxide/tests/scanner.rs
1 participant
Summary
Preserves symlink paths when optimizing negated
@source notentries, so ignores are matched against the same path shape the scanner sees while following symlinks.This fixes cases where
@source not "./symlink"was optimized to the symlink target path and therefore failed to ignore files discovered through the symlink.Fixes #17985
Test plan
cargo fmt --checkcargo test -p tailwindcss-oxide test_source_not_can_ignore_symlinked_directory --test scannercargo test -p tailwindcss-oxide --test scanner