diff --git a/src/e-cherry-pick.ts b/src/e-cherry-pick.ts index 99a71f6d..b64a09cb 100644 --- a/src/e-cherry-pick.ts +++ b/src/e-cherry-pick.ts @@ -6,9 +6,11 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import * as os from 'node:os'; -import { program } from 'commander'; +import { Command } from 'commander'; import { Octokit } from '@octokit/rest'; +const program = new Command(); + import * as evmConfig from './evm-config.js'; import debug from 'debug'; import { getGerritPatchDetailsFromURL } from './utils/gerrit.js'; @@ -22,7 +24,7 @@ const ELECTRON_REPO_DATA = { repo: 'electron', }; -interface PatchDetails { +export interface PatchDetails { patchDirName: string; shortCommit: string; patch: string; @@ -77,14 +79,84 @@ async function getGitHubPatchDetailsFromURL( }; } -function isUrl(arg: string) { +export function isUrl(arg: string) { return arg.startsWith('https://') || arg.startsWith('http://'); } -function commitSubject(patch: string) { +export function commitSubject(patch: string) { return /Subject: \[PATCH\] (.+?)$/m.exec(patch)?.[1]?.trim() ?? ''; } +// The first positional argument is expected to be a patch URL and the second a +// target branch, but users frequently transpose them — accept either order. +// Any remaining positional that looks like a URL is another patch to include +// in the same PR; everything else is another target branch. +export function splitPositionalArgs( + patchUrlStr: string, + targetBranch: string, + rest: string[], +): { patchUrls: string[]; targetBranches: string[] } { + if (isUrl(targetBranch)) { + const tmp = patchUrlStr; + patchUrlStr = targetBranch; + targetBranch = tmp; + } + + const patchUrls = [patchUrlStr, ...rest.filter(isUrl)]; + const targetBranches = [targetBranch, ...rest.filter((a) => !isUrl(a))]; + return { patchUrls, targetBranches }; +} + +export function computeBatchId(patchUrls: string[]): string { + return crypto.createHash('sha256').update(patchUrls.join('\n')).digest('hex').slice(0, 12); +} + +export function formatPRTitleAndBody({ + patches, + security, +}: { + patches: PatchDetails[]; + security: boolean; +}): { title: string; body: string } { + const isBatch = patches.length > 1; + const first = patches[0]!; + + if (isBatch) { + const patchDirNames = [...new Set(patches.map((p) => p.patchDirName))]; + const title = `chore: cherry-pick ${patches.length} changes from ${patchDirNames.join(', ')}`; + const lines = patches.map((p) => { + const ref = p.cve || p.bugNumber || p.shortCommit; + return `* ${p.shortCommit} from ${p.patchDirName} — ${commitSubject(p.patch)} (${ref})`; + }); + const notes = patches + .map((p) => p.cve || p.bugNumber) + .filter(Boolean) + .join(', '); + const body = + `Backports the following changes:\n\n${lines.join('\n')}\n\n` + + `Notes: ${ + notes + ? security + ? `Security: backported fixes for ${notes}.` + : `Backported fixes for ${notes}.` + : `` + }`; + return { title, body }; + } + + const { shortCommit, patchDirName, patch, bugNumber, cve } = first; + const title = `chore: cherry-pick ${shortCommit} from ${patchDirName}`; + const commitMessage = /Subject: \[PATCH\] (.+?)^---$/ms.exec(patch)?.[1] ?? ''; + const body = `${commitMessage}\n\nNotes: ${ + bugNumber + ? security + ? `Security: backported fix for ${cve || bugNumber}.` + : `Backported fix for ${bugNumber}.` + : `` + }`; + return { title, body }; +} + program .arguments(' [additionalBranchesOrUrls...]') .option('--security', 'Whether this backport is for security reasons') @@ -103,17 +175,7 @@ program rest: string[], { security, cveLookup }: { security?: boolean; cveLookup: boolean }, ) => { - if (isUrl(targetBranch)) { - const tmp = patchUrlStr; - patchUrlStr = targetBranch; - targetBranch = tmp; - } - - // Any positional argument that looks like a URL is treated as an - // additional patch to include in the same PR; everything else is an - // additional target branch to also raise a PR against. - const patchUrls = [patchUrlStr, ...rest.filter(isUrl)]; - const targetBranches = [targetBranch, ...rest.filter((a) => !isUrl(a))]; + const { patchUrls, targetBranches } = splitPositionalArgs(patchUrlStr, targetBranch, rest); const octokit = new Octokit({ auth: await getGitHubAuthToken(['repo']), @@ -141,12 +203,7 @@ program } const isBatch = patches.length > 1; - const patchDirNames = [...new Set(patches.map((p) => p.patchDirName))]; - const batchId = crypto - .createHash('sha256') - .update(patchUrls.join('\n')) - .digest('hex') - .slice(0, 12); + const batchId = computeBatchId(patchUrls); d(`Cloning electron/electron to ${tmp}`); cp.execSync(`git clone ${evmConfig.current().remotes.electron.origin}`, { cwd: tmp }); @@ -215,39 +272,7 @@ program stdio: 'ignore', }); - let title: string; - let body: string; - if (isBatch) { - title = `chore: cherry-pick ${patches.length} changes from ${patchDirNames.join(', ')}`; - const lines = patches.map((p) => { - const ref = p.cve || p.bugNumber || p.shortCommit; - return `* ${p.shortCommit} from ${p.patchDirName} — ${commitSubject(p.patch)} (${ref})`; - }); - const notes = patches - .map((p) => p.cve || p.bugNumber) - .filter(Boolean) - .join(', '); - body = - `Backports the following changes:\n\n${lines.join('\n')}\n\n` + - `Notes: ${ - notes - ? security - ? `Security: backported fixes for ${notes}.` - : `Backported fixes for ${notes}.` - : `` - }`; - } else { - const { shortCommit, patchDirName, patch, bugNumber, cve } = first; - title = `chore: cherry-pick ${shortCommit} from ${patchDirName}`; - const commitMessage = /Subject: \[PATCH\] (.+?)^---$/ms.exec(patch)?.[1] ?? ''; - body = `${commitMessage}\n\nNotes: ${ - bugNumber - ? security - ? `Security: backported fix for ${cve || bugNumber}.` - : `Backported fix for ${bugNumber}.` - : `` - }`; - } + const { title, body } = formatPRTitleAndBody({ patches, security: !!security }); d(`Creating PR for ${branchName}`); const { data: pr } = await octokit.pulls.create({ @@ -288,5 +313,8 @@ program } } }, - ) - .parse(process.argv); + ); + +if (import.meta.main) { + program.parse(process.argv); +} diff --git a/src/e-open.ts b/src/e-open.ts index f7cc33ba..90335cf9 100644 --- a/src/e-open.ts +++ b/src/e-open.ts @@ -3,9 +3,11 @@ import * as cp from 'node:child_process'; import * as path from 'node:path'; -import { program } from 'commander'; +import { Command } from 'commander'; import * as evmConfig from './evm-config.js'; + +const program = new Command(); import { color, fatal } from './utils/logging.js'; import open from 'open'; diff --git a/src/e-pr.ts b/src/e-pr.ts index fe06e4eb..6fdc270b 100644 --- a/src/e-pr.ts +++ b/src/e-pr.ts @@ -10,11 +10,13 @@ import * as querystring from 'node:querystring'; import extractZip from 'extract-zip'; import * as semver from 'semver'; -import { program } from 'commander'; +import { Command } from 'commander'; import * as inquirer from '@inquirer/prompts'; import { Octokit } from '@octokit/rest'; import debug from 'debug'; + +const program = new Command(); import { progressStream } from './utils/download.js'; import { getGitHubAuthToken } from './utils/github-auth.js'; import open from 'open'; diff --git a/src/e-rcv.ts b/src/e-rcv.ts index 8e38b542..52b1b6b6 100644 --- a/src/e-rcv.ts +++ b/src/e-rcv.ts @@ -7,7 +7,9 @@ import { styleText } from 'node:util'; import * as inquirer from '@inquirer/prompts'; import { Octokit } from '@octokit/rest'; -import { program } from 'commander'; +import { Command } from 'commander'; + +const program = new Command(); import * as evmConfig from './evm-config.js'; import { spawnSync, type DepotOpts } from './utils/depot-tools.js'; diff --git a/tests/e-cherry-pick.spec.mjs b/tests/e-cherry-pick.spec.mjs new file mode 100644 index 00000000..98e209ad --- /dev/null +++ b/tests/e-cherry-pick.spec.mjs @@ -0,0 +1,244 @@ +import { describe, expect, it } from 'vitest'; + +import { + commitSubject, + computeBatchId, + formatPRTitleAndBody, + isUrl, + splitPositionalArgs, +} from '../dist/e-cherry-pick.js'; + +const samplePatch = (subject) => + [ + 'From abc123def4567890 Mon Sep 17 00:00:00 2001', + 'From: Dev ', + 'Date: Mon, 1 Jan 2024 00:00:00 +0000', + `Subject: [PATCH] ${subject}`, + '', + 'Commit body line one.', + 'Commit body line two.', + '', + '---', + ' file.cc | 1 +', + ' 1 file changed, 1 insertion(+)', + ].join('\n'); + +describe('e-cherry-pick.isUrl', () => { + it('returns true for https urls', () => { + expect(isUrl('https://chromium-review.googlesource.com/c/v8/v8/+/2465830')).toBe(true); + }); + + it('returns true for http urls', () => { + expect(isUrl('http://example.com/foo')).toBe(true); + }); + + it('returns false for branch names and plain strings', () => { + expect(isUrl('main')).toBe(false); + expect(isUrl('28-x-y')).toBe(false); + expect(isUrl('')).toBe(false); + }); + + it('returns false for protocol-relative or ftp urls', () => { + expect(isUrl('//example.com')).toBe(false); + expect(isUrl('ftp://example.com')).toBe(false); + }); +}); + +describe('e-cherry-pick.commitSubject', () => { + it('extracts the subject line of a git format-patch', () => { + expect(commitSubject(samplePatch('fix: add guard for null input'))).toBe( + 'fix: add guard for null input', + ); + }); + + it('returns empty string when no subject is present', () => { + expect(commitSubject('no subject line here')).toBe(''); + }); + + it('trims trailing whitespace', () => { + const patch = 'Subject: [PATCH] trailing whitespace '; + expect(commitSubject(patch)).toBe('trailing whitespace'); + }); + + it('stops at end of line for single-line subjects', () => { + const patch = 'Subject: [PATCH] first line\nsecond line'; + expect(commitSubject(patch)).toBe('first line'); + }); +}); + +describe('e-cherry-pick.splitPositionalArgs', () => { + const URL_A = 'https://chromium-review.googlesource.com/c/v8/v8/+/1111'; + const URL_B = 'https://chromium-review.googlesource.com/c/chromium/src/+/2222'; + + it('keeps url-first, branch-second order as-is', () => { + expect(splitPositionalArgs(URL_A, 'main', [])).toEqual({ + patchUrls: [URL_A], + targetBranches: ['main'], + }); + }); + + it('swaps args when the user passes branch first, url second', () => { + expect(splitPositionalArgs('main', URL_A, [])).toEqual({ + patchUrls: [URL_A], + targetBranches: ['main'], + }); + }); + + it('treats additional url positionals as patches', () => { + expect(splitPositionalArgs(URL_A, 'main', [URL_B])).toEqual({ + patchUrls: [URL_A, URL_B], + targetBranches: ['main'], + }); + }); + + it('treats additional non-url positionals as target branches', () => { + expect(splitPositionalArgs(URL_A, 'main', ['28-x-y', '29-x-y'])).toEqual({ + patchUrls: [URL_A], + targetBranches: ['main', '28-x-y', '29-x-y'], + }); + }); + + it('splits mixed rest args into patches and branches', () => { + expect(splitPositionalArgs(URL_A, 'main', [URL_B, '28-x-y'])).toEqual({ + patchUrls: [URL_A, URL_B], + targetBranches: ['main', '28-x-y'], + }); + }); +}); + +describe('e-cherry-pick.computeBatchId', () => { + it('is deterministic for the same input', () => { + const urls = ['https://a.example/1', 'https://b.example/2']; + expect(computeBatchId(urls)).toBe(computeBatchId(urls)); + }); + + it('returns a 12-char hex string', () => { + const id = computeBatchId(['https://a.example/1']); + expect(id).toMatch(/^[0-9a-f]{12}$/); + }); + + it('changes when patch URL order changes', () => { + const a = computeBatchId(['https://a.example/1', 'https://b.example/2']); + const b = computeBatchId(['https://b.example/2', 'https://a.example/1']); + expect(a).not.toBe(b); + }); +}); + +describe('e-cherry-pick.formatPRTitleAndBody', () => { + it('builds a single-patch title and body with a bug number', () => { + const { title, body } = formatPRTitleAndBody({ + patches: [ + { + patchDirName: 'v8', + shortCommit: 'abc1234', + patch: samplePatch('fix: do the thing'), + bugNumber: '123456', + }, + ], + security: false, + }); + expect(title).toBe('chore: cherry-pick abc1234 from v8'); + expect(body).toContain('fix: do the thing'); + expect(body).toContain('Notes: Backported fix for 123456.'); + }); + + it('prefers CVE over bug number when security=true', () => { + const { body } = formatPRTitleAndBody({ + patches: [ + { + patchDirName: 'v8', + shortCommit: 'abc1234', + patch: samplePatch('fix: secure'), + bugNumber: '123456', + cve: 'CVE-2024-0001', + }, + ], + security: true, + }); + expect(body).toContain('Notes: Security: backported fix for CVE-2024-0001.'); + }); + + it('emits a placeholder note when no bug number is known', () => { + const { body } = formatPRTitleAndBody({ + patches: [ + { + patchDirName: 'v8', + shortCommit: 'abc1234', + patch: samplePatch('fix: mystery'), + }, + ], + security: false, + }); + expect(body).toContain(""); + }); + + it('builds a batch title and body with deduped dir names', () => { + const { title, body } = formatPRTitleAndBody({ + patches: [ + { + patchDirName: 'v8', + shortCommit: 'aaaaaaa', + patch: samplePatch('fix: a'), + bugNumber: '111', + }, + { + patchDirName: 'v8', + shortCommit: 'bbbbbbb', + patch: samplePatch('fix: b'), + cve: 'CVE-2024-0002', + }, + { + patchDirName: 'chromium', + shortCommit: 'ccccccc', + patch: samplePatch('fix: c'), + }, + ], + security: false, + }); + expect(title).toBe('chore: cherry-pick 3 changes from v8, chromium'); + expect(body).toContain('* aaaaaaa from v8 — fix: a (111)'); + expect(body).toContain('* bbbbbbb from v8 — fix: b (CVE-2024-0002)'); + expect(body).toContain('* ccccccc from chromium — fix: c (ccccccc)'); + expect(body).toContain('Notes: Backported fixes for 111, CVE-2024-0002.'); + }); + + it('marks batch notes as security when security=true', () => { + const { body } = formatPRTitleAndBody({ + patches: [ + { + patchDirName: 'v8', + shortCommit: 'aaaaaaa', + patch: samplePatch('fix: a'), + cve: 'CVE-2024-0003', + }, + { + patchDirName: 'v8', + shortCommit: 'bbbbbbb', + patch: samplePatch('fix: b'), + cve: 'CVE-2024-0004', + }, + ], + security: true, + }); + expect(body).toContain('Notes: Security: backported fixes for CVE-2024-0003, CVE-2024-0004.'); + }); + + it('emits a batch placeholder when no bugs or CVEs are known', () => { + const { body } = formatPRTitleAndBody({ + patches: [ + { + patchDirName: 'v8', + shortCommit: 'aaaaaaa', + patch: samplePatch('fix: a'), + }, + { + patchDirName: 'v8', + shortCommit: 'bbbbbbb', + patch: samplePatch('fix: b'), + }, + ], + security: false, + }); + expect(body).toContain(""); + }); +});