fix(ignore): respect negation patterns in .gitnexusignore#654
Conversation
…ored
childrenIgnored checked `ig.ignores(rel) || ig.ignores(rel + '/')` which
short-circuited on the bare path — directory-only negation patterns like
`!iOS/` were missed because `ig.ignores('iOS')` treats the path as a file.
Now only checks with trailing slash since childrenIgnored is only called
for directories. Bare-name patterns (e.g. `local`) still match per gitignore spec.
Fixes #596
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@ivkond is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 5438 tests passed 92 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
xkonjin
left a comment
There was a problem hiding this comment.
Code Review: Fix negation patterns in .gitnexusignore
Summary
Clean fix for issue #596. The change correctly addresses the directory-only negation pattern handling by ensuring paths are tested with trailing slashes in .
Observations
1. Correctness
The logic change from to just is correct for the directory-only context. The gitignore spec does treat bare patterns as matching both files and directories, and the trailing slash forces directory-only matching.
2. Performance consideration
The original dual-check was slightly safer for mixed file/dir patterns but unnecessary overhead here. Since is only called for directories, the simplified single-check is appropriate.
3. Test coverage
Excellent test additions covering:
- Whitelist patterns ( + )
- Nested whitelists ()
- File-level negation under whitelisted directories
Suggestion (non-blocking)**
Consider adding a test case for edge case: without trailing slash in the negation pattern itself, to ensure the ignore library normalizes these consistently.
Approval
Approved. Fixes the reported bug with clear tests and minimal, targeted changes.
xkonjin
left a comment
There was a problem hiding this comment.
Code Review: Fix negation patterns in .gitnexusignore
Summary
Clean fix for issue #596. The change correctly addresses the directory-only negation pattern handling by ensuring paths are tested with trailing slashes in childrenIgnored.
Observations
1. Correctness
The logic change from ig.ignores(rel) || ig.ignores(rel + '/') to just ig.ignores(rel + '/') is correct for the directory-only context. The gitignore spec does treat bare patterns as matching both files and directories, and the trailing slash forces directory-only matching.
2. Performance consideration
The original dual-check was slightly safer for mixed file/dir patterns but unnecessary overhead here. Since childrenIgnored is only called for directories, the simplified single-check is appropriate.
3. Test coverage
Excellent test additions covering:
- Whitelist patterns (
*+!iOS/) - Nested whitelists (
!backend/living_plan/**) - File-level negation under whitelisted directories
Suggestion (non-blocking)**
Consider adding a test case for edge case: !dir without trailing slash in the negation pattern itself, to ensure the ignore library normalizes these consistently.
Approval
Approved. Fixes the reported bug with clear tests and minimal, targeted changes.
Verifies that `!iOS` (without trailing slash) also un-ignores the iOS/ directory — confirms the `ignore` package normalizes both `!dir` and `!dir/` forms consistently when tested with a trailing-slash path. Addresses non-blocking review suggestion on #654. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
xkonjin
left a comment
There was a problem hiding this comment.
Code Review: Negation Pattern Fix
Summary
Clean, minimal fix for directory-only negation patterns in .gitnexusignore. Addresses issue #596.
Correctness
The logic change from dual-check (ig.ignores(rel) || ig.ignores(rel + /)) to single-check (ig.ignores(rel + /)) is correct for the directory-only context. Since childrenIgnored is only invoked for directories, testing with trailing slash ensures negation patterns like !iOS/ are properly respected.
Test Coverage
Excellent test additions:
- Whitelist patterns (
*+!iOS/) - Nested whitelists (
!backend/living_plan/**) - File-level negation under whitelisted directories
- Bare negation without trailing slash (
!iOSvs!iOS/)
The tests directly validate the reported bug scenario and edge cases.
Suggestions (non-blocking)
-
Performance consideration: The comment notes the dual-check was removed for performance. Consider quantifying this — is the overhead measurable in large repos? The clarity of correctness may outweigh micro-optimizations.
-
Edge case documentation: The test for bare negation
!iOSrelies on theignorelibrary normalizing patterns. A brief inline comment linking to the ignore package docs would help future maintainers.
Nit
Line 404 comment has a trailing space after "applied correctly —".
Verdict
Approved. Fixes the reported bug with clear tests and minimal, targeted changes.
Adds references to the `ignore` package documentation in both the childrenIgnored comment and the bare-negation test, explaining why `!iOS` (without trailing slash) also re-includes the iOS/ directory. Addresses non-blocking review suggestion on #654. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review! Addressed the suggestions in 17560ec: #2 (edge case documentation) — applied. Added inline links to the
#1 (trailing space nit) — not reproduced. Checked the committed line with #3 (performance justification) — already reflects the semantic reasoning. The code comment reads "Since childrenIgnored is only called for directories, always test with a trailing slash" — the dual-check was removed because it's semantically redundant in directory-only context, not for perf. No micro-benchmark needed. |
|
@ivkond Thank you for your contribution! |
…twari#654) * fix(ignore): respect negation patterns in .gitnexusignore childrenIgnored childrenIgnored checked `ig.ignores(rel) || ig.ignores(rel + '/')` which short-circuited on the bare path — directory-only negation patterns like `!iOS/` were missed because `ig.ignores('iOS')` treats the path as a file. Now only checks with trailing slash since childrenIgnored is only called for directories. Bare-name patterns (e.g. `local`) still match per gitignore spec. Fixes abhigyanpatwari#596 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(ignore): add edge-case for bare `!dir` negation pattern Verifies that `!iOS` (without trailing slash) also un-ignores the iOS/ directory — confirms the `ignore` package normalizes both `!dir` and `!dir/` forms consistently when tested with a trailing-slash path. Addresses non-blocking review suggestion on abhigyanpatwari#654. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs(ignore): link ignore package docs for bare-name normalization Adds references to the `ignore` package documentation in both the childrenIgnored comment and the bare-negation test, explaining why `!iOS` (without trailing slash) also re-includes the iOS/ directory. Addresses non-blocking review suggestion on abhigyanpatwari#654. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
childrenIgnored()inignore-service.tscheckedig.ignores(rel) || ig.ignores(rel + '/')— the bare-path check short-circuited ontruebecause directory-only negation patterns (e.g.!iOS/) don't apply to paths without a trailing slash.gitnexusignore(*→!iOS/→!iOS/**), ALL directories were pruned during glob traversal, producing 0 nodesig.ignores(rel + '/')sincechildrenIgnoredis exclusively called for directories. Bare-name patterns (e.g.local) still matchlocal/per gitignore specFixes #596
Related to #231 (PR that introduced
.gitnexusignoresupport)Test plan
ignored()respects negation for files under whitelisted dirs.gitnexusignoreusing*+!src/+!src/**pattern and rungitnexus analyze🤖 Generated with Claude Code