From b1059f891f18c478c2afa0c44766f36654fd7001 Mon Sep 17 00:00:00 2001 From: Eric Rahm Date: Fri, 24 Oct 2025 18:55:12 -0700 Subject: [PATCH] refactor: Switch over to unified shouldIgnoreFile (#11815) --- .../cli/src/zed-integration/zedIntegration.ts | 17 +++---- .../src/services/fileDiscoveryService.test.ts | 22 ++++----- .../core/src/services/fileDiscoveryService.ts | 46 ++----------------- packages/core/src/tools/read-file.test.ts | 8 +++- packages/core/src/tools/read-file.ts | 7 ++- packages/core/src/utils/getFolderStructure.ts | 35 +++++++------- 6 files changed, 51 insertions(+), 84 deletions(-) diff --git a/packages/cli/src/zed-integration/zedIntegration.ts b/packages/cli/src/zed-integration/zedIntegration.ts index 29739850ae..c320bbe3a9 100644 --- a/packages/cli/src/zed-integration/zedIntegration.ts +++ b/packages/cli/src/zed-integration/zedIntegration.ts @@ -12,6 +12,7 @@ import type { ToolResult, ToolCallConfirmationDetails, GeminiCLIExtension, + FilterFilesOptions, } from '@google/gemini-cli-core'; import { AuthType, @@ -571,7 +572,8 @@ class Session { // Get centralized file discovery service const fileDiscovery = this.config.getFileService(); - const respectGitIgnore = this.config.getFileFilteringRespectGitIgnore(); + const fileFilteringOptions: FilterFilesOptions = + this.config.getFileFilteringOptions(); const pathSpecsToRead: string[] = []; const contentLabelsForDisplay: string[] = []; @@ -587,13 +589,10 @@ class Session { for (const atPathPart of atPathCommandParts) { const pathName = atPathPart.fileData!.fileUri; - // Check if path should be ignored by git - if (fileDiscovery.shouldGitIgnoreFile(pathName)) { + // Check if path should be ignored + if (fileDiscovery.shouldIgnoreFile(pathName, fileFilteringOptions)) { ignoredPaths.push(pathName); - const reason = respectGitIgnore - ? 'git-ignored and will be skipped' - : 'ignored by custom patterns'; - debugLogger.warn(`Path ${pathName} is ${reason}.`); + debugLogger.warn(`Path ${pathName} is ignored and will be skipped.`); continue; } let currentPathSpec = pathName; @@ -730,9 +729,8 @@ class Session { initialQueryText = initialQueryText.trim(); // Inform user about ignored paths if (ignoredPaths.length > 0) { - const ignoreType = respectGitIgnore ? 'git-ignored' : 'custom-ignored'; this.debug( - `Ignored ${ignoredPaths.length} ${ignoreType} files: ${ignoredPaths.join(', ')}`, + `Ignored ${ignoredPaths.length} files: ${ignoredPaths.join(', ')}`, ); } @@ -747,7 +745,6 @@ class Session { if (pathSpecsToRead.length > 0) { const toolArgs = { paths: pathSpecsToRead, - respectGitIgnore, // Use configuration setting }; const callId = `${readManyFilesTool.name}-${Date.now()}`; diff --git a/packages/core/src/services/fileDiscoveryService.test.ts b/packages/core/src/services/fileDiscoveryService.test.ts index de7c561e4d..c09309b13b 100644 --- a/packages/core/src/services/fileDiscoveryService.test.ts +++ b/packages/core/src/services/fileDiscoveryService.test.ts @@ -40,8 +40,8 @@ describe('FileDiscoveryService', () => { const service = new FileDiscoveryService(projectRoot); // Let's check the effect of the parser instead of mocking it. - expect(service.shouldGitIgnoreFile('node_modules/foo.js')).toBe(true); - expect(service.shouldGitIgnoreFile('src/foo.js')).toBe(false); + expect(service.shouldIgnoreFile('node_modules/foo.js')).toBe(true); + expect(service.shouldIgnoreFile('src/foo.js')).toBe(false); }); it('should not load git repo patterns when not in a git repo', async () => { @@ -50,15 +50,15 @@ describe('FileDiscoveryService', () => { const service = new FileDiscoveryService(projectRoot); // .gitignore is not loaded in non-git repos - expect(service.shouldGitIgnoreFile('node_modules/foo.js')).toBe(false); + expect(service.shouldIgnoreFile('node_modules/foo.js')).toBe(false); }); it('should load .geminiignore patterns even when not in a git repo', async () => { await createTestFile('.geminiignore', 'secrets.txt'); const service = new FileDiscoveryService(projectRoot); - expect(service.shouldGeminiIgnoreFile('secrets.txt')).toBe(true); - expect(service.shouldGeminiIgnoreFile('src/index.js')).toBe(false); + expect(service.shouldIgnoreFile('secrets.txt')).toBe(true); + expect(service.shouldIgnoreFile('src/index.js')).toBe(false); }); }); @@ -184,7 +184,7 @@ describe('FileDiscoveryService', () => { const service = new FileDiscoveryService(projectRoot); expect( - service.shouldGitIgnoreFile( + service.shouldIgnoreFile( path.join(projectRoot, 'node_modules/package/index.js'), ), ).toBe(true); @@ -194,7 +194,7 @@ describe('FileDiscoveryService', () => { const service = new FileDiscoveryService(projectRoot); expect( - service.shouldGitIgnoreFile(path.join(projectRoot, 'src/index.ts')), + service.shouldIgnoreFile(path.join(projectRoot, 'src/index.ts')), ).toBe(false); }); @@ -202,7 +202,7 @@ describe('FileDiscoveryService', () => { const service = new FileDiscoveryService(projectRoot); expect( - service.shouldGeminiIgnoreFile(path.join(projectRoot, 'debug.log')), + service.shouldIgnoreFile(path.join(projectRoot, 'debug.log')), ).toBe(true); }); @@ -210,7 +210,7 @@ describe('FileDiscoveryService', () => { const service = new FileDiscoveryService(projectRoot); expect( - service.shouldGeminiIgnoreFile(path.join(projectRoot, 'src/index.ts')), + service.shouldIgnoreFile(path.join(projectRoot, 'src/index.ts')), ).toBe(false); }); }); @@ -224,10 +224,10 @@ describe('FileDiscoveryService', () => { ); expect( - service.shouldGitIgnoreFile(path.join(projectRoot, 'ignored.txt')), + service.shouldIgnoreFile(path.join(projectRoot, 'ignored.txt')), ).toBe(true); expect( - service.shouldGitIgnoreFile(path.join(projectRoot, 'not-ignored.txt')), + service.shouldIgnoreFile(path.join(projectRoot, 'not-ignored.txt')), ).toBe(false); }); diff --git a/packages/core/src/services/fileDiscoveryService.ts b/packages/core/src/services/fileDiscoveryService.ts index 981e81127e..7b4d3398bd 100644 --- a/packages/core/src/services/fileDiscoveryService.ts +++ b/packages/core/src/services/fileDiscoveryService.ts @@ -37,21 +37,13 @@ export class FileDiscoveryService { /** * Filters a list of file paths based on git ignore rules */ - filterFiles( - filePaths: string[], - options: FilterFilesOptions = { - respectGitIgnore: true, - respectGeminiIgnore: true, - }, - ): string[] { + filterFiles(filePaths: string[], options: FilterFilesOptions = {}): string[] { + const { respectGitIgnore = true, respectGeminiIgnore = true } = options; return filePaths.filter((filePath) => { - if (options.respectGitIgnore && this.shouldGitIgnoreFile(filePath)) { + if (respectGitIgnore && this.gitIgnoreFilter?.isIgnored(filePath)) { return false; } - if ( - options.respectGeminiIgnore && - this.shouldGeminiIgnoreFile(filePath) - ) { + if (respectGeminiIgnore && this.geminiIgnoreFilter?.isIgnored(filePath)) { return false; } return true; @@ -78,26 +70,6 @@ export class FileDiscoveryService { }; } - /** - * Checks if a single file should be git-ignored - */ - shouldGitIgnoreFile(filePath: string): boolean { - if (this.gitIgnoreFilter) { - return this.gitIgnoreFilter.isIgnored(filePath); - } - return false; - } - - /** - * Checks if a single file should be gemini-ignored - */ - shouldGeminiIgnoreFile(filePath: string): boolean { - if (this.geminiIgnoreFilter) { - return this.geminiIgnoreFilter.isIgnored(filePath); - } - return false; - } - /** * Unified method to check if a file should be ignored based on filtering options */ @@ -105,14 +77,6 @@ export class FileDiscoveryService { filePath: string, options: FilterFilesOptions = {}, ): boolean { - const { respectGitIgnore = true, respectGeminiIgnore = true } = options; - - if (respectGitIgnore && this.shouldGitIgnoreFile(filePath)) { - return true; - } - if (respectGeminiIgnore && this.shouldGeminiIgnoreFile(filePath)) { - return true; - } - return false; + return this.filterFiles([filePath], options).length === 0; } } diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index 74b94e8af9..825d807cc4 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -38,6 +38,10 @@ describe('ReadFileTool', () => { getFileSystemService: () => new StandardFileSystemService(), getTargetDir: () => tempRootDir, getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), + getFileFilteringOptions: () => ({ + respectGitIgnore: true, + respectGeminiIgnore: true, + }), storage: { getProjectTempDir: () => path.join(tempRootDir, '.temp'), }, @@ -462,7 +466,7 @@ describe('ReadFileTool', () => { const params: ReadFileToolParams = { absolute_path: ignoredFilePath, }; - const expectedError = `File path '${ignoredFilePath}' is ignored by .geminiignore pattern(s).`; + const expectedError = `File path '${ignoredFilePath}' is ignored by configured ignore patterns.`; expect(() => tool.build(params)).toThrow(expectedError); }); @@ -474,7 +478,7 @@ describe('ReadFileTool', () => { const params: ReadFileToolParams = { absolute_path: ignoredFilePath, }; - const expectedError = `File path '${ignoredFilePath}' is ignored by .geminiignore pattern(s).`; + const expectedError = `File path '${ignoredFilePath}' is ignored by configured ignore patterns.`; expect(() => tool.build(params)).toThrow(expectedError); }); diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index 9584865746..affb428907 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -210,8 +210,11 @@ export class ReadFileTool extends BaseDeclarativeTool< } const fileService = this.config.getFileService(); - if (fileService.shouldGeminiIgnoreFile(params.absolute_path)) { - return `File path '${filePath}' is ignored by .geminiignore pattern(s).`; + const fileFilteringOptions = this.config.getFileFilteringOptions(); + if ( + fileService.shouldIgnoreFile(params.absolute_path, fileFilteringOptions) + ) { + return `File path '${filePath}' is ignored by configured ignore patterns.`; } return null; diff --git a/packages/core/src/utils/getFolderStructure.ts b/packages/core/src/utils/getFolderStructure.ts index 0b9c54cb90..141d4f542d 100644 --- a/packages/core/src/utils/getFolderStructure.ts +++ b/packages/core/src/utils/getFolderStructure.ts @@ -8,7 +8,10 @@ import * as fs from 'node:fs/promises'; import type { Dirent } from 'node:fs'; import * as path from 'node:path'; import { getErrorMessage, isNodeError } from './errors.js'; -import type { FileDiscoveryService } from '../services/fileDiscoveryService.js'; +import type { + FileDiscoveryService, + FilterFilesOptions, +} from '../services/fileDiscoveryService.js'; import type { FileFilteringOptions } from '../config/constants.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; import { debugLogger } from './debugLogger.js'; @@ -119,6 +122,10 @@ async function readFullStructure( const filesInCurrentDir: string[] = []; const subFoldersInCurrentDir: FullFolderInfo[] = []; + const filterFileOptions: FilterFilesOptions = { + respectGitIgnore: options.fileFilteringOptions?.respectGitIgnore, + respectGeminiIgnore: options.fileFilteringOptions?.respectGeminiIgnore, + }; // Process files first in the current directory for (const entry of entries) { @@ -129,15 +136,10 @@ async function readFullStructure( } const fileName = entry.name; const filePath = path.join(currentPath, fileName); - if (options.fileService) { - const shouldIgnore = - (options.fileFilteringOptions.respectGitIgnore && - options.fileService.shouldGitIgnoreFile(filePath)) || - (options.fileFilteringOptions.respectGeminiIgnore && - options.fileService.shouldGeminiIgnoreFile(filePath)); - if (shouldIgnore) { - continue; - } + if ( + options.fileService?.shouldIgnoreFile(filePath, filterFileOptions) + ) { + continue; } if ( !options.fileIncludePattern || @@ -168,14 +170,11 @@ async function readFullStructure( const subFolderName = entry.name; const subFolderPath = path.join(currentPath, subFolderName); - let isIgnored = false; - if (options.fileService) { - isIgnored = - (options.fileFilteringOptions.respectGitIgnore && - options.fileService.shouldGitIgnoreFile(subFolderPath)) || - (options.fileFilteringOptions.respectGeminiIgnore && - options.fileService.shouldGeminiIgnoreFile(subFolderPath)); - } + const isIgnored = + options.fileService?.shouldIgnoreFile( + subFolderPath, + filterFileOptions, + ) ?? false; if (options.ignoredFolders.has(subFolderName) || isIgnored) { const ignoredSubFolder: FullFolderInfo = {