From 4cfd1a955b06969b7449b9d257505096d457d715 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sat, 5 Apr 2025 17:02:33 +0200 Subject: [PATCH 1/5] chore: remove duplicated code --- lib/cherry_pick.js | 193 +++-------------------------------------- lib/landing_session.js | 16 +++- 2 files changed, 29 insertions(+), 180 deletions(-) diff --git a/lib/cherry_pick.js b/lib/cherry_pick.js index 78755a61..2c98f6e2 100644 --- a/lib/cherry_pick.js +++ b/lib/cherry_pick.js @@ -1,23 +1,13 @@ -import os from 'node:os'; import path from 'node:path'; import { getMetadata } from '../components/metadata.js'; import { - runAsync, runSync, forceRunAsync + runAsync, runSync } from './run.js'; -import { writeFile } from './file.js'; -import { - shortSha, getEditor -} from './utils.js'; import { getNcuDir } from './config.js'; +import LandingSession, { LINT_RESULTS } from './landing_session.js'; -const LINT_RESULTS = { - SKIPPED: 'skipped', - FAILED: 'failed', - SUCCESS: 'success' -}; - -export default class CheckPick { +export default class CherryPick { constructor(prid, dir, cli, { owner, repo, @@ -73,16 +63,6 @@ export default class CheckPick { return path.resolve(this.ncuDir, `${this.prid}`); } - getMessagePath(rev) { - return path.resolve(this.pullDir, `${shortSha(rev)}.COMMIT_EDITMSG`); - } - - saveMessage(rev, message) { - const file = this.getMessagePath(rev); - writeFile(file, message); - return file; - } - async start() { const { cli } = this; @@ -120,68 +100,6 @@ export default class CheckPick { } } - async downloadAndPatch(expectedCommitShas) { - const { cli, repo, owner, prid } = this; - - cli.startSpinner(`Downloading patch for ${prid}`); - // fetch via ssh to handle private repo - await runAsync('git', [ - 'fetch', `git@github.com:${owner}/${repo}.git`, - `refs/pull/${prid}/merge`]); - // We fetched the commit that would result if we used `git merge`. - // ^1 and ^2 refer to the PR base and the PR head, respectively. - const [base, head] = await runAsync('git', - ['rev-parse', 'FETCH_HEAD^1', 'FETCH_HEAD^2'], - { captureStdout: 'lines' }); - const commitShas = await runAsync('git', - ['rev-list', `${base}..${head}`], - { captureStdout: 'lines' }); - cli.stopSpinner(`Fetched commits as ${shortSha(base)}..${shortSha(head)}`); - cli.separator(); - - const mismatchedCommits = [ - ...commitShas.filter((sha) => !expectedCommitShas.includes(sha)) - .map((sha) => `Unexpected commit ${sha}`), - ...expectedCommitShas.filter((sha) => !commitShas.includes(sha)) - .map((sha) => `Missing commit ${sha}`) - ].join('\n'); - if (mismatchedCommits.length > 0) { - throw new Error(`Mismatched commits:\n${mismatchedCommits}`); - } - - const commitInfo = { base, head, shas: commitShas }; - - try { - await forceRunAsync('git', ['cherry-pick', `${base}..${head}`], { - ignoreFailure: false - }); - } catch (ex) { - await forceRunAsync('git', ['cherry-pick', '--abort']); - throw new Error('Failed to apply patches'); - } - - cli.ok('Patches applied'); - return commitInfo; - } - - async validateLint() { - // The linter is currently only run on non-Windows platforms. - if (os.platform() === 'win32') { - return LINT_RESULTS.SKIPPED; - } - - if (!this.lint) { - return LINT_RESULTS.SKIPPED; - } - - try { - await runAsync('make', ['lint']); - return LINT_RESULTS.SUCCESS; - } catch { - return LINT_RESULTS.FAILED; - } - } - async amend(metadata, commitInfo) { const { cli } = this; const subjects = await runAsync('git', @@ -203,102 +121,19 @@ export default class CheckPick { await runAsync('git', ['commit', '--amend', '--no-edit']); } - return this._amend(metadata); + return LandingSession.prototype.amend.call(this, metadata); } - async _amend(metadataStr) { - const { cli } = this; - - const rev = this.getCurrentRev(); - const original = runSync('git', [ - 'show', 'HEAD', '-s', '--format=%B' - ]).trim(); - // git has very specific rules about what is a trailer and what is not. - // Instead of trying to implement those ourselves, let git parse the - // original commit message and see if it outputs any trailers. - const originalHasTrailers = runSync('git', [ - 'interpret-trailers', '--parse', '--no-divider' - ], { - input: `${original}\n` - }).trim().length !== 0; - const metadata = metadataStr.trim().split('\n'); - const amended = original.split('\n'); - - // If the original commit message already contains trailers (such as - // "Co-authored-by"), we simply add our own metadata after those. Otherwise, - // we have to add an empty line so that git recognizes our own metadata as - // trailers in the amended commit message. - if (!originalHasTrailers) { - amended.push(''); - } - - const BACKPORT_RE = /BACKPORT-PR-URL\s*:\s*(\S+)/i; - const PR_RE = /PR-URL\s*:\s*(\S+)/i; - const REVIEW_RE = /Reviewed-By\s*:\s*(\S+)/i; - const CVE_RE = /CVE-ID\s*:\s*(\S+)/i; - - let containCVETrailer = false; - for (const line of metadata) { - if (line.length !== 0 && original.includes(line)) { - if (line.match(CVE_RE)) { - containCVETrailer = true; - } - if (originalHasTrailers) { - cli.warn(`Found ${line}, skipping..`); - } else { - throw new Error( - 'Git found no trailers in the original commit message, ' + - `but '${line}' is present and should be a trailer.`); - } - } else { - if (line.match(BACKPORT_RE)) { - let prIndex = amended.findIndex(datum => datum.match(PR_RE)); - if (prIndex === -1) { - prIndex = amended.findIndex(datum => datum.match(REVIEW_RE)) - 1; - } - amended.splice(prIndex + 1, 0, line); - } else { - amended.push(line); - } - } - } - - if (!containCVETrailer && this.includeCVE) { - const cveID = await cli.prompt( - 'Git found no CVE-ID trailer in the original commit message. ' + - 'Please, provide the CVE-ID', - { questionType: 'input', defaultAnswer: 'CVE-2023-XXXXX' } - ); - amended.push('CVE-ID: ' + cveID); - } - - const message = amended.join('\n'); - const messageFile = this.saveMessage(rev, message); - cli.separator('New Message'); - cli.log(message.trim()); - const takeMessage = await cli.prompt('Use this message?'); - if (takeMessage) { - await runAsync('git', ['commit', '--amend', '-F', messageFile]); - return true; - } + readyToAmend() { + return true; + } - const editor = await getEditor({ git: true }); - if (editor) { - try { - await forceRunAsync( - editor, - [`"${messageFile}"`], - { ignoreFailure: false, spawnArgs: { shell: true } } - ); - await runAsync('git', ['commit', '--amend', '-F', messageFile]); - return true; - } catch { - cli.warn(`Please manually edit ${messageFile}, then run\n` + - `\`git commit --amend -F ${messageFile}\` ` + - 'to finish amending the message'); - throw new Error( - 'Failed to edit the message using the configured editor'); - } - } + startAmending() { + // No-op } } + +CherryPick.prototype.downloadAndPatch = LandingSession.prototype.downloadAndPatch; +CherryPick.prototype.validateLint = LandingSession.prototype.validateLint; +CherryPick.prototype.getMessagePath = LandingSession.prototype.getMessagePath; +CherryPick.prototype.saveMessage = LandingSession.prototype.saveMessage; diff --git a/lib/landing_session.js b/lib/landing_session.js index 7c86efc5..88282430 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -10,7 +10,7 @@ import { const isWindows = process.platform === 'win32'; -const LINT_RESULTS = { +export const LINT_RESULTS = { SKIPPED: 'skipped', FAILED: 'failed', SUCCESS: 'success' @@ -315,9 +315,14 @@ export default class LandingSession extends Session { const BACKPORT_RE = /BACKPORT-PR-URL\s*:\s*(\S+)/i; const PR_RE = /PR-URL\s*:\s*(\S+)/i; const REVIEW_RE = /Reviewed-By\s*:\s*(\S+)/i; + const CVE_RE = /CVE-ID\s*:\s*(\S+)/i; + let containCVETrailer = false; for (const line of metadata) { if (line.length !== 0 && original.includes(line)) { + if (line.match(CVE_RE)) { + containCVETrailer = true; + } if (originalHasTrailers) { cli.warn(`Found ${line}, skipping..`); } else { @@ -338,6 +343,15 @@ export default class LandingSession extends Session { } } + if (!containCVETrailer && this.includeCVE) { + const cveID = await cli.prompt( + 'Git found no CVE-ID trailer in the original commit message. ' + + 'Please, provide the CVE-ID', + { questionType: 'input', defaultAnswer: 'CVE-2023-XXXXX' } + ); + amended.push('CVE-ID: ' + cveID); + } + const message = amended.join('\n'); const messageFile = this.saveMessage(rev, message); cli.separator('New Message'); From 4cc321f0e82548951aa8123de89ec0b198ab4cd9 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 6 Apr 2025 11:58:54 +0200 Subject: [PATCH 2/5] fix `downloadAndPatch` --- lib/cherry_pick.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cherry_pick.js b/lib/cherry_pick.js index 2c98f6e2..0b859fb1 100644 --- a/lib/cherry_pick.js +++ b/lib/cherry_pick.js @@ -71,7 +71,7 @@ export default class CherryPick { owner: this.owner, repo: this.repo }, false, cli); - const expectedCommitShas = + this.expectedCommitShas = metadata.data.commits.map(({ commit }) => commit.oid); const amend = await cli.prompt( @@ -84,7 +84,7 @@ export default class CherryPick { } try { - const commitInfo = await this.downloadAndPatch(expectedCommitShas); + const commitInfo = await this.downloadAndPatch(); const cleanLint = await this.validateLint(); if (cleanLint === LINT_RESULTS.FAILED) { cli.error('Patch still contains lint errors. ' + From 37d63039ad534f7bb74d7dcc3a15f60c1ca1d5ff Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 6 Apr 2025 12:00:04 +0200 Subject: [PATCH 3/5] Update cherry_pick.js --- lib/cherry_pick.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/cherry_pick.js b/lib/cherry_pick.js index 0b859fb1..3af01426 100644 --- a/lib/cherry_pick.js +++ b/lib/cherry_pick.js @@ -131,6 +131,10 @@ export default class CherryPick { startAmending() { // No-op } + + saveCommitInfo() { + // No-op + } } CherryPick.prototype.downloadAndPatch = LandingSession.prototype.downloadAndPatch; From 3a4eca36d092440f7fb7b4008993f1a9dccd3082 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 6 Apr 2025 16:03:00 +0200 Subject: [PATCH 4/5] Update cherry_pick.js --- lib/cherry_pick.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/cherry_pick.js b/lib/cherry_pick.js index 3af01426..a45405f3 100644 --- a/lib/cherry_pick.js +++ b/lib/cherry_pick.js @@ -36,11 +36,6 @@ export default class CherryPick { return this.options.lint; } - getUpstreamHead() { - const { upstream, branch } = this; - return runSync('git', ['rev-parse', `${upstream}/${branch}`]).trim(); - } - getCurrentRev() { return runSync('git', ['rev-parse', 'HEAD']).trim(); } From c7fa478050140a53f2ac5ceda2593731c73af366 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 6 Apr 2025 16:03:25 +0200 Subject: [PATCH 5/5] Update landing_session.js --- lib/landing_session.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/landing_session.js b/lib/landing_session.js index 88282430..40e60e8f 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -84,11 +84,11 @@ export default class LandingSession extends Session { } async downloadAndPatch() { - const { cli, repo, owner, prid, expectedCommitShas } = this; + const { cli, upstream, prid, expectedCommitShas } = this; cli.startSpinner(`Downloading patch for ${prid}`); await runAsync('git', [ - 'fetch', `https://github.com/${owner}/${repo}.git`, + 'fetch', upstream, `refs/pull/${prid}/merge`]); // We fetched the commit that would result if we used `git merge`. // ^1 and ^2 refer to the PR base and the PR head, respectively.