diff --git a/lib/review-collect.ts b/lib/review-collect.ts index ca052d958..63b26e6d7 100644 --- a/lib/review-collect.ts +++ b/lib/review-collect.ts @@ -114,7 +114,7 @@ export const CONSUMER_SCAN_PATHSPEC_BYTES = 16_000; export function consumerScanTimeoutMs(): number { const v = Number(process.env.GSTACK_CONSUMER_SCAN_TIMEOUT_MS); - return Number.isFinite(v) && v > 0 ? v : CONSUMER_SCAN_TIMEOUT_MS; + return Number.isFinite(v) && v >= 1 ? Math.floor(v) : CONSUMER_SCAN_TIMEOUT_MS; } export type GrepFailure = { timedOut: boolean; status: number | null; err: string }; @@ -123,7 +123,7 @@ 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 - * literal pathspecs per call. 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). 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', '--']; @@ -131,12 +131,12 @@ export function grepConsumers(root: string, patterns: readonly string[], rev: st 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 flags = ['grep', '-I', '-F', '-o', '--null', '--no-line-number', '--no-column', '--no-color', ...patterns.flatMap(b => ['-e', b]), ...where]; + const flags = ['--literal-pathspecs', 'grep', '-I', '-F', '-o', '--null', '--no-line-number', '--no-column', '--no-color', ...patterns.flatMap(b => ['-e', b]), ...where]; const lines: string[] = []; for (let i = 0; i < files.length;) { const chunk: string[] = []; for (let bytes = 0; i < files.length; i++) { - const spec = `:(literal)${files[i]}`; + const spec = files[i]!; if (chunk.length && bytes + spec.length > chunkBytes) break; chunk.push(spec); bytes += spec.length; diff --git a/test/review-collect.test.ts b/test/review-collect.test.ts index fc2e364ca..a02b5a3a1 100644 --- a/test/review-collect.test.ts +++ b/test/review-collect.test.ts @@ -3,7 +3,7 @@ import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; -import { collectCommit, collectLive, CONSUMER_SCAN_CAP, CONSUMER_SCAN_TIMEOUT_MS, grepConsumers, loadRegistryAt, scanConsumers } from '../lib/review-collect'; +import { collectCommit, collectLive, CONSUMER_SCAN_CAP, CONSUMER_SCAN_TIMEOUT_MS, consumerScanTimeoutMs, grepConsumers, loadRegistryAt, scanConsumers } from '../lib/review-collect'; import { depthOfCommitInput } from '../lib/review-depth'; const dirs: string[] = []; @@ -239,6 +239,21 @@ describe('GST-544: the consumer scan reads only files that can match, under one expect(scanConsumers(r.dir, ['data/rows.json'], [r.head], false, r.dir, failed, CONSUMER_SCAN_TIMEOUT_MS)).toBe('all'); expect(failed[0]!.detail).toMatch(new RegExp(`^git grep exited \\d+: .*\\(deadline ${CONSUMER_SCAN_TIMEOUT_MS / 1000} s\\)$`)); }); + const withEnv = (name: string, value: string, fn: () => void) => { + const saved = process.env[name]; + process.env[name] = value; + try { fn(); } finally { if (saved === undefined) delete process.env[name]; else process.env[name] = saved; } + }; + // 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', () => { + 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)); + }); // 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', () => {