-
Notifications
You must be signed in to change notification settings - Fork 1
[EVAL] Fix/issue 7921 search files includes files ignored by nested gitignore #46
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
Draft
daniel-lxs
wants to merge
2
commits into
main
Choose a base branch
from
eval-pr-review-fix-test-and-path-resolution-bugs-1760371620922
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| import path from "path" | ||
| import fsSync from "fs" | ||
| import ignore, { Ignore } from "ignore" | ||
| import * as vscode from "vscode" | ||
|
|
||
| /** | ||
| * Base class for ignore controllers that provides common functionality | ||
| * for handling ignore patterns and file validation. | ||
| */ | ||
| export abstract class BaseIgnoreController { | ||
| protected cwd: string | ||
| protected ignoreInstance: Ignore | ||
| protected disposables: vscode.Disposable[] = [] | ||
|
|
||
| constructor(cwd: string) { | ||
| this.cwd = cwd | ||
| this.ignoreInstance = ignore() | ||
| } | ||
|
|
||
| /** | ||
| * Initialize the controller - must be implemented by subclasses | ||
| */ | ||
| abstract initialize(): Promise<void> | ||
|
|
||
| /** | ||
| * Check if a file should be accessible (not ignored by patterns) | ||
| * Automatically resolves symlinks | ||
| * @param filePath - Path to check (relative to cwd) | ||
| * @returns true if file is accessible, false if ignored | ||
| */ | ||
| validateAccess(filePath: string): boolean { | ||
| // Allow subclasses to override the "no patterns" check | ||
| if (!this.hasPatterns()) { | ||
| return true | ||
| } | ||
|
|
||
| try { | ||
| const absolutePath = path.resolve(this.cwd, filePath) | ||
|
|
||
| // Follow symlinks to get the real path | ||
| let realPath: string | ||
| try { | ||
| realPath = fsSync.realpathSync(absolutePath) | ||
| } catch { | ||
| // If realpath fails (file doesn't exist, broken symlink, etc.), | ||
| // use the original path | ||
| realPath = absolutePath | ||
| } | ||
|
|
||
| // Convert real path to relative for ignore checking | ||
| const relativePath = path.relative(this.cwd, realPath).toPosix() | ||
|
|
||
| // Check if the real path is ignored | ||
| return !this.ignoreInstance.ignores(relativePath) | ||
| } catch (error) { | ||
| // Allow access to files outside cwd or on errors (backward compatibility) | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Filter an array of paths, removing those that should be ignored | ||
| * @param paths - Array of paths to filter (relative to cwd) | ||
| * @returns Array of allowed paths | ||
| */ | ||
| filterPaths(paths: string[]): string[] { | ||
| try { | ||
| return paths | ||
| .map((p) => ({ | ||
| path: p, | ||
| allowed: this.validateAccess(p), | ||
| })) | ||
| .filter((x) => x.allowed) | ||
| .map((x) => x.path) | ||
| } catch (error) { | ||
| console.error("Error filtering paths:", error) | ||
| return [] // Fail closed for security | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Clean up resources when the controller is no longer needed | ||
| */ | ||
| dispose(): void { | ||
| this.disposables.forEach((d) => d.dispose()) | ||
| this.disposables = [] | ||
| } | ||
|
|
||
| /** | ||
| * Check if the controller has any patterns loaded | ||
| * Must be implemented by subclasses | ||
| */ | ||
| protected abstract hasPatterns(): boolean | ||
|
|
||
| /** | ||
| * Set up file watchers with debouncing to avoid rapid reloads | ||
| * @param pattern - VSCode RelativePattern for the files to watch | ||
| * @param reloadCallback - Function to call when files change | ||
| */ | ||
| protected setupFileWatcher(pattern: vscode.RelativePattern, reloadCallback: () => void): void { | ||
| const fileWatcher = vscode.workspace.createFileSystemWatcher(pattern) | ||
|
|
||
| // Debounce rapid changes | ||
| let reloadTimeout: NodeJS.Timeout | undefined | ||
| const debouncedReload = () => { | ||
| if (reloadTimeout) { | ||
| clearTimeout(reloadTimeout) | ||
| } | ||
| reloadTimeout = setTimeout(reloadCallback, 100) | ||
| } | ||
|
|
||
| // Watch for changes, creation, and deletion | ||
| this.disposables.push( | ||
| fileWatcher.onDidChange(debouncedReload), | ||
| fileWatcher.onDidCreate(debouncedReload), | ||
| fileWatcher.onDidDelete(debouncedReload), | ||
| ) | ||
|
|
||
| // Add fileWatcher itself to disposables | ||
| this.disposables.push(fileWatcher) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,230 @@ | ||
| import path from "path" | ||
| import { fileExistsAtPath } from "../../utils/fs" | ||
| import fs from "fs/promises" | ||
| import ignore from "ignore" | ||
| import * as vscode from "vscode" | ||
| import { BaseIgnoreController } from "./BaseIgnoreController" | ||
|
|
||
| /** | ||
| * Controls file access by enforcing nested .gitignore patterns. | ||
| * Handles multiple .gitignore files throughout the directory tree, unlike ripgrep which only honors top-level .gitignore. | ||
| * Designed to be instantiated once and passed to file manipulation services. | ||
| * Uses the 'ignore' library to support standard .gitignore syntax. | ||
| */ | ||
| export class GitIgnoreController extends BaseIgnoreController { | ||
| private gitignoreFiles: string[] = [] | ||
| private gitignoreContents: Map<string, string> = new Map() | ||
|
|
||
| constructor(cwd: string) { | ||
| super(cwd) | ||
| this.gitignoreFiles = [] | ||
| this.gitignoreContents = new Map() | ||
| } | ||
|
|
||
| /** | ||
| * Initialize the controller by discovering and loading all .gitignore files | ||
| * Must be called after construction and before using the controller | ||
| */ | ||
| async initialize(): Promise<void> { | ||
| await this.discoverAndLoadGitignoreFiles() | ||
| this.setupGitIgnoreWatchers() | ||
| } | ||
|
|
||
| /** | ||
| * Discover and load .gitignore files (root + common subdirectories) | ||
| */ | ||
| private async discoverAndLoadGitignoreFiles(): Promise<void> { | ||
| try { | ||
| // Reset state | ||
| this.ignoreInstance = ignore() | ||
| this.gitignoreFiles = [] | ||
| this.gitignoreContents.clear() | ||
|
|
||
| // Check for common .gitignore file locations (manually defined for simplicity) | ||
| const commonGitignorePaths = [ | ||
| path.join(this.cwd, ".gitignore"), // Root | ||
| path.join(this.cwd, "src", ".gitignore"), // src/ | ||
| path.join(this.cwd, "lib", ".gitignore"), // lib/ | ||
| path.join(this.cwd, "test", ".gitignore"), // test/ | ||
| path.join(this.cwd, "tests", ".gitignore"), // tests/ | ||
| ] | ||
|
|
||
| // Check each location and load if it exists | ||
| for (const gitignorePath of commonGitignorePaths) { | ||
| const exists = await fileExistsAtPath(gitignorePath) | ||
|
|
||
| if (exists) { | ||
| this.gitignoreFiles.push(gitignorePath) | ||
| await this.loadGitignoreFile(gitignorePath) | ||
| } | ||
| } | ||
|
|
||
| // Also discover arbitrary nested .gitignore files across the workspace | ||
| await this.findGitignoreFilesRecursively(this.cwd) | ||
|
|
||
| // Load any files discovered by recursion that weren't loaded yet | ||
| for (const p of this.gitignoreFiles) { | ||
| if (!this.gitignoreContents.has(p)) { | ||
| await this.loadGitignoreFile(p) | ||
| } | ||
| } | ||
|
|
||
| // Always ignore .gitignore files themselves | ||
| this.ignoreInstance.add(".gitignore") | ||
| } catch (error) { | ||
| console.error("Error discovering .gitignore files:", error) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Recursively find all .gitignore files in the directory tree | ||
| */ | ||
| private async findGitignoreFilesRecursively(dirPath: string): Promise<void> { | ||
| try { | ||
| // Skip the root directory since we already checked it in discoverAndLoadGitignoreFiles | ||
| if (dirPath === this.cwd) { | ||
| // Get all subdirectories | ||
| const entries = await fs.readdir(dirPath, { withFileTypes: true }) | ||
| const subdirs = entries | ||
| .filter((entry) => entry.isDirectory() && !entry.name.startsWith(".")) | ||
| .map((entry) => path.join(dirPath, entry.name)) | ||
|
|
||
| // Recursively search subdirectories | ||
| for (const subdir of subdirs) { | ||
| await this.findGitignoreFilesRecursively(subdir) | ||
| } | ||
| } else { | ||
| // For subdirectories, check for .gitignore and continue recursively | ||
| const gitignorePath = path.join(dirPath, ".gitignore") | ||
|
|
||
| // Check if .gitignore exists in current directory | ||
| if (await fileExistsAtPath(gitignorePath)) { | ||
| this.gitignoreFiles.push(gitignorePath) | ||
| } | ||
|
|
||
| // Get all subdirectories | ||
| const entries = await fs.readdir(dirPath, { withFileTypes: true }) | ||
| const subdirs = entries | ||
| .filter((entry) => entry.isDirectory() && !entry.name.startsWith(".")) | ||
| .map((entry) => path.join(dirPath, entry.name)) | ||
|
|
||
| // Recursively search subdirectories | ||
| for (const subdir of subdirs) { | ||
| await this.findGitignoreFilesRecursively(subdir) | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // Skip directories we can't read | ||
| console.debug(`Could not read directory ${dirPath}:`, error) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Load content from a specific .gitignore file | ||
| */ | ||
| private async loadGitignoreFile(gitignoreFile: string): Promise<void> { | ||
| try { | ||
| const content = await fs.readFile(gitignoreFile, "utf8") | ||
| this.gitignoreContents.set(gitignoreFile, content) | ||
|
|
||
| // Add patterns to ignore instance with proper context | ||
| // For nested .gitignore files, we need to adjust patterns relative to the workspace root | ||
| const relativeDir = path.relative(this.cwd, path.dirname(gitignoreFile)) | ||
|
|
||
| if (relativeDir) { | ||
| // For nested .gitignore files, we need to create patterns that match files within that directory | ||
| const lines = content.split(/\r?\n/).filter((line) => line.trim() && !line.startsWith("#")) | ||
| // Convert Windows paths to POSIX for consistent pattern matching | ||
| const normalizedRelativeDir = relativeDir.split(path.sep).join("/") | ||
|
|
||
| const adjustedPatterns = lines.flatMap((pattern) => { | ||
| const trimmed = pattern.trim() | ||
|
|
||
| if (trimmed.startsWith("/")) { | ||
| // Absolute patterns (starting with /) are relative to the .gitignore location | ||
| return [normalizedRelativeDir + trimmed] | ||
| } else if (trimmed.startsWith("!")) { | ||
| // Negation patterns | ||
| const negatedPattern = trimmed.slice(1) | ||
| if (negatedPattern.startsWith("/")) { | ||
| return ["!" + normalizedRelativeDir + negatedPattern] | ||
| } else { | ||
| // For relative negation patterns, match in the directory and subdirectories | ||
| return [ | ||
| "!" + normalizedRelativeDir + "/" + negatedPattern, | ||
| "!" + normalizedRelativeDir + "/**/" + negatedPattern, | ||
| ] | ||
| } | ||
| } else { | ||
| // Relative patterns - match files in the directory and all subdirectories | ||
| // For "*.tmp" in src/.gitignore, we need TWO patterns: | ||
| // - src/*.tmp (matches direct children like src/temp.tmp) | ||
| // - src/**/*.tmp (matches descendants like src/subdir/temp.tmp) | ||
| const patterns = [ | ||
| normalizedRelativeDir + "/" + trimmed, | ||
| normalizedRelativeDir + "/**/" + trimmed, | ||
| ] | ||
| return patterns | ||
| } | ||
| }) | ||
|
|
||
| this.ignoreInstance.add(adjustedPatterns) | ||
| } else { | ||
| // Root .gitignore file - add patterns as-is (like RooIgnoreController) | ||
| this.ignoreInstance.add(content) | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Could not read .gitignore at ${gitignoreFile}:`, error) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Set up file watchers for all .gitignore files in the workspace | ||
| */ | ||
| private setupGitIgnoreWatchers(): void { | ||
| // Create a watcher for .gitignore files throughout the workspace | ||
| const gitignorePattern = new vscode.RelativePattern(this.cwd, "**/.gitignore") | ||
| this.setupFileWatcher(gitignorePattern, () => this.discoverAndLoadGitignoreFiles()) | ||
| } | ||
|
|
||
| /** | ||
| * Check if the controller has any patterns loaded | ||
| */ | ||
| protected hasPatterns(): boolean { | ||
| return this.gitignoreFiles.length > 0 | ||
| } | ||
|
|
||
| /** | ||
| * Get all discovered .gitignore file paths | ||
| * @returns Array of absolute paths to .gitignore files | ||
| */ | ||
| getGitignoreFiles(): string[] { | ||
| return [...this.gitignoreFiles] | ||
| } | ||
|
|
||
| /** | ||
| * Get the content of a specific .gitignore file | ||
| * @param gitignoreFile - Absolute path to the .gitignore file | ||
| * @returns Content of the file or undefined if not found | ||
| */ | ||
| getGitignoreContent(gitignoreFile: string): string | undefined { | ||
| return this.gitignoreContents.get(gitignoreFile) | ||
| } | ||
|
|
||
| /** | ||
| * Check if any .gitignore files exist in the workspace | ||
| * @returns true if at least one .gitignore file exists | ||
| */ | ||
| hasGitignoreFiles(): boolean { | ||
| return this.gitignoreFiles.length > 0 | ||
| } | ||
|
|
||
| /** | ||
| * Clean up resources when the controller is no longer needed | ||
| */ | ||
| override dispose(): void { | ||
| super.dispose() | ||
| this.gitignoreContents.clear() | ||
| this.gitignoreFiles = [] | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 filter
!line.startsWith("#")incorrectly removes escaped literal patterns like\#foowhich should match files literally named#foo. Theignorelibrary handles escaped patterns correctly, but this pre-filter prevents them from reaching it. The test at line 308 ("should treat \# as a literal # pattern in nested .gitignore (exposes bug in line 136)") documents this exact issue.Consider using
!line.trim().startsWith("#")or checking for the escape sequence:!(line.startsWith("#") && !line.startsWith("\\#"))