From 25dfe16ca4009c6d745cdf468e9b7cb39fb38dfc Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 14 Apr 2026 09:09:47 +0200 Subject: [PATCH 1/3] fix(git-node): handle single-line commit messages --- lib/landing_session.js | 70 +++++++++++------ test/unit/amend.test.js | 170 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 214 insertions(+), 26 deletions(-) create mode 100644 test/unit/amend.test.js diff --git a/lib/landing_session.js b/lib/landing_session.js index 8e1cfa44..147b9ad1 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -306,25 +306,14 @@ export default class LandingSession extends Session { await this.tryCompleteLanding(commitInfo); } - async amend() { - const { cli } = this; - if (!this.readyToAmend()) { - cli.warn('Not yet ready to amend, run `git node land --abort`'); - return; - } + async generateAmendedMessage(original) { + const { cli, metadata } = this; 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+)$/im; - this.startAmending(); - - 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. @@ -332,22 +321,35 @@ export default class LandingSession extends Session { 'interpret-trailers', '--parse', '--no-divider' ], { input: `${original}\n` - }); + }).split('\n').map(trailer => { + const separatorIndex = trailer.indexOf(':'); + return [trailer.slice(0, separatorIndex), trailer.slice(separatorIndex + 2)]; + }).slice(0, -1); // Remove the last line return entry + const containCVETrailer = CVE_RE.test(originalTrailers); - const metadata = this.metadata.trim().split('\n'); - const amended = original.slice(0, -originalTrailers.length).trim().split('\n'); + const filterTrailer = (line) => ([key]) => + line.startsWith(key) && + new RegExp(`^${RegExp.escape(key)}\\s*:`).test(line); + const amended = original.trim().split('\n') + .filter(line => + !line.includes(':') || + !originalTrailers.some(filterTrailer(line))); + for (let i = amended.length - 1; amended[i] === ''; i--) { + amended.pop(); + } // Only keep existing trailers that we won't add ourselves - const keptTrailers = originalTrailers.split('\n').filter(line => - !!line && - (!PR_RE.test(line) || this.backport) && - !BACKPORT_RE.test(line) && - !REVIEW_RE.test(line)); - amended.push('', ...keptTrailers); - - for (const line of metadata) { - if (keptTrailers.includes(line)) { + const trailersToFilterOut = ['BACKPORT-PR-URL', 'REVIEWED-BY']; + if (!this.backport) trailersToFilterOut.push('PR-URL'); + const keptTrailers = originalTrailers.filter( + ([key]) => !trailersToFilterOut.includes(key.toUpperCase()) + ); + amended.push('', ...keptTrailers.map(([key, value]) => `${key}: ${value}`)); + + for (const line of metadata.trim().split('\n')) { + const foundMatchingTrailer = keptTrailers.find(filterTrailer(line)); + if (foundMatchingTrailer && line === foundMatchingTrailer.join(': ')) { cli.warn(`Found ${line}, skipping..`); continue; } @@ -399,8 +401,24 @@ export default class LandingSession extends Session { amended.push('CVE-ID: ' + cveID); } } + return amended.join('\n'); + } + + async amend() { + const { cli } = this; + if (!this.readyToAmend()) { + cli.warn('Not yet ready to amend, run `git node land --abort`'); + return; + } + + this.startAmending(); + + const rev = this.getCurrentRev(); + const original = runSync('git', [ + 'show', 'HEAD', '-s', '--format=%B' + ]).trim(); - const message = amended.join('\n'); + const message = await this.generateAmendedMessage(original); const messageFile = this.saveMessage(rev, message); cli.separator('New Message'); cli.log(message.trim()); diff --git a/test/unit/amend.test.js b/test/unit/amend.test.js new file mode 100644 index 00000000..efa2faa6 --- /dev/null +++ b/test/unit/amend.test.js @@ -0,0 +1,170 @@ +import { describe, it } from 'node:test'; +import LandingSession from '../../lib/landing_session.js'; + +const createMockCli = () => ({ + prompt: () => Promise.resolve(false), + ok: () => {}, + warn: () => {}, + info: () => {}, + log: () => {}, + separator: () => {}, + startSpinner: () => ({ stop: () => {} }), + stopSpinner: () => {}, + setExitCode: () => {} +}); + +const createSession = (overrides = undefined) => { + const options = { + owner: 'nodejs', + repo: 'node', + upstream: 'upstream', + branch: 'main', + prid: 123, + oneCommitMax: false, + backport: false, + ...overrides + }; + const session = new LandingSession(createMockCli(), {}, '/', options); + + let metadata = `${options.backport ? 'Backport-' : ''}PR-URL: http://example.com/${options.prid}\nReviewed-By: user1 \n`; + + if (options.metadata) { + metadata += options.metadata; + } + + return Object.defineProperty(session, 'metadata', { + __proto__: null, + configurable: true, + value: metadata + }); +}; + +describe('LandingSession.prototype.generateAmendedMessage', () => { + it('should append PR-URL when there are no trailers', async(t) => { + const session = createSession(); + const result = await session.generateAmendedMessage('foo: bar'); + + t.assert.strictEqual( + result, + 'foo: bar\n\nPR-URL: http://example.com/123\nReviewed-By: user1 ' + ); + }); + + it('should not duplicate trailers that are in metadata', async(t) => { + const session = createSession({ metadata: 'Refs: http://example.com/321\nRefs: http://example.com/456' }); + const result = await session.generateAmendedMessage('foo: bar\n\nRefs: http://example.com/321'); + + t.assert.strictEqual( + result, + 'foo: bar\n\nRefs: http://example.com/321\nPR-URL: http://example.com/123\nReviewed-By: user1 \nRefs: http://example.com/456' + ); + }); + + it('should strip trailers added by NCU', async(t) => { + const session = createSession(); + const result = await session.generateAmendedMessage( + 'subsystem: fix bug\n\nReviewed-By: user1 \nPR-URL: http://example.com/123\nBackport-PR-URL: http://example.com/321\nOther-Trailer: Value\n' + ); + + t.assert.strictEqual( + result, + 'subsystem: fix bug\n\nOther-Trailer: Value\nPR-URL: http://example.com/123\nReviewed-By: user1 ' + ); + }); + + it('should strip trailers added by NCU regardless of case', async(t) => { + const session = createSession(); + const result = await session.generateAmendedMessage( + 'subsystem: fix bug\n\nReViEWed-bY: user1 \npr-url: http://example.com/123\nBACKPORT-PR-URL: http://example.com/321\nOther-Trailer: Value\n' + ); + + t.assert.strictEqual( + result, + 'subsystem: fix bug\n\nOther-Trailer: Value\nPR-URL: http://example.com/123\nReviewed-By: user1 ' + ); + }); + + it('should not strip PR-URL trailer when backporting', async(t) => { + const session = createSession({ backport: true, prid: 456 }); + const result = await session.generateAmendedMessage( + 'subsystem: foobar\n\nOther-Trailer: Value\nPR-URL: http://example.com/999\nReviewed-By: foobar \n' + ); + + t.assert.strictEqual( + result, + 'subsystem: foobar\n\nOther-Trailer: Value\nPR-URL: http://example.com/999\nBackport-PR-URL: http://example.com/456\nReviewed-By: user1 ' + ); + }); + + it('should clean-up trailers with extra space', async(t) => { + const session = createSession(); + const result = await session.generateAmendedMessage( + 'subsystem: foobar\n\n\nTrailer: Value \n\n\n' + ); + + t.assert.strictEqual(result, 'subsystem: foobar\n\nTrailer: Value\nPR-URL: http://example.com/123\nReviewed-By: user1 '); + }); + + it('should handle empty message', async(t) => { + const session = createSession(); + const result = await session.generateAmendedMessage(''); + + t.assert.strictEqual(result, '\nPR-URL: http://example.com/123\nReviewed-By: user1 '); + }); + + it('should handle cherry-pick from upstream', async(t) => { + const session = createSession({ metadata: 'Refs: https://github.com/v8/v8/commit/cf1bce40a5ef4c7c1da351754f5bf526c0c96463\n' }); + const result = await session.generateAmendedMessage(`deps: V8: cherry-pick cf1bce40a5ef + +Original commit message: + + [wasm] Fix S128Const on big endian + + Since http://crrev.com/c/2944437 globals are no longer little endian + enforced. + + S128Const handling in the initializer needs to take this into account + and byte reverse values which are hard coded in little endian order. + + This is currently causing failures on Node.js upstream: + https://github.com/nodejs/node/pull/59034#issuecomment-4129144461 + + Change-Id: Ifcc9ade93ee51565ab19b16e9dadf0ff5752f7a6 + Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7704213 + Commit-Queue: Milad Farazmand + Reviewed-by: Manos Koukoutos + Cr-Commit-Position: refs/heads/main@{#106082} + +PR-URL: https://github.com/nodejs/node/pull/62449 +Refs: https://github.com/v8/v8/commit/cf1bce40a5ef4c7c1da351754f5bf526c0c96463 +Reviewed-By: Guy Bedford +Reviewed-By: Luigi Pinca +`); + + t.assert.strictEqual(result, `deps: V8: cherry-pick cf1bce40a5ef + +Original commit message: + + [wasm] Fix S128Const on big endian + + Since http://crrev.com/c/2944437 globals are no longer little endian + enforced. + + S128Const handling in the initializer needs to take this into account + and byte reverse values which are hard coded in little endian order. + + This is currently causing failures on Node.js upstream: + https://github.com/nodejs/node/pull/59034#issuecomment-4129144461 + + Change-Id: Ifcc9ade93ee51565ab19b16e9dadf0ff5752f7a6 + Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7704213 + Commit-Queue: Milad Farazmand + Reviewed-by: Manos Koukoutos + Cr-Commit-Position: refs/heads/main@{#106082} + +Refs: https://github.com/v8/v8/commit/cf1bce40a5ef4c7c1da351754f5bf526c0c96463 +PR-URL: http://example.com/123 +Reviewed-By: user1 ` + ); + }); +}); From be03357d742759ab9ab6708eb60f4a32574c35c2 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 14 Apr 2026 09:18:25 +0200 Subject: [PATCH 2/3] fixup! fix(git-node): handle single-line commit messages --- test/unit/amend.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/amend.test.js b/test/unit/amend.test.js index efa2faa6..c205c8df 100644 --- a/test/unit/amend.test.js +++ b/test/unit/amend.test.js @@ -17,7 +17,7 @@ const createSession = (overrides = undefined) => { const options = { owner: 'nodejs', repo: 'node', - upstream: 'upstream', + upstream: 'origin', branch: 'main', prid: 123, oneCommitMax: false, From d941035e4f29da38b426ba4a0a324ace387dfc49 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 14 Apr 2026 09:21:45 +0200 Subject: [PATCH 3/3] fixup! fix(git-node): handle single-line commit messages --- lib/landing_session.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/landing_session.js b/lib/landing_session.js index 147b9ad1..89d6e400 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -330,7 +330,7 @@ export default class LandingSession extends Session { const filterTrailer = (line) => ([key]) => line.startsWith(key) && - new RegExp(`^${RegExp.escape(key)}\\s*:`).test(line); + new RegExp(`^${RegExp.escape?.(key) ?? key}\\s*:`).test(line); const amended = original.trim().split('\n') .filter(line => !line.includes(':') ||