Fix gitignore patterns with ** glob being silently dropped#5
Fix gitignore patterns with ** glob being silently dropped#5avarayr wants to merge 1 commit intoglommer:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where gitignore patterns containing ** glob syntax (like **/Pods/ and **/vendor/) were being silently ignored. The fix extracts directory names from **/name patterns and adds them to the ignore set, while intentionally skipping more complex glob patterns that aren't supported by the simplified directory-name-only matching approach.
Changes:
- Enhanced
loadIgnorePatternsfunction to handle**/nameand**/name/patterns by extracting the directory name - Added comprehensive test suite for gitignore pattern handling in walker functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/scan/walker.ts | Modified loadIgnorePatterns to extract directory names from **/name patterns using regex, while still skipping other glob patterns |
| test/walker.test.ts | Added new test file with 5 test cases covering plain patterns, **/name patterns with/without trailing slashes, and verification that unsupported glob patterns are skipped |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test("*.ext glob patterns are still skipped", async () => { | ||
| const tmp = setupFixture("*.log\n", [], { "app.ts": "export const x = 1", "debug.log": "some log" }) | ||
| try { | ||
| const { files } = await scanDirectory(tmp) | ||
| const names = files.map(f => f.relPath) | ||
| expect(names).toContain("app.ts") | ||
| } finally { | ||
| cleanupFixture() | ||
| } | ||
| }) |
There was a problem hiding this comment.
The test description says "*.ext glob patterns are still skipped" but doesn't verify the actual behavior of debug.log. The test should explicitly check that debug.log is included in the results (not filtered out), since *.log patterns are intentionally not processed. Consider adding: expect(names).toContain("debug.log") to make the test's intent clearer and verify the expected behavior.
| test("**/name/ patterns are ignored", async () => { | ||
| const tmp = setupFixture("**/Pods/\n**/vendor/\n", ["Pods", "vendor"], { "Pods/lib.ts": "export const a = 1", "vendor/dep.ts": "export const b = 2", "app.ts": "export const c = 3" }) | ||
| try { | ||
| const { files } = await scanDirectory(tmp) | ||
| const names = files.map(f => f.relPath) | ||
| expect(names).toContain("app.ts") | ||
| expect(names).not.toContain("Pods/lib.ts") | ||
| expect(names).not.toContain("vendor/dep.ts") | ||
| } finally { | ||
| cleanupFixture() | ||
| } | ||
| }) |
There was a problem hiding this comment.
Consider adding a test case for nested directories to verify that **/Pods/ correctly ignores directories like ios/Pods or app/vendor, not just root-level directories. This would better validate that the pattern matching works at any directory depth, which is the expected behavior of ** patterns in gitignore.
Summary
loadIgnorePatternswas skipping any line containing*, so**/name/patterns were silently ignored**/namepatterns and add to the ignore set