Measure staleness from where trunk really is, not from the calendar

src/patch-apply.js

@@ -377,6 +377,74 @@ function patchIsAbsent(dir, files) {
 	return text.every((f) => !resolveFile(dir, f, { diagnose: false }).error);
 }
 
+/**
+ * Whether the patch the app applied could still be taken back out of this
+ * checkout, asked right now rather than remembered (#306).
+ *
+ * The record the app keeps says only that a patch text was stored. That is not
+ * the question the banner needs answering: once the contributor's own edits sit
+ * on the patch's lines it has been **absorbed** — it has become their changes,
+ * and no revert can separate the two again. Undoing those edits makes it
+ * removable again, which is why this is measured on demand and never tracked as
+ * a flag.
+ *
+ * Nothing here writes. `resolveFile` reads each file and works out what a
+ * reverse *would* produce, and `diagnoseHunks` behind it says how much of the
+ * patch the file no longer holds — the same machinery the apply path uses, so
+ * there is no second way to ask whether a patch still fits. The resolved
+ * contents are discarded.
+ *
+ * A patch that is simply gone — a trunk update or a discard reset the tree — is
+ * not absorbed, and gets the same two-part guard the revert itself uses: not one
+ * file reversed, *and* the whole patch resolves forwards. Its record is stale,
+ * and Revert stays offered because pressing it is what clears it.
+ *
+ * @param {Object} root0
+ * @param {string} root0.dir       Site working directory.
+ * @param {string} root0.patchText The stored patch, forward as it was applied.
+ * @return {{absorbed: Array<{path: string, editedOver: boolean, failed: number, total: number}>, missing: boolean, error?: string}}
+ */
+function diagnoseRemoval({ dir, patchText }) {
+	const parsed = parsePatchFiles(String(patchText || ''));
+	if (!parsed.ok) return { absorbed: [], missing: false, error: parsed.error };
+
+	const absorbed = [];
+	let intact = 0;
+	for (const file of parsed.files) {
+		// Binary files were never applied, so they cannot be in the way of a
+		// removal either — the apply path skips them the same way.
+		if (file.kind === 'binary') continue;
+		const reversed = reverseFile(file);
+		const resolved = resolveFile(dir, reversed);
+		if (!resolved.error) {
+			intact += 1;
+			continue;
+		}
+		const conflict = resolved.conflict;
+		absorbed.push({
+			// The forward path, not the reversed one: a rename's reverse names the
+			// file it moves *back* to, while the record of what was applied holds
+			// the name the patch moved it to. Keyed on the reversed name, the
+			// caller's attribution could never match its own record.
+			path: file.path,
+			// Not every refusal is an edit over the patch's lines: a file the
+			// contributor deleted outright, or one a reversed add can no longer
+			// find, blocks the removal without anything having been written over.
+			// Withholding Revert is right either way — the revert is all or
+			// nothing — but the sentence about it is not, so the two are told
+			// apart here rather than guessed at by the caller.
+			editedOver: Boolean(conflict),
+			failed: conflict ? conflict.regions.length : 0,
+			total: conflict ? conflict.total : 0
+		});
+	}
+
+	if (absorbed.length && !intact && patchIsAbsent(dir, parsed.files)) {
+		return { absorbed: [], missing: true };
+	}
+	return { absorbed, missing: false };
+}
+
 /**
  * Puts back everything a failed run had already written.
  *
@@ -450,10 +518,12 @@ async function applyPatchToDir({ dir, patchText, reverse = false, onLog = () =>
 			skipped.push(file.path);
 			continue;
 		}
-		// No diagnosis on a reverse: the panel discards it — the patch being
-		// reverted came through this app, and the ticket's other patches are no
-		// answer to a revert that failed.
-		const resolved = resolveFile(dir, file, { diagnose: !reverse });
+		// Diagnosed on a reverse too (#306). The ticket's other patches are still
+		// no answer to a revert that failed, but the *reason* it failed is: a
+		// revert only fails because the contributor's own edits are on the
+		// patch's lines, and naming how many of them, and where, is the whole
+		// difference between an explanation and a generic error.
+		const resolved = resolveFile(dir, file);
 		if (resolved.error) {
 			failures.push(resolved.error);
 			if (resolved.conflict) conflicts.push(resolved.conflict);
@@ -536,4 +606,4 @@ async function applyPatchToDir({ dir, patchText, reverse = false, onLog = () =>
 	return { ok: true, applied, skipped };
 }
 
-module.exports = { applyPatchToDir, resolveInside, reverseFile, dominantEol, rollback, diagnoseHunks };
+module.exports = { applyPatchToDir, diagnoseRemoval, resolveInside, reverseFile, dominantEol, rollback, diagnoseHunks };

src/patch-provenance.cjs

@@ -124,15 +124,16 @@ function ticketNumber(ticketId) {
  * recorded.
  *
  * @param {Object}        details
- * @param {string}        [details.handle]      WordPress.org handle, already validated.
- * @param {string}        [details.event]       Where it was written — a WordCamp, a meetup.
+ * @param {string}        [details.handle]          WordPress.org handle, already validated.
+ * @param {string}        [details.event]           Where it was written — a WordCamp, a meetup.
  * @param {number|string} [details.ticketId]
  * @param {string}        [details.trunkOid]
- * @param {string}        [details.trunkDate]   ISO timestamp of the base commit.
- * @param {string}        [details.generatedAt] ISO timestamp for "now".
+ * @param {string}        [details.trunkDate]       ISO timestamp of the base commit.
+ * @param {boolean}       [details.baseApproximate] The branch point was not recorded (#308).
+ * @param {string}        [details.generatedAt]     ISO timestamp for "now".
  * @return {string}
  */
