diff --git a/src/main.js b/src/main.js index 2cb85ce..864c493 100644 --- a/src/main.js +++ b/src/main.js @@ -29,8 +29,10 @@ const { buildMenuTemplate } = require('./menu'); const { killChildTree } = require('./kill-tree'); const { normalizeEol } = require('./git-update.cjs'); const { ensureAutocrlf, readTrunkInfo, collectDirtyFiles, discardChanges, discardToBase, updateToLatestTrunk } = require('./trunk-update'); -const { applyPatchToDir } = require('./patch-apply'); +const { remoteProbeDue, readRemoteTrunkOid } = require('./trunk-remote'); +const { applyPatchToDir, diagnoseRemoval } = require('./patch-apply'); const { parsePatchFiles, planApply } = require('./patch-plan.cjs'); +const { BASE_STATUS, baseIsApproximate, baseUnreadableMessage } = require('./renderer/ticket-base.cjs'); const { fetchLinkedPrs, fetchPrDiff } = require('./github-prs'); const { getClientId: getGithubClientId, requestDeviceCode, pollForToken, fetchViewer } = require('./github-auth.cjs'); const { openPullRequest, buildPullRequestBody, testMode: githubTestMode } = require('./github-pr.cjs'); @@ -65,6 +67,11 @@ const SWITCH_PROGRESS_CHANNEL = 'switch:progress'; // step of an operation, and describing it as progress would have the panel say // "Saving your work…" about trunk — which is the one thing this refuses to do. const CARRIED_WORK_CHANNEL = 'ticket:carried-work'; +// The probe's answer, pushed when it lands (#307). site:status is not polled — +// it is read on mount and after the long operations — so without this the oid +// would sit in the site record until the next launch and the signal would +// always be a session late. +const REMOTE_TRUNK_CHANNEL = 'trunk:remote'; const { parseTicketRef } = require('./renderer/trac-ticket.cjs'); const { parseHandle } = require('./wporg-handle.cjs'); const { parseEventName, buildProvenanceHeader, handoffFilename } = require('./patch-provenance.cjs'); @@ -579,9 +586,17 @@ async function collectPullRequestFiles(dir, baseOid = null) { return { baseOid: base, files: entries }; } +// A base the app could not read is a patch it cannot take (#308). Every +// generator below refuses instead of falling back, because the fallback is +// HEAD — and HEAD on a resumed ticket is the contributor's own parked work, so +// the "patch" would come back empty and look like there was nothing to submit. +const NO_BASE_PATCH_ERROR = baseUnreadableMessage('no patch was created'); + ipcMain.handle('git:get-patch', async (_e, sitePath) => { try { - const patch = await createMinimalPatchForDir(sitePath, await patchBaseOid(sitePath)); + const base = await patchBase(sitePath); + if (base.status === BASE_STATUS.UNREADABLE) return { ok: false, error: NO_BASE_PATCH_ERROR }; + const patch = await createMinimalPatchForDir(sitePath, base.baseOid); return { ok: true, patch }; } catch (e) { return { ok: false, error: String(e) }; @@ -589,14 +604,20 @@ ipcMain.handle('git:get-patch', async (_e, sitePath) => { }); ipcMain.handle('git:create-patch', async (_e, sitePath) => { - try { - const patch = await createMinimalPatchForDir(sitePath, await patchBaseOid(sitePath)); + const showPatchWindow = (text) => { const win = new BrowserWindow({ width: 900, height: 700, webPreferences: { contextIsolation: true, nodeIntegration: false } }); - win.loadURL('data:text/html;charset=utf-8,' + encodeURIComponent(buildPatchHtml(patch))); + win.loadURL('data:text/html;charset=utf-8,' + encodeURIComponent(buildPatchHtml(text))); + }; + try { + const base = await patchBase(sitePath); + if (base.status === BASE_STATUS.UNREADABLE) { + showPatchWindow(NO_BASE_PATCH_ERROR); + return { ok: false, error: NO_BASE_PATCH_ERROR }; + } + showPatchWindow(await createMinimalPatchForDir(sitePath, base.baseOid)); return { ok: true }; } catch (e) { - const win = new BrowserWindow({ width: 900, height: 700, webPreferences: { contextIsolation: true, nodeIntegration: false } }); - win.loadURL('data:text/html;charset=utf-8,' + encodeURIComponent(buildPatchHtml('Failed to generate diff: ' + String(e)))); + showPatchWindow('Failed to generate diff: ' + String(e)); return { ok: false, error: String(e) }; } }); @@ -612,8 +633,9 @@ ipcMain.handle('git:create-patch', async (_e, sitePath) => { ipcMain.handle('git:save-patch', async (_e, sitePath, options) => { try { const handoff = Boolean(options && options.handoff); - const baseOid = await patchBaseOid(sitePath); - const patch = await createMinimalPatchForDir(sitePath, baseOid); + const base = await patchBase(sitePath); + if (base.status === BASE_STATUS.UNREADABLE) return { ok: false, error: NO_BASE_PATCH_ERROR }; + const patch = await createMinimalPatchForDir(sitePath, base.baseOid); // The header describes what was diffed, so it is read from the same // recorded state the status handler reports, not asked of the caller: @@ -635,7 +657,7 @@ ipcMain.handle('git:save-patch', async (_e, sitePath, options) => { // forward since (#108). Reading the date off that commit rather // than the site record keeps the two halves of the line // describing the same commit. - ...(await baseProvenance(sitePath, baseOid, meta)), + ...(await baseProvenance(sitePath, base, meta)), generatedAt: new Date().toISOString() }); name = handoffFilename({ handle, ticketId: meta.tracTicket }); @@ -779,7 +801,14 @@ ipcMain.handle('github:open-pr', async (event, sitePath, options = {}) => { let collected; try { - collected = await collectPullRequestFiles(sitePath, await patchBaseOid(sitePath)); + // The base is the pull request's parent commit, so an unreadable one + // cannot be papered over with HEAD: that would open a pull request + // against the contributor's own parked work (#308). + const base = await patchBase(sitePath); + if (base.status === BASE_STATUS.UNREADABLE) { + return { ok: false, reason: 'error', error: baseUnreadableMessage('no pull request was opened'), stage: 'collect' }; + } + collected = await collectPullRequestFiles(sitePath, base.baseOid); } catch (e) { return { ok: false, reason: 'error', error: String(e), stage: 'collect' }; } @@ -823,6 +852,61 @@ async function mergeSiteMeta(sitePath, patch) { s.set('siteMeta', meta); } +/** + * Refresh this site's record of where trunk is on the remote (#307). + * + * Started, never awaited: `site:status` is what a site being opened waits on, + * so a network round trip inside it would make opening a site depend on the + * network — the one thing this must not cost. The answer is written to the site + * record *and* pushed on REMOTE_TRUNK_CHANNEL, because `site:status` is not on + * a timer: it is read on mount and after the long operations, so a stored-only + * answer would first be read on the next launch. + * + * The attempt is stamped *before* it runs, which does two jobs at once: a + * `site:status` a moment later (mount, then an install finishing) sees a fresh + * stamp and does not launch a second probe, and a failure backs off for the + * whole interval instead of retrying on every call. Offline is not an error + * condition to retry out of — it is a normal state for this app's users, and + * the calendar fallback already covers it. + * + * The failure is logged to the app log and nowhere else, deliberately. The "no + * silent catch" rule exists so a contributor is never left with a button that + * did nothing; nobody pressed anything here, and streaming "could not reach + * github.com" into the terminal of an offline contributor every hour would be + * noise about a thing that is working as designed. + * + * @param {string} sitePath + * @param {?Object} sender The renderer to push the answer to, when it lands. + * @return {Promise} + */ +async function refreshRemoteTrunk(sitePath, sender) { + // The probe holds this path across a network wait the site can be deleted + // during, and mergeSiteMeta creates the key it writes to. Without this the + // answer would resurrect the record `sites:delete` just removed, leaving a + // phantom entry for a directory that is gone — and a site later created at + // the same path would adopt its oid and skip its first probe. + const stillRegistered = async () => { + const store = await getStore(); + return (store.get('sites') || []).includes(sitePath); + }; + try { + if (!await stillRegistered()) return; + await mergeSiteMeta(sitePath, { remoteTrunkCheckedAt: new Date().toISOString() }); + // `null` is stored, not skipped: an answer of "this remote has no trunk" + // has to be able to clear an oid an earlier probe stored, or a site + // would compare against a commit nothing will ever match and report + // itself behind for good. + const remoteTrunkOid = await readRemoteTrunkOid({ url: WORDPRESS_GIT_URL }); + if (!await stillRegistered()) return; + await mergeSiteMeta(sitePath, { remoteTrunkOid }); + try { + if (sender && !sender.isDestroyed()) sender.send(REMOTE_TRUNK_CHANNEL, { sitePath, remoteTrunkOid }); + } catch {} + } catch (e) { + logError('trunk-remote', `could not read trunk from the remote: ${String(e && e.message ? e.message : e)}`); + } +} + // --- Ticket branches (#108) --- git mechanics live in src/ticket-branches.js; // what follows is the electron-store half: which branch is active and what // context each one carries. @@ -866,9 +950,10 @@ async function migrateSiteToBranches(sitePath) { const existing = await listTicketBranches(sitePath); // A branch that already exists was not created by this app, so its fork // point is not on record and cannot be recovered on a depth-1 clone. - // Left null deliberately: patchBaseOid has one documented fallback for - // exactly this, and guessing here would put a second, wrong one in the - // codebase. (`trunkOid` is the *current* tip, not the fork point.) + // Left null deliberately: `patchBase` answers `unrecorded` for exactly + // this and hedges every surface that shows it, where guessing here + // would put a second, silent guess in the codebase. (`trunkOid` is the + // *current* tip, not the fork point.) const baseOid = existing.includes(ref) ? null : (await startTicketBranch(sitePath, m.tracTicket)).baseOid; @@ -1059,30 +1144,46 @@ async function mergeBranchMeta(sitePath, ref, patch) { } /** - * The diff base for whatever is checked out: a ticket branch's recorded branch - * point, or null on trunk — where the patch generator falls back to HEAD, which - * is what it has always diffed against. + * The diff base for whatever is checked out, and how well the app knows it. + * + * Four answers, not two (#308): `trunk` has no branch point and the callers + * fall back to HEAD as they always have; `recorded` is the branch point the app + * wrote down when it created the branch; `unrecorded` is a ticket branch with + * none, measured against today's trunk because there is nothing better and + * qualified as approximate wherever it is shown; `unreadable` is a read that + * failed, which is emphatically not "this site is on trunk" — resolving it to + * that sends every measurement back to HEAD through the failure path. * * @param {string} sitePath + * @return {Promise<{status: string, baseOid: ?string}>} */ -async function patchBaseOid(sitePath) { +async function patchBase(sitePath) { try { // What is checked out decides this, so ask the worktree before the // registry: a site on trunk — every site before #108, and every site // whose owner never linked a ticket — answers without a store read at // all, and takes exactly the path it always took. - let ref = null; - try { ref = await currentBranchName(sitePath); } catch {} - if (!ref || ref === TRUNK) return null; + // + // A *null* branch (a detached HEAD, an empty repository) is trunk's + // answer and always was. A *throw* is not: that is the read failing, + // and it falls through to the catch below. + const ref = await currentBranchName(sitePath); + if (!ref || ref === TRUNK) return { status: BASE_STATUS.TRUNK, baseOid: null }; const recorded = ((await readSiteMeta(sitePath)).branches || {})[ref]; - if (recorded && recorded.baseOid) return recorded.baseOid; - // A ticket branch with no recorded base (registry edited by hand, or a - // branch the user made themselves). The live trunk ref is the closest - // honest answer available. - return await git.resolveRef({ fs, dir: sitePath, ref: 'refs/heads/trunk' }); + if (recorded && recorded.baseOid) return { status: BASE_STATUS.RECORDED, baseOid: recorded.baseOid }; + // A ticket branch with no recorded base (registry edited by hand, a + // site adopted from disk, a branch the contributor made themselves). + // The live trunk ref is the closest thing available — but it is where + // trunk is *now*, which an update has very likely moved past the point + // the branch was born at, so the status says so and every surface that + // shows this answer hedges it. + return { + status: BASE_STATUS.UNRECORDED, + baseOid: await git.resolveRef({ fs, dir: sitePath, ref: 'refs/heads/trunk' }) + }; } catch { - return null; + return { status: BASE_STATUS.UNREADABLE, baseOid: null }; } } @@ -1097,20 +1198,27 @@ async function patchBaseOid(sitePath) { * dropped rather than guessed: a header with no date is honest, one that dates a * commit it is not describing is not. * - * @param {string} dir Site working directory. - * @param {?string} baseOid Base the patch was diffed against, or null on trunk. - * @param {Object} meta The site's stored metadata. - * @return {Promise<{trunkOid: ?string, trunkDate: ?string}>} Header fields. + * A branch with no recorded base is the same argument one step further (#308): + * today's trunk is what the patch was diffed against, so it is what the header + * names, but it is flagged approximate — a mentor reading "Base: trunk @ ..." + * as fact would rebase against a commit this ticket may never have seen. + * + * @param {string} dir Site working directory. + * @param {Object} base A `patchBase` result. + * @param {Object} meta The site's stored metadata. + * @return {Promise<{trunkOid: ?string, trunkDate: ?string, baseApproximate: boolean}>} Header fields. */ -async function baseProvenance(dir, baseOid, meta) { +async function baseProvenance(dir, base, meta) { + const baseApproximate = baseIsApproximate(base.status); + const { baseOid } = base; if (!baseOid || baseOid === meta.trunkOid) { - return { trunkOid: meta.trunkOid, trunkDate: meta.trunkDate }; + return { trunkOid: meta.trunkOid, trunkDate: meta.trunkDate, baseApproximate }; } try { const { commit } = await git.readCommit({ fs, dir, oid: baseOid }); - return { trunkOid: baseOid, trunkDate: new Date(commit.committer.timestamp * 1000).toISOString() }; + return { trunkOid: baseOid, trunkDate: new Date(commit.committer.timestamp * 1000).toISOString(), baseApproximate }; } catch { - return { trunkOid: baseOid, trunkDate: null }; + return { trunkOid: baseOid, trunkDate: null, baseApproximate }; } } @@ -1118,25 +1226,38 @@ async function baseProvenance(dir, baseOid, meta) { * The files standing between where this ticket started and where it is now — * the note's measurement (#239). Same base and same walk as the patch, filtered * to the rows the patch would actually speak about, so the note and the modal - * are two renderings of one answer. On trunk `patchBaseOid` answers null and - * the walk falls back to HEAD, which is what the note has always read there. + * are two renderings of one answer. On trunk `patchBase` answers no oid and the + * walk falls back to HEAD, which is what the note has always read there. + * + * Throws when the base could not be read (#308) rather than measuring against + * HEAD: on a resumed ticket HEAD is the contributor's own parked work, so the + * fallback answers "nothing here" about the very tree this exists to speak for. + * The status rides back with the files so the surfaces that show this answer + * can say how exact it is. * * @param {string} sitePath - * @return {Promise>} Paths, gitignored ones excluded. + * @return {Promise<{baseStatus: string, files: Array}>} Paths, gitignored ones excluded. */ async function collectUnsubmittedFiles(sitePath) { - const baseOid = await patchBaseOid(sitePath); - const { files } = await collectChangedFiles(sitePath, baseOid); - return files - .filter((file) => classifyChangedFile(file).kind !== 'unchanged') - .map((file) => file.path); + const base = await patchBase(sitePath); + if (base.status === BASE_STATUS.UNREADABLE) { + throw new Error(baseUnreadableMessage('your work could not be measured against it')); + } + const { files } = await collectChangedFiles(sitePath, base.baseOid); + return { + baseStatus: base.status, + files: files + .filter((file) => classifyChangedFile(file).kind !== 'unchanged') + .map((file) => file.path) + }; } // Two questions, deliberately two channels (#239). This one asks "are there // edits not written down yet" — the narrow reading the checkout guards need: -// the trunk-update dirty dialog and the patch-apply collision scan protect -// exactly the files a force checkout would overwrite, and parked work is not -// among them. The card's note asks `git:unsubmitted-work` instead. +// the trunk-update dirty dialog protects exactly the files a force checkout +// would overwrite, and parked work is not among them. Everything that asks +// what this ticket has done — the card's note, and the patch-apply collision +// scan (#301) — asks `git:unsubmitted-work`'s question instead. ipcMain.handle('git:worktree-dirty', async (_e, sitePath) => { try { const files = await collectDirtyFiles(sitePath); @@ -1154,7 +1275,10 @@ ipcMain.handle('git:worktree-dirty', async (_e, sitePath) => { // switch — which is exactly the work the note exists to speak about. ipcMain.handle('git:unsubmitted-work', async (_e, sitePath) => { try { - const files = await collectUnsubmittedFiles(sitePath); + // The reply keeps its shape: the card's note counts files and does not + // yet say how exact the measurement is — qualifying it there is a + // follow-up, not something to smuggle in on this channel. + const { files } = await collectUnsubmittedFiles(sitePath); return { ok: true, dirty: files.length > 0, changedCount: files.length, files }; } catch (e) { return { ok: false, error: String(e) }; @@ -1163,22 +1287,34 @@ ipcMain.handle('git:unsubmitted-work', async (_e, sitePath) => { // "Discard all changes" in the review-and-submit modal: throws away everything // the modal shows, which on a ticket branch is measured from the branch point -// (#108/#239) and so includes the parked WIP commit. Rewinds to `patchBaseOid` +// (#108/#239) and so includes the parked WIP commit. Rewinds to `patchBase` // — the same base the diff was taken against — so the modal ends on "No // changes". The branch survives and the ticket stays linked; only its work is -// gone. On trunk (or a branch with no recorded base) there is nothing past -// HEAD to rewind, so it falls back to the uncommitted-only reset. +// gone. On trunk there is nothing past HEAD to rewind, so it falls back to the +// uncommitted-only reset. +// +// A base the app could not read refuses outright (#308). The uncommitted-only +// reset is not a safe stand-in for it: it looks like a discard, leaves the +// parked WIP commit untouched, and the modal would come back still listing the +// work it just promised to throw away. ipcMain.handle('git:discard-to-base', async (_e, sitePath) => { try { - const baseOid = await patchBaseOid(sitePath); - if (baseOid) { - await discardToBase(sitePath, baseOid); + const base = await patchBase(sitePath); + if (base.status === BASE_STATUS.UNREADABLE) { + return { ok: false, error: baseUnreadableMessage('nothing was discarded') }; + } + if (base.baseOid) { + // On an unrecorded base this is today's trunk, which need not be an + // ancestor of the branch — `discardToBase` refuses to rewind onto a + // base that is not one and clears the uncommitted work only, so the + // guess cannot take a commit with it. + await discardToBase(sitePath, base.baseOid); } else { await discardChanges(sitePath); } await writeWorkMeta(sitePath, { appliedPatch: null }); let files = null; - try { files = await collectUnsubmittedFiles(sitePath); } catch {} + try { ({ files } = await collectUnsubmittedFiles(sitePath)); } catch {} return files ? { ok: true, dirty: files.length > 0, changedCount: files.length } : { ok: true }; @@ -1203,7 +1339,7 @@ ipcMain.handle('git:discard-changes', async (_e, sitePath) => { // that fails does not turn a discard that succeeded into an error, the // reply just says less and the next probe fills it in. let files = null; - try { files = await collectUnsubmittedFiles(sitePath); } catch {} + try { ({ files } = await collectUnsubmittedFiles(sitePath)); } catch {} return files ? { ok: true, dirty: files.length > 0, changedCount: files.length } : { ok: true }; @@ -1266,7 +1402,13 @@ ipcMain.handle('git:update-trunk', async (event, sitePath) => { // the dirty-tree modal always discards and then updates.) await mergeSiteMeta(sitePath, { trunkOid: result.upToDate ? result.oldOid : result.newOid, - trunkDate: result.trunkDate + trunkDate: result.trunkDate, + // The fetch just asked the remote where trunk is, which is the + // same question the background probe asks (#307) — recording it + // here means the staleness signal clears the moment the update + // finishes, instead of an hour later when the probe next runs. + remoteTrunkOid: result.newOid, + remoteTrunkCheckedAt: new Date().toISOString() }); // HEAD has moved but install/build have not run yet: persist the // incomplete flag now so the state survives a crash or quit @@ -1433,21 +1575,34 @@ const REVERTABLE_PATCH_LIMIT = 512 * 1024; // Reading a patch without touching the checkout, so the contributor sees which // files it would change — and which of their own edits it collides with — // before deciding. +// +// Measured from the ticket's base, not from HEAD (#301). Under the +// ticket-as-branch model a ticket's work lives in its parked WIP commit, so a +// ticket that has been left and resumed has a worktree matching HEAD exactly: +// HEAD-relative, the warning goes silent on precisely the tree it exists to +// protect, and speaks up about files edited back to what the base holds. This +// is the same measurement the patch itself is taken with, so what the warning +// calls "your own edits" is what the patch modal would show. ipcMain.handle('git:preview-patch', async (_e, sitePath, patchText) => { try { const parsed = parsePatchFiles(patchText); if (!parsed.ok) return { ok: false, error: parsed.error }; let dirtyPaths; + let baseStatus; try { - dirtyPaths = await collectDirtyFiles(sitePath); + ({ files: dirtyPaths, baseStatus } = await collectUnsubmittedFiles(sitePath)); } catch (e) { // Failing open here would promise "no collisions" precisely when the - // app could not look — surface the failure instead. + // app could not look — surface the failure instead. An unreadable + // base arrives here as a throw for exactly that reason (#308). logError('git:preview-patch', String(e && e.stack ? e.stack : e)); - return { ok: false, error: 'Could not check your working tree for conflicts, so the preview was not shown.' }; + return { ok: false, error: 'Could not check your work for conflicts, so the preview was not shown.' }; } const plan = planApply({ files: parsed.files, dirtyPaths }); - return { ok: true, ...plan, files: parsed.files.map((f) => ({ kind: f.kind, path: f.path })) }; + // How exact that collision list is rides back with it: on a branch with + // no recorded base it was measured against today's trunk, and the panel + // says so rather than presenting it in the same voice as an exact one. + return { ok: true, ...plan, baseStatus, files: parsed.files.map((f) => ({ kind: f.kind, path: f.path })) }; } catch (e) { return { ok: false, error: String(e) }; } @@ -1631,7 +1786,7 @@ ipcMain.handle('sites:getAll', async () => { return { sites: s.get('sites'), siteMeta: s.get('siteMeta') }; }); -ipcMain.handle('site:status', async (_e, sitePath) => { +ipcMain.handle('site:status', async (event, sitePath) => { try { const nmDir = path.join(sitePath, 'node_modules'); const hasNodeModules = fs.existsSync(nmDir) && (() => { try { return fs.readdirSync(nmDir).length > 0; } catch { return false; } })(); @@ -1657,6 +1812,16 @@ ipcMain.handle('site:status', async (_e, sitePath) => { } } catch {} + // Where trunk is on the remote (#307), which is what actually decides + // staleness — the snapshot's age is only the offline fallback. The + // stored answer is returned as it stands and the refresh runs behind + // this reply, so nothing on this path touches the network before + // returning. See refreshRemoteTrunk. + const remoteTrunkOid = m.remoteTrunkOid || null; + if (remoteProbeDue({ checkedAt: m.remoteTrunkCheckedAt })) { + void refreshRemoteTrunk(sitePath, event && event.sender); + } + // The applied patch and the incomplete-update flag belong to the ticket // being worked on, not to the site (#108) — otherwise switching tickets // would carry the other one's "patch applied · Revert" banner over, and @@ -1665,18 +1830,47 @@ ipcMain.handle('site:status', async (_e, sitePath) => { // Summarised rather than passed through: the stored patch text is only // needed by the main process to reverse it, and this is polled. + // + // `revertable` used to mean "we kept the text". It now means what the + // banner actually asks — can this layer still be lifted out, right now + // (#306) — which is a fact about the tree and has to be measured here, + // because the text does not cross IPC and the renderer cannot ask. + // The measurement is in-memory: it reads the patch's own files (one to a + // handful, on a call that already stats the checkout and reads a git + // object) and matches hunks against them. Nothing is written. It is + // bounded twice over — a patch bigger than REVERTABLE_PATCH_LIMIT has no + // stored text to check, and this handler is called on mount and after + // long operations, not on a timer. The costly shape is a large absorbed + // patch, which pays its per-hunk diagnosis on every one of those reads; + // if that is ever felt, a ceiling belongs in diagnoseRemoval, not here. + // + // Guarded, and it fails towards offering Revert: a check that could not + // run must not hide the exit. Pressing Revert then gives the real answer + // from the apply path, with its own explanation. + let absorbed = []; + if (work.appliedPatch && work.appliedPatch.text) { + try { + absorbed = diagnoseRemoval({ dir: sitePath, patchText: work.appliedPatch.text }).absorbed; + } catch (e) { + logError('site:status', `could not check whether ${describeRefused(work.appliedPatch.label)} still comes out of ${describeRefused(sitePath)}: ${String(e && e.stack ? e.stack : e)}`); + } + } const appliedPatch = work.appliedPatch ? { label: work.appliedPatch.label, appliedAt: work.appliedPatch.appliedAt, files: work.appliedPatch.files || [], - revertable: Boolean(work.appliedPatch.text) + // Whether a text was kept at all — the separate reason a patch + // cannot be reverted, and a different sentence from absorption. + kept: Boolean(work.appliedPatch.text), + absorbed, + revertable: Boolean(work.appliedPatch.text) && absorbed.length === 0 } : null; - return { hasNodeModules, hasBuilt, skipInitWizard: Boolean(m.skipInitWizard), initialized: Boolean(m.initialized), installFailed: Boolean(m.installFailed), trunkOid, trunkDate, updateIncomplete: Boolean(work.updateIncomplete), tracTicket: m.tracTicket || null, appliedPatch }; + return { hasNodeModules, hasBuilt, skipInitWizard: Boolean(m.skipInitWizard), initialized: Boolean(m.initialized), installFailed: Boolean(m.installFailed), trunkOid, trunkDate, remoteTrunkOid, updateIncomplete: Boolean(work.updateIncomplete), tracTicket: m.tracTicket || null, appliedPatch }; } catch { - return { hasNodeModules: false, hasBuilt: false, skipInitWizard: false, initialized: false, installFailed: false, trunkOid: null, trunkDate: null, updateIncomplete: false, tracTicket: null, appliedPatch: null }; + return { hasNodeModules: false, hasBuilt: false, skipInitWizard: false, initialized: false, installFailed: false, trunkOid: null, trunkDate: null, remoteTrunkOid: null, updateIncomplete: false, tracTicket: null, appliedPatch: null }; } }); diff --git a/src/patch-apply.js b/src/patch-apply.js index e222a1a..8e74bee 100644 --- a/src/patch-apply.js +++ b/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 }; diff --git a/src/patch-provenance.cjs b/src/patch-provenance.cjs index d528cb8..a65464e 100644 --- a/src/patch-provenance.cjs +++ b/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); diff --git a/src/preload.js b/src/preload.js index 780f557..c63b329 100644 --- a/src/preload.js +++ b/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); diff --git a/src/renderer/applied-layer.cjs b/src/renderer/applied-layer.cjs new file mode 100644 index 0000000..fe96446 --- /dev/null +++ b/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 }; diff --git a/src/renderer/apply-conflict.cjs b/src/renderer/apply-conflict.cjs index 05d56d3..2cdbe5a 100644 --- a/src/renderer/apply-conflict.cjs +++ b/src/renderer/apply-conflict.cjs @@ -30,17 +30,29 @@ const REASONS = { moved: 'the code around it has changed' }; +// The same two statuses read backwards, for a revert (#306). `already-applied` +// is derived by testing the inverse of what is being applied, so on a reverse it +// means the *forward* hunk fits — that region's change is not in the checkout +// any more. Rendering the forward wording there would put "already in your +// checkout" under a headline saying the contributor has edited over it. +const REVERT_REASONS = { + 'already-applied': 'looks like that change is not in your checkout any more', + moved: 'the code around it has changed' +}; + /** * One failing region, ready to render. * - * @param {Object} region + * @param {Object} region + * @param {boolean} [reversing] Whether the failed run was a revert. * @return {Object} */ -function describeRegion(region) { +function describeRegion(region, reversing = false) { + const reasons = reversing ? REVERT_REASONS : REASONS; return { line: region.line, status: region.status, - reason: REASONS[region.status] || 'it no longer fits', + reason: reasons[region.status] || 'it no longer fits', // A line to search for, not a number to go to: the hunk's line numbers // are in the patched file's coordinates and miss by the drift the patch // failed on. The panel leads with this and keeps `line` as the fallback. @@ -106,6 +118,33 @@ function otherPatchCount({ label, prs = [], attachments = [] } = {}) { + attachments.filter((att) => att.filename !== failedLabel).length; } +// How many paths a sentence will name before it starts counting instead. The +// panel already lists every failing file underneath, so the headline's job is +// the attribution, not the inventory — and a pull request failing in a dozen +// files would otherwise open the notice with a paragraph of paths. Same reason +// `REGION_DETAIL_LIMIT` exists in patch-apply.js. +const PATH_NAME_LIMIT = 3; + +/** + * The files on one side of the failure, named for a sentence. + * + * Deduplicated because a concatenated patch can fail the same file twice — the + * same reason `describeApplyFailure` consumes conflicts one by one instead of + * keying them by path. + * + * @param {Array} rows Conflicts. + * @return {{count: number, text: string}} + */ +function namePaths(rows) { + const paths = [...new Set(rows.map((c) => c.path))]; + if (paths.length <= PATH_NAME_LIMIT) return { count: paths.length, text: paths.join(', ') }; + const rest = paths.length - PATH_NAME_LIMIT; + return { + count: paths.length, + text: `${paths.slice(0, PATH_NAME_LIMIT).join(', ')} and ${rest} more file${rest === 1 ? '' : 's'}` + }; +} + /** * The pull-request framing: whose problem this is, and how big. * @@ -127,11 +166,26 @@ function otherPatchCount({ label, prs = [], attachments = [] } = {}) { * merge would settle — close enough to size the problem, not a claim GitHub * will show the identical number. * - * @param {Array} conflicts - * @param {?string} prState 'open' | 'merged' | 'closed' | null when unknown. + * That same missing base is why `ownWorkPaths` is here (#303). Matching two-way + * cannot prove which side a single failed region belongs to, so the framing used + * to answer from the pull request's state alone: open meant stale, always. On a + * ticket someone has come back to, the ordinary reason a change will not fit is + * the contributor's own work sitting in the same file — and sending them to ask + * a stranger for a rebase that would not help is worse than saying nothing. + * + * The app cannot prove the region, but it knows the file: `ownWorkPaths` is the + * preview's own collision list, the files this ticket has work in measured from + * its base (#301). A file on that list gets the collision named for what it is + * and the ways out that are the contributor's own; a file off it keeps the + * stale-pull-request framing. When both are present the notice says both, since + * choosing one would be guessing again. + * + * @param {Array} conflicts + * @param {?string} prState 'open' | 'merged' | 'closed' | null when unknown. + * @param {string[]} [ownWorkPaths] Files this ticket has its own work in. * @return {{headline: string, advice: string, prButton: ?string}} */ -function prFraming(conflicts, prState) { +function prFraming(conflicts, prState, ownWorkPaths = []) { const failed = conflicts.reduce((sum, c) => sum + c.regions.length, 0); const total = conflicts.reduce((sum, c) => sum + c.total, 0); const allApplied = conflicts.every((c) => c.regions.every((r) => r.status === 'already-applied')); @@ -151,7 +205,10 @@ function prFraming(conflicts, prState) { }; } - const files = conflicts.length; + // Distinct files, not conflict rows: a concatenated patch can fail the same + // file twice, and the sentences below go on to name the files, so counting + // the rows would have the count disagreeing with the list beside it. + const files = new Set(conflicts.map((c) => c.path)).size; const scale = `${failed} of its ${total} change${total === 1 ? '' : 's'}, in ${files} file${files === 1 ? '' : 's'},`; // A closed pull request has no author coming back to it: asking for a @@ -165,13 +222,74 @@ function prFraming(conflicts, prState) { }; } + const own = new Set(ownWorkPaths); + const mine = namePaths(conflicts.filter((c) => own.has(c.path))); + const theirs = namePaths(conflicts.filter((c) => !own.has(c.path))); + const REBASE_IS_THEIRS = 'Bringing it up to date is its author\'s work — a rebase, or merging trunk in. Leaving a comment on the pull request to let them know is a real contribution in itself.'; + // A ticket's changes are cheap to redo and expensive to untangle, so keeping + // a copy and starting clean is a recommended way forward here, not a defeat. + // What must never happen is work going quietly; going on purpose, with the + // copy already saved, is a good outcome. + const YOUR_WORK_WAY_OUT = 'Save a patch of your work first to keep a copy. Then apply this on a ticket that does not have that work, or discard it here once the copy is saved.'; + + // Every failing file is one this ticket has work in, so nothing here is the + // pull request being behind trunk, and there is no rebase to ask anyone for. + // The pull request is still worth opening — reading it is how the + // contributor decides whether to start a clean ticket for it — so the button + // stays and only the ask goes. + if (!theirs.count) { + return { + headline: `This pull request does not fit your checkout: ${scale} would need rework. Your own work is in the way, in ${mine.text}.`, + advice: `This is your work meeting the pull request, not a pull request that has gone stale, so asking its author for a rebase would not help. ${YOUR_WORK_WAY_OUT}`, + prButton: 'Open the pull request' + }; + } + + if (mine.count) { + return { + headline: `This pull request does not fit your checkout: ${scale} would need rework. Your own work is in the way in ${mine.text}. The rest is the pull request being behind trunk: ${theirs.text}.`, + advice: `${REBASE_IS_THEIRS} Your own files are yours to sort out. ${YOUR_WORK_WAY_OUT}`, + prButton: 'Ask its author for a rebase' + }; + } + return { headline: `This pull request was written against an older trunk and no longer fits it: ${scale} would need rework.`, - advice: 'Bringing it up to date is its author\'s work — a rebase, or merging trunk in. Leaving a comment on the pull request to let them know is a real contribution in itself.', + advice: REBASE_IS_THEIRS, prButton: 'Ask its author for a rebase' }; } +/** + * The framing for a revert that would not come back out (#306). + * + * A revert fails for one reason: the contributor's own edits are on the lines + * the patch brought. There is no third party here and no rebase to ask anyone + * for — the patch and the edits are one body of work now, which is what + * absorption means. So the sentence names that, and the two ways forward are + * undoing the overlapping edits, or saving a copy and discarding the ticket to + * its base, which on this project is a normal step rather than a defeat. + * + * Regions are kept, unlike the pull-request framing: these lines are the + * contributor's own, so pointing at them is pointing at their work. + * + * @param {Array} conflicts + * @param {string} label + * @return {{headline: string, advice: string, prButton: ?string}} + */ +function revertFraming(conflicts, label) { + const failed = conflicts.reduce((sum, c) => sum + c.regions.length, 0); + const total = conflicts.reduce((sum, c) => sum + c.total, 0); + const files = conflicts.length; + const where = files === 1 ? '' : `, across ${files} files`; + + return { + headline: `${label} cannot be lifted back out on its own: your own edits are on ${failed} of its ${total} change${total === 1 ? '' : 's'}${where}.`, + advice: 'It is part of your changes now. Undoing your edits on those lines brings Revert back; otherwise save a copy of your work and discard the ticket to its base — on this project that is a normal way forward, not a lost afternoon.', + prButton: null + }; +} + /** * Everything the panel needs to explain a failed apply, or null when there is * nothing to explain. @@ -190,14 +308,17 @@ function prFraming(conflicts, prState) { * folder, a file to add that already exists) as well as the conflict that could * not be broken down. * - * @param {Object} result The apply payload from main. - * @param {Object} [options] - * @param {number} [options.otherPatchCount] Other patches on this ticket. - * @param {?string} [options.prUrl] The failing patch's pull request. - * @param {?string} [options.prState] Its state, when known. + * @param {Object} result The apply payload from main. + * @param {Object} [options] + * @param {number} [options.otherPatchCount] Other patches on this ticket. + * @param {?string} [options.prUrl] The failing patch's pull request. + * @param {?string} [options.prState] Its state, when known. + * @param {string[]} [options.ownWorkPaths] Files this ticket has work in, from + * the preview's collision list (#303). + * @param {?string} [options.reverting] Label of the layer being reverted. * @return {?Object} */ -function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, prUrl = null, prState = null } = {}) { +function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, prUrl = null, prState = null, ownWorkPaths = [], reverting = null } = {}) { if (!result || result.ok) return null; const failures = Array.isArray(result.failures) ? result.failures : []; @@ -207,7 +328,10 @@ function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, pr // the panel keeps rendering it exactly as it did before. if (!failures.length && !conflicts.length) return null; - const fromPr = Boolean(prUrl); + // A revert is never "someone else's patch does not fit": it is the + // contributor's own edits sitting on lines they applied. The pull-request + // framing would send them to an author who has nothing to do with it. + const fromPr = Boolean(prUrl) && !reverting; // Each conflict is consumed as it is matched, not looked up in a map: a // concatenated patch can fail the same file twice with the identical @@ -226,7 +350,7 @@ function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, pr failed: conflict.regions.length, // For a pull request the regions are the author's problem; the file // row with its counts is the whole story the contributor needs. - regions: fromPr ? [] : conflict.regions.map(describeRegion) + regions: fromPr ? [] : conflict.regions.map((region) => describeRegion(region, Boolean(reverting))) }; }); @@ -236,9 +360,13 @@ function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, pr let advice = ''; let prButton = fromPr ? 'Open the pull request' : null; if (conflicts.length) { - const framing = fromPr - ? prFraming(conflicts, prState) - : { headline: headlineFor(conflicts), advice: '', prButton: null }; + // A failed revert is never the pull request having gone stale, so it is + // asked first: the only thing that stops a layer coming back out is the + // contributor's own work sitting on its lines (#306). + let framing; + if (reverting) framing = revertFraming(conflicts, reverting); + else if (fromPr) framing = prFraming(conflicts, prState, ownWorkPaths); + else framing = { headline: headlineFor(conflicts), advice: '', prButton: null }; headline = framing.headline; advice = framing.advice; prButton = framing.prButton; @@ -247,13 +375,17 @@ function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, pr headline, advice, items, + // The absorbed exit, for the panel to offer alongside the sentence. Only + // a revert has one: every other failure left the checkout untouched, so + // there is nothing to save a copy of that is not already safe. + offerDiscardToBase: Boolean(reverting) && conflicts.length > 0, // Both are offered only when they lead somewhere, the way open-failure.cjs // withholds its picker: a way out that returns to the same dead end is // worse than no button, because it costs a click to find that out. - offerOtherPatches: othersAvailable > 0, + offerOtherPatches: !reverting && othersAvailable > 0, prUrl: prUrl && prButton ? prUrl : null, prButton }; } -module.exports = { describeApplyFailure, headlineFor, otherPatchCount, REASONS }; +module.exports = { describeApplyFailure, headlineFor, otherPatchCount, revertFraming, REASONS, REVERT_REASONS }; diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index d51d1d6..329c6d8 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -30,7 +30,8 @@ import { pathBasename } from './path-basename.cjs'; import { sanitizeSiteFolder, resolveTargetDir, directoryFromFileEntry } from './site-folder.cjs'; import { noticeForOpenResult } from './open-failure.cjs'; import { describeApplyFailure, otherPatchCount } from './apply-conflict.cjs'; -import { trunkAgeInfo, planUpdateSteps, updateStepStatuses, SKIP_INSTALL_MESSAGE, planApplySteps, planWatchImpact, APPLY_STATE_TO_STEP, planSetupSteps, SETUP_STATE_TO_STEP, setupOutcome } from './update-plan.cjs'; +import { describeAppliedLayer, describePreviewNotice, absorbedExitFailure } from './applied-layer.cjs'; +import { trunkAgeInfo, trunkUpdateAdvice, planUpdateSteps, updateStepStatuses, SKIP_INSTALL_MESSAGE, planApplySteps, planWatchImpact, APPLY_STATE_TO_STEP, planSetupSteps, SETUP_STATE_TO_STEP, setupOutcome } from './update-plan.cjs'; import { pickLatest } from '../latest-patch.cjs'; import { beginSetup, adoptSetupPath, discardSetup, rowPathAfterStatus } from './pending-setup.cjs'; import { parsePrRef } from '../patch-sources.cjs'; @@ -99,6 +100,15 @@ const UPDATE_STEP_MARKS = { complete: { symbol: '✓', color: '#0f5132' }, current: { symbol: '›', color: '#0b5d95' } }; +// How the preview renders what it has to say about the contributor's own work +// (#308). `warning` is the amber block that has always been there — a patch is +// about to land on files someone edited. `note` is the quieter case: nothing +// collided, but the base it was measured against was approximate, so the +// silence needs a sentence rather than an alert. +const OWN_WORK_NOTICE_STYLES = { + warning: { role: 'alert', style: { marginTop: 10, padding: '8px 10px', background: '#fcf9e8', border: '1px solid #dba617', borderRadius: 6, fontSize: 12, color: '#6e5406' } }, + note: { role: undefined, style: { marginTop: 10, fontSize: 12, color: '#6c6f72' } } +}; // The file manager has a name on the two platforms that have one; everywhere // else it is whatever the desktop provides, so it is called what it is. // @@ -774,13 +784,17 @@ function App() { // Staleness surfaces in the sidebar before the site is even // opened (#94): amber = old trunk snapshot, red = an update that // moved trunk but never finished install/build. - const trunkAge = trunkAgeInfo({ trunkDate: meta.trunkDate }); + const trunkAge = trunkAgeInfo({ trunkDate: meta.trunkDate, trunkOid: meta.trunkOid, remoteTrunkOid: meta.remoteTrunkOid }); + // The dot is shown whenever trunk has moved on, including when the + // site has work that updating would disturb: the advice below it is + // what softens, never the fact. + const trunkAdvice = trunkUpdateAdvice({ trunkAge }); let staleDotColor = null; if (meta.updateIncomplete) staleDotColor = '#d63638'; else if (trunkAge.stale) staleDotColor = '#dba617'; const staleDotTitle = meta.updateIncomplete ? 'Update incomplete — code is new, built assets are old' - : `WordPress code is ${trunkAge.ageDays} days old — update to latest trunk`; + : trunkAdvice.dotTitle; const staleDot = staleDotColor ? ( { loadStatus(); }, [loadStatus]); + // The remote probe (#307) answers after site:status has already replied — it + // is started behind that reply so opening a site never waits on the network. + // Without this subscription the answer would only be read on the next launch, + // because site:status is called on mount and after long operations, not on a + // timer. Two consumers: this card, and the sidebar dot through the meta patch. + useEffect(() => { + const unsub = window.api.subscribeRemoteTrunk((p) => { + if (!p || p.sitePath !== sitePath) return; + setRemoteTrunkOid(p.remoteTrunkOid || null); + if (metaPatchRef.current) metaPatchRef.current(sitePath, { remoteTrunkOid: p.remoteTrunkOid || null }); + }); + return () => { if (unsub) unsub(); }; + }, [sitePath]); + // Deliberately not part of loadStatus: that one is called after every long // operation, and the branch list only changes when a ticket is linked, // resumed or deleted — the three paths that call this themselves. @@ -2655,7 +2690,14 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit ); // --- Update to latest trunk (#94) --- - const age = trunkAgeInfo({ trunkDate }); + const age = trunkAgeInfo({ trunkDate, trunkOid, remoteTrunkOid }); + // Every sentence the contributor reads about staleness, and whether the app + // pushes at all — it stays quiet when updating would cost them something. + const trunkAdvice = trunkUpdateAdvice({ + trunkAge: age, + appliedPatch: Boolean(appliedPatch), + ticketLinked: Boolean(tracTicket) + }); const isUpdating = updateState !== 'idle'; // Where the note goes moves with the ticket: a change that belongs to // #12345 is news for the ticket card, one that belongs to nothing is news @@ -2728,6 +2770,25 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit }); const applySteps = planApplySteps({ needsInstall: applyNeedsInstall, buildByWatcher: applyBuildByWatcher }); const applyStepStates = updateStepStatuses(applySteps, applyState, APPLY_STATE_TO_STEP); + // The applied patch as a layer with a name (#306), not an undo blob. Both + // answers come from the same record: whether it can still be lifted out — + // measured in main on every status read, so it comes back on its own when the + // overlapping edit is undone — and whose changes the next patch would land on. + const appliedLayer = describeAppliedLayer(appliedPatch, { + when: appliedPatch?.appliedAt ? new Date(appliedPatch.appliedAt).toLocaleString() : '' + }); + // What the preview says about the contributor's own work: whose it is (#306), + // and how confidently, since the preview carries the status of the base it was + // measured against (#308). + const previewNotice = applyPreview + ? describePreviewNotice({ conflicts: applyPreview.conflicts, appliedPatch, baseStatus: applyPreview.baseStatus }) + : null; + // The absorbed exits reach the same two operations the changes note does, so + // they go through the same guard: a discard is a force checkout, and running + // it under a live dev server or a half-finished install rewrites the tree + // from under it. Not re-derived here — that is how a second answer starts. + const absorbedExitBlocked = discardBlocked({ isUpdating, installing, building, devServerActive: isDevProcessActive, discarding }); + const absorbedExit = absorbedExitFailure({ patchSaveError, discardError }); // --- Initial setup, as one chain (#246) --- // The third chain, and the only one nobody starts: between the clone, the @@ -2996,12 +3057,20 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // an already-linked site would open a Trac window nobody asked for (#292). const autoReadTicketRef = useRef(null); const tracScrapeRef = useRef(null); + // A Trac scrape can run up to 90s. Bump a generation on every ticket change so + // a scrape that resolves after the ticket has moved on is dropped, rather than + // shown under the wrong ticket or clearing a newer request's loading flag. + const scrapeGenRef = useRef(0); useEffect(() => { + // Bumped first, synchronously, before anything below can trigger a scrape + // (#299): loadTracAttachments reads this ref at call time, so the auto-read + // a few lines down must never run against last ticket's generation. + scrapeGenRef.current += 1; if (!tracTicket) { setTicketPatches(null); // Attachments are per-ticket and loaded on demand; a stale list from the // previous ticket must not linger, and a scrape dropped by the generation - // bump below must not leave a stuck spinner. + // bump above must not leave a stuck spinner. setTracAttachments(null); setTracAttachmentsLoading(false); loadedTicketRef.current = null; @@ -3010,10 +3079,10 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit if (!isActive || loadedTicketRef.current === tracTicket) return; // A new ticket on the active site: drop any attachments the previous one // loaded (and clear its loading flag, so a scrape dropped by the generation - // bump cannot leave a stuck spinner with no button to recover), then fetch - // its PRs. Marked loaded before the fetch resolves, on purpose: a failed - // initial fetch is not retried on every re-activation (which could keep - // spending a rate-limited quota) — Refresh is the retry. + // bump above cannot leave a stuck spinner with no button to recover), then + // fetch its PRs. Marked loaded before the fetch resolves, on purpose: a + // failed initial fetch is not retried on every re-activation (which could + // keep spending a rate-limited quota) — Refresh is the retry. setTracAttachments(null); setTracAttachmentsLoading(false); loadedTicketRef.current = tracTicket; @@ -3031,12 +3100,6 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit } }, [tracTicket, isActive, loadTicketPatches]); - // A Trac scrape can run up to 90s. Bump a generation on every ticket change so - // a scrape that resolves after the ticket has moved on is dropped, rather than - // shown under the wrong ticket or clearing a newer request's loading flag. - const scrapeGenRef = useRef(0); - useEffect(() => { scrapeGenRef.current += 1; }, [tracTicket]); - // Fetches a PR's diff and drops into the same preview the file picker uses, // so applying a PR and applying a downloaded patch are one path from here on. const previewPr = async (pr) => { @@ -3174,18 +3237,27 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // A conflict is where the panel used to stop: one file named, the // rest of the failures left in the terminal, and no sense of whether // one region of twenty missed or all of them. The breakdown is what - // turns that into a decision (#282). A reverse is left out — the - // patch it names came from this app and the ticket's other patches - // are no help against it. - setApplyConflict(reverse ? null : describeApplyFailure(res, { - otherPatchCount: otherPatchCount({ - label: preview?.label, - prs: ticketPatches?.items, - attachments: patchAttachments - }), - prUrl: preview?.prUrl || null, - prState: preview?.prState || null - })); + // turns that into a decision (#282). A reverse gets its own framing + // (#306): it fails only because the contributor's own edits are on the + // patch's lines, so the ticket's other patches and the pull request's + // author are both the wrong place to send them. + setApplyConflict(describeApplyFailure(res, reverse + ? { reverting: appliedPatch?.label || 'That patch' } + : { + otherPatchCount: otherPatchCount({ + label: preview?.label, + prs: ticketPatches?.items, + attachments: patchAttachments + }), + prUrl: preview?.prUrl || null, + prState: preview?.prState || null, + // The preview's own collision list: the files this ticket has work + // in, measured from its base (#301). Without it an open pull + // request is always narrated as stale, so a failure caused by the + // contributor's own edits sends them to ask a stranger for a + // rebase that would not help (#303). + ownWorkPaths: preview?.conflicts || [] + })); finishApply(); return; } @@ -4007,7 +4079,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit applyPreview: Boolean(applyPreview), updateIncomplete, isUpdating, - stale: age.stale, + stale: trunkAdvice.recommendUpdate, running, hasChanges: Boolean(worktreeDirty && worktreeDirty.dirty), ticketLinked: Boolean(tracTicket) @@ -4060,7 +4132,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit {age.stale ? (