-
Notifications
You must be signed in to change notification settings - Fork 123
fix(git-node): handle multi-line trailers #1062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if there's a better way than spawning a git process per line until the trailers are completely identified. Doesn't
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fair, I've added a shortcut so it skips all the lines until it finds an empty string, so most (all?) commit messages only need 2 spawn of |
||
|
|
||
| // 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')) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,6 +112,26 @@ describe('LandingSession.prototype.generateAmendedMessage', () => { | |
| t.assert.strictEqual(result, '\nPR-URL: http://example.com/123\nReviewed-By: user1 <[email protected]>'); | ||
| }); | ||
|
|
||
| it('should handle multi-line trailers', async(t) => { | ||
| const session = createSession(); | ||
| const result = await session.generateAmendedMessage( | ||
| 'subsystem: foobar\n\nSigned-off-by: user1\n <[email protected]>\n' | ||
| ); | ||
|
|
||
| t.assert.strictEqual(result, | ||
| 'subsystem: foobar\n\nSigned-off-by: user1 <[email protected]>\nPR-URL: http://example.com/123\nReviewed-By: user1 <[email protected]>'); | ||
| }); | ||
|
|
||
| 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: user1\n <[email protected]>\n' | ||
| ); | ||
|
|
||
| t.assert.strictEqual(result, | ||
| 'subsystem: foobar\n\nNot-A-Trailer: http://example.com/\n\nSigned-off-by: user1 <[email protected]>\nPR-URL: http://example.com/123\nReviewed-By: user1 <[email protected]>'); | ||
| }); | ||
|
|
||
| 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably we could skip this step if
originalTrailersis empty?