From 72fa01f62d01e6cf79b78fc9fed6d042386ace21 Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Sun, 8 Jun 2025 18:32:19 -0700 Subject: [PATCH] feat(git): Refactor gitignore handling to use `ignore` library instead of `minimatch` (#864) --- package-lock.json | 28 +-- packages/core/package.json | 2 +- .../src/services/fileDiscoveryService.test.ts | 27 --- .../core/src/services/fileDiscoveryService.ts | 9 - .../core/src/utils/gitIgnoreParser.test.ts | 160 +++++------------- packages/core/src/utils/gitIgnoreParser.ts | 77 +++------ 6 files changed, 65 insertions(+), 238 deletions(-) diff --git a/package-lock.json b/package-lock.json index 8af3f4dd..b0b73310 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3100,6 +3100,7 @@ "version": "1.0.2", "resolved": "https://registry.npmjs.org/balanced-match/-/balanced-match-1.0.2.tgz", "integrity": "sha512-3oSeUO0TMV67hN1AmbXsK4yaqU7tjiHlbxRDZOpH0KW9+CeX4bRAaX0Anxt0tx2MrpRpWwQaPwIlISEJhYU5Pw==", + "dev": true, "license": "MIT" }, "node_modules/base64-js": { @@ -10408,7 +10409,7 @@ "diff": "^7.0.0", "dotenv": "^16.4.7", "fast-glob": "^3.3.3", - "minimatch": "^10.0.0", + "ignore": "^7.0.0", "shell-quote": "^1.8.2", "strip-ansi": "^7.1.0" }, @@ -10423,28 +10424,13 @@ "node": ">=18" } }, - "packages/core/node_modules/brace-expansion": { - "version": "2.0.1", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.0.1.tgz", - "integrity": "sha512-XnAIvQ8eM+kC6aULx6wuQiwVsnzsi9d3WxzV3FpWTGA19F621kwdbsAcFKXgKUHZWsy+mY6iL1sHTxWEFCytDA==", + "packages/core/node_modules/ignore": { + "version": "7.0.5", + "resolved": "https://registry.npmjs.org/ignore/-/ignore-7.0.5.tgz", + "integrity": "sha512-Hs59xBNfUIunMFgWAbGX5cq6893IbWg4KnrjbYwX3tx0ztorVgTDA6B2sxf8ejHJ4wz8BqGUMYlnzNBer5NvGg==", "license": "MIT", - "dependencies": { - "balanced-match": "^1.0.0" - } - }, - "packages/core/node_modules/minimatch": { - "version": "10.0.1", - "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-10.0.1.tgz", - "integrity": "sha512-ethXTt3SGGR+95gudmqJ1eNhRO7eGEGIgYA9vnPatK4/etz2MEVDno5GMCibdMTuBMyElzIlgxMna3K94XDIDQ==", - "license": "ISC", - "dependencies": { - "brace-expansion": "^2.0.1" - }, "engines": { - "node": "20 || >=22" - }, - "funding": { - "url": "https://github.com/sponsors/isaacs" + "node": ">= 4" } } } diff --git a/packages/core/package.json b/packages/core/package.json index ae6e6fc7..1bcaaaaa 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -26,7 +26,7 @@ "diff": "^7.0.0", "dotenv": "^16.4.7", "fast-glob": "^3.3.3", - "minimatch": "^10.0.0", + "ignore": "^7.0.0", "shell-quote": "^1.8.2", "strip-ansi": "^7.1.0", "@opentelemetry/api": "^1.9.0", diff --git a/packages/core/src/services/fileDiscoveryService.test.ts b/packages/core/src/services/fileDiscoveryService.test.ts index 2ef83bfa..368f4d23 100644 --- a/packages/core/src/services/fileDiscoveryService.test.ts +++ b/packages/core/src/services/fileDiscoveryService.test.ts @@ -140,33 +140,6 @@ describe('FileDiscoveryService', () => { }); }); - describe('getIgnoreInfo', () => { - beforeEach(async () => { - await service.initialize(); - }); - - it('should return git ignored patterns', () => { - const info = service.getIgnoreInfo(); - - expect(info.gitIgnored).toEqual(['.git/**', 'node_modules/**']); - }); - - it('should return empty arrays when git ignore parser is not initialized', async () => { - const uninitializedService = new FileDiscoveryService(mockProjectRoot); - const info = uninitializedService.getIgnoreInfo(); - - expect(info.gitIgnored).toEqual([]); - }); - - it('should handle git ignore parser returning null patterns', async () => { - mockGitIgnoreParser.getIgnoredPatterns.mockReturnValue([] as string[]); - - const info = service.getIgnoreInfo(); - - expect(info.gitIgnored).toEqual([]); - }); - }); - describe('isGitRepository', () => { it('should return true when isGitRepo is explicitly set to true in options', () => { const result = service.isGitRepository({ isGitRepo: true }); diff --git a/packages/core/src/services/fileDiscoveryService.ts b/packages/core/src/services/fileDiscoveryService.ts index 5329507e..3874e752 100644 --- a/packages/core/src/services/fileDiscoveryService.ts +++ b/packages/core/src/services/fileDiscoveryService.ts @@ -51,15 +51,6 @@ export class FileDiscoveryService { }); } - /** - * Gets patterns that would be ignored for debugging/transparency - */ - getIgnoreInfo(): { gitIgnored: string[] } { - return { - gitIgnored: this.gitIgnoreFilter?.getIgnoredPatterns() || [], - }; - } - /** * Checks if a single file should be ignored */ diff --git a/packages/core/src/utils/gitIgnoreParser.test.ts b/packages/core/src/utils/gitIgnoreParser.test.ts index 0b99115a..1646a5b9 100644 --- a/packages/core/src/utils/gitIgnoreParser.test.ts +++ b/packages/core/src/utils/gitIgnoreParser.test.ts @@ -15,7 +15,6 @@ vi.mock('fs/promises'); // Mock gitUtils module vi.mock('./gitUtils.js', () => ({ isGitRepository: vi.fn(() => true), - findGitRoot: vi.fn(() => '/test/project'), })); describe('GitIgnoreParser', () => { @@ -24,7 +23,9 @@ describe('GitIgnoreParser', () => { beforeEach(() => { parser = new GitIgnoreParser(mockProjectRoot); - vi.clearAllMocks(); + // Reset mocks before each test + vi.mocked(fs.readFile).mockClear(); + vi.mocked(fs.readFile).mockRejectedValue(new Error('ENOENT')); // Default to no file }); afterEach(() => { @@ -33,8 +34,6 @@ describe('GitIgnoreParser', () => { describe('initialization', () => { it('should initialize without errors when no .gitignore exists', async () => { - vi.mocked(fs.readFile).mockRejectedValue(new Error('ENOENT')); - await expect(parser.initialize()).resolves.not.toThrow(); }); @@ -43,106 +42,35 @@ describe('GitIgnoreParser', () => { # Comment node_modules/ *.log -dist +/dist .env `; - vi.mocked(fs.readFile).mockResolvedValue(gitignoreContent); + vi.mocked(fs.readFile) + .mockResolvedValueOnce(gitignoreContent) + .mockRejectedValue(new Error('ENOENT')); await parser.initialize(); - const patterns = parser.getIgnoredPatterns(); - expect(patterns).toContain('.git/**'); - expect(patterns).toContain('.git'); - expect(patterns).toContain('node_modules/**'); - expect(patterns).toContain('**/*.log'); - expect(patterns).toContain('**/dist'); - expect(patterns).toContain('**/.env'); + expect(parser.isIgnored('node_modules/some-lib')).toBe(true); + expect(parser.isIgnored('src/app.log')).toBe(true); + expect(parser.isIgnored('dist/index.js')).toBe(true); + expect(parser.isIgnored('.env')).toBe(true); }); it('should handle git exclude file', async () => { vi.mocked(fs.readFile).mockImplementation(async (filePath) => { - if (filePath === path.join(mockProjectRoot, '.gitignore')) { - throw new Error('ENOENT'); - } if ( filePath === path.join(mockProjectRoot, '.git', 'info', 'exclude') ) { return 'temp/\n*.tmp'; } - throw new Error('Unexpected file'); + throw new Error('ENOENT'); }); await parser.initialize(); - const patterns = parser.getIgnoredPatterns(); - expect(patterns).toContain('temp/**'); - expect(patterns).toContain('**/*.tmp'); - }); - }); - - describe('pattern parsing', () => { - it('should handle directory patterns correctly', async () => { - const gitignoreContent = 'node_modules/\nbuild/\n'; - vi.mocked(fs.readFile).mockResolvedValue(gitignoreContent); - - await parser.initialize(); - const patterns = parser.getIgnoredPatterns(); - - expect(patterns).toContain('node_modules/**'); - expect(patterns).toContain('build/**'); - }); - - it('should handle file patterns correctly', async () => { - const gitignoreContent = '*.log\n.env\nconfig.json\n'; - vi.mocked(fs.readFile).mockResolvedValue(gitignoreContent); - - await parser.initialize(); - const patterns = parser.getIgnoredPatterns(); - - expect(patterns).toContain('**/*.log'); - expect(patterns).toContain('**/.env'); - expect(patterns).toContain('**/config.json'); - }); - - it('should skip comments and empty lines', async () => { - const gitignoreContent = ` -# This is a comment -*.log - -# Another comment -.env -`; - vi.mocked(fs.readFile).mockResolvedValue(gitignoreContent); - - await parser.initialize(); - const patterns = parser.getIgnoredPatterns(); - - expect(patterns).not.toContain('# This is a comment'); - expect(patterns).not.toContain('# Another comment'); - expect(patterns).toContain('**/*.log'); - expect(patterns).toContain('**/.env'); - }); - - it('should skip negation patterns for now', async () => { - const gitignoreContent = '*.log\n!important.log\n'; - vi.mocked(fs.readFile).mockResolvedValue(gitignoreContent); - - await parser.initialize(); - const patterns = parser.getIgnoredPatterns(); - - expect(patterns).toContain('**/*.log'); - expect(patterns).not.toContain('!important.log'); - }); - - it('should handle paths with slashes correctly', async () => { - const gitignoreContent = 'src/*.log\ndocs/temp/\n'; - vi.mocked(fs.readFile).mockResolvedValue(gitignoreContent); - - await parser.initialize(); - const patterns = parser.getIgnoredPatterns(); - - expect(patterns).toContain('src/*.log'); - expect(patterns).toContain('docs/temp/**'); + expect(parser.isIgnored('temp/file.txt')).toBe(true); + expect(parser.isIgnored('src/file.tmp')).toBe(true); }); }); @@ -151,28 +79,32 @@ dist const gitignoreContent = ` node_modules/ *.log -dist -.env +/dist +/.env src/*.tmp +!src/important.tmp `; - vi.mocked(fs.readFile).mockResolvedValue(gitignoreContent); + vi.mocked(fs.readFile) + .mockResolvedValueOnce(gitignoreContent) + .mockRejectedValue(new Error('ENOENT')); await parser.initialize(); }); it('should always ignore .git directory', () => { expect(parser.isIgnored('.git')).toBe(true); expect(parser.isIgnored('.git/config')).toBe(true); - expect(parser.isIgnored('.git/objects/abc123')).toBe(true); + expect(parser.isIgnored(path.join(mockProjectRoot, '.git', 'HEAD'))).toBe( + true, + ); }); it('should ignore files matching patterns', () => { - expect(parser.isIgnored('node_modules')).toBe(true); - expect(parser.isIgnored('node_modules/package')).toBe(true); + expect(parser.isIgnored('node_modules/package/index.js')).toBe(true); expect(parser.isIgnored('app.log')).toBe(true); expect(parser.isIgnored('logs/app.log')).toBe(true); - expect(parser.isIgnored('dist')).toBe(true); + expect(parser.isIgnored('dist/bundle.js')).toBe(true); expect(parser.isIgnored('.env')).toBe(true); - expect(parser.isIgnored('config/.env')).toBe(true); + expect(parser.isIgnored('config/.env')).toBe(false); // .env is anchored to root }); it('should ignore files with path-specific patterns', () => { @@ -180,29 +112,28 @@ src/*.tmp expect(parser.isIgnored('other/temp.tmp')).toBe(false); }); + it('should handle negation patterns', () => { + expect(parser.isIgnored('src/important.tmp')).toBe(false); + }); + it('should not ignore files that do not match patterns', () => { expect(parser.isIgnored('src/index.ts')).toBe(false); expect(parser.isIgnored('README.md')).toBe(false); - expect(parser.isIgnored('package.json')).toBe(false); }); it('should handle absolute paths correctly', () => { - const absolutePath = path.join( - mockProjectRoot, - 'node_modules', - 'package', - ); + const absolutePath = path.join(mockProjectRoot, 'node_modules', 'lib'); expect(parser.isIgnored(absolutePath)).toBe(true); }); - it('should handle paths outside project root', () => { - const outsidePath = '/other/project/file.txt'; + it('should handle paths outside project root by not ignoring them', () => { + const outsidePath = path.resolve(mockProjectRoot, '../other/file.txt'); expect(parser.isIgnored(outsidePath)).toBe(false); }); it('should handle relative paths correctly', () => { - expect(parser.isIgnored('./node_modules')).toBe(true); - expect(parser.isIgnored('../file.txt')).toBe(false); + expect(parser.isIgnored('node_modules/some-package')).toBe(true); + expect(parser.isIgnored('../some/other/file.txt')).toBe(false); }); it('should normalize path separators on Windows', () => { @@ -212,26 +143,11 @@ src/*.tmp }); describe('getIgnoredPatterns', () => { - it('should return a copy of patterns array', async () => { - const gitignoreContent = '*.log\n'; - vi.mocked(fs.readFile).mockResolvedValue(gitignoreContent); + it('should return the raw patterns added', async () => { + const gitignoreContent = '*.log\n!important.log'; + vi.mocked(fs.readFile).mockResolvedValueOnce(gitignoreContent); await parser.initialize(); - const patterns1 = parser.getIgnoredPatterns(); - const patterns2 = parser.getIgnoredPatterns(); - - expect(patterns1).not.toBe(patterns2); // Different array instances - expect(patterns1).toEqual(patterns2); // Same content - }); - - it('should always include .git patterns', async () => { - vi.mocked(fs.readFile).mockRejectedValue(new Error('ENOENT')); - - await parser.initialize(); - const patterns = parser.getIgnoredPatterns(); - - expect(patterns).toContain('.git/**'); - expect(patterns).toContain('.git'); }); }); }); diff --git a/packages/core/src/utils/gitIgnoreParser.ts b/packages/core/src/utils/gitIgnoreParser.ts index 098281ca..ae1a7a01 100644 --- a/packages/core/src/utils/gitIgnoreParser.ts +++ b/packages/core/src/utils/gitIgnoreParser.ts @@ -6,18 +6,17 @@ import * as fs from 'fs/promises'; import * as path from 'path'; -import { minimatch } from 'minimatch'; +import ignore, { Ignore } from 'ignore'; import { isGitRepository } from './gitUtils.js'; export interface GitIgnoreFilter { isIgnored(filePath: string): boolean; - getIgnoredPatterns(): string[]; } export class GitIgnoreParser implements GitIgnoreFilter { - private ignorePatterns: string[] = []; private projectRoot: string; private isGitRepo: boolean = false; + private ig: Ignore = ignore(); constructor(projectRoot: string) { this.projectRoot = path.resolve(projectRoot); @@ -32,13 +31,13 @@ export class GitIgnoreParser implements GitIgnoreFilter { ]; // Always ignore .git directory regardless of .gitignore content - this.ignorePatterns = ['.git/**', '.git']; + this.addPatterns(['.git']); for (const gitIgnoreFile of gitIgnoreFiles) { try { const content = await fs.readFile(gitIgnoreFile, 'utf-8'); - const patterns = this.parseGitIgnoreContent(content); - this.ignorePatterns.push(...patterns); + const patterns = content.split('\n').map((p) => p.trim()); + this.addPatterns(patterns); } catch (_error) { // File doesn't exist or can't be read, continue silently } @@ -46,71 +45,33 @@ export class GitIgnoreParser implements GitIgnoreFilter { } } - private parseGitIgnoreContent(content: string): string[] { - return content - .split('\n') - .map((line) => line.trim()) - .filter((line) => line && !line.startsWith('#')) - .map((pattern) => { - // Handle negation patterns (!) - for now we'll skip them - if (pattern.startsWith('!')) { - return null; - } - - // Convert gitignore patterns to minimatch-compatible patterns - if (pattern.endsWith('/')) { - // Directory pattern - match directory and all contents - const dirPattern = pattern.slice(0, -1); // Remove trailing slash - return [dirPattern, dirPattern + '/**']; - } - - // If pattern doesn't contain /, it should match at any level - if (!pattern.includes('/') && !pattern.startsWith('**/')) { - return '**/' + pattern; - } - - return pattern; - }) - .filter((pattern) => pattern !== null) - .flat() as string[]; + private addPatterns(patterns: string[]) { + this.ig.add(patterns); } isIgnored(filePath: string): boolean { - // If not a git repository, nothing is ignored if (!this.isGitRepo) { return false; } - // Normalize the input path (handle ./ prefixes) - let cleanPath = filePath; - if (cleanPath.startsWith('./')) { - cleanPath = cleanPath.slice(2); - } + const relativePath = path.isAbsolute(filePath) + ? path.relative(this.projectRoot, filePath) + : filePath; - // Convert to relative path from project root - const relativePath = path.relative( - this.projectRoot, - path.resolve(this.projectRoot, cleanPath), - ); - - // Handle paths that go outside project root - if (relativePath.startsWith('..')) { + if (relativePath === '' || relativePath.startsWith('..')) { return false; } - // Normalize path separators for cross-platform compatibility - const normalizedPath = relativePath.replace(/\\/g, '/'); + let normalizedPath = relativePath.replace(/\\/g, '/'); + if (normalizedPath.startsWith('./')) { + normalizedPath = normalizedPath.substring(2); + } - return this.ignorePatterns.some((pattern) => - minimatch(normalizedPath, pattern, { - dot: true, - matchBase: false, - flipNegate: false, - }), - ); + const ignored = this.ig.ignores(normalizedPath); + return ignored; } - getIgnoredPatterns(): string[] { - return [...this.ignorePatterns]; + getGitRepoRoot(): string { + return this.projectRoot; } }