centralize file filtering in FileDiscoveryService (#1039)

packages/core/src/services/fileDiscoveryService.test.ts

@@ -8,15 +8,13 @@ import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest';
 import type { Mocked } from 'vitest';
 import { FileDiscoveryService } from './fileDiscoveryService.js';
 import { GitIgnoreParser } from '../utils/gitIgnoreParser.js';
+import * as gitUtils from '../utils/gitUtils.js';
 
 // Mock the GitIgnoreParser
 vi.mock('../utils/gitIgnoreParser.js');
 
 // Mock gitUtils module
-vi.mock('../utils/gitUtils.js', () => ({
-  isGitRepository: vi.fn(() => true),
-  findGitRoot: vi.fn(() => '/test/project'),
-}));
+vi.mock('../utils/gitUtils.js');
 
 describe('FileDiscoveryService', () => {
   let service: FileDiscoveryService;
@@ -24,16 +22,16 @@ describe('FileDiscoveryService', () => {
   const mockProjectRoot = '/test/project';
 
   beforeEach(() => {
-    service = new FileDiscoveryService(mockProjectRoot);
-
     mockGitIgnoreParser = {
       initialize: vi.fn(),
       isIgnored: vi.fn(),
-      getIgnoredPatterns: vi.fn(() => ['.git/**', 'node_modules/**']),
-      parseGitIgnoreContent: vi.fn(),
+      loadPatterns: vi.fn(),
+      loadGitRepoPatterns: vi.fn(),
     } as unknown as Mocked<GitIgnoreParser>;
 
     vi.mocked(GitIgnoreParser).mockImplementation(() => mockGitIgnoreParser);
+    vi.mocked(gitUtils.isGitRepository).mockReturnValue(true);
+    vi.mocked(gitUtils.findGitRoot).mockReturnValue('/test/project');
     vi.clearAllMocks();
   });
 
@@ -42,36 +40,30 @@ describe('FileDiscoveryService', () => {
   });
 
   describe('initialization', () => {
-    it('should initialize git ignore parser by default', async () => {
-      await service.initialize();
-
+    it('should initialize git ignore parser by default', () => {
+      service = new FileDiscoveryService(mockProjectRoot);
       expect(GitIgnoreParser).toHaveBeenCalledWith(mockProjectRoot);
-      expect(mockGitIgnoreParser.initialize).toHaveBeenCalled();
+      expect(GitIgnoreParser).toHaveBeenCalledTimes(2);
+      expect(mockGitIgnoreParser.loadGitRepoPatterns).toHaveBeenCalled();
+      expect(mockGitIgnoreParser.loadPatterns).toHaveBeenCalled();
     });
 
-    it('should not initialize git ignore parser when respectGitIgnore is false', async () => {
-      await service.initialize({ respectGitIgnore: false });
+    it('should not initialize git ignore parser when not a git repo', () => {
+      vi.mocked(gitUtils.isGitRepository).mockReturnValue(false);
+      service = new FileDiscoveryService(mockProjectRoot);
 
-      expect(GitIgnoreParser).not.toHaveBeenCalled();
-      expect(mockGitIgnoreParser.initialize).not.toHaveBeenCalled();
-    });
-
-    it('should handle initialization errors gracefully', async () => {
-      mockGitIgnoreParser.initialize.mockRejectedValue(
-        new Error('Init failed'),
-      );
-
-      await expect(service.initialize()).rejects.toThrow('Init failed');
+      expect(GitIgnoreParser).toHaveBeenCalledOnce();
+      expect(mockGitIgnoreParser.loadGitRepoPatterns).not.toHaveBeenCalled();
     });
   });
 
   describe('filterFiles', () => {
-    beforeEach(async () => {
+    beforeEach(() => {
       mockGitIgnoreParser.isIgnored.mockImplementation(
         (path: string) =>
           path.includes('node_modules') || path.includes('.git'),
       );
-      await service.initialize();
+      service = new FileDiscoveryService(mockProjectRoot);
     });
 
     it('should filter out git-ignored files by default', () => {
@@ -106,102 +98,22 @@ describe('FileDiscoveryService', () => {
     });
   });
 
-  describe('shouldIgnoreFile', () => {
-    beforeEach(async () => {
+  describe('shouldGitIgnoreFile', () => {
+    beforeEach(() => {
       mockGitIgnoreParser.isIgnored.mockImplementation((path: string) =>
         path.includes('node_modules'),
       );
-      await service.initialize();
+      service = new FileDiscoveryService(mockProjectRoot);
     });
 
     it('should return true for git-ignored files', () => {
-      expect(service.shouldIgnoreFile('node_modules/package/index.js')).toBe(
+      expect(service.shouldGitIgnoreFile('node_modules/package/index.js')).toBe(
         true,
       );
     });
 
     it('should return false for non-ignored files', () => {
-      expect(service.shouldIgnoreFile('src/index.ts')).toBe(false);
-    });
-
-    it('should return false when respectGitIgnore is false', () => {
-      expect(
-        service.shouldIgnoreFile('node_modules/package/index.js', {
-          respectGitIgnore: false,
-        }),
-      ).toBe(false);
-    });
-
-    it('should return false when git ignore parser is not initialized', async () => {
-      const uninitializedService = new FileDiscoveryService(mockProjectRoot);
-      expect(
-        uninitializedService.shouldIgnoreFile('node_modules/package/index.js'),
-      ).toBe(false);
-    });
-  });
-
-  describe('isGitRepository', () => {
-    it('should return true when isGitRepo is explicitly set to true in options', () => {
-      const result = service.isGitRepository({ isGitRepo: true });
-      expect(result).toBe(true);
-    });
-
-    it('should return false when isGitRepo is explicitly set to false in options', () => {
-      const result = service.isGitRepository({ isGitRepo: false });
-      expect(result).toBe(false);
-    });
-
-    it('should use git utility function when isGitRepo is not specified', () => {
-      const result = service.isGitRepository();
-      expect(result).toBe(true); // mocked to return true
-    });
-
-    it('should use git utility function when options are undefined', () => {
-      const result = service.isGitRepository(undefined);
-      expect(result).toBe(true); // mocked to return true
-    });
-  });
-
-  describe('initialization with isGitRepo config', () => {
-    it('should initialize git ignore parser when isGitRepo is true in options', async () => {
-      await service.initialize({ isGitRepo: true });
-
-      expect(GitIgnoreParser).toHaveBeenCalledWith(mockProjectRoot);
-      expect(mockGitIgnoreParser.initialize).toHaveBeenCalled();
-    });
-
-    it('should not initialize git ignore parser when isGitRepo is false in options', async () => {
-      await service.initialize({ isGitRepo: false });
-
-      expect(GitIgnoreParser).not.toHaveBeenCalled();
-      expect(mockGitIgnoreParser.initialize).not.toHaveBeenCalled();
-    });
-
-    it('should initialize git ignore parser when isGitRepo is not specified but respectGitIgnore is true', async () => {
-      await service.initialize({ respectGitIgnore: true });
-
-      expect(GitIgnoreParser).toHaveBeenCalledWith(mockProjectRoot);
-      expect(mockGitIgnoreParser.initialize).toHaveBeenCalled();
-    });
-  });
-
-  describe('shouldIgnoreFile with isGitRepo config', () => {
-    it('should respect isGitRepo option when checking if file should be ignored', async () => {
-      mockGitIgnoreParser.isIgnored.mockImplementation((path: string) =>
-        path.includes('node_modules'),
-      );
-      await service.initialize({ isGitRepo: true });
-
-      expect(
-        service.shouldIgnoreFile('node_modules/package/index.js', {
-          isGitRepo: true,
-        }),
-      ).toBe(true);
-      expect(
-        service.shouldIgnoreFile('node_modules/package/index.js', {
-          isGitRepo: false,
-        }),
-      ).toBe(false);
+      expect(service.shouldGitIgnoreFile('src/index.ts')).toBe(false);
     });
   });
 
@@ -211,13 +123,7 @@ describe('FileDiscoveryService', () => {
       expect(relativeService).toBeInstanceOf(FileDiscoveryService);
     });
 
-    it('should handle undefined options', async () => {
-      await service.initialize(undefined);
-      expect(GitIgnoreParser).toHaveBeenCalled();
-    });
-
-    it('should handle filterFiles with undefined options', async () => {
-      await service.initialize();
+    it('should handle filterFiles with undefined options', () => {
       const files = ['src/index.ts'];
       const filtered = service.filterFiles(files, undefined);
       expect(filtered).toEqual(files);

packages/core/src/services/fileDiscoveryService.ts

@@ -7,41 +7,37 @@
 import { GitIgnoreParser, GitIgnoreFilter } from '../utils/gitIgnoreParser.js';
 import { isGitRepository } from '../utils/gitUtils.js';
 import * as path from 'path';
-import { glob, type GlobOptions } from 'glob';
 
-export interface FileDiscoveryOptions {
+const GEMINI_IGNORE_FILE_NAME = '.geminiignore';
+
+export interface FilterFilesOptions {
   respectGitIgnore?: boolean;
-  includeBuildArtifacts?: boolean;
-  isGitRepo?: boolean;
+  respectGeminiIgnore?: boolean;
 }
 
 export class FileDiscoveryService {
   private gitIgnoreFilter: GitIgnoreFilter | null = null;
+  private geminiIgnoreFilter: GitIgnoreFilter | null = null;
   private projectRoot: string;
 
   constructor(projectRoot: string) {
     this.projectRoot = path.resolve(projectRoot);
-  }
-
-  async initialize(options: FileDiscoveryOptions = {}): Promise<void> {
-    const isGitRepo = options.isGitRepo ?? isGitRepository(this.projectRoot);
-
-    if (options.respectGitIgnore !== false && isGitRepo) {
+    if (isGitRepository(this.projectRoot)) {
       const parser = new GitIgnoreParser(this.projectRoot);
-      await parser.initialize();
+      try {
+        parser.loadGitRepoPatterns();
+      } catch (_error) {
+        // ignore file not found
+      }
       this.gitIgnoreFilter = parser;
     }
-  }
-
-  async glob(
-    pattern: string | string[],
-    options: GlobOptions = {},
-  ): Promise<string[]> {
-    const files = (await glob(pattern, {
-      ...options,
-      nocase: true,
-    })) as string[];
-    return this.filterFiles(files);
+    const gParser = new GitIgnoreParser(this.projectRoot);
+    try {
+      gParser.loadPatterns(GEMINI_IGNORE_FILE_NAME);
+    } catch (_error) {
+      // ignore file not found
+    }
+    this.geminiIgnoreFilter = gParser;
   }
 
   /**
@@ -49,42 +45,49 @@ export class FileDiscoveryService {
    */
   filterFiles(
     filePaths: string[],
-    options: FileDiscoveryOptions = {},
+    options: FilterFilesOptions = {
+      respectGitIgnore: true,
+      respectGeminiIgnore: true,
+    },
   ): string[] {
     return filePaths.filter((filePath) => {
-      // Always respect git ignore unless explicitly disabled
-      if (options.respectGitIgnore !== false && this.gitIgnoreFilter) {
-        if (this.gitIgnoreFilter.isIgnored(filePath)) {
-          return false;
-        }
+      if (options.respectGitIgnore && this.shouldGitIgnoreFile(filePath)) {
+        return false;
+      }
+      if (
+        options.respectGeminiIgnore &&
+        this.shouldGeminiIgnoreFile(filePath)
+      ) {
+        return false;
       }
-
       return true;
     });
   }
 
   /**
-   * Checks if a single file should be ignored
+   * Checks if a single file should be git-ignored
    */
-  shouldIgnoreFile(
-    filePath: string,
-    options: FileDiscoveryOptions = {},
-  ): boolean {
-    const isGitRepo = options.isGitRepo ?? isGitRepository(this.projectRoot);
-    if (
-      options.respectGitIgnore !== false &&
-      isGitRepo &&
-      this.gitIgnoreFilter
-    ) {
+  shouldGitIgnoreFile(filePath: string): boolean {
+    if (this.gitIgnoreFilter) {
       return this.gitIgnoreFilter.isIgnored(filePath);
     }
     return false;
   }
 
   /**
-   * Returns whether the project is a git repository
+   * Checks if a single file should be gemini-ignored
    */
-  isGitRepository(options: FileDiscoveryOptions = {}): boolean {
-    return options.isGitRepo ?? isGitRepository(this.projectRoot);
+  shouldGeminiIgnoreFile(filePath: string): boolean {
+    if (this.geminiIgnoreFilter) {
+      return this.geminiIgnoreFilter.isIgnored(filePath);
+    }
+    return false;
+  }
+
+  /**
+   * Returns loaded patterns from .geminiignore
+   */
+  getGeminiIgnorePatterns(): string[] {
+    return this.geminiIgnoreFilter?.getPatterns() ?? [];
   }
 }