-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: make search_files respect .gitignore files by default #7924
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 |
---|---|---|
|
@@ -18,6 +18,7 @@ export async function searchFilesTool( | |
const relDirPath: string | undefined = block.params.path | ||
const regex: string | undefined = block.params.regex | ||
const filePattern: string | undefined = block.params.file_pattern | ||
const includeIgnored: string | undefined = block.params.include_ignored | ||
|
||
const absolutePath = relDirPath ? path.resolve(cline.cwd, relDirPath) : cline.cwd | ||
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) | ||
|
@@ -27,6 +28,7 @@ export async function searchFilesTool( | |
path: getReadablePath(cline.cwd, removeClosingTag("path", relDirPath)), | ||
regex: removeClosingTag("regex", regex), | ||
filePattern: removeClosingTag("file_pattern", filePattern), | ||
includeIgnored: removeClosingTag("include_ignored", includeIgnored), | ||
isOutsideWorkspace, | ||
} | ||
|
||
|
@@ -52,12 +54,16 @@ export async function searchFilesTool( | |
|
||
cline.consecutiveMistakeCount = 0 | ||
|
||
// Parse includeIgnored as boolean (default to false) | ||
const shouldIncludeIgnored = includeIgnored === "true" | ||
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. Is this intentional that we only check for the exact string "true"? What happens if someone passes "yes", "1", or "TRUE"? Should we consider adding validation or at least normalizing the input to handle common boolean representations? |
||
|
||
const results = await regexSearchFiles( | ||
cline.cwd, | ||
absolutePath, | ||
regex, | ||
filePattern, | ||
cline.rooIgnoreController, | ||
shouldIncludeIgnored, | ||
) | ||
|
||
const completeMessage = JSON.stringify({ ...sharedMessageProps, content: results } satisfies ClineSayTool) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,3 +48,9 @@ describe("Ripgrep line truncation", () => { | |
expect(truncated).toContain("[truncated...]") | ||
}) | ||
}) | ||
|
||
// Note: Integration tests for gitignore handling would require actual file system setup | ||
// The implementation has been updated to: | ||
// 1. By default, ripgrep respects .gitignore files (no --no-ignore flag) | ||
// 2. When includeIgnored is true, it adds --no-ignore flag to include ignored files | ||
// This behavior is implemented in src/services/ripgrep/index.ts lines 154-159 | ||
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. While I appreciate my own comment about integration tests requiring file system setup, wouldn't it be better to actually write those tests? We could use a temporary directory structure to verify the gitignore behavior works correctly with nested .gitignore files. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,6 +142,7 @@ export async function regexSearchFiles( | |
regex: string, | ||
filePattern?: string, | ||
rooIgnoreController?: RooIgnoreController, | ||
includeIgnored: boolean = false, | ||
): Promise<string> { | ||
const vscodeAppRoot = vscode.env.appRoot | ||
const rgPath = await getBinPath(vscodeAppRoot) | ||
|
@@ -150,7 +151,14 @@ export async function regexSearchFiles( | |
throw new Error("Could not find ripgrep binary") | ||
} | ||
|
||
const args = ["--json", "-e", regex, "--glob", filePattern || "*", "--context", "1", "--no-messages", directoryPath] | ||
const args = ["--json", "-e", regex, "--glob", filePattern || "*", "--context", "1", "--no-messages"] | ||
|
||
// By default, ripgrep respects .gitignore files. Add --no-ignore to include ignored files | ||
if (includeIgnored) { | ||
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. A comment here explaining the design decision would help future maintainers (including future me who will forget why I did this). Something like: |
||
args.push("--no-ignore") | ||
} | ||
|
||
args.push(directoryPath) | ||
|
||
let output: string | ||
try { | ||
|
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.
Could we be more explicit here about the nested .gitignore behavior? Something like: "By default, the tool respects ALL .gitignore files in the directory tree (including nested ones), not just the root .gitignore." This would make the behavior crystal clear to users.