From 96d90a4d6599f3caa5a35c1005ce5c7f30a97bc1 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Mon, 7 Sep 2026 15:54:42 +0200 Subject: [PATCH 1/6] Add the write primitives for the bundled Git, with no callers The commands the ticket-branch flows need, one Git command each, in src/git-write.cjs: stage a list of paths (additions, modifications and deletions in one `add -A` scoped to a NUL-separated, literal pathspec), write the tree, write a commit with exactly one parent under an identity passed per call, move a branch ref with an expected old value, create a branch, point HEAD at it without touching the index, delete it, and a forced checkout that reports its progress. Every command that touches the index or the worktree carries the Windows CRLF view git-read.cjs gives sites the old engine made. The progress reader leaves git-clone.cjs for src/git-progress.cjs, with the failure-reason extraction beside it: a checkout prints the same lines a clone does, and one parser is one place the app reads Git's words. Nothing calls the new module yet; the swap is the next commit. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_013bqghuMjo6GQFEdRrZtNJS --- src/git-clone.cjs | 71 +------- src/git-progress.cjs | 83 +++++++++ src/git-write.cjs | 211 ++++++++++++++++++++++ tests/unit/git-clone.test.cjs | 41 +---- tests/unit/git-progress.test.cjs | 57 ++++++ tests/unit/git-write.integration.test.cjs | 120 ++++++++++++ tests/unit/git-write.test.cjs | 195 ++++++++++++++++++++ 7 files changed, 675 insertions(+), 103 deletions(-) create mode 100644 src/git-progress.cjs create mode 100644 src/git-write.cjs create mode 100644 tests/unit/git-progress.test.cjs create mode 100644 tests/unit/git-write.integration.test.cjs create mode 100644 tests/unit/git-write.test.cjs diff --git a/src/git-clone.cjs b/src/git-clone.cjs index 6067690a..dbc40275 100644 --- a/src/git-clone.cjs +++ b/src/git-clone.cjs @@ -21,14 +21,13 @@ * assumes; `core.symlinks=false` matches what the old engine wrote; and on * Windows `core.longpaths=true` because the tree has paths past MAX_PATH. * - * Progress arrives on stderr as the lines Git prints for a human, which is - * the one place the app reads non-porcelain output: Git has no machine - * format for progress, the lines have had the same shape for fifteen years, - * and a line that does not parse is dropped rather than shown. + * Progress arrives on stderr as the lines Git prints for a human, parsed by + * git-progress.cjs, the one place the app reads non-porcelain output. */ const path = require('path'); const { spawnGit, GitError } = require('./git-run.cjs'); +const { createProgressReader, failureReason } = require('./git-progress.cjs'); /** * The branch a new site checks out. `trunk` is the pristine snapshot every @@ -60,61 +59,6 @@ function cloneArgs({ url, dir, branch = DEFAULT_BRANCH, platform = process.platf ]; } -// `Receiving objects: 42% (1234/5678), 12.00 MiB | 3.00 MiB/s`, with or -// without the `remote: ` prefix the server-side phases carry. -const PROGRESS_LINE = /^(?:remote: )?([A-Za-z][A-Za-z ]*?):\s+(\d+)% \((\d+)\/(\d+)\)/; - -/** - * One progress event per phase line, from a chunk that may hold several - * lines and may end mid-line. Git separates updates to the same phase with - * `\r` and phases with `\n`; both end a segment here. - * - * @param {string} text - * @return {Array<{phase: string, percent: number, loaded: number, total: number}>} - */ -function parseProgressLines(text) { - const events = []; - for (const segment of text.split(/[\r\n]/)) { - const match = PROGRESS_LINE.exec(segment); - if (!match) continue; - events.push({ - phase: match[1].toLowerCase(), - percent: Number(match[2]), - loaded: Number(match[3]), - total: Number(match[4]) - }); - } - return events; -} - -/** - * Feeds chunks in, emits complete progress lines out, and holds back the - * tail that has not ended yet so a percentage split across two chunks is not - * reported twice, or wrongly. - * - * @param {Function} onEvent - * @return {{push: Function, flush: Function}} - */ -function createProgressReader(onEvent) { - let pending = ''; - const emit = (text) => { - for (const event of parseProgressLines(text)) onEvent(event); - }; - return { - push(chunk) { - pending += chunk; - const cut = Math.max(pending.lastIndexOf('\r'), pending.lastIndexOf('\n')); - if (cut === -1) return; - emit(pending.slice(0, cut + 1)); - pending = pending.slice(cut + 1); - }, - flush() { - if (pending) emit(pending); - pending = ''; - } - }; -} - /** * Clones `url` into `dir`. Resolves when the checkout is complete; rejects * with a GitError carrying Git's stderr when it is not, in which case `dir` @@ -169,15 +113,10 @@ function cloneSite({ url, dir, branch = DEFAULT_BRANCH, onProgress = null, onChi return; } const text = stderr.join(''); - // Git's reason is its `fatal:` (or `error:`) line, which is not always - // the last one: "Please make sure you have the correct access rights - // and the repository exists." follows it. Fall back to the last line - // that is not a progress update. - const lines = text.split(/[\r\n]/).filter((line) => line.trim() && !PROGRESS_LINE.test(line)); - const reason = lines.filter((line) => /^(fatal|error):/.test(line)).pop() || lines.pop() || (signal ? `killed by ${signal}` : 'no output'); + const reason = failureReason(text, signal); reject(new GitError(`git clone failed (${status === null ? signal : status}): ${reason}`, { code: status, signal, stderr: text, args, cwd })); }); }); } -module.exports = { DEFAULT_BRANCH, cloneArgs, parseProgressLines, createProgressReader, cloneSite }; +module.exports = { DEFAULT_BRANCH, cloneArgs, cloneSite }; diff --git a/src/git-progress.cjs b/src/git-progress.cjs new file mode 100644 index 00000000..a43fd229 --- /dev/null +++ b/src/git-progress.cjs @@ -0,0 +1,83 @@ +'use strict'; + +/** + * Git's progress lines, and its reason for failing, read off stderr. The one + * place the app parses non-porcelain output: Git has no machine format for + * progress, the lines have had the same shape for fifteen years, and a line + * that does not parse is dropped rather than shown. Shared by the clone + * (git-clone.cjs) and the checkout (git-write.cjs), which print the same + * `Phase: 42% (1234/5678)` shape; the environment pins `LC_ALL=C` + * (git-binary.cjs) so the words are the ones these patterns expect. + */ + +// `Receiving objects: 42% (1234/5678), 12.00 MiB | 3.00 MiB/s`, with or +// without the `remote: ` prefix the server-side phases carry. +const PROGRESS_LINE = /^(?:remote: )?([A-Za-z][A-Za-z ]*?):\s+(\d+)% \((\d+)\/(\d+)\)/; + +/** + * One progress event per phase line, from a chunk that may hold several + * lines and may end mid-line. Git separates updates to the same phase with + * `\r` and phases with `\n`; both end a segment here. + * + * @param {string} text + * @return {Array<{phase: string, percent: number, loaded: number, total: number}>} + */ +function parseProgressLines(text) { + const events = []; + for (const segment of text.split(/[\r\n]/)) { + const match = PROGRESS_LINE.exec(segment); + if (!match) continue; + events.push({ + phase: match[1].toLowerCase(), + percent: Number(match[2]), + loaded: Number(match[3]), + total: Number(match[4]) + }); + } + return events; +} + +/** + * Feeds chunks in, emits complete progress lines out, and holds back the + * tail that has not ended yet so a percentage split across two chunks is not + * reported twice, or wrongly. + * + * @param {Function} onEvent + * @return {{push: Function, flush: Function}} + */ +function createProgressReader(onEvent) { + let pending = ''; + const emit = (text) => { + for (const event of parseProgressLines(text)) onEvent(event); + }; + return { + push(chunk) { + pending += chunk; + const cut = Math.max(pending.lastIndexOf('\r'), pending.lastIndexOf('\n')); + if (cut === -1) return; + emit(pending.slice(0, cut + 1)); + pending = pending.slice(cut + 1); + }, + flush() { + if (pending) emit(pending); + pending = ''; + } + }; +} + +/** + * Git's reason for a failed command, for the error message. Its `fatal:` (or + * `error:`) line is not always the last one: "Please make sure you have the + * correct access rights and the repository exists." follows it. Falls back to + * the last line that is not a progress update, then to the signal. + * + * @param {string} stderr + * @param {?string} [signal] + * @return {string} + */ +function failureReason(stderr, signal = null) { + const lines = String(stderr || '').split(/[\r\n]/).filter((line) => line.trim() && !PROGRESS_LINE.test(line)); + return lines.filter((line) => /^(fatal|error):/.test(line)).pop() || lines.pop() || (signal ? `killed by ${signal}` : 'no output'); +} + +module.exports = { PROGRESS_LINE, parseProgressLines, createProgressReader, failureReason }; diff --git a/src/git-write.cjs b/src/git-write.cjs new file mode 100644 index 00000000..8fa3a3b5 --- /dev/null +++ b/src/git-write.cjs @@ -0,0 +1,211 @@ +'use strict'; + +/** + * The writes the bundled Git makes inside an existing repository (#385): the + * index, a tree, a commit, a ref, a checkout. Primitives only, one Git + * command each, with no knowledge of tickets or trunk; ticket-branches.js + * composes them and owns the invariants. Same split as git-read.cjs, and for + * the same reason: an argument list is testable on an injected runner, a + * flow is testable on a real repository, and mixing the two hides which one + * broke. + * + * Every command that touches the index or the worktree is prefixed with + * `crlfArgs`, the Windows view of `core.autocrlf` that git-read.cjs gives + * sites the old engine made; a site the binary cloned carries the value in + * its own config and the prefix is empty. + * + * Nothing here is a porcelain command with output to parse except `write-tree` + * and `commit-tree`, which print exactly one object id; `checkout` reports its + * progress on stderr through git-progress.cjs, the same lines the clone reads. + */ + +const { spawnGit, runGit, GitError } = require('./git-run.cjs'); +const { crlfArgs } = require('./git-read.cjs'); +const { createProgressReader, failureReason } = require('./git-progress.cjs'); + +const oidOf = ({ stdout }) => stdout.toString('utf8').trim(); + +/** + * Stages exactly `paths`: modifications and additions are added, deletions + * are removed from the index (`-A` scoped to a pathspec does all three). + * The paths are the ones a status scan returned, byte for byte, so they are + * fed on stdin NUL-separated and taken literally: a `*`, `?` or `[` in a + * filename is a character, not a glob. + * + * @param {string} dir + * @param {string[]} paths + * @param {Object} [options] + * @param {string} [options.platform] + * @param {Function} [options.run] + * @return {Promise} How many paths were handed to Git. + */ +async function stagePaths(dir, paths, { platform = process.platform, run = runGit } = {}) { + if (!paths.length) return 0; + const crlf = await crlfArgs(dir, { platform, run }); + await run([...crlf, '--literal-pathspecs', 'add', '-A', '--pathspec-from-file=-', '--pathspec-file-nul'], { + cwd: dir, + input: Buffer.from(`${paths.join('\0')}\0`, 'utf8') + }); + return paths.length; +} + +/** + * The tree object the index describes. + * + * @param {string} dir + * @param {Object} [options] + * @param {Function} [options.run] + * @return {Promise} + */ +async function writeTree(dir, { run = runGit } = {}) { + return oidOf(await run(['write-tree'], { cwd: dir })); +} + +/** + * A commit object for `tree` with exactly the parent given, written to the + * object store and nothing else: no ref moves, so a caller decides where it + * lands (`updateBranch`). Author and committer are the same identity, passed + * per call because the bundled Git reads no host config and the repository + * has none to give. + * + * @param {string} dir + * @param {Object} root0 + * @param {string} root0.tree + * @param {string} root0.parent + * @param {string} root0.message + * @param {{name: string, email: string}} root0.author + * @param {Function} [root0.run] + * @return {Promise} + */ +async function commitTree(dir, { tree, parent, message, author, run = runGit }) { + const identity = ['-c', `user.name=${author.name}`, '-c', `user.email=${author.email}`]; + return oidOf(await run([...identity, 'commit-tree', tree, '-p', parent, '-m', message], { cwd: dir })); +} + +/** + * Points `refs/heads/` at `oid`. With `expected`, Git refuses unless + * the ref still holds that value, so a second writer (a mentor's own client + * in the same site) fails loudly instead of being overwritten. + * + * @param {string} dir + * @param {string} branch + * @param {string} oid + * @param {Object} [options] + * @param {string} [options.expected] + * @param {Function} [options.run] + */ +async function updateBranch(dir, branch, oid, { expected, run = runGit } = {}) { + await run(['update-ref', `refs/heads/${branch}`, oid, ...(expected ? [expected] : [])], { cwd: dir }); +} + +/** + * Creates `ref` at `startPoint` without checking it out. + * + * @param {string} dir + * @param {string} ref + * @param {string} startPoint + * @param {Object} [options] + * @param {Function} [options.run] + */ +async function createBranchAt(dir, ref, startPoint, { run = runGit } = {}) { + await run(['branch', '--', ref, startPoint], { cwd: dir }); +} + +/** + * Moves HEAD to `ref` and touches nothing else: the index and the worktree + * stay exactly as they are, which is what carries uncommitted edits onto a + * branch just created at the same commit. + * + * @param {string} dir + * @param {string} ref + * @param {Object} [options] + * @param {Function} [options.run] + */ +async function pointHeadAt(dir, ref, { run = runGit } = {}) { + await run(['symbolic-ref', 'HEAD', `refs/heads/${ref}`], { cwd: dir }); +} + +/** + * Deletes `ref` whether or not it is merged anywhere. + * + * @param {string} dir + * @param {string} ref + * @param {Object} [options] + * @param {Function} [options.run] + */ +async function deleteBranch(dir, ref, { run = runGit } = {}) { + await run(['branch', '-D', '--', ref], { cwd: dir }); +} + +/** + * Checks `ref` out, overwriting tracked files that differ. Ignored and + * untracked files are not Git's to touch and survive. Progress comes from + * stderr as `{ phase, percent, loaded, total }` (`updating files` is the + * phase a checkout reports); a small checkout prints none at all. + * + * Git writes HEAD last, after every file operation succeeded, so a failure + * part-way leaves HEAD where it was over a partly swapped worktree. The + * rejection carries Git's own reason; what to record about that state is + * the caller's (ticket-branches.js tags it with the stage). + * + * @param {string} dir + * @param {string} ref + * @param {Object} [options] + * @param {Function} [options.onProgress] + * @param {Function} [options.onChild] Handed the ChildProcess. + * @param {string} [options.platform] + * @param {Function} [options.run] For `crlfArgs`. + * @param {Function} [options.spawn] Injection point for tests. + * @return {Promise<{ref: string}>} + */ +async function checkoutBranch(dir, ref, { onProgress = null, onChild = null, platform = process.platform, run = runGit, spawn } = {}) { + const crlf = await crlfArgs(dir, { platform, run }); + const args = [...crlf, 'checkout', '--force', '--progress', ref]; + return new Promise((resolve, reject) => { + let child; + try { + child = spawnGit(args, { cwd: dir, ...(spawn ? { spawn } : {}) }); + } catch (error) { + reject(error); + return; + } + if (onChild) onChild(child); + + const reader = createProgressReader((event) => { if (onProgress) onProgress(event); }); + const stderr = []; + let settled = false; + child.stdout.on('data', () => {}); + child.stderr.on('data', (chunk) => { + const text = chunk.toString('utf8'); + stderr.push(text); + reader.push(text); + }); + child.on('error', (error) => { + if (settled) return; + settled = true; + reject(new GitError(`git checkout could not start: ${error.message}`, { code: error.code, signal: null, stderr: '', args, cwd: dir })); + }); + child.on('close', (status, signal) => { + if (settled) return; + settled = true; + reader.flush(); + if (status === 0) { + resolve({ ref }); + return; + } + const text = stderr.join(''); + reject(new GitError(`git checkout failed (${status === null ? signal : status}): ${failureReason(text, signal)}`, { code: status, signal, stderr: text, args, cwd: dir })); + }); + }); +} + +module.exports = { + stagePaths, + writeTree, + commitTree, + updateBranch, + createBranchAt, + pointHeadAt, + deleteBranch, + checkoutBranch +}; diff --git a/tests/unit/git-clone.test.cjs b/tests/unit/git-clone.test.cjs index 9c04eaff..a744cfe9 100644 --- a/tests/unit/git-clone.test.cjs +++ b/tests/unit/git-clone.test.cjs @@ -1,10 +1,11 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { cloneArgs, parseProgressLines, createProgressReader } = require('../../src/git-clone.cjs'); +const { cloneArgs } = require('../../src/git-clone.cjs'); -// The argument list and the progress reader, without a Git. The clone itself -// runs in git-clone.integration.test.cjs. +// The argument list, without a Git. The progress reader it shares with the +// checkout is tested in git-progress.test.cjs; the clone itself runs in +// git-clone.integration.test.cjs. test('the clone is partial, single-branch on trunk, and writes the repository config it relies on', () => { const args = cloneArgs({ url: 'https://example.test/wp.git', dir: '/sites/wp', platform: 'darwin' }); @@ -25,37 +26,3 @@ test('Windows also gets long paths, which wordpress-develop needs', () => { assert.ok(args.includes('core.longpaths=true')); assert.ok(!cloneArgs({ url: 'u', dir: '/s', platform: 'linux' }).includes('core.longpaths=true')); }); - -test('progress lines parse with and without the remote: prefix, and other lines are dropped', () => { - const text = [ - "Cloning into '/sites/wp'...", - 'remote: Enumerating objects: 4, done. ', - 'remote: Counting objects: 25% (1/4) \rremote: Counting objects: 100% (4/4), done. ', - 'Receiving objects: 42% (1234/5678), 12.00 MiB | 3.00 MiB/s', - 'Resolving deltas: 100% (10/10), done.', - 'Updating files: 50% (100/200)', - 'warning: something unrelated' - ].join('\n'); - assert.deepEqual(parseProgressLines(text), [ - { phase: 'counting objects', percent: 25, loaded: 1, total: 4 }, - { phase: 'counting objects', percent: 100, loaded: 4, total: 4 }, - { phase: 'receiving objects', percent: 42, loaded: 1234, total: 5678 }, - { phase: 'resolving deltas', percent: 100, loaded: 10, total: 10 }, - { phase: 'updating files', percent: 50, loaded: 100, total: 200 } - ]); -}); - -test('a percentage split across two chunks is reported once, whole', () => { - const seen = []; - const reader = createProgressReader((e) => seen.push(e)); - reader.push('Receiving objects: 4'); - assert.deepEqual(seen, [], 'nothing until the line ends'); - reader.push('2% (1234/5678)\rReceiving objects: 5'); - assert.deepEqual(seen, [{ phase: 'receiving objects', percent: 42, loaded: 1234, total: 5678 }]); - reader.push('0% (2900/5678)\n'); - assert.equal(seen.length, 2); - assert.equal(seen[1].percent, 50); - reader.push('Updating files: 100% (200/200)'); - reader.flush(); - assert.equal(seen[2].phase, 'updating files'); -}); diff --git a/tests/unit/git-progress.test.cjs b/tests/unit/git-progress.test.cjs new file mode 100644 index 00000000..290c4c2d --- /dev/null +++ b/tests/unit/git-progress.test.cjs @@ -0,0 +1,57 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { parseProgressLines, createProgressReader, failureReason } = require('../../src/git-progress.cjs'); + +// The one non-porcelain parser, fed the exact lines Git prints. Shared by the +// clone and the checkout; each caller's own arguments are tested beside it. + +test('progress lines parse with and without the remote: prefix, and other lines are dropped', () => { + const text = [ + "Cloning into '/sites/wp'...", + 'remote: Enumerating objects: 4, done. ', + 'remote: Counting objects: 25% (1/4) \rremote: Counting objects: 100% (4/4), done. ', + 'Receiving objects: 42% (1234/5678), 12.00 MiB | 3.00 MiB/s', + 'Resolving deltas: 100% (10/10), done.', + 'Updating files: 50% (100/200)', + 'warning: something unrelated' + ].join('\n'); + assert.deepEqual(parseProgressLines(text), [ + { phase: 'counting objects', percent: 25, loaded: 1, total: 4 }, + { phase: 'counting objects', percent: 100, loaded: 4, total: 4 }, + { phase: 'receiving objects', percent: 42, loaded: 1234, total: 5678 }, + { phase: 'resolving deltas', percent: 100, loaded: 10, total: 10 }, + { phase: 'updating files', percent: 50, loaded: 100, total: 200 } + ]); +}); + +test('a percentage split across two chunks is reported once, whole', () => { + const seen = []; + const reader = createProgressReader((e) => seen.push(e)); + reader.push('Receiving objects: 4'); + assert.deepEqual(seen, [], 'nothing until the line ends'); + reader.push('2% (1234/5678)\rReceiving objects: 5'); + assert.deepEqual(seen, [{ phase: 'receiving objects', percent: 42, loaded: 1234, total: 5678 }]); + reader.push('0% (2900/5678)\n'); + assert.equal(seen.length, 2); + assert.equal(seen[1].percent, 50); + reader.push('Updating files: 100% (200/200)'); + reader.flush(); + assert.equal(seen[2].phase, 'updating files'); +}); + +test('the failure reason is the fatal line even when Git keeps talking after it', () => { + const stderr = [ + 'Receiving objects: 42% (1234/5678)', + "fatal: unable to access 'https://example.test/': Could not resolve host", + 'Please make sure you have the correct access rights', + 'and the repository exists.' + ].join('\n'); + assert.equal(failureReason(stderr), "fatal: unable to access 'https://example.test/': Could not resolve host"); +}); + +test('without a fatal line the last non-progress line is the reason, then the signal, then nothing', () => { + assert.equal(failureReason('Updating files: 50% (1/2)\nsomething odd happened\n'), 'something odd happened'); + assert.equal(failureReason('Updating files: 50% (1/2)\r', 'SIGTERM'), 'killed by SIGTERM'); + assert.equal(failureReason('', null), 'no output'); +}); diff --git a/tests/unit/git-write.integration.test.cjs b/tests/unit/git-write.integration.test.cjs new file mode 100644 index 00000000..5c72b846 --- /dev/null +++ b/tests/unit/git-write.integration.test.cjs @@ -0,0 +1,120 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { stagePaths, writeTree, commitTree, updateBranch, createBranchAt, pointHeadAt, deleteBranch, checkoutBranch } = require('../../src/git-write.cjs'); +const { git, tempDir } = require('./helpers/git.cjs'); + +// The primitives against the real binary: the argument tests prove what is +// asked, this proves Git does what the names promise. The flows built on top +// live in ticket-branches.integration.test.cjs. + +const IDENTITY = ['-c', 'user.name=T', '-c', 'user.email=t@example.com']; + +function makeRepo(t) { + const dir = tempDir(t, 'toolkit git-write-'); + git(['init', '-q', '-b', 'trunk'], dir); + fs.writeFileSync(path.join(dir, 'kept.txt'), 'kept\n'); + fs.writeFileSync(path.join(dir, 'doomed.txt'), 'doomed\n'); + fs.writeFileSync(path.join(dir, 'weird[1]*.txt'), 'literal\n'); + git(['add', '.'], dir); + git([...IDENTITY, 'commit', '-q', '-m', 'first'], dir); + return { dir, base: git(['rev-parse', 'HEAD'], dir).stdout }; +} + +test('stagePaths stages a modification, an addition and a deletion, and a glob-looking name is one file', async (t) => { + const { dir } = makeRepo(t); + fs.writeFileSync(path.join(dir, 'kept.txt'), 'changed\n'); + fs.writeFileSync(path.join(dir, 'new.txt'), 'new\n'); + fs.writeFileSync(path.join(dir, 'w.txt'), 'would match the glob\n'); + fs.unlinkSync(path.join(dir, 'doomed.txt')); + fs.writeFileSync(path.join(dir, 'weird[1]*.txt'), 'still literal\n'); + + await stagePaths(dir, ['kept.txt', 'new.txt', 'doomed.txt', 'weird[1]*.txt']); + + const { stdout } = git(['status', '--porcelain=v2', '-z', '--untracked-files=all'], dir); + // ` ... ` per entry; the path is the last field. + const byPath = Object.fromEntries(stdout.split('\0').filter(Boolean).map((e) => { + const fields = e.split(' '); + return [fields[fields.length - 1], fields.slice(0, 2).join(' ')]; + })); + assert.deepEqual(byPath, { + 'kept.txt': '1 M.', + 'doomed.txt': '1 D.', + 'new.txt': '1 A.', + 'weird[1]*.txt': '1 M.', + 'w.txt': '? w.txt' + }, 'the bracketed name was staged as itself, and the file its glob would match was not'); +}); + +test('write-tree, commit-tree and update-ref make one commit with the parent asked for, under the identity given', async (t) => { + const { dir, base } = makeRepo(t); + fs.writeFileSync(path.join(dir, 'kept.txt'), 'changed\n'); + await stagePaths(dir, ['kept.txt']); + + const tree = await writeTree(dir); + const author = { name: 'WordPress Contributor Toolkit', email: 'noreply@localhost' }; + const oid = await commitTree(dir, { tree, parent: base, message: 'Work in progress', author }); + await updateBranch(dir, 'trunk', oid, { expected: base }); + + assert.equal(git(['rev-parse', 'HEAD'], dir).stdout, oid); + assert.equal(git(['log', '-1', '--format=%P%x00%an%x00%ae%x00%cn%x00%ce%x00%s', 'HEAD'], dir).stdout, + [base, author.name, author.email, author.name, author.email, 'Work in progress'].join('\0')); + assert.equal(git(['status', '--porcelain=v2'], dir).stdout, '', 'index, HEAD and worktree agree'); +}); + +test('update-ref with a stale expected value refuses rather than overwriting', async (t) => { + const { dir, base } = makeRepo(t); + const tree = await writeTree(dir); + const oid = await commitTree(dir, { tree, parent: base, message: 'x', author: { name: 'a', email: 'a@b' } }); + + await assert.rejects(updateBranch(dir, 'trunk', oid, { expected: '0000000000000000000000000000000000000001' }), (e) => e.name === 'GitError' && e.code === 128); + assert.equal(git(['rev-parse', 'HEAD'], dir).stdout, base); +}); + +test('createBranchAt and pointHeadAt move HEAD without touching the index or the worktree', async (t) => { + const { dir, base } = makeRepo(t); + fs.writeFileSync(path.join(dir, 'kept.txt'), 'loose edit\n'); + const indexBefore = fs.statSync(path.join(dir, '.git', 'index')).mtimeMs; + + await createBranchAt(dir, 'ticket/1', 'trunk'); + await pointHeadAt(dir, 'ticket/1'); + + assert.equal(git(['symbolic-ref', '--short', 'HEAD'], dir).stdout, 'ticket/1'); + assert.equal(git(['rev-parse', 'ticket/1'], dir).stdout, base); + assert.equal(fs.readFileSync(path.join(dir, 'kept.txt'), 'utf8'), 'loose edit\n'); + assert.equal(fs.statSync(path.join(dir, '.git', 'index')).mtimeMs, indexBefore, 'the index was not rewritten'); +}); + +test('checkoutBranch restores tracked files, leaves ignored ones alone, and deleteBranch removes the ref', async (t) => { + const { dir } = makeRepo(t); + fs.writeFileSync(path.join(dir, '.gitignore'), 'node_modules/\n'); + git(['add', '.gitignore'], dir); + git([...IDENTITY, 'commit', '-q', '-m', 'ignore'], dir); + fs.mkdirSync(path.join(dir, 'node_modules')); + fs.writeFileSync(path.join(dir, 'node_modules', 'expensive.js'), 'expensive\n'); + await createBranchAt(dir, 'ticket/2', 'trunk'); + fs.writeFileSync(path.join(dir, 'kept.txt'), 'dirty\n'); + + const phases = []; + await checkoutBranch(dir, 'ticket/2', { onProgress: (e) => phases.push(e.phase) }); + + assert.equal(git(['symbolic-ref', '--short', 'HEAD'], dir).stdout, 'ticket/2'); + assert.equal(fs.readFileSync(path.join(dir, 'kept.txt'), 'utf8'), 'kept\n', 'forced: the dirty file was reset'); + assert.equal(fs.readFileSync(path.join(dir, 'node_modules', 'expensive.js'), 'utf8'), 'expensive\n'); + for (const phase of phases) assert.match(phase, /^[a-z ]+$/); + + await checkoutBranch(dir, 'trunk'); + await deleteBranch(dir, 'ticket/2'); + assert.equal(git(['for-each-ref', '--format=%(refname:short)', 'refs/heads/'], dir).stdout, 'trunk'); +}); + +test('a checkout of a ref that does not exist rejects with Git\'s reason', async (t) => { + const { dir } = makeRepo(t); + await assert.rejects(checkoutBranch(dir, 'ticket/404'), (e) => { + assert.equal(e.name, 'GitError'); + assert.match(e.message, /^git checkout failed \(1\): error: pathspec 'ticket\/404'/); + return true; + }); +}); diff --git a/tests/unit/git-write.test.cjs b/tests/unit/git-write.test.cjs new file mode 100644 index 00000000..f6faa7fc --- /dev/null +++ b/tests/unit/git-write.test.cjs @@ -0,0 +1,195 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { EventEmitter } = require('node:events'); +const { PassThrough } = require('node:stream'); + +const { + stagePaths, + writeTree, + commitTree, + updateBranch, + createBranchAt, + pointHeadAt, + deleteBranch, + checkoutBranch +} = require('../../src/git-write.cjs'); + +// The argument lists, without a Git: `run` and `spawn` are injected and record +// what they were asked. The primitives meet the real binary in +// git-write.integration.test.cjs, and the flows built on them in +// ticket-branches.integration.test.cjs. + +// A `run` that answers every call with `stdout` and records the argv and +// options it saw. `crlfArgs` asks `config --local --get core.autocrlf` first +// on Windows; `autocrlfUnset` scripts that answer. +function recordingRun({ stdout = '', autocrlfUnset = true } = {}) { + const calls = []; + const run = async (args, options) => { + calls.push({ args, options }); + if (args[0] === 'config') return { status: autocrlfUnset ? 1 : 0, stdout: Buffer.from(autocrlfUnset ? '' : 'false\n'), stderr: '' }; + return { status: 0, stdout: Buffer.from(stdout), stderr: '' }; + }; + return { run, calls, last: () => calls[calls.length - 1] }; +} + +function fakeChild({ stderr = [], status = 0, signal = null, error = null } = {}) { + const child = new EventEmitter(); + child.pid = 4242; + child.stdout = new PassThrough(); + child.stderr = new PassThrough(); + setTimeout(() => { + if (error) { + child.emit('error', error); + child.emit('close', null, null); + return; + } + for (const chunk of stderr) child.stderr.write(chunk); + child.stdout.end(); + child.stderr.end(); + setTimeout(() => child.emit('close', status, signal), 0); + }, 0); + 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('stagePaths hands Git exactly the paths, NUL-separated on stdin, taken literally, with deletions included', async () => { + const { run, last } = recordingRun(); + const paths = ['wp-login.php', 'weird[1]*.php', 'with space.txt', 'doomed.php']; + + const count = await stagePaths('/sites/wp', paths, { platform: 'darwin', run }); + + assert.equal(count, 4); + const { args, options } = last(); + assert.deepEqual(args, ['--literal-pathspecs', 'add', '-A', '--pathspec-from-file=-', '--pathspec-file-nul']); + assert.equal(options.cwd, '/sites/wp'); + assert.equal(options.input.toString('utf8'), 'wp-login.php\0weird[1]*.php\0with space.txt\0doomed.php\0'); +}); + +test('stagePaths with nothing to stage runs nothing', async () => { + const { run, calls } = recordingRun(); + assert.equal(await stagePaths('/sites/wp', [], { platform: 'darwin', run }), 0); + assert.deepEqual(calls, []); +}); + +test('on Windows the CRLF view prefixes the commands that touch the index or the worktree, and only those', async () => { + const { run, calls } = recordingRun({ stdout: 'abc\n' }); + + await stagePaths('C:\\Sites\\wp', ['a.php'], { platform: 'win32', run }); + const add = calls.find((c) => c.args.includes('add')); + assert.deepEqual(add.args.slice(0, 2), ['-c', 'core.autocrlf=true']); + + const { spawn, calls: spawns } = recordingSpawn(); + await checkoutBranch('C:\\Sites\\wp', 'ticket/1', { platform: 'win32', run, spawn }); + const checkout = spawns[0].args; + assert.deepEqual(checkout.slice(checkout.indexOf('core.autocrlf=true') - 1, checkout.indexOf('core.autocrlf=true') + 2), ['-c', 'core.autocrlf=true', 'checkout']); + + // Object and ref writes see no worktree and get no prefix. + calls.length = 0; + await writeTree('C:\\Sites\\wp', { run }); + await commitTree('C:\\Sites\\wp', { tree: 't', parent: 'p', message: 'm', author: { name: 'n', email: 'e' }, run }); + await updateBranch('C:\\Sites\\wp', 'ticket/1', 'abc', { run }); + for (const { args } of calls) assert.ok(!args.includes('core.autocrlf=true'), `${args.join(' ')} carries the CRLF view`); +}); + +test('an explicit local core.autocrlf is left alone, as it is for the reads', async () => { + const { run, last } = recordingRun({ autocrlfUnset: false }); + await stagePaths('C:\\Sites\\wp', ['a.php'], { platform: 'win32', run }); + assert.equal(last().args[0], '--literal-pathspecs'); +}); + +test('writeTree and commitTree return the object id Git printed, and the commit carries exactly one parent and the identity given', async () => { + const { run, last } = recordingRun({ stdout: '0123456789abcdef0123456789abcdef01234567\n' }); + + assert.equal(await writeTree('/sites/wp', { run }), '0123456789abcdef0123456789abcdef01234567'); + assert.deepEqual(last().args, ['write-tree']); + + const author = { name: 'WordPress Contributor Toolkit', email: 'noreply@localhost' }; + const oid = await commitTree('/sites/wp', { tree: 'tree1', parent: 'base1', message: 'Work in progress', author, run }); + assert.equal(oid, '0123456789abcdef0123456789abcdef01234567'); + assert.deepEqual(last().args, [ + '-c', 'user.name=WordPress Contributor Toolkit', + '-c', 'user.email=noreply@localhost', + 'commit-tree', 'tree1', '-p', 'base1', '-m', 'Work in progress' + ]); + assert.equal(last().args.filter((a) => a === '-p').length, 1, 'one parent, never a stack'); +}); + +test('updateBranch writes the full ref, and passes the expected old value when given one', async () => { + const { run, last } = recordingRun(); + await updateBranch('/sites/wp', 'ticket/59234', 'new1', { run }); + assert.deepEqual(last().args, ['update-ref', 'refs/heads/ticket/59234', 'new1']); + await updateBranch('/sites/wp', 'ticket/59234', 'new1', { expected: 'old1', run }); + assert.deepEqual(last().args, ['update-ref', 'refs/heads/ticket/59234', 'new1', 'old1']); +}); + +test('createBranchAt, pointHeadAt and deleteBranch are the ref commands, nothing that touches files', async () => { + const { run, calls } = recordingRun(); + await createBranchAt('/sites/wp', 'ticket/1', 'trunk', { run }); + await pointHeadAt('/sites/wp', 'ticket/1', { run }); + await deleteBranch('/sites/wp', 'ticket/1', { run }); + assert.deepEqual(calls.map((c) => c.args), [ + ['branch', '--', 'ticket/1', 'trunk'], + ['symbolic-ref', 'HEAD', 'refs/heads/ticket/1'], + ['branch', '-D', '--', 'ticket/1'] + ]); + for (const { options } of calls) assert.equal(options.cwd, '/sites/wp'); +}); + +test('checkoutBranch forces, asks for progress, reports it from stderr and hands the child out', async () => { + const { run } = recordingRun(); + const { spawn, calls } = recordingSpawn({ + stderr: ['Updating files: 50% (100/200)\rUpdating files: 100% (200/200), done.\n'] + }); + const seen = []; + let child = null; + + const result = await checkoutBranch('/sites/wp', 'ticket/1', { platform: 'darwin', run, spawn, onProgress: (e) => seen.push(e), onChild: (c) => { child = c; } }); + + assert.deepEqual(result, { ref: 'ticket/1' }); + const { args, options } = calls[0]; + assert.deepEqual(args.slice(-4), ['checkout', '--force', '--progress', 'ticket/1']); + assert.equal(options.cwd, '/sites/wp'); + assert.equal(options.stdio[0], 'ignore'); + assert.equal(child.pid, 4242); + assert.deepEqual(seen.map((e) => [e.phase, e.loaded, e.total]), [['updating files', 100, 200], ['updating files', 200, 200]]); +}); + +test('a failed checkout rejects with a GitError carrying the fatal line and the stderr', async () => { + const { run } = recordingRun(); + const { spawn } = recordingSpawn({ + status: 1, + stderr: ['error: Your local changes would be overwritten\n', 'fatal: cannot switch\n'] + }); + + await assert.rejects( + checkoutBranch('/sites/wp', 'ticket/1', { platform: 'darwin', run, spawn }), + (error) => { + assert.equal(error.name, 'GitError'); + assert.equal(error.code, 1); + assert.match(error.message, /^git checkout failed \(1\): fatal: cannot switch$/); + assert.match(error.stderr, /local changes/); + assert.equal(error.cwd, '/sites/wp'); + return true; + } + ); +}); + +test('a checkout whose Git never started rejects the same way', async () => { + const { run } = recordingRun(); + const enoent = Object.assign(new Error('spawn git ENOENT'), { code: 'ENOENT' }); + const { spawn } = recordingSpawn({ error: enoent }); + + await assert.rejects( + checkoutBranch('/sites/wp', 'ticket/1', { platform: 'darwin', run, spawn }), + (error) => error.name === 'GitError' && error.code === 'ENOENT' + ); +}); From c1aab0a3ce163132a9688f473f49b94eb788ef6c Mon Sep 17 00:00:00 2001 From: JuanMa Date: Mon, 7 Sep 2026 16:09:56 +0200 Subject: [PATCH 2/6] Move the ticket-branch writes onto the bundled Git Park, start, switch and delete in src/ticket-branches.js run on the primitives in git-write.cjs; isomorphic-git and the CRLF filesystem view leave the module. Facade signatures, return shapes, error codes and the progress vocabulary on switch:progress are unchanged, so main.js and the renderer do not know. The one-WIP-commit invariant is now literal: the park stages exactly the rows the scan returned (one `add -A` over a NUL-separated, literal pathspec, so a bracket in a filename is a character), writes the tree, writes one commit with `commit-tree -p ` under the app's own identity, and moves the branch ref onto it with the HEAD it read first as the expected old value, so a second writer in the same site fails loudly instead of being overwritten. Starting a ticket is a ref and a symbolic-ref, never a checkout, which is what carries loose edits onto the branch. The checkout is forced and reports `updating files`, the one phase Git prints, through the same reader the clone uses; the old engine's `analyze` phase has no counterpart and is no longer emitted. The fixture repositories are still built by isomorphic-git and read back with it after the binary writes them, which turns the agreement check around: a site the old engine made keeps working, and what the new one writes is a repository the old one still understands. New tests pin the commit's author, committer and message, a clean status right after a park, glob-looking and spaced filenames, an untouched index across startTicketBranch, a held ref lock (rejects, no `stage` tag) and a held index lock (rejects with `stage: 'checkout'`). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_013bqghuMjo6GQFEdRrZtNJS --- .../instructions/code-review.instructions.md | 2 +- AGENTS.md | 2 +- TESTING.md | 2 +- src/switch-progress.cjs | 32 ++-- src/ticket-branches.js | 159 ++++++++++-------- tests/unit/switch-progress.test.cjs | 26 +-- .../unit/ticket-branches.integration.test.cjs | 117 ++++++++++++- 7 files changed, 232 insertions(+), 108 deletions(-) diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index db5199f5..ba1f856b 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. `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 outside the write flows 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 new-site clone runs on it too (`src/git-clone.cjs`, the one place that reads Git's human-facing progress lines, because there is no porcelain for progress); the remaining writes, with the reads inside them, 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. +**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 outside the write flows 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 new-site clone (`src/git-clone.cjs`) and the ticket-branch writes (`src/ticket-branches.js` over the primitives in `src/git-write.cjs`: stage, commit-tree, update-ref, branch, checkout) run on it too, with `src/git-progress.cjs` as the one place that reads Git's human-facing progress lines, because there is no porcelain for progress; a second parser of those lines anywhere else is a finding. The remaining writes, the trunk update and patch apply, with the reads inside them, still run on `isomorphic-git` until #385 moves them, 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 35d7fdd2..20622a55 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