From 205494b7ba3e1bc97c10409b635c2e3f44e40959 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Wed, 3 Jun 2026 01:11:37 -0400 Subject: [PATCH] Fix file search match parsing (#4542) Co-authored-by: Orca --- src/main/ipc/filesystem-search-git.test.ts | 15 ++- src/relay/fs-handler-git-search.test.ts | 2 +- src/shared/text-search.test.ts | 124 +++++++++++++++++++-- src/shared/text-search.ts | 60 ++++++---- 4 files changed, 163 insertions(+), 38 deletions(-) diff --git a/src/main/ipc/filesystem-search-git.test.ts b/src/main/ipc/filesystem-search-git.test.ts index 3f29e3c10..a828a4ac6 100644 --- a/src/main/ipc/filesystem-search-git.test.ts +++ b/src/main/ipc/filesystem-search-git.test.ts @@ -43,9 +43,12 @@ describe('filesystem-search-git', () => { setTimeout(() => { ;(proc.stdout as unknown as EventEmitter).emit( 'data', - 'src/index.ts\x005: console.log("hello world")\n' + 'src/index.ts\x005\x00 console.log("hello world")\n' + ) + ;(proc.stdout as unknown as EventEmitter).emit( + 'data', + 'src/main.ts\x0012\x00 return "hello"\n' ) - ;(proc.stdout as unknown as EventEmitter).emit('data', 'src/main.ts\x0012: return "hello"\n') proc.emit('close') }, 10) @@ -74,7 +77,7 @@ describe('filesystem-search-git', () => { const promise = searchWithGitGrep('/mock/root', { query: 'ab', rootPath: '/mock/root' }, 100) setTimeout(() => { - ;(proc.stdout as unknown as EventEmitter).emit('data', 'file.txt\x001:ab cd ab ef ab\n') + ;(proc.stdout as unknown as EventEmitter).emit('data', 'file.txt\x001\x00ab cd ab ef ab\n') proc.emit('close') }, 10) @@ -98,7 +101,7 @@ describe('filesystem-search-git', () => { setTimeout(() => { ;(proc.stdout as unknown as EventEmitter).emit( 'data', - 'a.ts\x001:x\n' + 'b.ts\x001:x\n' + 'c.ts\x001:x\n' + 'a.ts\x001\x00x\n' + 'b.ts\x001\x00x\n' + 'c.ts\x001\x00x\n' ) proc.emit('close') }, 10) @@ -177,7 +180,7 @@ describe('filesystem-search-git', () => { const promise = searchWithGitGrep('/mock/root', { query: 'ok', rootPath: '/mock/root' }, 100) - ;(proc.stdout as unknown as EventEmitter).emit('data', 'valid.ts\x001:ok\npartial') + ;(proc.stdout as unknown as EventEmitter).emit('data', 'valid.ts\x001\x00ok\npartial') await vi.runOnlyPendingTimersAsync() @@ -203,7 +206,7 @@ describe('filesystem-search-git', () => { setTimeout(() => { // A line without \0 should be skipped (e.g. git header output) ;(proc.stdout as unknown as EventEmitter).emit('data', 'no-null-here:1:ok\n') - ;(proc.stdout as unknown as EventEmitter).emit('data', 'valid.ts\x003:ok\n') + ;(proc.stdout as unknown as EventEmitter).emit('data', 'valid.ts\x003\x00ok\n') proc.emit('close') }, 10) diff --git a/src/relay/fs-handler-git-search.test.ts b/src/relay/fs-handler-git-search.test.ts index 0272392cd..7b3654578 100644 --- a/src/relay/fs-handler-git-search.test.ts +++ b/src/relay/fs-handler-git-search.test.ts @@ -39,7 +39,7 @@ describe('relay git grep fallback', () => { const promise = searchWithGitGrep('/remote/root', 'ok', { maxResults: 100 }) - ;(proc.stdout as unknown as EventEmitter).emit('data', 'valid.ts\x001:ok\npartial') + ;(proc.stdout as unknown as EventEmitter).emit('data', 'valid.ts\x001\x00ok\npartial') await vi.runOnlyPendingTimersAsync() diff --git a/src/shared/text-search.test.ts b/src/shared/text-search.test.ts index 3ad5cc462..3a0498944 100644 --- a/src/shared/text-search.test.ts +++ b/src/shared/text-search.test.ts @@ -1,4 +1,8 @@ import { describe, expect, it } from 'vitest' +import { execFileSync } from 'child_process' +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs' +import { tmpdir } from 'os' +import { join } from 'path' import { buildGitGrepArgs, buildRgArgs, @@ -112,6 +116,22 @@ describe('ingestRgJsonLine', () => { expect(acc.totalMatches).toBe(0) }) + it('creates a navigable fallback match when rg omits submatch ranges', () => { + const acc = createAccumulator() + const verdict = ingestRgJsonLine(makeMatch('/root/a.ts', 4, [], 'foobar'), '/root', acc, 100) + expect(verdict).toBe('continue') + expect(acc.totalMatches).toBe(1) + const file = Array.from(acc.fileMap.values())[0] + expect(file.matches).toEqual([{ line: 4, column: 1, matchLength: 1, lineContent: 'foobar' }]) + }) + + it('keeps empty-line rg matches navigable when rg omits submatch ranges', () => { + const acc = createAccumulator() + ingestRgJsonLine(makeMatch('/root/a.ts', 5, [], ''), '/root', acc, 100) + const file = Array.from(acc.fileMap.values())[0] + expect(file.matches).toEqual([{ line: 5, column: 1, matchLength: 0, lineContent: '' }]) + }) + it('stops at maxResults and sets truncated synchronously', () => { const acc = createAccumulator() const verdict = ingestRgJsonLine( @@ -241,10 +261,49 @@ describe('buildSubmatchRegex', () => { }) describe('ingestGitGrepLine', () => { - it('parses null-byte delimited line, finds all submatch positions', () => { + it('parses actual git grep null-delimited output from the current git binary', () => { + const rootPath = mkdtempSync(join(tmpdir(), 'orca-search-git-')) + try { + execFileSync('git', ['init'], { cwd: rootPath, stdio: 'ignore' }) + mkdirSync(join(rootPath, 'src')) + writeFileSync( + join(rootPath, 'src', 'a.ts'), + [ + "reportError(err, { action: 'save' })", + 'reportError(err); reportError(next)', + 'unrelated' + ].join('\n') + ) + + const stdout = execFileSync( + 'git', + buildGitGrepArgs('reportError(', { caseSensitive: false, useRegex: false }), + { cwd: rootPath, encoding: 'utf8' } + ) + const acc = createAccumulator() + const re = buildSubmatchRegex('reportError(', {}) + for (const line of stdout.split('\n')) { + ingestGitGrepLine(line, rootPath, re, acc, 100) + } + + const result = finalize(acc) + expect(result.totalMatches).toBe(3) + expect(result.files).toHaveLength(1) + expect(result.files[0].relativePath).toBe('src/a.ts') + expect(result.files[0].matches.map((match) => [match.line, match.column])).toEqual([ + [1, 1], + [2, 1], + [2, 19] + ]) + } finally { + rmSync(rootPath, { recursive: true, force: true }) + } + }) + + it('parses git grep null-delimited line, finds all submatch positions', () => { const acc = createAccumulator() const re = buildSubmatchRegex('foo', {}) - const verdict = ingestGitGrepLine('src/a.ts\x005:foo and foo again\n', '/root', re, acc, 100) + const verdict = ingestGitGrepLine('src/a.ts\x005\x00foo and foo again\n', '/root', re, acc, 100) expect(verdict).toBe('continue') const f = Array.from(acc.fileMap.values())[0] expect(f.matches).toHaveLength(2) @@ -252,10 +311,33 @@ describe('ingestGitGrepLine', () => { expect(f.matches[1]).toMatchObject({ line: 5, column: 9 }) }) + it('keeps compatibility with colon-delimited git grep lines', () => { + const acc = createAccumulator() + const re = buildSubmatchRegex('foo', {}) + ingestGitGrepLine('src/a.ts\x005:foo', '/root', re, acc, 100) + const f = Array.from(acc.fileMap.values())[0] + expect(f.matches[0]).toMatchObject({ line: 5, column: 1 }) + }) + + it('does not treat colons in matched content as the line-number delimiter', () => { + const acc = createAccumulator() + const re = buildSubmatchRegex('reportError(', {}) + ingestGitGrepLine( + "src/a.ts\x0010\x00reportError(err, { action: 'save' })\n", + '/root', + re, + acc, + 100 + ) + const f = Array.from(acc.fileMap.values())[0] + expect(f.matches).toHaveLength(1) + expect(f.matches[0]).toMatchObject({ line: 10, column: 1, matchLength: 12 }) + }) + it('handles colons in filenames via null delimiter', () => { const acc = createAccumulator() const re = buildSubmatchRegex('x', {}) - ingestGitGrepLine('weird:name.ts\x001:x', '/root', re, acc, 100) + ingestGitGrepLine('weird:name.ts\x001\x00x', '/root', re, acc, 100) const f = Array.from(acc.fileMap.values())[0] expect(f.relativePath).toBe('weird:name.ts') }) @@ -273,7 +355,7 @@ describe('ingestGitGrepLine', () => { const acc = createAccumulator() // A pattern that matches zero-length at every position. const re = new RegExp('', 'g') - ingestGitGrepLine('a.ts\x001:abc', '/r', re, acc, 5) + ingestGitGrepLine('a.ts\x001\x00abc', '/r', re, acc, 5) expect(acc.totalMatches).toBeGreaterThan(0) expect(acc.totalMatches).toBeLessThanOrEqual(5) }) @@ -281,7 +363,7 @@ describe('ingestGitGrepLine', () => { it('stops at maxResults boundary and sets truncated synchronously', () => { const acc = createAccumulator() const re = buildSubmatchRegex('a', {}) - const verdict = ingestGitGrepLine('f\x001:aaaa', '/r', re, acc, 2) + const verdict = ingestGitGrepLine('f\x001\x00aaaa', '/r', re, acc, 2) expect(verdict).toBe('stop') expect(acc.truncated).toBe(true) expect(acc.totalMatches).toBe(2) @@ -289,7 +371,7 @@ describe('ingestGitGrepLine', () => { it('falls back to whole-line highlight when submatchRegex is null', () => { const acc = createAccumulator() - const verdict = ingestGitGrepLine('a.ts\x003:hello world', '/r', null, acc, 100) + const verdict = ingestGitGrepLine('a.ts\x003\x00hello world', '/r', null, acc, 100) expect(verdict).toBe('continue') const f = Array.from(acc.fileMap.values())[0] expect(f.matches).toHaveLength(1) @@ -305,13 +387,35 @@ describe('ingestGitGrepLine', () => { describe('finalize', () => { it('returns the expected SearchResult shape', () => { const acc = createAccumulator() - acc.fileMap.set('/r/a.ts', { filePath: '/r/a.ts', relativePath: 'a.ts', matches: [] }) - acc.totalMatches = 3 + acc.fileMap.set('/r/a.ts', { + filePath: '/r/a.ts', + relativePath: 'a.ts', + matches: [{ line: 1, column: 1, matchLength: 3, lineContent: 'foo' }] + }) + acc.totalMatches = 1 acc.truncated = true expect(finalize(acc)).toEqual({ - files: [{ filePath: '/r/a.ts', relativePath: 'a.ts', matches: [] }], - totalMatches: 3, + files: [ + { + filePath: '/r/a.ts', + relativePath: 'a.ts', + matches: [{ line: 1, column: 1, matchLength: 3, lineContent: 'foo' }] + } + ], + totalMatches: 1, truncated: true }) }) + + it('filters impossible empty file rows before returning results', () => { + const acc = createAccumulator() + acc.fileMap.set('/r/a.ts', { filePath: '/r/a.ts', relativePath: 'a.ts', matches: [] }) + acc.fileMap.set('/r/b.ts', { + filePath: '/r/b.ts', + relativePath: 'b.ts', + matches: [{ line: 1, column: 1, matchLength: 3, lineContent: 'foo' }] + }) + acc.totalMatches = 1 + expect(finalize(acc).files.map((file) => file.relativePath)).toEqual(['b.ts']) + }) }) diff --git a/src/shared/text-search.ts b/src/shared/text-search.ts index ee47bf073..c3cc43ca2 100644 --- a/src/shared/text-search.ts +++ b/src/shared/text-search.ts @@ -237,15 +237,19 @@ export function ingestRgJsonLine( const relPath = normalizeRelativePath(relative(rootPath, absPath)) const lineContent = (data.lines?.text ?? '').replace(/\n$/, '') const lineNumber = data.line_number ?? 0 - const submatches = data.submatches ?? [] - - let fileResult = acc.fileMap.get(absPath) - if (!fileResult) { - fileResult = { filePath: absPath, relativePath: relPath, matches: [] } - acc.fileMap.set(absPath, fileResult) + let submatches = data.submatches ?? [] + if (submatches.length === 0) { + // Why: some rg regex matches report the line but no submatch ranges. + // Surface a navigable line-level result instead of a file row with count 0. + submatches = [{ start: 0, end: lineContent.length > 0 ? 1 : 0 }] } for (const sub of submatches) { + let fileResult = acc.fileMap.get(absPath) + if (!fileResult) { + fileResult = { filePath: absPath, relativePath: relPath, matches: [] } + acc.fileMap.set(absPath, fileResult) + } const clamped = clampLineContext(lineContent, sub.start, sub.end - sub.start) fileResult.matches.push({ line: lineNumber, @@ -386,28 +390,42 @@ export function ingestGitGrepLine( return 'continue' } - // Why: with --null -n the output format is filename\0linenum:content. + // Why: with --null -n, modern git emits filename\0linenum\0content. + // Keep the older colon parser too so relay hosts with different git output + // remain searchable. const nullIdx = line.indexOf('\0') if (nullIdx === -1) { return 'continue' } const relPath = normalizeRelativePath(line.substring(0, nullIdx)) const rest = line.substring(nullIdx + 1) - const colonIdx = rest.indexOf(':') - if (colonIdx === -1) { + const secondNullIdx = rest.indexOf('\0') + let lineNumberText: string + let lineContent: string + if (secondNullIdx >= 0) { + lineNumberText = rest.substring(0, secondNullIdx) + lineContent = rest.substring(secondNullIdx + 1).replace(/\n$/, '') + } else { + const colonIdx = rest.indexOf(':') + if (colonIdx === -1) { + return 'continue' + } + lineNumberText = rest.substring(0, colonIdx) + lineContent = rest.substring(colonIdx + 1).replace(/\n$/, '') + } + if (!/^\d+$/.test(lineNumberText)) { return 'continue' } - const lineNum = parseInt(rest.substring(0, colonIdx), 10) - if (isNaN(lineNum)) { - return 'continue' - } - const lineContent = rest.substring(colonIdx + 1).replace(/\n$/, '') + const lineNum = Number(lineNumberText) const absPath = join(rootPath, relPath) - let fileResult = acc.fileMap.get(absPath) - if (!fileResult) { - fileResult = { filePath: absPath, relativePath: relPath, matches: [] } - acc.fileMap.set(absPath, fileResult) + const getFileResult = (): SearchFileResult => { + let fileResult = acc.fileMap.get(absPath) + if (!fileResult) { + fileResult = { filePath: absPath, relativePath: relPath, matches: [] } + acc.fileMap.set(absPath, fileResult) + } + return fileResult } // Why: git grep already confirmed the line matched — if we can't build a @@ -416,7 +434,7 @@ export function ingestGitGrepLine( // whole-line highlight so the result still shows up in the UI. if (submatchRegex === null) { const clamped = clampLineContext(lineContent, 0, lineContent.length) - fileResult.matches.push({ + getFileResult().matches.push({ line: lineNum, column: clamped.column, matchLength: clamped.matchLength, @@ -438,7 +456,7 @@ export function ingestGitGrepLine( let m: RegExpExecArray | null while ((m = submatchRegex.exec(lineContent)) !== null) { const clamped = clampLineContext(lineContent, m.index, m[0].length) - fileResult.matches.push({ + getFileResult().matches.push({ line: lineNum, column: clamped.column, matchLength: clamped.matchLength, @@ -465,7 +483,7 @@ export function ingestGitGrepLine( export function finalize(acc: SearchAccumulator): SearchResult { return { - files: Array.from(acc.fileMap.values()), + files: Array.from(acc.fileMap.values()).filter((file) => file.matches.length > 0), totalMatches: acc.totalMatches, truncated: acc.truncated }