diff --git a/lib/review-collect.ts b/lib/review-collect.ts index 63b26e6d7..ab1d9bcd7 100644 --- a/lib/review-collect.ts +++ b/lib/review-collect.ts @@ -39,7 +39,7 @@ function gitUntil(root: string, args: string[], deadline: number): { status: num const left = deadline - Date.now(); if (left <= 0) return { status: null, out: '', err: '', timedOut: true }; const r = spawnSync('git', ['-c', 'core.quotePath=false', ...args], { cwd: root, encoding: 'utf8', timeout: left, maxBuffer: 256 * 1024 * 1024 }); - return { status: r.status, out: r.stdout ?? '', err: (r.stderr ?? '').trim(), timedOut: r.status === null && Date.now() >= deadline }; + return { status: r.status, out: r.stdout ?? '', err: (r.stderr ?? '').trim() || ((r.error as NodeJS.ErrnoException | undefined)?.code ?? ''), timedOut: r.status === null && Date.now() >= deadline }; } const scratch = (p: string) => p.startsWith('.gstack/tmp/'); const oneLine = (s: string) => s.replace(/[\0-\x1f]+/g, ' ').trim().slice(0, 200); @@ -123,15 +123,21 @@ export type GrepFailure = { timedOut: boolean; status: number | null; err: strin * The lines `git grep -I -F -o --null` prints for `patterns` at `rev` ('' is the working tree with untracked files), for * the files `keep` accepts, in some order. Every pattern holds its own extension, so a `-l` pass for the extensions alone * lists every file that can match; the full pattern list then reads only those files, CONSUMER_SCAN_PATHSPEC_BYTES of - * paths per call under --literal-pathspecs (an inherited GIT_GLOB_PATHSPECS makes git refuse, which fails closed). Each call gets what is left of `deadline` (epoch ms). + * paths per call under --literal-pathspecs (an inherited GIT_GLOB_PATHSPECS makes git refuse, which fails closed). A listed name + * that is not valid UTF-8 cannot be passed back as a path, so then one grep reads the whole tree, as the scan did before GST-544. Each call gets what is left of `deadline` (epoch ms). */ export function grepConsumers(root: string, patterns: readonly string[], rev: string, keep: (file: string) => boolean, deadline: number, chunkBytes = CONSUMER_SCAN_PATHSPEC_BYTES): string[] | GrepFailure { const where = rev ? [rev, '--'] : ['--untracked', '--']; const exts = [...new Set(patterns.map(b => b.slice(b.lastIndexOf('.'))))].flatMap(e => ['-e', e]); const listed = gitUntil(root, ['grep', '-I', '-F', '-l', '-z', '--no-color', ...exts, ...where], deadline); if (listed.status !== 0 && listed.status !== 1) return listed; - const files = listed.out.split('\0').filter(Boolean).map(f => (rev ? f.slice(rev.length + 1) : f)).filter(keep); + const names = listed.out.split('\0').filter(Boolean).map(f => (rev ? f.slice(rev.length + 1) : f)); const flags = ['--literal-pathspecs', 'grep', '-I', '-F', '-o', '--null', '--no-line-number', '--no-column', '--no-color', ...patterns.flatMap(b => ['-e', b]), ...where]; + if (names.some(f => f.includes('\uFFFD'))) { + const whole = gitUntil(root, flags, deadline); + return whole.status !== 0 && whole.status !== 1 ? whole : whole.out.split('\n'); + } + const files = names.filter(keep); const lines: string[] = []; for (let i = 0; i < files.length;) { const chunk: string[] = []; @@ -143,7 +149,7 @@ export function grepConsumers(root: string, patterns: readonly string[], rev: st } const r = gitUntil(root, [...flags, ...chunk], deadline); if (r.status !== 0 && r.status !== 1) return r; - lines.push(...r.out.split('\n')); + for (const line of r.out.split('\n')) lines.push(line); } return lines; } diff --git a/test/review-collect.test.ts b/test/review-collect.test.ts index a02b5a3a1..3c14b2887 100644 --- a/test/review-collect.test.ts +++ b/test/review-collect.test.ts @@ -247,13 +247,32 @@ describe('GST-544: the consumer scan reads only files that can match, under one // Value: protects=an inherited GIT_LITERAL_PATHSPECS (git --literal-pathspecs exports it to children) cannot empty the narrowed // search and drop data to docs; a fractional override cannot crash spawnSync; fails_when=the pathspecs use magic the variable // turns off, or the timeout is not an integer; seam=the process environment - test('an inherited GIT_LITERAL_PATHSPECS keeps the same lines; a fractional timeout override is whole milliseconds', () => { + test('an inherited GIT_LITERAL_PATHSPECS keeps the same lines; a fractional timeout is whole milliseconds; a spawn error is named', () => { const r = mixed(); withEnv('GIT_LITERAL_PATHSPECS', '1', () => { for (const at of [r.head, '']) expect((grepConsumers(r.dir, patterns, at, () => true, Date.now() + 10_000) as string[]).filter(Boolean).sort()).toEqual(wholeTree(r.dir, at)); }); withEnv('GSTACK_CONSUMER_SCAN_TIMEOUT_MS', '1500.5', () => expect(consumerScanTimeoutMs()).toBe(1500)); + const errors: Parameters[5] = []; + withEnv('PATH', path.join(r.dir, 'no-bin'), () => expect(scanConsumers(r.dir, ['data/rows.json'], [r.head], false, r.dir, errors, 10_000)).toBe('all')); + expect(errors[0]!.detail).toMatch(/^git grep exited null: .*ENOENT/); }); + // Value: protects=a consumer whose name is not valid UTF-8 still counts, and a file with a million references does not crash the + // planner; fails_when=the -l pass hands back a mangled name as a pathspec, or the matches are spread into one call; + // seam=git tree objects and git grep output size + test('a consumer named with invalid UTF-8 bytes still counts; a million matches in one file are all returned', () => { + const r = repo({ 'src/keep.ts': 'x\n' }); + const head = r.commit({ 'data/results.json': rows }); + const run = (args: string[], input?: Buffer) => { const x = spawnSync('git', args, { cwd: r.dir, input, timeout: 10_000 }); if (x.status !== 0) throw new Error(`git ${args.join(' ')}: ${x.stderr}`); return x.stdout as Buffer; }; + const sha = run(['hash-object', '-w', '--stdin'], Buffer.from("read('results.json')\n")).toString().trim(); + const entry = Buffer.concat([Buffer.from(`100644 blob ${sha}\tloader`), Buffer.from([0xff]), Buffer.from('.py\0')]); + const tree = run(['mktree', '-z'], Buffer.concat([run(['ls-tree', '-z', head]), entry])).toString().trim(); + const named = run(['commit-tree', tree, '-p', head, '-m', 'consumer'], Buffer.alloc(0)).toString().trim(); + expect(collectCommit(r.dir, r.baseSha, named).consumed).toEqual(['data/results.json']); + r.write({ 'src/many.ts': 'rows.json\n'.repeat(1_100_000) }); + const got = grepConsumers(r.dir, ['rows.json'], '', () => true, Date.now() + 30_000); + expect(Array.isArray(got) && got.filter(Boolean).length).toBe(1_100_000); + }, 30_000); // Value: protects=a slow scan ends in its own fail-closed consumer_scan_failed, not in the parent killing gstack-review-plan; // fails_when=the scan deadline reaches the timeout the dispatch and preflight callers give the whole plan; seam=caller source test('the default scan deadline leaves a minute of the plan timeout its callers set', () => {