-function buildProvenanceHeader({ handle, event, ticketId, trunkOid, trunkDate, generatedAt } = {}) {
+function buildProvenanceHeader({ handle, event, ticketId, trunkOid, trunkDate, baseApproximate, generatedAt } = {}) {
 	const lines = [];
 
 	const contributor = field(handle);
@@ -152,7 +153,13 @@ function buildProvenanceHeader({ handle, event, ticketId, trunkOid, trunkDate, g
 	const based = day(trunkDate);
 	if (oid || based) {
 		const base = [oid ? oid.slice(0, SHORT_OID_LENGTH) : null, based].filter(Boolean).join(', ');
-		lines.push(`# Base: trunk @ ${base}`);
+		// A base the app worked out rather than recorded is still worth naming —
+		// a mentor with no commit at all has nothing to apply this against — but
+		// it is named as the guess it is (#308). Read as fact, it would send
+		// someone rebasing onto a commit this ticket may never have been on.
+		lines.push(baseApproximate
+			? `# Base: trunk @ ${base} (approximate — this ticket's starting point was not recorded)`
+			: `# Base: trunk @ ${base}`);
 	}
 
 	const generated = day(generatedAt);

src/preload.js

@@ -98,6 +98,15 @@ contextBridge.exposeInMainWorld('api', {
 		ipcRenderer.on('ticket:carried-work', h);
 		return () => ipcRenderer.removeListener('ticket:carried-work', h);
 	}
+,
+	// Where trunk is on the remote (#307), pushed when the probe lands rather
+	// than waited for: site:status answers immediately with whatever was last
+	// known, and this arrives a moment later if it changed anything.
+	subscribeRemoteTrunk: (handler) => {
+		const h = (_e, payload) => handler && handler(payload);
+		ipcRenderer.on('trunk:remote', h);
+		return () => ipcRenderer.removeListener('trunk:remote', h);
+	}
 ,
 	subscribeSetupProgress: (handler) => {
 		const h = (_e, payload) => handler && handler(payload);

src/renderer/applied-layer.cjs

@@ -0,0 +1,234 @@
+// What the app says about the one patch a ticket has applied (#306).
+//
+// A ticket is a branch (#108): trunk, plus at most one applied patch or pull
+// request, plus the contributor's own edits. The branch holds that faithfully.
+// What the app *said* about it did not — the applied patch was remembered as an
+// undo blob, so every file it brought was announced as the contributor's own
+// writing, and "can this be reverted" meant no more than "we kept the text".
+//
+// Two answers live here, both pure:
+//
+//   - `attributeConflicts` — whose changes are the ones a new patch would land
+//     on. A file only the applied layer touched is named as the layer's; a file
+//     the contributor has also edited over keeps naming their work, because that
+//     is the one that decides what they do next.
+//   - `describeAppliedLayer` — the banner's two faces. While the layer still
+//     comes out cleanly, Revert is offered. Once the contributor's edits sit on
+//     its lines it is **absorbed**: it has become their changes, and the honest
+//     exits are saving a copy and discarding the ticket to its base.
+//
+// Absorption is measured, never tracked — the main process re-answers it on
+// every status read, so undoing the overlapping edit brings Revert back on its
+// own. And it never frees the one-patch slot: the record survives as provenance.
+//
+// Pure and dependency-free like update-plan.cjs and apply-conflict.cjs, for the
+// same reason: the renderer bundle imports it, `node --test` requires it
+// directly, and neither needs a DOM.
+'use strict';
+
+const { baseIsApproximate, UNRECORDED_CLEAR_NOTE, UNRECORDED_MEASUREMENT_NOTE } = require('./ticket-base.cjs');
+
+// Saving a copy and then discarding is a recommendable way forward on this
+// project, not a defeat: a ticket's changes are one afternoon's work on a
+// checkout that gets thrown away, and redoing them is cheaper than untangling
+// them. Said once, here, so both faces that offer it say it the same way.
+const DISPOSABLE_EXIT = 'Save a copy of your work first and the ticket is safe to discard back to its base — on this project that is a normal way forward, not a lost afternoon.';
+
+// The slot does not open when a patch is absorbed. Saying so is what stops the
+// contributor reading "it is part of your changes now" as "so I can apply
+// another one".
+const SLOT_HELD = 'It still counts as this ticket\'s one applied patch, so another cannot be applied until this ticket is reverted or discarded.';
+
+/**
+ * `a`, `a and b`, `a, b and c` — a list a person reads rather than a join.
+ *
+ * @param {string[]} items
+ * @return {string}
+ */
+function listOf(items) {
+	if (items.length <= 1) return items[0] || '';
+	return `${items.slice(0, -1).join(', ')} and ${items[items.length - 1]}`;
+}
+
+/**
+ * The paths of an applied layer whose lines the contributor has edited over.
+ *
+ * @param {?Object} appliedPatch
+ * @return {string[]}
+ */
+function absorbedPaths(appliedPatch) {
+	const list = appliedPatch && Array.isArray(appliedPatch.absorbed) ? appliedPatch.absorbed : [];
+	return list.map((entry) => (typeof entry === 'string' ? entry : entry && entry.path)).filter(Boolean);
+}
+
+/**
+ * Who owns each file a patch about to be applied would land on.
+ *
+ * The pre-apply warning exists to say "your work is here, and this could fail
+ * without touching it". Counting the applied layer's files as the contributor's
+ * own writing is the kind of wrong that teaches people to ignore the warning —
+ * so both are named, separately, and neither is dropped.
+ *
+ * The measurement behind `absorbed` is per-region: a file is the layer's when
+ * the layer's own lines are untouched. A contributor edit *elsewhere* in the
+ * same file therefore reads as the layer's rather than as theirs. The file is
+ * still named and the "fails without touching anything" caveat still covers it,
+ * so the warning does not go quiet — it is the attribution that is coarse, and
+ * only in that direction.
+ *
+ * @param {Object}   root0
+ * @param {string[]} [root0.conflicts]    Paths from the preview's plan.
+ * @param {?Object}  [root0.appliedPatch] The status record, or null.
+ * @return {{yours: string[], fromLayer: string[], sentences: string[]}}
+ */
+function attributeConflicts({ conflicts = [], appliedPatch = null } = {}) {
+	const paths = Array.isArray(conflicts) ? conflicts.filter(Boolean) : [];
+	const layerFiles = new Set(appliedPatch && Array.isArray(appliedPatch.files) ? appliedPatch.files : []);
+	const editedOver = new Set(absorbedPaths(appliedPatch));
+
+	const fromLayer = appliedPatch ? paths.filter((p) => layerFiles.has(p) && !editedOver.has(p)) : [];
+	const claimed = new Set(fromLayer);
+	const yours = paths.filter((p) => !claimed.has(p));
+
+	const sentences = [];
+	if (yours.length) {
+		sentences.push(`You have your own edits to ${listOf(yours)}. Save a patch of your work first if you want a copy.`);
+	}
+	if (fromLayer.length) {
+		const label = appliedPatch.label || 'the patch you applied';
+		sentences.push(`${listOf(fromLayer)} ${fromLayer.length === 1 ? 'was' : 'were'} changed by ${label}, which you applied — not by you.`);
+	}
+	if (sentences.length) {
+		sentences.push('The patch is applied on top of those changes: it succeeds if they do not overlap, and fails without touching anything if they do.');
+	}
+	return { yours, fromLayer, sentences };
+}
+
+/**
+ * The applied-layer banner, in whichever face the checkout has earned.
+ *
+ * Three, not two, because "cannot be reverted" has two different causes and
+ * they need different sentences: a patch too large to keep a copy of was never
+ * revertable, and a patch whose lines have been edited over stopped being.
+ *
+ * `when` is passed in already formatted — the locale-dependent part is the
+ * component's, and keeping it out of here is what lets this be asserted on.
+ *
+ * @param {?Object} appliedPatch   The `site:status` record, or null.
+ * @param {Object}  [options]
+ * @param {string}  [options.when] Formatted apply time, or '' when unknown.
+ * @return {?Object}
+ */
+function describeAppliedLayer(appliedPatch, { when = '' } = {}) {
+	if (!appliedPatch) return null;
+
+	const label = appliedPatch.label || 'A patch';
+	const files = Array.isArray(appliedPatch.files) ? appliedPatch.files : [];
+	const absorbed = absorbedPaths(appliedPatch);
+	const kept = appliedPatch.kept === undefined ? Boolean(appliedPatch.revertable) : Boolean(appliedPatch.kept);
+
+	const summary = `is applied — ${files.length} file${files.length === 1 ? '' : 's'}${when ? `, ${when}` : ''}.`;
+
+	if (kept && !absorbed.length) {
+		return { label, summary, canRevert: true, absorbed: false, explanation: '', detail: [], note: '', offerCopy: false };
+	}
+
+	// Too large to have kept a copy of. Nothing about the tree changes this one,
+	// so it says so plainly and goes straight to the exit that always works.
+	if (!kept) {
+		return {
+			label,
+			summary,
+			canRevert: false,
+			absorbed: false,
+			explanation: `${label} was too large to keep a copy of for an undo, so it cannot be lifted back out on its own.`,
+			detail: [],
+			note: `${DISPOSABLE_EXIT} ${SLOT_HELD}`,
+			offerCopy: true
+		};
+	}
+
+	// Absorbed: the contributor's edits are on the patch's own lines, so there
+	// is no longer a patch and an edit — there is one body of changes. Said as a
+	// state of the work rather than as a failure, and with the way back named,
+	// because undoing the overlapping edit really does bring Revert back.
+	//
+	// Not every file in the way was written over, though. One the contributor
+	// deleted outright blocks the removal just as firmly, and calling that "your
+	// edits are on its lines" would send them looking at lines that are not
+	// there — so the two get their own sentence, and either alone is enough for
+	// the revert to be off, because a revert is all or nothing.
+	const entries = (appliedPatch.absorbed || []).filter((entry) => entry && entry.path);
+	const written = entries.filter((entry) => entry.editedOver !== false).map((entry) => entry.path);
+	const gone = entries.filter((entry) => entry.editedOver === false).map((entry) => entry.path);
+
+	const reasons = [];
+	if (written.length) reasons.push(`your own edits are on the lines it brought to ${listOf(written)}`);
+	if (gone.length) reasons.push(`${listOf(gone)} ${gone.length === 1 ? 'is' : 'are'} no longer where it left ${gone.length === 1 ? 'it' : 'them'}`);
+
+	return {
+		label,
+		summary,
+		canRevert: false,
+		absorbed: true,
+		explanation: `${label} is part of your changes now and cannot be lifted back out on its own: ${listOf(reasons)}. Undo those edits and Revert comes back on its own.`,
+		detail: entries
+			.filter((entry) => entry.total)
+			.map((entry) => `${entry.path} — you have edited ${entry.failed} of the ${entry.total} change${entry.total === 1 ? '' : 's'} it brought`),
+		note: `${DISPOSABLE_EXIT} ${SLOT_HELD}`,
+		offerCopy: true
+	};
+}
+
+/**
+ * Whichever of the two absorbed exits failed, said where they were offered.
+ *
+ * Both report through state that belongs to somewhere else on screen — the
+ * changes note and the patch modal — and the absorbed banner is neither. A
+ * refusal that lands there is a button that did nothing, on the one way out
+ * this banner recommends, so it is repeated here rather than left behind.
+ *
+ * The save goes first: it is the step that makes discarding safe, and its
+ * failure is the one that must not be missed.
+ *
+ * @param {Object} root0
+ * @param {string} [root0.patchSaveError]
+ * @param {string} [root0.discardError]
+ * @return {{message: string}}
+ */
+function absorbedExitFailure({ patchSaveError = '', discardError = '' } = {}) {
+	if (patchSaveError) return { message: `The copy could not be saved: ${patchSaveError}` };
+	if (discardError) return { message: discardError };
+	return { message: '' };
+}
+
+/**
+ * The whole block above Apply: whose work a new patch would land on (#306), and
+ * how sure the app is of the base that was measured from (#308).
+ *
+ * The two were separate blocks that replaced each other, which is a choice
+ * neither of them should win: the attribution owns the file list, and the base
+ * is what says how much that list can be trusted. On an unrecorded base it can
+ * name files the contributor never touched and miss ones they did, so the
+ * sentences carry the hedge rather than standing unqualified.
+ *
+ * With nothing colliding the hedge is all that is left, and it is said quietly:
+ * "nothing collided" is not a promise an approximate base can make, but it is
+ * not an alert either.
+ *
+ * @param {Object}   [root0]
+ * @param {string[]} [root0.conflicts]    Files the incoming patch touches that the ticket has work in.
+ * @param {?Object}  [root0.appliedPatch] The layer record, when one is applied.
+ * @param {?string}  [root0.baseStatus]   A `BASE_STATUS` value from the preview.
+ * @return {?{level: 'warning'|'note', sentences: string[]}}
+ */
+function describePreviewNotice({ conflicts = [], appliedPatch = null, baseStatus = null } = {}) {
+	const { sentences } = attributeConflicts({ conflicts, appliedPatch });
+	const approximate = baseIsApproximate(baseStatus);
+	if (sentences.length) {
+		return { level: 'warning', sentences: approximate ? sentences.concat(UNRECORDED_MEASUREMENT_NOTE) : sentences };
+	}
+	return approximate ? { level: 'note', sentences: [UNRECORDED_CLEAR_NOTE] } : null;
+}
+
+module.exports = { attributeConflicts, describeAppliedLayer, describePreviewNotice, absorbedExitFailure, listOf, DISPOSABLE_EXIT, SLOT_HELD };