From 96da79fbe1e49e9608ce9340d414d73f638fe7dc Mon Sep 17 00:00:00 2001 From: JuanMa Date: Mon, 7 Sep 2026 13:20:32 +0200 Subject: [PATCH 1/5] Add the runner and the read parsers for the bundled Git, with no callers Phase 2 of #364 (#384) starts with the two pieces every later phase reuses and nothing depends on yet. src/git-run.cjs spawns the binary and nothing else: argv from git-binary.cjs, an explicit cwd or a TypeError, --no-optional-locks so a read never writes .git/index, safe.directory pinned to the one directory in use, stdout as bytes under a cap that becomes an error instead of an out-of-memory, and a GitError that carries the exit code, stderr and the arguments. spawn rather than execFile, so the child can be streamed and killed by a later phase. src/git-read.cjs holds one parser per porcelain format the app will read (status --porcelain=v2 -z, diff --name-status -z, cat-file --batch, ls-tree -z, for-each-ref) and the read functions that return the shapes the isomorphic-git calls return today, status rows included, so the facades can swap engines without changing signature. crlfArgs carries the Windows-only autocrlf view createCrlfCompatibleFs gives isomorphic-git. The git() and removeRepo() test helpers move to tests/unit/helpers/git.cjs so the new tests and the existing integration test share them. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01J4uRM5JosAcMbKXowaXyVu --- src/git-read.cjs | 395 +++++++++++++++++++++ src/git-run.cjs | 188 ++++++++++ tests/unit/git-binary.integration.test.cjs | 40 +-- tests/unit/git-read.test.cjs | 210 +++++++++++ tests/unit/git-run.test.cjs | 166 +++++++++ tests/unit/helpers/git.cjs | 70 ++++ 6 files changed, 1030 insertions(+), 39 deletions(-) create mode 100644 src/git-read.cjs create mode 100644 src/git-run.cjs create mode 100644 tests/unit/git-read.test.cjs create mode 100644 tests/unit/git-run.test.cjs create mode 100644 tests/unit/helpers/git.cjs diff --git a/src/git-read.cjs b/src/git-read.cjs new file mode 100644 index 00000000..f85814eb --- /dev/null +++ b/src/git-read.cjs @@ -0,0 +1,395 @@ +'use strict'; + +/** + * Reads through the bundled Git (#384): every question the app asks a + * repository without changing it, and the parser for each answer. + * + * Every command here asks for porcelain-stable output with the flag that pins + * it (`--porcelain=v2`, `-z`, an explicit `--format`), and every parser is a + * pure function over the bytes so it can be tested on fixture strings without + * a repository. The read functions return the same shapes the isomorphic-git + * calls they replace returned, so the facades that call them + * (trunk-update.js, ticket-branches.js, pr-files.cjs, main.js) keep their + * signatures and the renderer does not know the engine changed. + * + * Status rows keep the `[path, head, workdir, stage]` shape documented in + * git-update.cjs. The mapping from `status --porcelain=v2` is at + * `rowFromStatusEntry`; the one known divergence from isomorphic-git is + * described there. + */ + +const { runGit } = require('./git-run.cjs'); + +const NUL = 0; + +/** + * NUL-separated fields, as Buffers. A trailing NUL terminates the last field + * rather than opening an empty one. + * + * @param {Buffer} buf + * @return {Buffer[]} + */ +function splitNul(buf) { + const fields = []; + let start = 0; + for (let i = 0; i < buf.length; i++) { + if (buf[i] === NUL) { + fields.push(buf.subarray(start, i)); + start = i + 1; + } + } + if (start < buf.length) fields.push(buf.subarray(start)); + return fields; +} + +/** + * The row `status --porcelain=v2` describes, in statusMatrix's vocabulary. + * X is index against HEAD, Y is worktree against index; `.` means unchanged. + * + * head 0 absent from HEAD, 1 present + * workdir 0 absent from disk, 1 identical to HEAD, 2 different + * stage 0 absent from index, 1 identical to HEAD, 2 staged change, + * 3 staged change with further unstaged edits + * + * Known divergence: a file staged and then edited back to its HEAD content is + * `workdir = 2` here (Y reports it as modified against the index) where + * isomorphic-git, which hashes, said 1. Every consumer that acts on the file + * byte-compares afterwards (isCrlfOnlyChange, classifyChangedFile), so only a + * count can differ, and only for a user who staged with their own client. + * + * @param {string} xy + * @param {string} filepath + * @return {Array} + */ +function rowFromStatusEntry(xy, filepath) { + const x = xy[0]; + const y = xy[1]; + const head = x === 'A' ? 0 : 1; + let workdir; + if (y === 'D') workdir = 0; + else if (x === '.' && y === '.') workdir = 1; + else workdir = 2; + let stage; + if (x === '.') stage = head; + else if (x === 'D') stage = 0; + else if (y === '.') stage = 2; + else stage = 3; + return [filepath, head, workdir, stage]; +} + +/** + * `git status --porcelain=v2 -z` → status rows. Ignored entries (`!`) are not + * requested; untracked (`?`) become `[path, 0, 2, 0]`; unmerged (`u`) are + * reported as dirty on every axis, which is the conservative reading for an + * app that never merges. Renames are not requested either (`--no-renames`), + * but a `2` entry is still consumed whole, both of its paths, so a stray one + * cannot shift every field after it. + * + * @param {Buffer} buf + * @return {Array[]} + */ +function parseStatusV2Z(buf) { + const rows = []; + const fields = splitNul(buf); + for (let i = 0; i < fields.length; i++) { + const entry = fields[i].toString('utf8'); + const kind = entry[0]; + if (kind === '?') { + rows.push([entry.slice(2), 0, 2, 0]); + } else if (kind === '1') { + // 1 + const parts = entry.split(' '); + rows.push(rowFromStatusEntry(parts[1], parts.slice(8).join(' '))); + } else if (kind === '2') { + // 2 , then + const parts = entry.split(' '); + rows.push(rowFromStatusEntry(parts[1], parts.slice(9).join(' '))); + i += 1; + } else if (kind === 'u') { + // u

