From 127ba36b30cb8b0284f1f5f85f7e20c390950c20 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 15 Apr 2026 12:12:38 +0200 Subject: [PATCH 1/3] fix(git-node): handle multi-line trailers --- lib/landing_session.js | 38 +++++++++++++++++++++++--------------- test/unit/amend.test.js | 20 ++++++++++++++++++++ 2 files changed, 43 insertions(+), 15 deletions(-) diff --git a/lib/landing_session.js b/lib/landing_session.js index 89d6e400..4d49cbaa 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -316,35 +316,43 @@ export default class LandingSession extends Session { // 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 originalTrailers = runSync('git', [ + // commit message and see if it outputs any trailers. + const interpretTrailers = commitMessage => runSync('git', [ '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 + input: `${commitMessage}\n` + }); + const originalTrailers = interpretTrailers(original); const containCVETrailer = CVE_RE.test(originalTrailers); const filterTrailer = (line) => ([key]) => line.startsWith(key) && new RegExp(`^${RegExp.escape?.(key) ?? 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--) { + const amended = original.trim().split('\n'); + const stillInTrailers = () => { + const result = interpretTrailers(amended.join('\n')); + return result.length && originalTrailers.startsWith(result.trim()); + } + for (let i = amended.length - 1; amended[i] === '' || stillInTrailers(); i--) { + // Remove last line until git no longer detects any trailers amended.pop(); } // Only keep existing trailers that we won't add ourselves const trailersToFilterOut = ['BACKPORT-PR-URL', 'REVIEWED-BY']; if (!this.backport) trailersToFilterOut.push('PR-URL'); - const keptTrailers = originalTrailers.filter( - ([key]) => !trailersToFilterOut.includes(key.toUpperCase()) - ); + const keptTrailers = + originalTrailers + .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 + .filter( + ([key]) => !trailersToFilterOut.includes(key.toUpperCase()) + ); amended.push('', ...keptTrailers.map(([key, value]) => `${key}: ${value}`)); for (const line of metadata.trim().split('\n')) { diff --git a/test/unit/amend.test.js b/test/unit/amend.test.js index c205c8df..93c67e80 100644 --- a/test/unit/amend.test.js +++ b/test/unit/amend.test.js @@ -112,6 +112,26 @@ describe('LandingSession.prototype.generateAmendedMessage', () => { t.assert.strictEqual(result, '\nPR-URL: http://example.com/123\nReviewed-By: user1 '); }); + it('should handle multi-line trailers', async(t) => { + const session = createSession(); + const result = await session.generateAmendedMessage( + 'subsystem: foobar\n\nSigned-off-by: Mike McCready\n <66998419+MikeMcC399@users.noreply.github.com>\n' + ); + + t.assert.strictEqual(result, + 'subsystem: foobar\n\nSigned-off-by: Mike McCready <66998419+MikeMcC399@users.noreply.github.com>\nPR-URL: http://example.com/123\nReviewed-By: user1 '); + }); + + it('should not remove lines that look like trailers in the commit body', async(t) => { + const session = createSession(); + const result = await session.generateAmendedMessage( + 'subsystem: foobar\n\nNot-A-Trailer: http://example.com/\n\nSigned-off-by: Mike McCready\n <66998419+MikeMcC399@users.noreply.github.com>\n' + ); + + t.assert.strictEqual(result, + 'subsystem: foobar\n\nNot-A-Trailer: http://example.com/\n\nSigned-off-by: Mike McCready <66998419+MikeMcC399@users.noreply.github.com>\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 From 1e929e2915e945d0c6ae4f17a4d03e8328bde788 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 15 Apr 2026 12:21:18 +0200 Subject: [PATCH 2/3] fixup! fix(git-node): handle multi-line trailers --- lib/landing_session.js | 2 +- test/unit/amend.test.js | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/landing_session.js b/lib/landing_session.js index 4d49cbaa..1e56d835 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -333,7 +333,7 @@ export default class LandingSession extends Session { const stillInTrailers = () => { const result = interpretTrailers(amended.join('\n')); return result.length && originalTrailers.startsWith(result.trim()); - } + }; for (let i = amended.length - 1; amended[i] === '' || stillInTrailers(); i--) { // Remove last line until git no longer detects any trailers amended.pop(); diff --git a/test/unit/amend.test.js b/test/unit/amend.test.js index 93c67e80..1a23d2f7 100644 --- a/test/unit/amend.test.js +++ b/test/unit/amend.test.js @@ -115,21 +115,21 @@ describe('LandingSession.prototype.generateAmendedMessage', () => { it('should handle multi-line trailers', async(t) => { const session = createSession(); const result = await session.generateAmendedMessage( - 'subsystem: foobar\n\nSigned-off-by: Mike McCready\n <66998419+MikeMcC399@users.noreply.github.com>\n' + 'subsystem: foobar\n\nSigned-off-by: user1\n \n' ); t.assert.strictEqual(result, - 'subsystem: foobar\n\nSigned-off-by: Mike McCready <66998419+MikeMcC399@users.noreply.github.com>\nPR-URL: http://example.com/123\nReviewed-By: user1 '); + 'subsystem: foobar\n\nSigned-off-by: user1 \nPR-URL: http://example.com/123\nReviewed-By: user1 '); }); it('should not remove lines that look like trailers in the commit body', async(t) => { const session = createSession(); const result = await session.generateAmendedMessage( - 'subsystem: foobar\n\nNot-A-Trailer: http://example.com/\n\nSigned-off-by: Mike McCready\n <66998419+MikeMcC399@users.noreply.github.com>\n' + 'subsystem: foobar\n\nNot-A-Trailer: http://example.com/\n\nSigned-off-by: user1\n \n' ); t.assert.strictEqual(result, - 'subsystem: foobar\n\nNot-A-Trailer: http://example.com/\n\nSigned-off-by: Mike McCready <66998419+MikeMcC399@users.noreply.github.com>\nPR-URL: http://example.com/123\nReviewed-By: user1 '); + 'subsystem: foobar\n\nNot-A-Trailer: http://example.com/\n\nSigned-off-by: user1 \nPR-URL: http://example.com/123\nReviewed-By: user1 '); }); it('should handle cherry-pick from upstream', async(t) => { From ca131b7fadea073af68f6da61a4892fea175d8b7 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Fri, 17 Apr 2026 18:52:45 +0200 Subject: [PATCH 3/3] fixup! fix(git-node): handle multi-line trailers --- lib/landing_session.js | 37 +++++++++++++++++++------------------ test/unit/amend.test.js | 4 ++-- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/lib/landing_session.js b/lib/landing_session.js index 1e56d835..4bf1d6a3 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -326,24 +326,25 @@ export default class LandingSession extends Session { const originalTrailers = interpretTrailers(original); const containCVETrailer = CVE_RE.test(originalTrailers); - const filterTrailer = (line) => ([key]) => - line.startsWith(key) && - new RegExp(`^${RegExp.escape?.(key) ?? key}\\s*:`).test(line); const amended = original.trim().split('\n'); - const stillInTrailers = () => { - const result = interpretTrailers(amended.join('\n')); - return result.length && originalTrailers.startsWith(result.trim()); - }; - for (let i = amended.length - 1; amended[i] === '' || stillInTrailers(); i--) { - // Remove last line until git no longer detects any trailers - amended.pop(); - } + let keptTrailers = []; + if (originalTrailers) { + const stillInTrailers = () => { + const result = interpretTrailers(amended.join('\n')); + return result.length && originalTrailers.startsWith(result.trim()); + }; + // Remove all lines until we find an empty string, which should get of all + // trailers in most cases. + amended.splice(amended.lastIndexOf(''), Infinity); + for (let i = amended.length - 1; amended[i] === '' || stillInTrailers(); i--) { + // Remove last line until git no longer detects any trailers + amended.pop(); + } - // Only keep existing trailers that we won't add ourselves - const trailersToFilterOut = ['BACKPORT-PR-URL', 'REVIEWED-BY']; - if (!this.backport) trailersToFilterOut.push('PR-URL'); - const keptTrailers = - originalTrailers + // Only keep existing trailers that we won't add ourselves + const trailersToFilterOut = ['BACKPORT-PR-URL', 'REVIEWED-BY']; + if (!this.backport) trailersToFilterOut.push('PR-URL'); + keptTrailers = originalTrailers .split('\n') .map(trailer => { const separatorIndex = trailer.indexOf(':'); @@ -353,11 +354,11 @@ export default class LandingSession extends Session { .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(': ')) { + if (keptTrailers.some((trailer) => line.toUpperCase() === trailer.join(': ').toUpperCase())) { cli.warn(`Found ${line}, skipping..`); continue; } diff --git a/test/unit/amend.test.js b/test/unit/amend.test.js index 1a23d2f7..4d1591a2 100644 --- a/test/unit/amend.test.js +++ b/test/unit/amend.test.js @@ -105,11 +105,11 @@ describe('LandingSession.prototype.generateAmendedMessage', () => { 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) => { + it('should handle empty message (although not supported)', async(t) => { const session = createSession(); const result = await session.generateAmendedMessage(''); - t.assert.strictEqual(result, '\nPR-URL: http://example.com/123\nReviewed-By: user1 '); + t.assert.strictEqual(result, '\n\nPR-URL: http://example.com/123\nReviewed-By: user1 '); }); it('should handle multi-line trailers', async(t) => {