fix(file): resolve .gitignore behavior when using --ignore "**/.gitignore" #633
Conversation
…nore" - Detect conflicts between ignore patterns and ignoreFiles patterns - Pre-read conflicting .gitignore files before they get excluded by ignore patterns - Extract patterns from pre-read .gitignore content and add to ignore list - Ensure .gitignore files are excluded from results but their patterns are still applied - Add comprehensive test case to verify the fix works correctly Fixes #624 Co-authored-by: yamadashy <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @yamadashy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request by @yamadashy. This PR addresses issue #624, which describes an unexpected behavior when using the --ignore option with a pattern like **/.gitignore. Previously, using --ignore "**/.gitignore" would not only exclude .gitignore files from the search results (which is the intended behavior of --ignore), but it would also prevent the application from reading these .gitignore files to apply the ignore patterns defined within them. This PR fixes that by implementing a mechanism to detect this conflict, pre-read the conflicting .gitignore files to extract their patterns, and then apply those patterns during the main file search, while still excluding the .gitignore files themselves from the final output.
Highlights
- Fix .gitignore conflict: The core issue where
--ignorepatterns conflicting withignoreFilespatterns (like**/.gitignore) caused.gitignorecontent patterns to be ignored is resolved. - Pre-read conflicting files: Introduced logic to identify
.gitignorefiles that are matched by bothignoreandignoreFilespatterns and pre-read their content before the main file search. - Extract and apply patterns: Patterns found within the pre-read
.gitignorefiles are extracted and added to the list of patterns used to filter files during the main search. - Maintain exclusion of .gitignore files: Ensures that while the patterns from
.gitignorefiles are applied, the.gitignorefiles themselves are still excluded from the final list of found files, respecting the--ignoredirective. - Comprehensive test case: A new test case has been added to specifically verify this fix, mocking file system operations to simulate the scenario with conflicting ignore patterns and
.gitignorecontent.
Changelog
Click here to see the changelog
- src/core/file/fileSearch.ts
- Added logic to detect patterns in
ignoreFilePatternsthat conflict withadjustedIgnorePatterns(lines 174-183). - Separated
ignoreFilePatternsintoconflictingIgnoreFilesandsafeIgnoreFilesbased on the conflict detection (lines 177-183). - Implemented a loop to asynchronously pre-read the content of files matching
conflictingIgnoreFilespatterns (lines 185-211). - Parsed ignore patterns from the content of pre-read files and collected them in
extraIgnorePatterns(line 200-201). - Modified the main
globbycall forfilePathsto includeextraIgnorePatternsin theignoreoption and usesafeIgnoreFilesin theignoreFilesoption (lines 213-219). - Modified the
globbycall fordirectories(whenincludeEmptyDirectoriesis true) similarly (lines 233-242). - Updated the
findEmptyDirectoriescall to use the combinedadjustedIgnorePatternsandextraIgnorePatterns(line 244).
- Added logic to detect patterns in
- tests/core/file/fileSearch.test.ts
- Added a new test case
should exclude .gitignore files but still apply their patterns when --ignore "**/.gitignore" is used(lines 346-422). - Configured a mock configuration to simulate the
--ignore "**/.gitignore"scenario (lines 347-354). - Mocked
globbyto simulate the pre-reading step for.gitignorefiles and the main file search with applied.gitignorepatterns (lines 365-396). - Mocked
fs.readFileto provide sample.gitignorecontent (lines 398-404). - Added assertions to verify that
.gitignorefiles are excluded from results, but files matching patterns within the mocked.gitignorecontent are also excluded (lines 409-417). - Added assertions to verify that non-ignored files are included in the results (lines 419-421).
- Added a new test case
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Ignore the file,
But read its rule,
A conflict solved,
No longer fooled.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This PR addresses an important edge case where explicitly ignoring .gitignore files can inadvertently disable their intended filtering behavior. The changes seem well-structured and the added test case provides good coverage. However, there are a few areas where the code could be improved for clarity and efficiency.
Summary of Findings
- Code Duplication: The filtering logic for conflicting and safe ignore files is duplicated. Extracting this logic into a named function would improve readability and maintainability.
- Potential Performance Improvement: The loop for pre-reading conflicting ignore files could be parallelized using
Promise.allto improve performance. - Test Mocking: The mock implementation of
globbyrelies on a side effect. Usingvi.spyOnwould make the test more robust and easier to understand. - Duplicated Test Logic: The filtering logic in the test is duplicated from the actual implementation. Creating a helper function would improve readability and maintainability.
Merge Readiness
The PR effectively addresses the issue of conflicting ignore patterns and ignore files. The added test case provides good coverage. However, addressing the code duplication and potential performance improvement would further enhance the quality of the code. I am unable to approve this pull request, and recommend that others review and approve this code before merging. I would recommend addressing the above comments before merging.
| const conflictingIgnoreFiles = ignoreFilePatterns.filter(pattern => | ||
| adjustedIgnorePatterns.some(ignorePattern => minimatch(pattern, ignorePattern)) | ||
| ); |
There was a problem hiding this comment.
This filter logic is repeated later for safeIgnoreFiles. Consider extracting this logic into a named function to improve readability and reduce code duplication. This would also make the code easier to test and maintain.
For example:
const isPatternConflicting = (pattern: string, ignorePatterns: string[]) =>
ignorePatterns.some(ignorePattern => minimatch(pattern, ignorePattern));
const conflictingIgnoreFiles = ignoreFilePatterns.filter(pattern => isPatternConflicting(pattern, adjustedIgnorePatterns));| const conflictingIgnoreFiles = ignoreFilePatterns.filter(pattern => | |
| adjustedIgnorePatterns.some(ignorePattern => minimatch(pattern, ignorePattern)) | |
| ); | |
| const isPatternConflicting = (pattern: string, ignorePatterns: string[]) => | |
| ignorePatterns.some(ignorePattern => minimatch(pattern, ignorePattern)); | |
| const conflictingIgnoreFiles = ignoreFilePatterns.filter(pattern => isPatternConflicting(pattern, adjustedIgnorePatterns)); |
| const safeIgnoreFiles = ignoreFilePatterns.filter(pattern => | ||
| !adjustedIgnorePatterns.some(ignorePattern => minimatch(pattern, ignorePattern)) | ||
| ); |
There was a problem hiding this comment.
Similar to the conflictingIgnoreFiles filter, this logic is also repeated. Use the isPatternConflicting function to avoid code duplication and improve readability.
| const safeIgnoreFiles = ignoreFilePatterns.filter(pattern => | |
| !adjustedIgnorePatterns.some(ignorePattern => minimatch(pattern, ignorePattern)) | |
| ); | |
| const isPatternConflicting = (pattern: string, ignorePatterns: string[]) => | |
| ignorePatterns.some(ignorePattern => minimatch(pattern, ignorePattern)); | |
| const safeIgnoreFiles = ignoreFilePatterns.filter(pattern => !isPatternConflicting(pattern, adjustedIgnorePatterns)); |
| for (const pattern of conflictingIgnoreFiles) { | ||
| try { | ||
| const matchingFiles = await globby(pattern, { | ||
| cwd: rootDir, | ||
| onlyFiles: true, | ||
| absolute: false, | ||
| dot: true, | ||
| followSymbolicLinks: false, | ||
| }); | ||
|
|
||
| for (const file of matchingFiles) { | ||
| try { | ||
| const content = await fs.readFile(path.join(rootDir, file), 'utf8'); | ||
| const filePatterns = parseIgnoreContent(content); | ||
| extraIgnorePatterns.push(...filePatterns); | ||
| } catch (error) { | ||
| // File might not be accessible, continue | ||
| logger.trace(`Could not read ignore file ${file}:`, error instanceof Error ? error.message : String(error)); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // Pattern might not match any files, continue | ||
| logger.trace(`Could not find files matching pattern ${pattern}:`, error instanceof Error ? error.message : String(error)); | ||
| } |
There was a problem hiding this comment.
This loop iterates over conflictingIgnoreFiles, reads each file, and extracts patterns. Consider using Promise.all to parallelize the file reading and pattern extraction, which could improve performance, especially when dealing with multiple conflicting .gitignore files. Make sure to handle errors appropriately when using Promise.all.
const extraIgnorePatterns: string[] = (await Promise.all(
conflictingIgnoreFiles.map(async (pattern) => {
try {
const matchingFiles = await globby(pattern, {
cwd: rootDir,
onlyFiles: true,
absolute: false,
dot: true,
followSymbolicLinks: false,
});
const patterns: string[] = [];
for (const file of matchingFiles) {
try {
const content = await fs.readFile(path.join(rootDir, file), 'utf8');
const filePatterns = parseIgnoreContent(content);
patterns.push(...filePatterns);
} catch (error) {
logger.trace(`Could not read ignore file ${file}:`, error instanceof Error ? error.message : String(error));
}
}
return patterns;
} catch (error) {
logger.trace(`Could not find files matching pattern ${pattern}:`, error instanceof Error ? error.message : String(error));
return [];
}
})
)).flat();| vi.mocked(globby).mockImplementation(async (patterns, options) => { | ||
| globbyCallCount++; | ||
|
|
||
| // First call is to find .gitignore files for pre-reading | ||
| if (globbyCallCount === 1 && patterns.includes('**/.gitignore')) { | ||
| return ['.gitignore', 'subdir/.gitignore']; | ||
| } | ||
|
|
||
| // Second call is the main file search | ||
| if (globbyCallCount === 2) { | ||
| // Simulate that .gitignore files are excluded but their patterns are applied | ||
| const allFiles = [ | ||
| 'file1.js', | ||
| 'file2.ts', | ||
| 'temp/ignored.js', // Should be ignored by .gitignore pattern | ||
| 'app.log', // Should be ignored by .gitignore pattern | ||
| 'subdir/code.js', | ||
| 'subdir/temp.log', // Should be ignored by .gitignore pattern | ||
| 'node_modules/package.json', // Should be ignored by .gitignore pattern | ||
| ]; | ||
|
|
||
| // Filter out files that would be ignored by .gitignore patterns | ||
| return allFiles.filter(file => { | ||
| return !file.endsWith('.log') && | ||
| !file.startsWith('temp/') && | ||
| !file.startsWith('node_modules/'); | ||
| }); | ||
| } | ||
|
|
||
| return []; | ||
| }); |
There was a problem hiding this comment.
The mock implementation of globby has a side effect of incrementing globbyCallCount. While this works for this specific test, it makes the mock less reusable and harder to understand. Consider using vi.spyOn to track the number of calls to globby instead of relying on a side effect within the mock implementation. This would make the test more robust and easier to maintain.
// Set up globby mock to simulate the conflict resolution
vi.mocked(globby).mockImplementation(async (patterns, options) => {
// First call is to find .gitignore files for pre-reading
if (patterns.includes('**/.gitignore')) {
return ['.gitignore', 'subdir/.gitignore'];
}
// Second call is the main file search
if (options?.ignore && patterns.includes('**/*')) {
// Simulate that .gitignore files are excluded but their patterns are applied
const allFiles = [
'file1.js',
'file2.ts',
'temp/ignored.js', // Should be ignored by .gitignore pattern
'app.log', // Should be ignored by .gitignore pattern
'subdir/code.js',
'subdir/temp.log', // Should be ignored by .gitignore pattern
'node_modules/package.json', // Should be ignored by .gitignore pattern
];
// Filter out files that would be ignored by .gitignore patterns
return allFiles.filter(file => {
return !file.endsWith('.log') &&
!file.startsWith('temp/') &&
!file.startsWith('node_modules/');
});
}
return [];
});| return allFiles.filter(file => { | ||
| return !file.endsWith('.log') && | ||
| !file.startsWith('temp/') && | ||
| !file.startsWith('node_modules/'); |
There was a problem hiding this comment.
This filtering logic is duplicated from the actual implementation. While this is a test, consider creating a helper function to encapsulate this logic. This would make the test more readable and easier to update if the filtering logic changes in the future.
const filterFiles = (files: string[]): string[] => {
return files.filter(file => {
return !file.endsWith('.log') &&
!file.startsWith('temp/') &&
!file.startsWith('node_modules/');
});
};
// Filter out files that would be ignored by .gitignore patterns
return filterFiles(allFiles);There was a problem hiding this comment.
Pull Request Overview
This PR ensures that when users explicitly ignore .gitignore files, their filtering rules are still applied to the file search.
- Detect and pre-read
.gitignorefiles that would be excluded by custom ignore patterns - Merge parsed ignore rules into
globby’signorelist while keeping.gitignorefiles out of the results - Add a test case covering
--ignore "**/.gitignore"behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/core/file/fileSearch.test.ts | New test to verify .gitignore files are excluded but their patterns still filter search results |
| src/core/file/fileSearch.ts | Logic to detect conflicting ignore-file patterns, pre-read their contents, and adjust globby options |
Comments suppressed due to low confidence (1)
src/core/file/fileSearch.ts:213
- [nitpick] The
globbyoptions block for file search is duplicated in the directory search block below. Extract the shared settings (e.g.,cwd,ignore,ignoreFiles,dot, etc.) into a common variable or helper to reduce code repetition.
const filePaths = await globby(includePatterns, {
| cwd: rootDir, | ||
| ignore: [...adjustedIgnorePatterns], | ||
| ignoreFiles: [...ignoreFilePatterns], | ||
| ignore: [...adjustedIgnorePatterns, ...extraIgnorePatterns], |
There was a problem hiding this comment.
Patterns extracted from .gitignore files are passed directly into extraIgnorePatterns without normalization. Consider applying normalizeGlobPattern (or a similar function) on each parsed pattern to ensure consistent glob matching (e.g., handling trailing slashes or escape sequences).
|
|
||
| // Pre-read conflicting ignore files and add their patterns to ignore list | ||
| const extraIgnorePatterns: string[] = []; | ||
| for (const pattern of conflictingIgnoreFiles) { |
There was a problem hiding this comment.
Reading and parsing each conflicting ignore file sequentially can become a bottleneck in large repositories. Consider using Promise.all or another parallel approach to read and process those files concurrently.
| for (const pattern of conflictingIgnoreFiles) { | |
| await Promise.all(conflictingIgnoreFiles.map(async (pattern) => { |
Fixes #624
This PR resolves the issue where
--ignore "**/.gitignore"incorrectly disables .gitignore filters instead of just excluding .gitignore files from output.Changes
Generated with Claude Code