-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: handle invalid gitignore patterns gracefully during codebase indexing #6882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -315,16 +315,49 @@ export class CodeIndexManager { | |
| const ignorePath = path.join(workspacePath, ".gitignore") | ||
| try { | ||
| const content = await fs.readFile(ignorePath, "utf8") | ||
| ignoreInstance.add(content) | ||
| ignoreInstance.add(".gitignore") | ||
|
|
||
| // Try to add the gitignore patterns, but handle invalid regex patterns gracefully | ||
| try { | ||
| ignoreInstance.add(content) | ||
| ignoreInstance.add(".gitignore") | ||
| } catch (ignoreError) { | ||
| // Log warning about invalid patterns but continue with indexing | ||
| console.warn( | ||
| `Warning: .gitignore contains invalid patterns that could not be parsed. Some files may not be properly ignored during indexing. Error: ${ | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The warning message could be more actionable. Is it intentional that we don't parse out which specific pattern caused the initial bulk parsing to fail? It might help users fix their .gitignore file more easily if we could identify the problematic pattern in the first error message. |
||
| ignoreError instanceof Error ? ignoreError.message : String(ignoreError) | ||
| }`, | ||
| ) | ||
|
|
||
| // Try to add individual lines to identify and skip problematic patterns | ||
| const lines = content.split("\n") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be cleaner to extract this line-by-line parsing logic into a separate helper method? Something like: This would improve readability and make the method easier to test in isolation. |
||
| for (const line of lines) { | ||
| const trimmedLine = line.trim() | ||
| // Skip empty lines and comments | ||
| if (!trimmedLine || trimmedLine.startsWith("#")) { | ||
| continue | ||
| } | ||
|
|
||
| try { | ||
| // Create a new ignore instance to test each pattern | ||
| const testIgnore = ignore() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Creating a new ignore instance for each pattern validation could be inefficient for large .gitignore files. Could we consider reusing a single test instance or implementing a more efficient validation approach? |
||
| testIgnore.add(trimmedLine) | ||
| // If successful, add to the main instance | ||
| ignoreInstance.add(trimmedLine) | ||
| } catch (lineError) { | ||
| console.warn(`Skipping invalid .gitignore pattern: "${trimmedLine}"`) | ||
| } | ||
| } | ||
|
|
||
| // Always add .gitignore itself to the ignore list | ||
| try { | ||
| ignoreInstance.add(".gitignore") | ||
| } catch { | ||
| // Even this basic pattern failed, but continue anyway | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // Should never happen: reading file failed even though it exists | ||
| console.error("Unexpected error loading .gitignore:", error) | ||
| TelemetryService.instance.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| stack: error instanceof Error ? error.stack : undefined, | ||
| location: "_recreateServices", | ||
| }) | ||
| // File reading error - .gitignore might not exist or be inaccessible | ||
| console.info(".gitignore file not found or could not be read, proceeding without gitignore patterns") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor inconsistency: we're using here but for warnings above. Should we consider using a consistent logging approach or perhaps the VSCode output channel for better user visibility? |
||
| } | ||
|
|
||
| // (Re)Create shared service instances | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test coverage is comprehensive! Could we also add an edge case test for .gitignore files with extremely long lines or unusual encoding? This would ensure our solution is robust against all possible user inputs.