[Add] Apply Gutenberg pull requests to a Gutenberg site (#251)

src/github-prs.js

@@ -19,28 +19,33 @@
  * request (#167) needs the same one with a method and a body.
  */
 
-const { parseLinkedPrs, classifyHttpFailure } = require('./patch-sources.cjs');
+const { parseLinkedPrs, classifyHttpFailure, citesWorkItemFor, PR_REPO_PATH } = require('./patch-sources.cjs');
 const { httpGet } = require('./github-http.cjs');
 
-const REPO = 'WordPress/wordpress-develop';
+// The default upstream. Both functions take a `repo` so a Gutenberg site reads
+// its own pull requests (#251); left unset they behave exactly as before.
+const REPO = PR_REPO_PATH;
 
 /**
  * The pull requests that cite a ticket, newest first.
  *
  * @param {number|string} ticketId
  * @param {Object}        [deps]
+ * @param {string}        [deps.repo]     `owner/repo` to search (defaults to wordpress-develop).
+ * @param {string}        [deps.provider] Work-item provider deciding what "cites" means.
  * @return {Promise<{status: 'ok'|'rate-limited'|'error'|'offline', items: Array, error?: string}>}
  */
 async function fetchLinkedPrs(ticketId, deps = {}) {
 	const get = deps.httpGet || httpGet;
+	const repo = deps.repo || REPO;
 	const id = String(ticketId).replace(/[^0-9]/g, '');
 	if (!id) return { status: 'error', items: [], error: 'No ticket number' };
 
 	// 100 is GitHub's per-page maximum. One request covers any realistic ticket;
 	// paginating would multiply requests against the shared unauthenticated quota
 	// this whole feature is careful with, so instead a result that does not fit in
 	// one page is treated as incomplete below.
-	const query = encodeURIComponent(`repo:${REPO} is:pr ${id}`);
+	const query = encodeURIComponent(`repo:${repo} is:pr ${id}`);
 	const url = `https://api.github.com/search/issues?q=${query}&per_page=100`;
 
 	let res;
@@ -67,22 +72,26 @@ async function fetchLinkedPrs(ticketId, deps = {}) {
 		return { status: 'error', items: [], error: 'Too many results to list reliably' };
 	}
 
-	return { status: 'ok', items: parseLinkedPrs(json, id) };
+	const cites = citesWorkItemFor(deps.provider, repo);
+	return { status: 'ok', items: parseLinkedPrs(json, id, { cites, repoPath: repo }) };
 }
 
 /**
  * The unified diff for one pull request.
  *
- * @param {number} number
+ * @param {number}   number
+ * @param {Object}   [options]
+ * @param {string}   [options.repo]    `owner/repo` the pull request belongs to.
+ * @param {Function} [options.httpGet] Injected for tests, like fetchLinkedPrs.
  * @return {Promise<{ok: true, text: string}|{ok: false, status: string, error: string}>}
  */
-async function fetchPrDiff(number) {
+async function fetchPrDiff(number, { repo = REPO, httpGet: get = httpGet } = {}) {
 	const n = String(number).replace(/[^0-9]/g, '');
 	if (!n) return { ok: false, status: 'error', error: 'No pull request number' };
 
 	let res;
 	try {
-		res = await httpGet(`https://api.github.com/repos/${REPO}/pulls/${n}`, { Accept: 'application/vnd.github.v3.diff' });
+		res = await get(`https://api.github.com/repos/${repo}/pulls/${n}`, { Accept: 'application/vnd.github.v3.diff' });
 	} catch (e) {
 		return { ok: false, status: 'offline', error: String(e && e.message ? e.message : e) };
 	}

src/main.js

@@ -1317,7 +1317,16 @@ ipcMain.handle('git:update-trunk', async (event, sitePath) => {
 // GitHub; the network code is in src/github-prs.js, these handlers add the
 // cache and IPC. A last-known-good copy per ticket, in electron-store, is what
 // lets a rate-limited or offline lookup still show the work that exists.
-const patchCacheKey = (ticketId) => `ticketPatches:${ticketId}`;
+// Keyed by repository as well as number (#251): a Trac ticket #123 and a
+// Gutenberg issue #123 are different work items, and one cache entry for both
+// would show a Core site's pull requests under a Gutenberg issue.
+const patchCacheKey = (ticketId, repoPath) => `ticketPatches:${repoPath}#${ticketId}`;
+
+// `owner/repo` for a site's upstream, defaulting to Core's.
+const upstreamRepoPath = (meta) => {
+    const up = projectTypeForSite(meta).upstream;
+    return `${up.owner}/${up.repo}`;
+};
 
 ipcMain.handle('git:list-ticket-patches', async (_e, sitePath) => {
     try {
@@ -1326,16 +1335,22 @@ ipcMain.handle('git:list-ticket-patches', async (_e, sitePath) => {
         const ticketId = meta.tracTicket;
         if (!ticketId) return { ok: true, ticket: null, prs: { status: 'no-ticket', items: [] } };
 
-        const result = await fetchLinkedPrs(ticketId);
+        // Which repository holds the pull requests, and what "cites this work
+        // item" means, both follow the site's project type (#251).
+        const type = projectTypeForSite(meta);
+        const repo = upstreamRepoPath(meta);
+        const cacheKey = patchCacheKey(ticketId, repo);
+
+        const result = await fetchLinkedPrs(ticketId, { repo, provider: type.workItem.provider });
         if (result.status === 'ok') {
-            s.set(patchCacheKey(ticketId), { checkedAt: new Date().toISOString(), items: result.items });
+            s.set(cacheKey, { checkedAt: new Date().toISOString(), items: result.items });
             return { ok: true, ticket: ticketId, prs: { status: 'ok', items: result.items } };
         }
 
         // Could not read GitHub. Fall back to whatever was last seen for this
         // ticket, labelled with when — a stale-but-shown list beats a short one
         // presented as complete.
-        const cached = s.get(patchCacheKey(ticketId)) || null;
+        const cached = s.get(cacheKey) || null;
         return {
             ok: true,
             ticket: ticketId,
@@ -1346,9 +1361,11 @@ ipcMain.handle('git:list-ticket-patches', async (_e, sitePath) => {
     }
 });
 
-ipcMain.handle('git:fetch-pr-diff', async (_e, number) => {
+ipcMain.handle('git:fetch-pr-diff', async (_e, sitePath, number) => {
     try {
-        return await fetchPrDiff(number);
+        // The pull request belongs to this site's own upstream (#251) — a
+        // Gutenberg site reads WordPress/gutenberg, not wordpress-develop.
+        return await fetchPrDiff(number, { repo: upstreamRepoPath(await readSiteMeta(sitePath)) });
     } catch (e) {
         return { ok: false, status: 'error', error: String(e) };
     }
@@ -1391,7 +1408,12 @@ const REVERTABLE_PATCH_LIMIT = 512 * 1024;
 // before deciding.
 ipcMain.handle('git:preview-patch', async (_e, sitePath, patchText) => {
     try {
-        const parsed = parsePatchFiles(patchText);
+        // The patch layout follows the site's project (#251): Core rewrites
+        // pre-src/ paths, Gutenberg diffs are already repo-relative. The apply
+        // below must be given the same layout or the two disagree about where a
+        // file lives.
+        const layout = projectTypeForSite(await readSiteMeta(sitePath)).patch.layout;
+        const parsed = parsePatchFiles(patchText, { layout });
         if (!parsed.ok) return { ok: false, error: parsed.error };
         let dirtyPaths;
         try {
@@ -1469,7 +1491,10 @@ ipcMain.handle('git:apply-patch', async (event, sitePath, options = {}) => {
             }
             sendLog(`\n${reverse ? 'Reverting' : 'Applying'} ${label}…\n`);
 
-            const result = await applyPatchToDir({ dir: sitePath, patchText, reverse, onLog: sendLog });
+            // Same layout the preview used (#251) — a Gutenberg diff is already
+            // repo-relative and must not go through Core's src/ rewrite.
+            const layout = projectTypeForSite(await readSiteMeta(sitePath)).patch.layout;
+            const result = await applyPatchToDir({ dir: sitePath, patchText, reverse, layout, onLog: sendLog });
             if (!result.ok) {
                 // Nothing to revert means the record is describing a patch the
                 // checkout no longer has. Keeping it would leave the site stuck:
@@ -1513,7 +1538,7 @@ ipcMain.handle('git:apply-patch', async (event, sitePath, options = {}) => {
                     // rather than leave a patch the app cannot revert. If the undo
                     // also fails, say so plainly instead of reporting a clean fail.
                     logError('git:apply-patch', `persist failed, undoing apply: ${String(persistErr && persistErr.stack ? persistErr.stack : persistErr)}`);
-                    const undo = await applyPatchToDir({ dir: sitePath, patchText, reverse: true, onLog: sendLog });
+                    const undo = await applyPatchToDir({ dir: sitePath, patchText, reverse: true, layout, onLog: sendLog });
                     const why = String(persistErr && persistErr.message ? persistErr.message : persistErr);
                     if (undo.ok) {
                         sendDone({ ok: false, error: `The patch applied but its revert record could not be saved, so it was undone. ${why}` });

src/patch-apply.js

@@ -272,11 +272,15 @@ function rollback(done) {
  * @param {string}   root0.dir
  * @param {string}   root0.patchText
  * @param {boolean}  [root0.reverse]
+ * @param {string}   [root0.layout]  Path layout: 'src-layout' (default) or 'repo-relative'.
  * @param {Function} [root0.onLog]
  * @return {Promise<Object>}
  */
-async function applyPatchToDir({ dir, patchText, reverse = false, onLog = () => {} }) {
-	const parsed = parsePatchFiles(patchText);
+async function applyPatchToDir({ dir, patchText, reverse = false, layout, onLog = () => {} }) {
+	// The layout must match the one the preview used, or the two disagree about
+	// where a file lives and the patch applies somewhere the contributor was
+	// never shown (#251). Defaults to Core's src-layout.
+	const parsed = parsePatchFiles(patchText, { layout });
 	if (!parsed.ok) return { ok: false, error: parsed.error };
 
 	await ensureAutocrlf(dir);

src/patch-plan.cjs

@@ -137,14 +137,33 @@ function classify(file, oldPath, newPath) {
 	return 'modify';
 }
 
+/**
+ * Chooses the per-path normalisation for a project's layout (#251).
+ *
+ * `src-layout` (WordPress Core, the default) rewrites pre-`src/` paths — a patch
+ * attached to a ticket years ago still names `wp-admin/…`. `repo-relative`
+ * (Gutenberg) leaves paths alone: its diffs are already repo-relative
+ * (`packages/…`), and running them through the Core rewrite would move any
+ * top-level `wp-`-prefixed path under a `src/` directory that does not exist
+ * there.
+ *
+ * @param {string} [layout]
+ * @return {(filePath: string) => string}
+ */
+function pathMapperFor(layout) {
+	return layout === 'repo-relative' ? (filePath) => filePath : mapToSrcLayout;
+}
+
 /**
  * Parses a patch into the files it touches, with paths normalised to
- * repo-relative form for today's layout.
+ * repo-relative form for the target project's layout.
  *
  * @param {string} text
+ * @param {Object} [options]
+ * @param {string} [options.layout] 'src-layout' (default) or 'repo-relative'.
  * @return {{ok: true, files: Array}|{ok: false, error: string}}
  */
-function parsePatchFiles(text) {
+function parsePatchFiles(text, { layout } = {}) {
 	const raw = typeof text === 'string' ? text : '';
 	if (!raw.trim()) return { ok: false, error: 'The patch is empty.' };
 
@@ -162,6 +181,7 @@ function parsePatchFiles(text) {
 	}
 
 	const sections = scanSections(normalizeEol(raw));
+	const mapPath = pathMapperFor(layout);
 	const files = [];
 
 	for (let i = 0; i < parsed.length; i++) {
@@ -172,13 +192,13 @@ function parsePatchFiles(text) {
 			// what this was.
 			const section = sections[i];
 			if (section && section.renameFrom && section.renameTo) {
-				const oldPath = mapToSrcLayout(section.renameFrom);
-				const newPath = mapToSrcLayout(section.renameTo);
+				const oldPath = mapPath(section.renameFrom);
+				const newPath = mapPath(section.renameTo);
 				files.push({ kind: 'rename', oldPath, newPath, path: newPath, hunks: [], patch: file });
 				continue;
 			}
 			if (section && section.isBinary) {
-				const binaryPath = mapToSrcLayout(stripPathPrefix(section.path, section.path).newPath);
+				const binaryPath = mapPath(stripPathPrefix(section.path, section.path).newPath);
 				files.push({ kind: 'binary', oldPath: binaryPath, newPath: binaryPath, path: binaryPath, hunks: [], patch: file });
 				continue;
 			}
@@ -192,9 +212,9 @@ function parsePatchFiles(text) {
 		const target = kind === 'delete' ? oldPath : newPath;
 		files.push({
 			kind,
-			oldPath: mapToSrcLayout(oldPath),
-			newPath: mapToSrcLayout(newPath),
-			path: mapToSrcLayout(target),
+			oldPath: mapPath(oldPath),
+			newPath: mapPath(newPath),
+			path: mapPath(target),
 			hunks: file.hunks,
 			patch: file
 		});
@@ -242,6 +262,7 @@ module.exports = {
 	SRC_FILES,
 	stripPathPrefix,
 	mapToSrcLayout,
+	pathMapperFor,
 	parsePatchFiles,
 	planApply
 };

src/patch-sources.cjs

@@ -24,14 +24,20 @@ const PR_REPO_PATH = 'WordPress/wordpress-develop';
 
 /**
  * Resolves what a contributor pastes into "apply a PR" to a pull request
- * number. Accepts a bare number or a wordpress-develop PR URL (with any
- * trailing `/files`, `#…`, `?…`). A PR from another repo is rejected by name —
- * its diff would not fit this checkout.
+ * number. Accepts a bare number or a PR URL (with any trailing `/files`, `#…`,
+ * `?…`) belonging to this site's own upstream. A PR from another repo is
+ * rejected by name — its diff would not fit this checkout.
+ *
+ * `repoPath` is the site's upstream (#251): a Gutenberg site applies
+ * WordPress/gutenberg PRs, a Core site wordpress-develop ones. It defaults to
+ * wordpress-develop so existing callers are unchanged.
  *
  * @param {string} input
+ * @param {Object} [options]
+ * @param {string} [options.repoPath] `owner/repo` this checkout accepts.
  * @return {{ok: true, number: number}|{ok: false, error: string}}
  */
-function parsePrRef(input) {
+function parsePrRef(input, { repoPath = PR_REPO_PATH } = {}) {
 	const raw = typeof input === 'string' ? input.trim() : '';
 	if (!raw) return { ok: false, error: 'Enter a pull request URL or number.' };
 
@@ -48,8 +54,8 @@ function parsePrRef(input) {
 	}
 	const match = /^\/([^/]+\/[^/]+)\/pull\/(\d+)(?:[/?#]|$)/.exec(parsed.pathname + (parsed.pathname.endsWith('/') ? '' : '/'));
 	if (!match) return { ok: false, error: 'That does not look like a pull request URL.' };
-	if (match[1].toLowerCase() !== PR_REPO_PATH.toLowerCase()) {
-		return { ok: false, error: `Only ${PR_REPO_PATH} pull requests can be applied here.` };
+	if (match[1].toLowerCase() !== String(repoPath).toLowerCase()) {
+		return { ok: false, error: `Only ${repoPath} pull requests can be applied here.` };
 	}
 	return { ok: true, number: Number(match[2]) };
 }
@@ -71,6 +77,44 @@ function bodyCitesTicket(body, ticketId) {
 	return re.test(body);
 }
 
+/**
+ * The same question for a GitHub-issue work item (#251). There is no ticket URL
+ * to look for: a Gutenberg pull request cites its issue the GitHub way, as
+ * `#1234` (usually behind a closing keyword) or as the issue's own URL.
+ *
+ * The `(?![0-9])` guard is why this is not a bare `includes`: `#658` must not
+ * match inside `#6580`. A bare number with no `#` is deliberately not accepted —
+ * that is the prose match the verification exists to reject.
+ *
+ * @param {string}        body
+ * @param {number|string} issueId
+ * @param {string}        [repoPath] `owner/repo`, for the URL form.
+ * @return {boolean}
+ */
+function bodyCitesIssue(body, issueId, repoPath = '') {
+	if (typeof body !== 'string') return false;
+	const id = String(issueId).replace(/[^0-9]/g, '');
+	if (!id) return false;
+	if (new RegExp(`#${id}(?![0-9])`).test(body)) return true;
+	if (!repoPath) return false;
+	const path = String(repoPath).replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+	return new RegExp(`github\\.com/${path}/issues/${id}(?![0-9])`, 'i').test(body);
+}
+
+/**
+ * Picks the citation test for a work-item provider, so a caller can ask "does
+ * this PR belong to this work item?" without knowing which kind it is.
+ *
+ * @param {string} provider   'trac' (default) or 'github-issue'.
+ * @param {string} [repoPath]
+ * @return {(body: string, id: number|string) => boolean}
+ */
+function citesWorkItemFor(provider, repoPath) {
+	return provider === 'github-issue'
+		? (body, id) => bodyCitesIssue(body, id, repoPath)
+		: bodyCitesTicket;
+}
+
 /**
  * What happened to one pull request, from a `search/issues` item: open, merged
  * or closed-unmerged.
@@ -90,17 +134,20 @@ function prState(item) {
  *
  * @param {Object}        searchJson
  * @param {number|string} ticketId
+ * @param {Object}        [root0]
+ * @param {Function}      [root0.cites]    Citation test (defaults to the Trac one).
+ * @param {string}        [root0.repoPath] `owner/repo`, for the fallback URL.
  * @return {Array<{number: number, title: string, state: 'open'|'merged'|'closed', updatedAt: string, url: string}>}
  */
-function parseLinkedPrs(searchJson, ticketId) {
+function parseLinkedPrs(searchJson, ticketId, { cites = bodyCitesTicket, repoPath = PR_REPO_PATH } = {}) {
 	const items = searchJson && Array.isArray(searchJson.items) ? searchJson.items : [];
 	const seen = new Set();
 	const prs = [];
 	for (const item of items) {
 		// `search/issues` returns issues and PRs together; only PRs carry
 		// `pull_request`.
 		if (!item || !item.pull_request) continue;
-		if (!bodyCitesTicket(item.body, ticketId)) continue;
+		if (!cites(item.body, ticketId)) continue;
 		if (seen.has(item.number)) continue;
 		seen.add(item.number);
 		prs.push({
@@ -113,7 +160,7 @@ function parseLinkedPrs(searchJson, ticketId) {
 			// unauthenticated quota this file is careful with.
 			state: prState(item),
 			updatedAt: item.updated_at || item.created_at || '',
-			url: item.html_url || `https://github.com/WordPress/wordpress-develop/pull/${item.number}`
+			url: item.html_url || `https://github.com/${repoPath}/pull/${item.number}`
 		});
 	}
 	prs.sort((a, b) => (b.updatedAt || '').localeCompare(a.updatedAt || ''));
@@ -143,7 +190,10 @@ function classifyHttpFailure(status, headers = {}) {
 
 module.exports = {
 	TICKET_HOST,
+	PR_REPO_PATH,
 	bodyCitesTicket,
+	bodyCitesIssue,
+	citesWorkItemFor,
 	parseLinkedPrs,
 	classifyHttpFailure,
 	parsePrRef

src/preload.js

@@ -188,7 +188,7 @@ contextBridge.exposeInMainWorld('api', {
 ,
 	listTicketPatches: (sitePath) => ipcRenderer.invoke('git:list-ticket-patches', sitePath)
 ,
-	fetchPrDiff: (number) => ipcRenderer.invoke('git:fetch-pr-diff', number)
+	fetchPrDiff: (sitePath, number) => ipcRenderer.invoke('git:fetch-pr-diff', sitePath, number)
 ,
 	listTracAttachments: (sitePath) => ipcRenderer.invoke('trac:list-attachments', sitePath)
 ,