+ const parts = entry.split(' '); + rows.push([parts.slice(10).join(' '), 1, 2, 3]); + } + // '!' (ignored) and '#' (headers) are never requested; skip anything else. + } + return rows; +} + +/** + * `git diff --name-status -z --no-renames ` → status rows against + * that commit, index column 0 because the index is not what was compared. + * Statuses: A added, M modified, T type change, D deleted. Untracked files + * are not in a diff; `parseZList` on `ls-files --others` supplies them. + * + * @param {Buffer} buf + * @return {Array[]} + */ +function parseNameStatusZ(buf) { + const rows = []; + const fields = splitNul(buf); + for (let i = 0; i + 1 < fields.length; i += 2) { + const status = fields[i].toString('utf8')[0]; + const filepath = fields[i + 1].toString('utf8'); + if (status === 'A') rows.push([filepath, 0, 2, 0]); + else if (status === 'D') rows.push([filepath, 1, 0, 0]); + else rows.push([filepath, 1, 2, 0]); + } + return rows; +} + +/** + * A `-z` list of paths (`ls-files -z`, `for-each-ref` with `%00`). + * + * @param {Buffer} buf + * @return {string[]} + */ +function parseZList(buf) { + return splitNul(buf).map((field) => field.toString('utf8')).filter((s) => s.length > 0); +} + +/** + * `git cat-file --batch` output, in request order → the object bytes per + * request, or null where Git answered `missing`. Requests are `:` + * strings; the caller supplies them in the same order it wrote them to stdin. + * + * Each answer is a header line ` \n`, then `size` bytes, + * then a newline; or ` missing\n`. + * + * @param {Buffer} buf + * @param {string[]} requests + * @return {Map} + */ +function parseCatFileBatch(buf, requests) { + const answers = new Map(); + let pos = 0; + for (const request of requests) { + const nl = buf.indexOf(10, pos); + if (nl === -1) break; + const header = buf.subarray(pos, nl).toString('utf8'); + pos = nl + 1; + if (header.endsWith(' missing')) { + answers.set(request, null); + continue; + } + const size = Number(header.split(' ')[2]); + answers.set(request, Buffer.from(buf.subarray(pos, pos + size))); + pos += size + 1; + } + return answers; +} + +/** + * `git cat-file --batch-check` output → the object id per request, or null. + * + * @param {Buffer} buf + * @param {string[]} requests + * @return {Map} + */ +function parseCatFileBatchCheck(buf, requests) { + const answers = new Map(); + const lines = buf.toString('utf8').split('\n'); + requests.forEach((request, i) => { + const line = lines[i] || ''; + answers.set(request, line.endsWith(' missing') || !line ? null : line.split(' ')[0]); + }); + return answers; +} + +/** + * `git ls-tree -z -- ` → the entry, or null when the path is + * not in that tree. Format: ` \t`. + * + * @param {Buffer} buf + * @return {?{mode: string, type: string, oid: string, path: string}} + */ +function parseLsTreeZ(buf) { + const first = splitNul(buf)[0]; + if (!first || first.length === 0) return null; + const text = first.toString('utf8'); + const tab = text.indexOf('\t'); + if (tab === -1) return null; + const [mode, type, oid] = text.slice(0, tab).split(' '); + return { mode, type, oid, path: text.slice(tab + 1) }; +} + +/** + * The Windows-only view createCrlfCompatibleFs gives isomorphic-git, for the + * binary: a site checked out by a host Git with a global `autocrlf = true` + * sits on disk as CRLF, its repository config says nothing, and the app's Git + * reads no global config (git-binary.cjs). Without this, `status` reports + * every text file. An explicit local value passes through untouched (#341). + * + * @param {string} dir + * @param {Object} [options] + * @param {string} [options.platform] + * @param {Function} [options.run] + * @return {Promise} Arguments to place before the subcommand. + */ +async function crlfArgs(dir, { platform = process.platform, run = runGit } = {}) { + if (platform !== 'win32') return []; + const { status } = await run(['config', '--local', '--get', 'core.autocrlf'], { cwd: dir, okCodes: [0, 1] }); + return status === 1 ? ['-c', 'core.autocrlf=true'] : []; +} + +/** + * The commit a ref points at, or null when it does not resolve. + * + * @param {string} dir + * @param {string} ref + * @return {Promise} + */ +async function resolveRef(dir, ref) { + const { status, stdout } = await runGit(['rev-parse', '--verify', '--quiet', `${ref}^{commit}`], { cwd: dir, okCodes: [0, 1] }); + return status === 0 ? stdout.toString('utf8').trim() : null; +} + +/** + * A commit's id and committer date, as the ISO string the app has always + * stored. `%ct` is the epoch second, so the string is UTC regardless of the + * committer's own offset, which is what `%cI` would have carried. + * + * @param {string} dir + * @param {string} ref + * @return {Promise<{oid: string, date: string}>} Throws when the ref is missing. + */ +async function readCommitInfo(dir, ref) { + const { stdout } = await runGit(['log', '-1', '--format=%H%x00%ct', ref, '--'], { cwd: dir }); + const [oid, seconds] = stdout.toString('utf8').trim().split('\0'); + return { oid, date: new Date(Number(seconds) * 1000).toISOString() }; +} + +/** + * The checked-out branch, or null when HEAD is detached. `symbolic-ref + * --quiet` answers a detached HEAD with exit 1; `rev-parse --abbrev-ref` + * would print the literal `HEAD` instead, and `--short` abbreviates against + * tags, so the prefix is stripped here. + * + * @param {string} dir + * @return {Promise} + */ +async function currentBranch(dir) { + const { status, stdout } = await runGit(['symbolic-ref', '--quiet', 'HEAD'], { cwd: dir, okCodes: [0, 1] }); + if (status !== 0) return null; + return stdout.toString('utf8').trim().replace(/^refs\/heads\//, ''); +} + +/** + * Every local branch name. Ref names cannot contain a newline (or a space, or + * control characters: `git check-ref-format`), so the default one-per-line + * output is already unambiguous. + * + * @param {string} dir + * @return {Promise} + */ +async function listBranches(dir) { + const { stdout } = await runGit(['for-each-ref', '--format=%(refname)', 'refs/heads/'], { cwd: dir }); + return stdout.toString('utf8').split('\n').filter((line) => line.length > 0).map((ref) => ref.replace(/^refs\/heads\//, '')); +} + +/** + * The worktree against HEAD, every non-ignored file, in status rows. This is + * the scan every park and every dirty check runs. + * + * @param {string} dir + * @param {Object} [options] + * @param {string} [options.platform] + * @return {Promise} + */ +async function statusRows(dir, { platform = process.platform } = {}) { + const crlf = await crlfArgs(dir, { platform }); + const { stdout } = await runGit([...crlf, 'status', '--porcelain=v2', '-z', '--untracked-files=all', '--no-renames'], { cwd: dir }); + return parseStatusV2Z(stdout); +} + +/** + * The worktree against an arbitrary commit (a ticket's branch point, which is + * not HEAD once work is parked), in status rows with the index column 0. + * `git diff ` compares by content, so a file whose stat data is stale + * is hashed rather than reported; untracked files come from `ls-files`. + * + * @param {string} dir + * @param {string} ref + * @param {Object} [options] + * @param {string} [options.platform] + * @return {Promise} + */ +async function changesAgainst(dir, ref, { platform = process.platform } = {}) { + const crlf = await crlfArgs(dir, { platform }); + const diff = await runGit([...crlf, 'diff', '--name-status', '-z', '--no-renames', ref, '--'], { cwd: dir }); + const others = await runGit([...crlf, 'ls-files', '--others', '--exclude-standard', '-z'], { cwd: dir }); + const rows = parseNameStatusZ(diff.stdout); + const seen = new Set(rows.map(([filepath]) => filepath)); + for (const filepath of parseZList(others.stdout)) { + if (seen.has(filepath)) continue; + seen.add(filepath); + rows.push([filepath, 0, 2, 0]); + } + return rows; +} + +/** + * The bytes of several paths at one commit, in a single spawn. Raw object + * content: no line-ending conversion, exactly what `readBlob` returned. + * + * @param {string} dir + * @param {string} oid + * @param {string[]} filepaths + * @return {Promise>} Keyed by path; null when absent. + */ +async function readBlobs(dir, oid, filepaths) { + if (filepaths.length === 0) return new Map(); + const requests = filepaths.map((filepath) => `${oid}:${filepath}`); + const { stdout } = await runGit(['cat-file', '--batch', '-z'], { cwd: dir, input: `${requests.join('\0')}\0` }); + const byRequest = parseCatFileBatch(stdout, requests); + return new Map(filepaths.map((filepath, i) => [filepath, byRequest.get(requests[i]) ?? null])); +} + +/** + * The blob id of one path at one commit, or null when absent. + * + * @param {string} dir + * @param {string} oid + * @param {string} filepath + * @return {Promise} + */ +async function blobOid(dir, oid, filepath) { + const request = `${oid}:${filepath}`; + const { stdout } = await runGit(['cat-file', '--batch-check', '-z'], { cwd: dir, input: `${request}\0` }); + return parseCatFileBatchCheck(stdout, [request]).get(request); +} + +/** + * The mode a tree records for one path, or null when the path is not there. + * One `ls-tree` on the path rather than a walk of the whole commit. + * + * @param {string} dir + * @param {string} oid + * @param {string} filepath + * @return {Promise} + */ +async function treeEntryMode(dir, oid, filepath) { + const { stdout } = await runGit(['ls-tree', '-z', oid, '--', filepath], { cwd: dir }); + const entry = parseLsTreeZ(stdout); + return entry ? entry.mode : null; +} + +module.exports = { + splitNul, + rowFromStatusEntry, + parseStatusV2Z, + parseNameStatusZ, + parseZList, + parseCatFileBatch, + parseCatFileBatchCheck, + parseLsTreeZ, + crlfArgs, + resolveRef, + readCommitInfo, + currentBranch, + listBranches, + statusRows, + changesAgainst, + readBlobs, + blobOid, + treeEntryMode +}; diff --git a/src/git-run.cjs b/src/git-run.cjs new file mode 100644 index 00000000..102d1ace --- /dev/null +++ b/src/git-run.cjs @@ -0,0 +1,188 @@ +'use strict'; + +/** + * Runs the bundled Git (#384, phase 2 of #364). The one place the binary is + * spawned: every caller hands in arguments and a working directory and gets + * bytes back. No parsing here; each read keeps its parser next to it in + * git-read.cjs, so a wrong flag and a wrong parser are found in the same file. + * + * `spawn`, not `execFile`: execFile buffers stdout against a `maxBuffer` and + * kills the child silently when it overflows, and it hides the ChildProcess + * that a cancellation needs. Here the cap is explicit, the overflow is an + * error the caller sees, and `spawnGit` returns the child so a later phase can + * stream a clone's progress and hand it to `killChildTree`. + * + * Two global options ride on every call: + * + * - `--no-optional-locks`: a read (`status`, `diff`) otherwise refreshes the + * index opportunistically, which writes `.git/index`. Reads must not write. + * - `safe.directory=`: with no system or global config in play + * (git-binary.cjs), a site on a folder Git considers "dubiously owned" fails + * every command with exit 128. The app only runs Git in directories it + * registered itself, so trusting exactly the one it is about to use is the + * narrow form; `*` is what a security review would flag. + */ + +const { spawn: nodeSpawn } = require('child_process'); +const { resolveGitBinary, buildGitEnv, BASE_ARGS, SPAWN_OPTIONS } = require('./git-binary.cjs'); +const { killChildTree } = require('./kill-tree.js'); + +/** + * Enough for any porcelain output the app asks for: `status -z` on a tree with + * 20,000 dirty paths is about 1 MiB. The cap exists so a runaway command is an + * error rather than an out-of-memory in the main process. + */ +const DEFAULT_MAX_STDOUT = 64 * 1024 * 1024; + +class GitError extends Error { + constructor(message, props) { + super(message); + this.name = 'GitError'; + Object.assign(this, props); + } +} + +/** + * The subcommand in an argument list, for messages: the first entry that is + * neither an option nor the value of a `-c key=value` pair. + * + * @param {string[]} args + * @return {string} + */ +function subcommandOf(args) { + for (let i = 0; i < args.length; i++) { + if (args[i] === '-c' || args[i] === '-C') { i += 1; continue; } + if (!args[i].startsWith('-')) return args[i]; + } + return '(no subcommand)'; +} + +/** + * Both spellings on Windows: Git compares the directory it resolved, which it + * prints with forward slashes, and the value is multi-valued so listing the + * native form too costs nothing and cannot be wrong. + * + * @param {string} cwd + * @return {string[]} + */ +function safeDirectoryArgs(cwd) { + const forward = cwd.replace(/\\/g, '/'); + const values = forward === cwd ? [cwd] : [cwd, forward]; + return values.flatMap((value) => ['-c', `safe.directory=${value}`]); +} + +/** + * Spawns the bundled Git and returns the child, for callers that stream + * (progress on stderr) or that may need to kill it. Most callers want + * `runGit`. + * + * @param {string[]} args + * @param {Object} options + * @param {string} options.cwd Required: Git never runs "wherever the + * app happens to be". + * @param {Buffer|string} [options.input] Written to stdin, then stdin is closed. + * @param {Object} [options.extraEnv] Passed to buildGitEnv. + * @param {Function} [options.spawn] Injection point for tests. + * @return {import('child_process').ChildProcess} + */ +function spawnGit(args, { cwd, input, extraEnv, spawn = nodeSpawn } = {}) { + if (typeof cwd !== 'string' || cwd.length === 0) { + throw new TypeError('spawnGit needs an explicit cwd'); + } + const argv = [...BASE_ARGS, '--no-optional-locks', ...safeDirectoryArgs(cwd), ...args]; + return spawn(resolveGitBinary(), argv, { + ...SPAWN_OPTIONS, + cwd, + env: buildGitEnv({ extraEnv }), + stdio: [input === undefined ? 'ignore' : 'pipe', 'pipe', 'pipe'] + }); +} + +/** + * Runs Git to completion. Resolves with `{ status, stdout, stderr }` where + * stdout is a Buffer (porcelain `-z` output is bytes, and paths are not + * guaranteed to be UTF-8) and stderr is a string. + * + * Rejects with a GitError carrying `code`, `signal`, `stderr`, `args` and + * `cwd` when the exit status is not in `okCodes`. Query commands that answer + * "no" with exit 1 (`rev-parse --verify`, `symbolic-ref`, `config --get`) + * pass `okCodes: [0, 1]` and read `status`. 128 is Git's "fatal" and always + * surfaces; so does 129 (usage), which means a bug in the caller. + * + * @param {string[]} args + * @param {Object} options As spawnGit, plus: + * @param {number[]} [options.okCodes] + * @param {number} [options.maxStdout] + * @return {Promise<{status: number, stdout: Buffer, stderr: string}>} + */ +function runGit(args, { okCodes = [0], maxStdout = DEFAULT_MAX_STDOUT, ...spawnOptions } = {}) { + return new Promise((resolve, reject) => { + let child; + try { + child = spawnGit(args, spawnOptions); + } catch (error) { + reject(error); + return; + } + + const { cwd, input } = spawnOptions; + const name = subcommandOf(args); + const out = []; + const err = []; + let outBytes = 0; + let overflow = false; + let settled = false; + const fail = (error) => { + if (settled) return; + settled = true; + reject(error); + }; + + child.stdout.on('data', (chunk) => { + if (overflow) return; + outBytes += chunk.length; + if (outBytes > maxStdout) { + overflow = true; + killChildTree(child); + return; + } + out.push(chunk); + }); + child.stderr.on('data', (chunk) => err.push(chunk)); + child.on('error', (error) => { + fail(new GitError(`git ${name} could not start: ${error.message}`, { + code: error.code, signal: null, stderr: '', args, cwd + })); + }); + child.on('close', (status, signal) => { + if (settled) return; + const stderr = Buffer.concat(err).toString('utf8'); + if (overflow) { + fail(new GitError(`git ${name} produced more than ${maxStdout} bytes of output`, { + code: 'stdout-overflow', signal, stderr, args, cwd + })); + return; + } + if (!okCodes.includes(status)) { + const reason = stderr.split(/\r?\n/).find((line) => line.trim()) || (signal ? `killed by ${signal}` : 'no output'); + fail(new GitError(`git ${name} failed (${status === null ? signal : status}): ${reason}`, { + code: status, signal, stderr, args, cwd + })); + return; + } + settled = true; + resolve({ status, stdout: Buffer.concat(out), stderr }); + }); + + if (input !== undefined) { + // EPIPE means Git exited before reading all of stdin, which its exit + // status already reports; anything else is a real write failure. + child.stdin.on('error', (error) => { + if (error.code !== 'EPIPE') fail(error); + }); + child.stdin.end(input); + } + }); +} + +module.exports = { spawnGit, runGit, GitError, DEFAULT_MAX_STDOUT, subcommandOf, safeDirectoryArgs }; diff --git a/tests/unit/git-binary.integration.test.cjs b/tests/unit/git-binary.integration.test.cjs index 58dcd2b9..8f934292 100644 --- a/tests/unit/git-binary.integration.test.cjs +++ b/tests/unit/git-binary.integration.test.cjs @@ -3,9 +3,8 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); -const { spawnSync } = require('node:child_process'); -const { resolveGitBinary, buildGitEnv, BASE_ARGS, SPAWN_OPTIONS } = require('../../src/git-binary.cjs'); +const { BINARY, GIT_VERSION, git, tempDir } = require('./helpers/git.cjs'); // The bundled Git actually runs, from the source tree, on whatever platform // runs this suite — and `npm run test:electron` repeats it on Electron's own @@ -20,43 +19,6 @@ const { resolveGitBinary, buildGitEnv, BASE_ARGS, SPAWN_OPTIONS } = require('../ // to keep it out (a `commit.gpgsign` there would otherwise fail the commit // below), and nothing here works around it. -const BINARY = resolveGitBinary(); -const ENV = buildGitEnv(); -// The Git dugite@3.2.3 embeds; a different one here means a different tree. -const GIT_VERSION = /^git version 2\.53\.0(?:$|[.\s])/; - -function git(args, cwd) { - const result = spawnSync(BINARY, [...BASE_ARGS, ...args], { ...SPAWN_OPTIONS, cwd, env: ENV, encoding: 'utf8' }); - return { - status: result.status, - stdout: (result.stdout || '').trim(), - stderr: (result.stderr || '').trim(), - error: result.error ? result.error.message : null - }; -} - -function tempDir(t, prefix) { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); - t.after(() => removeRepo(dir)); - return dir; -} - -// Git writes its objects read-only, and on Windows `rmSync` answers that with -// EPERM rather than deleting them (#381). Make everything writable first. -function removeRepo(dir) { - const walk = (entry) => { - const stat = fs.lstatSync(entry); - if (stat.isDirectory()) { - fs.chmodSync(entry, 0o777); - for (const child of fs.readdirSync(entry)) walk(path.join(entry, child)); - } else if (stat.isFile()) { - fs.chmodSync(entry, 0o666); - } - }; - if (fs.existsSync(dir)) walk(dir); - fs.rmSync(dir, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); -} - test('the bundled binary is present and runs', () => { assert.ok(fs.existsSync(BINARY), `${BINARY} is missing`); const result = git(['--version'], os.tmpdir()); diff --git a/tests/unit/git-read.test.cjs b/tests/unit/git-read.test.cjs new file mode 100644 index 00000000..5aca14e2 --- /dev/null +++ b/tests/unit/git-read.test.cjs @@ -0,0 +1,210 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const read = require('../../src/git-read.cjs'); +const { git, tempDir } = require('./helpers/git.cjs'); + +// Two halves. The parsers are pure functions over bytes, so the first half +// feeds them the exact byte layouts Git documents for each porcelain format, +// including the shapes that only a hostile repository produces (a stray +// rename entry, a NUL inside a blob, a path with a space). The second half +// runs the read functions against a repository the bundled Git built, so the +// flag set each command uses is proven on this platform too. + +const z = (...fields) => Buffer.from(fields.map((f) => `${f}\0`).join(''), 'utf8'); + +test('status v2: every XY combination lands on the documented row', () => { + const cases = [ + ['..', [1, 1, 1]], + ['.M', [1, 2, 1]], + ['.D', [1, 0, 1]], + ['M.', [1, 2, 2]], + ['MM', [1, 2, 3]], + ['MD', [1, 0, 3]], + ['A.', [0, 2, 2]], + ['AM', [0, 2, 3]], + ['AD', [0, 0, 3]], + ['D.', [1, 2, 0]], + ['T.', [1, 2, 2]], + ['.T', [1, 2, 1]] + ]; + for (const [xy, expected] of cases) { + const [, head, workdir, stage] = read.rowFromStatusEntry(xy, 'f'); + assert.deepEqual([head, workdir, stage], expected, xy); + } +}); + +test('status v2: ordinary, untracked, unmerged and a stray rename entry parse without drifting', () => { + const buf = z( + '1 .M N... 100644 100644 100644 aaaa bbbb src/wp-login.php', + '? new file.txt', + '2 R. N... 100644 100644 100644 cccc dddd R100 renamed.php', 'original.php', + 'u UU N... 100644 100644 100644 100644 eeee ffff gggg conflicted.php', + '1 D. N... 100644 000000 000000 hhhh 0000 gone.php' + ); + assert.deepEqual(read.parseStatusV2Z(buf), [ + ['src/wp-login.php', 1, 2, 1], + ['new file.txt', 0, 2, 0], + ['renamed.php', 1, 2, 2], + ['conflicted.php', 1, 2, 3], + ['gone.php', 1, 2, 0] + ]); +}); + +test('status v2: an empty answer is an empty matrix', () => { + assert.deepEqual(read.parseStatusV2Z(Buffer.alloc(0)), []); +}); + +test('name-status: A, M, T and D against a commit', () => { + const buf = z('A', 'added.php', 'M', 'src/wp-login.php', 'T', 'link', 'D', 'gone.php'); + assert.deepEqual(read.parseNameStatusZ(buf), [ + ['added.php', 0, 2, 0], + ['src/wp-login.php', 1, 2, 0], + ['link', 1, 2, 0], + ['gone.php', 1, 0, 0] + ]); +}); + +test('a -z list keeps spaces and drops the empty trailer', () => { + assert.deepEqual(read.parseZList(z('a b', 'c')), ['a b', 'c']); + assert.deepEqual(read.parseZList(Buffer.alloc(0)), []); +}); + +test('cat-file --batch: hits, a missing object and a blob with newlines and NUL bytes', () => { + const body = Buffer.concat([Buffer.from('line 1\n'), Buffer.from([0, 0xff]), Buffer.from('\nend')]); + const buf = Buffer.concat([ + Buffer.from(`1111111111111111111111111111111111111111 blob ${body.length}\n`), body, Buffer.from('\n'), + Buffer.from('abc:no such file missing\n'), + Buffer.from('2222222222222222222222222222222222222222 blob 0\n\n') + ]); + const requests = ['abc:with space.txt', 'abc:no such file', 'abc:empty']; + const answers = read.parseCatFileBatch(buf, requests); + assert.deepEqual(answers.get('abc:with space.txt'), body); + assert.equal(answers.get('abc:no such file'), null); + assert.deepEqual(answers.get('abc:empty'), Buffer.alloc(0)); +}); + +test('cat-file --batch-check: oid per request, null when missing', () => { + const buf = Buffer.from('1111111111111111111111111111111111111111 blob 12\nabc:nope missing\n'); + const answers = read.parseCatFileBatchCheck(buf, ['abc:package-lock.json', 'abc:nope']); + assert.equal(answers.get('abc:package-lock.json'), '1111111111111111111111111111111111111111'); + assert.equal(answers.get('abc:nope'), null); +}); + +test('ls-tree: one entry, or null for a path the tree does not have', () => { + const entry = read.parseLsTreeZ(z('100755 blob 3333333333333333333333333333333333333333\tbin/run me.sh')); + assert.deepEqual(entry, { mode: '100755', type: 'blob', oid: '3333333333333333333333333333333333333333', path: 'bin/run me.sh' }); + assert.equal(read.parseLsTreeZ(Buffer.alloc(0)), null); +}); + +test('crlfArgs: Windows adds autocrlf only when the repository does not say', async () => { + const seen = []; + const runWith = (status) => async (args, options) => { seen.push({ args, options }); return { status, stdout: Buffer.alloc(0), stderr: '' }; }; + assert.deepEqual(await read.crlfArgs('/site', { platform: 'win32', run: runWith(1) }), ['-c', 'core.autocrlf=true']); + assert.deepEqual(await read.crlfArgs('/site', { platform: 'win32', run: runWith(0) }), []); + assert.deepEqual(seen[0].args, ['config', '--local', '--get', 'core.autocrlf']); + assert.deepEqual(seen[0].options.okCodes, [0, 1]); + assert.deepEqual(await read.crlfArgs('/site', { platform: 'darwin', run: runWith(1) }), []); + assert.equal(seen.length, 2, 'no config read off Windows'); +}); + +// --- against a real repository ------------------------------------------ + +function makeRepo(t) { + const dir = tempDir(t, 'toolkit-git-read-'); + assert.equal(git(['init', '-b', 'trunk'], dir).status, 0); + fs.mkdirSync(path.join(dir, 'src')); + fs.writeFileSync(path.join(dir, 'src', 'wp-login.php'), ' { + const dir = makeRepo(t); + const head = git(['rev-parse', 'HEAD'], dir).stdout; + assert.equal(await read.resolveRef(dir, 'HEAD'), head); + assert.equal(await read.resolveRef(dir, 'refs/heads/trunk'), head); + assert.equal(await read.resolveRef(dir, 'refs/heads/nope'), null); + + const info = await read.readCommitInfo(dir, 'refs/heads/trunk'); + assert.equal(info.oid, head); + const seconds = Number(git(['log', '-1', '--format=%ct'], dir).stdout); + assert.equal(info.date, new Date(seconds * 1000).toISOString()); + await assert.rejects(read.readCommitInfo(dir, 'refs/heads/nope'), (error) => error.code === 128); + + assert.equal(await read.currentBranch(dir), 'trunk'); + assert.equal(git(['checkout', '-q', '--detach'], dir).status, 0); + assert.equal(await read.currentBranch(dir), null); +}); + +test('listBranches sees a branch made by hand', async (t) => { + const dir = makeRepo(t); + assert.equal(git(['branch', 'ticket/60001'], dir).status, 0); + assert.deepEqual((await read.listBranches(dir)).sort(), ['ticket/60001', 'trunk']); +}); + +test('statusRows: clean tree, then a modification, an untracked file, a deletion, and nothing ignored', async (t) => { + const dir = makeRepo(t); + assert.deepEqual(await read.statusRows(dir, { platform: 'darwin' }), []); + + fs.writeFileSync(path.join(dir, 'src', 'wp-login.php'), ' { + const dir = makeRepo(t); + const base = git(['rev-parse', 'HEAD'], dir).stdout; + // A second commit moves HEAD; the worktree then diverges from both. + fs.writeFileSync(path.join(dir, 'src', 'wp-login.php'), ' { + const dir = makeRepo(t); + const base = git(['rev-parse', 'HEAD'], dir).stdout; + fs.writeFileSync(path.join(dir, 'src', 'wp-login.php'), ' child.stdinChunks.push(chunk)); + child.killed = false; + child.kill = () => { child.killed = true; return true; }; + setTimeout(() => { + if (error) { + child.emit('error', error); + child.emit('close', null, null); + return; + } + for (const chunk of stdout) child.stdout.write(chunk); + for (const chunk of stderr) child.stderr.write(chunk); + child.stdout.end(); + child.stderr.end(); + setTimeout(() => { + child.exitCode = status; + child.signalCode = signal; + child.emit('close', status, signal); + }, 0); + }, delay); + return child; +} + +function recordingSpawn(childOptions) { + const calls = []; + const spawn = (file, args, options) => { + const child = fakeChild(childOptions); + calls.push({ file, args, options, child }); + return child; + }; + return { spawn, calls }; +} + +test('spawnGit refuses to run without an explicit cwd', () => { + assert.throws(() => spawnGit(['status'], {}), TypeError); + assert.throws(() => spawnGit(['status'], { cwd: '' }), TypeError); +}); + +test('every call carries the base arguments, no optional locks and the cwd as a safe directory', () => { + const { spawn, calls } = recordingSpawn(); + spawnGit(['status', '--porcelain=v2'], { cwd: '/sites/demo', spawn }); + + const [{ file, args, options }] = calls; + assert.ok(path.isAbsolute(file)); + assert.deepEqual(args.slice(0, BASE_ARGS.length), [...BASE_ARGS]); + assert.equal(args[BASE_ARGS.length], '--no-optional-locks'); + assert.ok(args.includes('safe.directory=/sites/demo')); + assert.deepEqual(args.slice(-2), ['status', '--porcelain=v2']); + assert.equal(options.cwd, '/sites/demo'); + assert.equal(options.shell, false); + assert.equal(options.windowsHide, true); + assert.equal(options.detached, process.platform !== 'win32'); + // The environment is the bundled Git's, never the host's. + assert.equal(options.env.GIT_CONFIG_NOSYSTEM, '1'); + assert.deepEqual(options.stdio, ['ignore', 'pipe', 'pipe']); +}); + +test('a Windows cwd is trusted under both spellings', () => { + assert.deepEqual(safeDirectoryArgs('C:\\Sites\\demo'), [ + '-c', 'safe.directory=C:\\Sites\\demo', + '-c', 'safe.directory=C:/Sites/demo' + ]); + assert.deepEqual(safeDirectoryArgs('/sites/demo'), ['-c', 'safe.directory=/sites/demo']); +}); + +test('the subcommand named in errors skips options and -c pairs', () => { + assert.equal(subcommandOf(['-c', 'core.autocrlf=true', '--no-pager', 'status', '-z']), 'status'); + assert.equal(subcommandOf(['--version']), '(no subcommand)'); +}); + +test('stdin is only opened when there is input, and the input is written whole', async () => { + const { spawn, calls } = recordingSpawn({ stdout: ['ok\n'] }); + await runGit(['cat-file', '--batch'], { cwd: '/sites/demo', input: 'abc:path\0', spawn }); + assert.deepEqual(calls[0].options.stdio, ['pipe', 'pipe', 'pipe']); + // The scripted child recorded what reached its stdin. + const written = Buffer.concat(calls[0].child.stdinChunks).toString('utf8'); + assert.equal(written, 'abc:path\0'); +}); + +test('stdout comes back as bytes and stderr as text', async () => { + const { spawn } = recordingSpawn({ stdout: [Buffer.from([0x61, 0x00, 0x62]), Buffer.from('c')], stderr: ['warning: x\n'] }); + const result = await runGit(['status'], { cwd: '/sites/demo', spawn }); + assert.equal(result.status, 0); + assert.ok(Buffer.isBuffer(result.stdout)); + assert.deepEqual([...result.stdout], [0x61, 0x00, 0x62, 0x63]); + assert.equal(result.stderr, 'warning: x\n'); +}); + +test('an exit status outside okCodes rejects with the stderr and the arguments', async () => { + const { spawn } = recordingSpawn({ status: 128, stderr: ['fatal: not a git repository\n'] }); + await assert.rejects( + runGit(['rev-parse', 'HEAD'], { cwd: '/sites/demo', spawn }), + (error) => { + assert.ok(error instanceof GitError); + assert.equal(error.code, 128); + assert.equal(error.stderr, 'fatal: not a git repository\n'); + assert.deepEqual(error.args, ['rev-parse', 'HEAD']); + assert.equal(error.cwd, '/sites/demo'); + assert.match(error.message, /^git rev-parse failed \(128\): fatal: not a git repository$/); + return true; + } + ); +}); + +test('okCodes lets a query answer "no" without throwing', async () => { + const { spawn } = recordingSpawn({ status: 1 }); + const result = await runGit(['rev-parse', '--verify', '--quiet', 'nope'], { cwd: '/sites/demo', okCodes: [0, 1], spawn }); + assert.equal(result.status, 1); + assert.equal(result.stdout.length, 0); +}); + +test('a spawn failure is a GitError with the system code, not an unhandled event', async () => { + const enoent = Object.assign(new Error('spawn git ENOENT'), { code: 'ENOENT' }); + const { spawn } = recordingSpawn({ error: enoent }); + await assert.rejects( + runGit(['status'], { cwd: '/sites/demo', spawn }), + (error) => error instanceof GitError && error.code === 'ENOENT' && /could not start/.test(error.message) + ); +}); + +test('output past maxStdout rejects rather than growing without bound', async () => { + const { spawn, calls } = recordingSpawn({ stdout: [Buffer.alloc(600, 0x41), Buffer.alloc(600, 0x41)], status: 0 }); + await assert.rejects( + runGit(['status'], { cwd: '/sites/demo', maxStdout: 1000, spawn }), + (error) => error instanceof GitError && error.code === 'stdout-overflow' + ); + assert.equal(calls.length, 1); +}); + +test('the real binary answers through runGit inside a repository', async (t) => { + const dir = tempDir(t, 'toolkit-git-run-'); + assert.equal(git(['init', '-b', 'trunk'], dir).status, 0); + const { status, stdout } = await runGit(['rev-parse', '--git-dir'], { cwd: dir }); + assert.equal(status, 0); + assert.equal(stdout.toString('utf8').trim(), '.git'); + + const missing = await runGit(['rev-parse', '--verify', '--quiet', 'refs/heads/nope'], { cwd: dir, okCodes: [0, 1] }); + assert.equal(missing.status, 1); + + await assert.rejects(runGit(['rev-parse', '--verify', 'refs/heads/nope'], { cwd: dir }), (error) => error.code === 128); +}); diff --git a/tests/unit/helpers/git.cjs b/tests/unit/helpers/git.cjs new file mode 100644 index 00000000..e1fd4c65 --- /dev/null +++ b/tests/unit/helpers/git.cjs @@ -0,0 +1,70 @@ +'use strict'; + +// The bundled Git, for tests that drive it directly: build a repository, put +// it in a state the app never creates (a detached HEAD, a hand-made branch), +// or check what the app wrote. Not a test file: discovery is by `*.test.cjs`, +// so nothing here runs on its own (see the note at the top of +// ipc-wiring.test.cjs). + +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +const { resolveGitBinary, buildGitEnv, BASE_ARGS, SPAWN_OPTIONS } = require('../../../src/git-binary.cjs'); + +const BINARY = resolveGitBinary(); +const ENV = buildGitEnv(); + +// The Git dugite@3.2.3 embeds; a different one here means a different tree. +const GIT_VERSION = /^git version 2\.53\.0(?:$|[.\s])/; + +/** + * Runs one Git command synchronously and returns trimmed text. + * + * @param {string[]} args + * @param {string} cwd + */ +function git(args, cwd) { + const result = spawnSync(BINARY, [...BASE_ARGS, ...args], { ...SPAWN_OPTIONS, cwd, env: ENV, encoding: 'utf8' }); + return { + status: result.status, + stdout: (result.stdout || '').trim(), + stderr: (result.stderr || '').trim(), + error: result.error ? result.error.message : null + }; +} + +/** + * A fresh directory under the OS temp dir, removed when the test ends. + * + * @param {import('node:test').TestContext} t + * @param {string} prefix + */ +function tempDir(t, prefix) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + t.after(() => removeRepo(dir)); + return dir; +} + +/** + * Git writes its objects read-only, and on Windows `rmSync` answers that with + * EPERM rather than deleting them (#381). Make everything writable first. + * + * @param {string} dir + */ +function removeRepo(dir) { + const walk = (entry) => { + const stat = fs.lstatSync(entry); + if (stat.isDirectory()) { + fs.chmodSync(entry, 0o777); + for (const child of fs.readdirSync(entry)) walk(path.join(entry, child)); + } else if (stat.isFile()) { + fs.chmodSync(entry, 0o666); + } + }; + if (fs.existsSync(dir)) walk(dir); + fs.rmSync(dir, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); +} + +module.exports = { BINARY, ENV, GIT_VERSION, git, tempDir, removeRepo }; From a1e0645c522188013841ef3d3e3b3fa8026fe629 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Mon, 7 Sep 2026 14:12:50 +0200 Subject: [PATCH 2/5] Read repositories through the bundled Git behind the existing facades Phase 2 of #364 (#384). Every question the app asks a repository without changing it now goes to the binary: the trunk snapshot and its date, the checked-out branch, the branch list, the dirty scan against HEAD, the scan against a ticket's branch point, base blobs for the patch, and the mode a commit records for a path. The facades keep their signatures (readTrunkInfo, collectDirtyFiles, currentBranchName, listTicketBranches, scanWorktree, collectChangedFiles, baseProvenance, modeInCommit), so the renderer and the IPC layer are untouched. Two things got cheaper on the way: collectDirtyFiles and collectChangedFiles read every base blob in one cat-file spawn instead of one object read per file, and modeInCommit is one ls-tree on the path instead of a tree walk. The writes and the clone still run on isomorphic-git; #385 moves them flow by flow. Every existing integration test builds its repository with isomorphic-git and now reads it through the binary, which is the two-engine agreement check the issue asks for; two new tests put the repository in states only a user's own client produces (detached HEAD, a hand-made branch) and check the reads say so. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01J4uRM5JosAcMbKXowaXyVu --- .../instructions/code-review.instructions.md | 2 +- AGENTS.md | 2 +- src/git-update.cjs | 10 ++-- src/main.js | 27 ++++++---- src/pr-files.cjs | 34 +++++-------- src/ticket-branches.js | 28 +++++----- src/trunk-update.js | 51 ++++++++++--------- tests/unit/pr-files.test.cjs | 35 +++---------- .../unit/ticket-branches.integration.test.cjs | 21 +++++++- 9 files changed, 108 insertions(+), 102 deletions(-) diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index 7d36b358..7de5b8e2 100644 --- a/.github/instructions/code-review.instructions.md +++ b/.github/instructions/code-review.instructions.md @@ -61,7 +61,7 @@ Invariants. Breaking one is a `[fix here]` finding even when the code works on t **Child processes run on Electron's bundled Node, never the host's.** Spawns go through `process.execPath` with `ELECTRON_RUN_AS_NODE=1` in the environment (see `runNpmWithEngineRetry` and the `playground:start` handler in `src/main.js`, and `buildChildEnv`). A bare `spawn('node')` or `spawn('npm')` assumes a host toolchain that is not there. On Windows child `npm` processes find a `node` at all only because of the `PATH` shim built by `ensureNodeShimDir` — new spawns must inherit that environment rather than build their own. The one exception is the bundled Git, which is not a Node process and gets its own environment from `src/git-binary.cjs` (next invariant). -**Git is the binary the app ships, never the host's.** Since #364 the app bundles Git through `dugite`, unpacked from `app.asar`. `require('dugite')` appears in exactly one file, `src/git-binary.cjs`; every Git spawn resolves the binary with its `resolveGitBinary`, takes its environment from `buildGitEnv`, its options from `SPAWN_OPTIONS` (which already sets `detached` the way section 4 asks), and starts its arguments with `BASE_ARGS`. That env drops every `GIT_*` variable the host had (dugite would otherwise honour `LOCAL_GIT_DIRECTORY` and `GIT_EXEC_PATH` and run a different Git), turns the host's system and global config off, and turns prompting off, so the host's shell or `~/.gitconfig` cannot change what the app does. A `spawn('git')` that relies on `PATH`, a hand-joined path into the dugite tree, a `GitProcess.exec` outside that file, a Git call given `buildChildEnv`'s environment, or one spawned without an explicit `cwd` is a regression. Parse only porcelain-stable output, with the flag that pins it (`--porcelain=v2`, `-z`, an explicit `--format`); parsing human-facing output is a finding however convenient. During the migration the flows still run on `isomorphic-git`, and patch and diff generation stays hand-rolled in `src/main.js` (stage untracked files, diff working tree against `origin/trunk`) until the phase that moves it. +**Git is the binary the app ships, never the host's.** Since #364 the app bundles Git through `dugite`, unpacked from `app.asar`. `require('dugite')` appears in exactly one file, `src/git-binary.cjs`; every Git spawn resolves the binary with its `resolveGitBinary`, takes its environment from `buildGitEnv`, its options from `SPAWN_OPTIONS` (which already sets `detached` the way section 4 asks), and starts its arguments with `BASE_ARGS`. That env drops every `GIT_*` variable the host had (dugite would otherwise honour `LOCAL_GIT_DIRECTORY` and `GIT_EXEC_PATH` and run a different Git), turns the host's system and global config off, and turns prompting off, so the host's shell or `~/.gitconfig` cannot change what the app does. A `spawn('git')` that relies on `PATH`, a hand-joined path into the dugite tree, a `GitProcess.exec` outside that file, a Git call given `buildChildEnv`'s environment, or one spawned without an explicit `cwd` is a regression. Parse only porcelain-stable output, with the flag that pins it (`--porcelain=v2`, `-z`, an explicit `--format`); parsing human-facing output is a finding however convenient. `src/git-run.cjs` is the only module that spawns the binary and `src/git-read.cjs` the only one that parses its output; a new read belongs there, with a parser test on fixture bytes, not inline at a call site. Since #384 every read runs on the bundled Git and returns the same shapes the `isomorphic-git` calls returned (status rows included), so a facade signature that changes with the engine is a finding; the writes and the clone still run on `isomorphic-git` until #385, and patch and diff generation stays hand-rolled in `src/main.js` until the phase that moves it. **`electron-store` is the only persistence layer.** No database, no sidecar JSON. It holds the site registry and per-site metadata and is the single source of truth for "known sites". A second store, a cache file, or state parked in a module-level variable that outlives a handler is architectural drift — flag it. diff --git a/AGENTS.md b/AGENTS.md index e4fb2727..5ade5ab7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -87,7 +87,7 @@ When asked to add an existing pull request to an existing stack, preserve its co ## Architecture notes (non-obvious) - **Child processes run on Electron's own Node, not the system Node.** `npm install`, `npm run