thunderbird-patch-review
Simple email patch review tool for Thunderbird
git clone git://mccd.space/thunderbird-patch-reviewcommit eb8ef8acdf954d9b169bb8682c7b7f2953883e0f
parent 368ca46edace56b940e794557559ef40fe012fc1
Author: Pi Agent <agent@pi.local>
Date: Sun, 19 Jul 2026 11:20:18 +0200
patchHost.check: detect applied when end-state matches HEAD
A patchset already-applied with slightly tweaked intermediate content (a
doubled-letter typo the actual commits cleaned up in-tree, mail-relay
mangled whitespace that the real commit trimmed, …) lands the same final
tree state per touched path as the in-mail series — but git am would
refuse on stage 2 (it can't recreate a file that's there, can't remove
one that's gone), patch-id won't match (the diffs differ), and git apply
--check fails both directions. Without intervention, check() reported
'conflict' and the UI glowed red, even though the user already has the
work.
Add stage 1.5: between the cheap --check fast path and the worktree
git am, parse each path's FINAL post-patch blob hash from the right
side of the last 'index <old>..<new>' line that touches it, and verify
against 'git ls-tree HEAD -- <paths>'. If every touched path is at its
target blob (or absent, for deletes), the series end-state matches HEAD
-> 'applied'. The parser is plain JS — no chrome APIs — and is unit
tested from tests/run.mjs via the same source-extract trick the
experiment-wiring tests already use for the implementation file.
Verified against synthetic scenarios (applicable, applied, divergent
conflict, dirty-tree conflict) and the user's real /srv/src/landdown
3-patch series: the latter reports 'applied' via stage 1.5, while the
others keep their previous classifications.
Quoted paths (spaces or special chars git renders with double quotes)
are not handled — collectFinalBlobs returns null on them so the caller
falls through to git am instead of silently misclassifying.
Diffstat:
2 files changed, 239 insertions(+), 6 deletions(-)
diff --git a/extension/api/patchHost/implementation.js b/extension/api/patchHost/implementation.js
@@ -128,6 +128,115 @@ async function apply(repo, strategy, patches) {
}
}
+// Map each path a patch series touches to its FINAL post-series state,
+// taken from the right side of the last `index <old>..<new>` line that
+// carried the path. Last-write-wins across the multi-patch mbox (a
+// sequel that re-touches a file overrides the earlier patch's target).
+//
+// Returns a Map<path, { blob: "<hex>" } | { deleted: true }>. Returns
+// null when the input had no parseable path or when a path git rendered
+// quoted was seen (paths with spaces or other special chars aren't
+// handled — bail so the caller falls through to the git am worktree
+// instead of silently misclassifying).
+//
+// We extract the path from `+++ b/path`/`--- a/path` rather than the
+// `diff --git a/path b/path` header, so a future rename (a/old b/new)
+// maps the target to the new name and the old name is recorded deleted.
+function collectFinalBlobs(text) {
+ const finals = new Map();
+ const lines = text.split("\n");
+ let block = null;
+ let unsupported = false;
+ const flush = (b) => {
+ if (!b || !b.targetPath) return;
+ if (b.isDelete) {
+ finals.set(b.targetPath, { deleted: true });
+ } else if (b.targetBlob) {
+ finals.set(b.targetPath, { blob: b.targetBlob });
+ }
+ };
+ for (const line of lines) {
+ if (line.startsWith("diff --git ")) {
+ flush(block);
+ block = { targetPath: null, srcPath: null, targetBlob: null, isDelete: false };
+ continue;
+ }
+ if (!block) continue;
+ if (line.startsWith("deleted file mode ")) { block.isDelete = true; continue; }
+ if (line === "+++ /dev/null") { block.isDelete = true; continue; }
+ // git quotes paths with spaces or other special chars, either as
+ // `+++ "b/path with space"` or `+++ b/"path with space"` (and the
+ // analogous forms for ---). We don't unquote; bail so the caller
+ // falls through to git am instead of misclassifying.
+ const linePath = line.replace(/^(?:\+\+\+ (?:b\/)?|--- (?:a\/)?)/, "").replace(/^\/dev\/null$/, "");
+ if ((line.startsWith("+++") || line.startsWith("---")) && linePath.startsWith('"')) {
+ unsupported = true;
+ break;
+ }
+ let m;
+ if ((m = /^index ([0-9a-f]+)\.\.([0-9a-f]+)/.exec(line))) {
+ block.targetBlob = m[2];
+ continue;
+ }
+ if ((m = /^--- a\/(.+)$/.exec(line))) {
+ block.srcPath = m[1];
+ if (block.targetPath == null) block.targetPath = m[1];
+ continue;
+ }
+ if ((m = /^\+\+\+ b\/(.+)$/.exec(line))) {
+ const newPath = m[1];
+ // The post-patch path differs from the source => a rename. The
+ // source vanishes; the target takes the (likely unchanged) blob.
+ if (block.srcPath && block.srcPath !== newPath) {
+ finals.set(block.srcPath, { deleted: true });
+ }
+ block.targetPath = newPath;
+ block.isDelete = false;
+ continue;
+ }
+ }
+ flush(block);
+ if (unsupported) return null;
+ return finals.size ? finals : null;
+}
+
+// Match the per-path target blobs against `git ls-tree HEAD -- <paths>`.
+// Returns true if every path in `finals` is already at its expected blob
+// in HEAD (or absent for deletes). False otherwise. Calls git once with
+// every path in one ls-tree to keep the spawn count down.
+async function blobsMatchHead(git, repo, finals) {
+ const paths = [...finals.keys()];
+ const ls = await run(git, ["-C", repo, "ls-tree", "HEAD", "--", ...paths]);
+ // ls-tree prints rows like `<mode> blob <hash>\t<path>`; paths absent
+ // from HEAD print no row at all.
+ const byPath = new Map();
+ for (const row of ls.output.split("\n")) {
+ if (!row) continue;
+ const m = /^(\S+) (\S+) ([0-9a-f]+)\t(.+)$/.exec(row);
+ if (m) byPath.set(m[4], m[3]);
+ }
+ for (const [path, info] of finals) {
+ if (info.deleted) {
+ if (byPath.has(path)) return false;
+ } else {
+ const actual = byPath.get(path);
+ if (!actual) return false;
+ // git format-patch truncates blob hashes (default 7 hex). The
+ // ls-tree output is full-length, so a prefix-equal either way marks
+ // the same blob.
+ const expected = info.blob;
+ if (
+ actual !== expected &&
+ !actual.startsWith(expected) &&
+ !expected.startsWith(actual)
+ ) {
+ return false;
+ }
+ }
+ }
+ return true;
+}
+
// Classify a patch series against a repository without writing anything
// to the user's working tree. apply() runs `git am`, so the most honest
// precheck is to run `git am` too and read what it would do — without
@@ -135,16 +244,21 @@ async function apply(repo, strategy, patches) {
// HEAD and force-removing it afterwards. (The user's branch is never
// touched; we never run am in the user's own worktree.)
//
-// Two stages:
+// Three stages:
// 1. Cheap `git apply --check` against the working tree fast-paths the
// common "cleanly applicable" and "exactly already applied" cases
// without paying for a worktree checkout.
-// 2. When --check can't decide (a context `git am --3way` will rebuild
-// from the blob hash — mailing lists love to mangle blank-context
-// whitespace, for one — or a dirty working tree am would refuse),
-// run `git am` itself in a fresh --detach worktree.
+// 2. --check couldn't decide, but the series' END STATE per touched
+// path already sits in HEAD. Patches committed with slightly tweaked
+// intermediate content (a doubled-letter typo, mail-relay-mangled
+// blank-context whitespace that the actual commit cleaned up mid-
+// series) won't patch-id-match — but the FINAL tree state per file
+// agrees. This catches the common "patchset already applied with
+// minor diff divergence" case without a worktree checkout.
+// 3. Failing the above, run `git am` itself in a fresh --detach worktree
+// to read am's own tri-state.
//
-// git am backs the tri-state with stable signals:
+// git am backs the tri-state with stable signals (stage 3):
// exit 0 + "Patch already applied" / "No changes" -> "applied"
// exit 0 (only "Applying: <subject>") -> "applicable"
// exit 128 (context mismatch, merge conflict, dirty-index refusal) -> "conflict"
@@ -181,6 +295,27 @@ async function check(repo, strategy, patches) {
IOUtils.remove(probe).catch(() => {});
}
+ // Stage 1.5: end-state check. --check refused in both directions — but
+ // if every path the series touches is already at its final post-patch
+ // blob in HEAD (or absent, for deletes), the series has effectively
+ // landed. git am would still flag a conflict (it can't recreate a file
+ // that's already there, can't delete a file that's already gone); the
+ // button would glow red. Compare blob hashes instead — they line up.
+ //
+ // collectFinalBlobs returns null when it can't parse all the paths
+ // (e.g. git quotes paths with spaces), which falls through cleanly.
+ const finals = collectFinalBlobs(patches.join("\n"));
+ if (finals) {
+ const headMatch = await blobsMatchHead(git, repo, finals);
+ if (headMatch) {
+ return {
+ ok: true,
+ status: "applied",
+ output: "Series end-state matches HEAD (every touched file is at its target blob).",
+ };
+ }
+ }
+
// Stage 2: --check couldn't decide. Run git am for real in a fresh
// --detach worktree, mirroring apply()'s strategy (am or am3). apply()
// runs git am with the same arguments on the same patches, so the probe
diff --git a/tests/run.mjs b/tests/run.mjs
@@ -364,6 +364,104 @@ function check(name, cond, extra = "") {
}
check("experiment: implementation parses", parses);
check("experiment: defines the patchHost class", /\bvar patchHost\s*=/.test(source));
+
+ // check() includes a pure-JS parser (collectFinalBlobs) for the stage
+ // 1.5 “already applied via end-state blob match” shortcut. It doesn't
+ // touch any chrome API, so lift it out of the source and exercise it
+ // against synthetic/fixture patches, mirroring how patchHost.check()
+ // uses it to detect a patchset whose final tree state already sits in
+ // HEAD (even when the individual commit diffs differ).
+ const cfbMatch = /^function collectFinalBlobs\(text\) \{[\s\S]*?^\}/m.exec(source);
+ check("check: collectFinalBlobs defined", Boolean(cfbMatch));
+ if (cfbMatch) {
+ const collect = new Function(cfbMatch[0] + "\nreturn collectFinalBlobs;")();
+ const eq = (a, b) => JSON.stringify([...(a || new Map()).entries()].sort())
+ === JSON.stringify([...(b || new Map()).entries()].sort());
+
+ check(
+ "check: collectFinalBlobs single modify",
+ eq(
+ collect(fixture("patch1.body")),
+ new Map([["src/socket.c", { blob: "6eab6c5" }]])
+ )
+ );
+
+ // Two sequenced patches to the same file: last-write-wins, target is
+ // the second patch's post-blob.
+ const two = [
+ "diff --git a/f b/f",
+ "index aaaaaaa..bbbbbbb 100644",
+ "--- a/f",
+ "+++ b/f",
+ "@@ -1 +1 @@",
+ "-a",
+ "+b",
+ "diff --git a/f b/f",
+ "index bbbbbbb..ccccccc 100644",
+ "--- a/f",
+ "+++ b/f",
+ "@@ -1 +1 @@",
+ "-b",
+ "+c",
+ "",
+ ].join("\n");
+ check(
+ "check: collectFinalBlobs last-write-wins across patches",
+ eq(collect(two), new Map([["f", { blob: "ccccccc" }]]))
+ );
+
+ // Create-then-delete cancels to a deleted final state.
+ const cycle = [
+ "diff --git a/tmp b/tmp",
+ "new file mode 100644",
+ "index 0000000..aaaaaaaa",
+ "--- /dev/null",
+ "+++ b/tmp",
+ "diff --git a/tmp b/tmp",
+ "deleted file mode 100644",
+ "index aaaaaaa..0000000",
+ "--- a/tmp",
+ "+++ /dev/null",
+ "",
+ ].join("\n");
+ check(
+ "check: create+delete cancels to deleted",
+ eq(collect(cycle), new Map([["tmp", { deleted: true }]]))
+ );
+
+ // A rename is recorded as new-path target + old-path deleted.
+ const rename = [
+ "diff --git a/old b/new",
+ "similarity index 100%",
+ "rename from old",
+ "rename to new",
+ "index abcdef0..abcdef0",
+ "--- a/old",
+ "+++ b/new",
+ "",
+ ].join("\n");
+ check(
+ "check: rename marks source deleted",
+ eq(
+ collect(rename),
+ new Map([["old", { deleted: true }], ["new", { blob: "abcdef0" }]])
+ )
+ );
+
+ // A path with a quoted name (spaces, special chars) is not handled:
+ // bail to null so the caller falls through to git am in the worktree
+ // instead of misclassifying the series.
+ const quoted = [
+ "diff --git a/\"foo bar.txt\" b/\"foo bar.txt\"",
+ "index 0000000..bbbbbbb",
+ '--- a/\"foo bar.txt\"',
+ '+++ b/\"foo bar.txt\"',
+ "@@ -0,0 +1 @@",
+ "+hi",
+ "",
+ ].join("\n");
+ check("check: quoted path bails to null", collect(quoted) === null);
+ }
}
// --- summary ----------------------------------------------------------------