thunderbird-patch-review
Simple email patch review tool for Thunderbird
git clone git://mccd.space/thunderbird-patch-reviewcommit 2b3c49d14386c575ef5cb6027b897f5f51920d76
parent 5a0a10d4d3abae6347354203f9dfd4f2356c761c
Author: Pi Agent <agent@pi.local>
Date: Sun, 19 Jul 2026 11:00:19 +0200
patchHost.check: run git am in a throwaway worktree to classify
apply() runs git am (with --3way by default), but check() used git apply
--check --cached. --cached tests against the index (not what am
operates on), and --check would mispredict any patch whose context
git am --3way would rebuild from the blob hash — mailing-list relays
and email clients love to mangle blank-context whitespace, exactly the
case that turned an applicable patch (the agendafs README comment adds)
into a red 'Apply...' on a real repo.
Mirror apply()'s actual mechanism instead. Two stages:
1. Fast path: plain --check (no --cached) against the working tree.
Forward exit 0 -> applicable; reverse exit 0 -> applied. Skips
the worktree cost in the common case.
2. --check could not decide -> add a --detach worktree at HEAD, run
git am with the strategy apply() uses. The user's branch is never
touched (--detach forks off HEAD; am runs in the worktree only).
git am gives a stable tri-state signal:
exit 0 + 'Patch already applied' / 'No changes' -> applied
exit 0 (only 'Applying: <subject>') -> applicable
exit 128 (context mismatch, conflict, dirty tree) -> conflict
The worktree is force-removed afterwards; an am-contributed failure is
cleaned up with git am --abort before the remove.
Diffstat:
1 file changed, 75 insertions(+), 30 deletions(-)
diff --git a/extension/api/patchHost/implementation.js b/extension/api/patchHost/implementation.js
@@ -128,55 +128,100 @@ async function apply(repo, strategy, patches) {
}
}
-// Classify a patch series against a repository without writing anything.
+// 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
+// dirtying the user's repo, by adding a throwaway --detach worktree at
+// HEAD and force-removing it afterwards. (The user's branch is never
+// touched; we never run am in the user's own worktree.)
//
-// `git apply --check` is the non-mutating sibling of `git am`, so we use it
-// here. It would also be the obvious building block for detecting
-// "already applied" — but its per-file mode checks each argument against
-// the *original* state independently, so a sequel whose context depends on
-// an earlier patch fails spuriously. Concatenate the mboxes into a single
-// file first: the concatenated file is parsed as a series and its hunks
-// verified in order, which matches `git am`'s sequential application.
+// Two 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.
//
-// Detection, run against the cached index (= HEAD when the working tree
-// is clean, the only state `git am` will operate on):
-// forward `git apply --check --cached` exits 0 -> "applicable"
-// else reverse `git apply --check --cached --reverse` exits 0 -> "applied"
-// else -> "conflict"
-//
-// We don't simulate apply()'s --3way path: a conflict that --3way would
-// auto-merge into a commit is reported "conflict" instead. That's the
-// conservative call — better to leave the button glowing red and let the
-// user discover the recoverable merge than silently promise an apply.
+// git am backs the tri-state with stable signals:
+// exit 0 + "Patch already applied" / "No changes" -> "applied"
+// exit 0 (only "Applying: <subject>") -> "applicable"
+// exit 128 (context mismatch, merge conflict, dirty-index refusal) -> "conflict"
async function check(repo, strategy, patches) {
const git = await findGit();
await describeRepo(git, repo);
if (!patches.length) {
throw new Error("no patches in request");
-
}
- const work = PathUtils.join(
+ // Stage 1: cheap --check against the working tree — fast-path the common
+ // "cleanly applicable" and "exactly already applied" cases without
+ // paying for a worktree checkout. The probe holds the mboxes
+ // concatenated: separate per-file args are each checked against the
+ // *original* tree independently, so a sequel whose context depends on an
+ // earlier patch fails spuriously; concatenated, git parses the file as a
+ // series and the contexts line up, just as am does.
+ const probe = PathUtils.join(
PathUtils.tempDir,
- `patch-review.check.${Date.now()}.${Math.floor(Math.random() * 1e9)}`
+ `patch-review.check.${Date.now()}.${Math.floor(Math.random() * 1e9)}.patch`
);
- await IOUtils.makeDirectory(work, { permissions: 0o700 });
+ await IOUtils.writeUTF8(probe, patches.join("\n"));
+ let fwd;
try {
- const file = PathUtils.join(work, "series.patch");
- await IOUtils.writeUTF8(file, patches.join("\n"));
- const fwd = await run(git, ["-C", repo, "apply", "--check", "--cached", file]);
+ fwd = await run(git, ["-C", repo, "apply", "--check", probe]);
if (fwd.exitCode === 0) {
return { ok: true, status: "applicable", output: fwd.output };
}
- const rev = await run(git, ["-C", repo, "apply", "--check", "--cached", "--reverse", file]);
+ const rev = await run(git, ["-C", repo, "apply", "--check", "--reverse", probe]);
if (rev.exitCode === 0) {
return { ok: true, status: "applied", output: fwd.output };
}
- // Only the forward run's output carries the useful 'why' (which file,
- // which hunk). The reverse run repeats the same failure as it also
- // can't apply; including it would just print every line twice.
- return { ok: true, status: "conflict", output: fwd.output };
} finally {
+ IOUtils.remove(probe).catch(() => {});
+ }
+
+ // 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
+ // predicts the Apply button's outcome exactly, including the --3way
+ // reconstruction --check skipped. The user's branch is never touched
+ // (--detach forks off HEAD; am runs in the temporary worktree only) and
+ // the worktree is force-removed afterwards.
+ const work = PathUtils.join(
+ PathUtils.tempDir,
+ `patch-review.am.${Date.now()}.${Math.floor(Math.random() * 1e9)}`
+ );
+ const make = await run(git, ["-C", repo, "worktree", "add", "--detach", work]);
+ if (make.exitCode !== 0) {
+ // Rare (bare repo, no worktree support, …). Don't trust --check's
+ // partial answer here — report "conflict" so the user clicks Apply
+ // and reads am's own error, instead of guessing.
+ return { ok: true, status: "conflict", output: fwd.output + make.output };
+ }
+ try {
+ const file = PathUtils.join(work, "series.mbox");
+ await IOUtils.writeUTF8(file, patches.join("\n"));
+ const args = ["-C", work, "am"];
+ if (strategy !== "am") {
+ args.push("--3way");
+ }
+ args.push(file);
+ const am = await run(git, args);
+ if (am.exitCode === 0) {
+ if (/Patch already applied|No changes --/.test(am.output)) {
+ return { ok: true, status: "applied", output: am.output };
+ }
+ return { ok: true, status: "applicable", output: am.output };
+ }
+ // am stopped mid-series (context mismatch, merge conflict, dirty
+ // tree). Roll the worktree back so its state isn't left dangling, then
+ // report conflict with am's diagnostic — it names the file and hunk.
+ await run(git, ["-C", work, "am", "--abort"]);
+ return { ok: true, status: "conflict", output: am.output };
+ } finally {
+ // --force drops the worktree even if am left its working tree dirty.
+ await run(git, ["-C", repo, "worktree", "remove", "--force", work]);
IOUtils.remove(work, { recursive: true }).catch(() => {});
}
}