Skip to content

Commit f510b8a

Browse files
aduh95aduh95
authored
fix(git-node): handle single-line commit messages (#1059)
1 parent a9048c8 commit f510b8a

File tree

2 files changed

+214
-26
lines changed

2 files changed

+214
-26
lines changed

‎lib/landing_session.js‎

Lines changed: 44 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -306,48 +306,50 @@ export default class LandingSession extends Session {
306306
await this.tryCompleteLanding(commitInfo);
307307
}
308308

309-
async amend() {
310-
const { cli } = this;
311-
if (!this.readyToAmend()) {
312-
cli.warn('Not yet ready to amend, run `git node land --abort`');
313-
return;
314-
}
309+
async generateAmendedMessage(original) {
310+
const { cli, metadata } = this;
315311

316312
const BACKPORT_RE = /^BACKPORT-PR-URL\s*:\s*(\S+)$/i;
317313
const PR_RE = /^PR-URL\s*:\s*(\S+)$/i;
318314
const REVIEW_RE = /^Reviewed-By\s*:\s*(\S+)$/i;
319315
const CVE_RE = /^CVE-ID\s*:\s*(\S+)$/im;
320316

321-
this.startAmending();
322-
323-
const rev = this.getCurrentRev();
324-
const original = runSync('git', [
325-
'show', 'HEAD', '-s', '--format=%B'
326-
]).trim();
327-
328317
// git has very specific rules about what is a trailer and what is not.
329318
// Instead of trying to implement those ourselves, let git parse the
330319
// original commit message and see if it outputs any trailers.
331320
const originalTrailers = runSync('git', [
332321
'interpret-trailers', '--parse', '--no-divider'
333322
], {
334323
input: `${original}\n`
335-
});
324+
}).split('\n').map(trailer => {
325+
const separatorIndex = trailer.indexOf(':');
326+
return [trailer.slice(0, separatorIndex), trailer.slice(separatorIndex + 2)];
327+
}).slice(0, -1); // Remove the last line return entry
328+
336329
const containCVETrailer = CVE_RE.test(originalTrailers);
337330

338-
const metadata = this.metadata.trim().split('\n');
339-
const amended = original.slice(0, -originalTrailers.length).trim().split('\n');
331+
const filterTrailer = (line) => ([key]) =>
332+
line.startsWith(key) &&
333+
new RegExp(`^${RegExp.escape?.(key) ?? key}\\s*:`).test(line);
334+
const amended = original.trim().split('\n')
335+
.filter(line =>
336+
!line.includes(':') ||
337+
!originalTrailers.some(filterTrailer(line)));
338+
for (let i = amended.length - 1; amended[i] === ''; i--) {
339+
amended.pop();
340+
}
340341

341342
// Only keep existing trailers that we won't add ourselves
342-
const keptTrailers = originalTrailers.split('\n').filter(line =>
343-
!!line &&
344-
(!PR_RE.test(line) || this.backport) &&
345-
!BACKPORT_RE.test(line) &&
346-
!REVIEW_RE.test(line));
347-
amended.push('', ...keptTrailers);
348-
349-
for (const line of metadata) {
350-
if (keptTrailers.includes(line)) {
343+
const trailersToFilterOut = ['BACKPORT-PR-URL', 'REVIEWED-BY'];
344+
if (!this.backport) trailersToFilterOut.push('PR-URL');
345+
const keptTrailers = originalTrailers.filter(
346+
([key]) => !trailersToFilterOut.includes(key.toUpperCase())
347+
);
348+
amended.push('', ...keptTrailers.map(([key, value]) => `${key}: ${value}`));
349+
350+
for (const line of metadata.trim().split('\n')) {
351+
const foundMatchingTrailer = keptTrailers.find(filterTrailer(line));
352+
if (foundMatchingTrailer && line === foundMatchingTrailer.join(': ')) {
351353
cli.warn(`Found ${line}, skipping..`);
352354
continue;
353355
}
@@ -399,8 +401,24 @@ export default class LandingSession extends Session {
399401
amended.push('CVE-ID: ' + cveID);
400402
}
401403
}
404+
return amended.join('\n');
405+
}
406+
407+
async amend() {
408+
const { cli } = this;
409+
if (!this.readyToAmend()) {
410+
cli.warn('Not yet ready to amend, run `git node land --abort`');
411+
return;
412+
}
413+
414+
this.startAmending();
415+
416+
const rev = this.getCurrentRev();
417+
const original = runSync('git', [
418+
'show', 'HEAD', '-s', '--format=%B'
419+
]).trim();
402420

403-
const message = amended.join('\n');
421+
const message = await this.generateAmendedMessage(original);
404422
const messageFile = this.saveMessage(rev, message);
405423
cli.separator('New Message');
406424
cli.log(message.trim());

‎test/unit/amend.test.js‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
import { describe, it } from 'node:test';
2+
import LandingSession from '../../lib/landing_session.js';
3+
4+
const createMockCli = () => ({
5+
prompt: () => Promise.resolve(false),
6+
ok: () => {},
7+
warn: () => {},
8+
info: () => {},
9+
log: () => {},
10+
separator: () => {},
11+
startSpinner: () => ({ stop: () => {} }),
12+
stopSpinner: () => {},
13+
setExitCode: () => {}
14+
});
15+
16+
const createSession = (overrides = undefined) => {
17+
const options = {
18+
owner: 'nodejs',
19+
repo: 'node',
20+
upstream: 'origin',
21+
branch: 'main',
22+
prid: 123,
23+
oneCommitMax: false,
24+
backport: false,
25+
...overrides
26+
};
27+
const session = new LandingSession(createMockCli(), {}, '/', options);
28+
29+
let metadata = `${options.backport ? 'Backport-' : ''}PR-URL: http://example.com/${options.prid}\nReviewed-By: user1 <collab@mail.net>\n`;
30+
31+
if (options.metadata) {
32+
metadata += options.metadata;
33+
}
34+
35+
return Object.defineProperty(session, 'metadata', {
36+
__proto__: null,
37+
configurable: true,
38+
value: metadata
39+
});
40+
};
41+
42+
describe('LandingSession.prototype.generateAmendedMessage', () => {
43+
it('should append PR-URL when there are no trailers', async(t) => {
44+
const session = createSession();
45+
const result = await session.generateAmendedMessage('foo: bar');
46+
47+
t.assert.strictEqual(
48+
result,
49+
'foo: bar\n\nPR-URL: http://example.com/123\nReviewed-By: user1 <collab@mail.net>'
50+
);
51+
});
52+
53+
it('should not duplicate trailers that are in metadata', async(t) => {
54+
const session = createSession({ metadata: 'Refs: http://example.com/321\nRefs: http://example.com/456' });
55+
const result = await session.generateAmendedMessage('foo: bar\n\nRefs: http://example.com/321');
56+
57+
t.assert.strictEqual(
58+
result,
59+
'foo: bar\n\nRefs: http://example.com/321\nPR-URL: http://example.com/123\nReviewed-By: user1 <collab@mail.net>\nRefs: http://example.com/456'
60+
);
61+
});
62+
63+
it('should strip trailers added by NCU', async(t) => {
64+
const session = createSession();
65+
const result = await session.generateAmendedMessage(
66+
'subsystem: fix bug\n\nReviewed-By: user1 <foo@bar.com>\nPR-URL: http://example.com/123\nBackport-PR-URL: http://example.com/321\nOther-Trailer: Value\n'
67+
);
68+
69+
t.assert.strictEqual(
70+
result,
71+
'subsystem: fix bug\n\nOther-Trailer: Value\nPR-URL: http://example.com/123\nReviewed-By: user1 <collab@mail.net>'
72+
);
73+
});
74+
75+
it('should strip trailers added by NCU regardless of case', async(t) => {
76+
const session = createSession();
77+
const result = await session.generateAmendedMessage(
78+
'subsystem: fix bug\n\nReViEWed-bY: user1 <foo@bar.com>\npr-url: http://example.com/123\nBACKPORT-PR-URL: http://example.com/321\nOther-Trailer: Value\n'
79+
);
80+
81+
t.assert.strictEqual(
82+
result,
83+
'subsystem: fix bug\n\nOther-Trailer: Value\nPR-URL: http://example.com/123\nReviewed-By: user1 <collab@mail.net>'
84+
);
85+
});
86+
87+
it('should not strip PR-URL trailer when backporting', async(t) => {
88+
const session = createSession({ backport: true, prid: 456 });
89+
const result = await session.generateAmendedMessage(
90+
'subsystem: foobar\n\nOther-Trailer: Value\nPR-URL: http://example.com/999\nReviewed-By: foobar <foo@bar.com>\n'
91+
);
92+
93+
t.assert.strictEqual(
94+
result,
95+
'subsystem: foobar\n\nOther-Trailer: Value\nPR-URL: http://example.com/999\nBackport-PR-URL: http://example.com/456\nReviewed-By: user1 <collab@mail.net>'
96+
);
97+
});
98+
99+
it('should clean-up trailers with extra space', async(t) => {
100+
const session = createSession();
101+
const result = await session.generateAmendedMessage(
102+
'subsystem: foobar\n\n\nTrailer: Value \n\n\n'
103+
);
104+
105+
t.assert.strictEqual(result, 'subsystem: foobar\n\nTrailer: Value\nPR-URL: http://example.com/123\nReviewed-By: user1 <collab@mail.net>');
106+
});
107+
108+
it('should handle empty message', async(t) => {
109+
const session = createSession();
110+
const result = await session.generateAmendedMessage('');
111+
112+
t.assert.strictEqual(result, '\nPR-URL: http://example.com/123\nReviewed-By: user1 <collab@mail.net>');
113+
});
114+
115+
it('should handle cherry-pick from upstream', async(t) => {
116+
const session = createSession({ metadata: 'Refs: https://github.com/v8/v8/commit/cf1bce40a5ef4c7c1da351754f5bf526c0c96463\n' });
117+
const result = await session.generateAmendedMessage(`deps: V8: cherry-pick cf1bce40a5ef
118+
119+
Original commit message:
120+
121+
[wasm] Fix S128Const on big endian
122+
123+
Since http://crrev.com/c/2944437 globals are no longer little endian
124+
enforced.
125+
126+
S128Const handling in the initializer needs to take this into account
127+
and byte reverse values which are hard coded in little endian order.
128+
129+
This is currently causing failures on Node.js upstream:
130+
https://github.com/nodejs/node/pull/59034#issuecomment-4129144461
131+
132+
Change-Id: Ifcc9ade93ee51565ab19b16e9dadf0ff5752f7a6
133+
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7704213
134+
Commit-Queue: Milad Farazmand <mfarazma@ibm.com>
135+
Reviewed-by: Manos Koukoutos <manoskouk@chromium.org>
136+
Cr-Commit-Position: refs/heads/main@{#106082}
137+
138+
PR-URL: https://github.com/nodejs/node/pull/62449
139+
Refs: https://github.com/v8/v8/commit/cf1bce40a5ef4c7c1da351754f5bf526c0c96463
140+
Reviewed-By: Guy Bedford <guybedford@gmail.com>
141+
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
142+
`);
143+
144+
t.assert.strictEqual(result, `deps: V8: cherry-pick cf1bce40a5ef
145+
146+
Original commit message:
147+
148+
[wasm] Fix S128Const on big endian
149+
150+
Since http://crrev.com/c/2944437 globals are no longer little endian
151+
enforced.
152+
153+
S128Const handling in the initializer needs to take this into account
154+
and byte reverse values which are hard coded in little endian order.
155+
156+
This is currently causing failures on Node.js upstream:
157+
https://github.com/nodejs/node/pull/59034#issuecomment-4129144461
158+
159+
Change-Id: Ifcc9ade93ee51565ab19b16e9dadf0ff5752f7a6
160+
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7704213
161+
Commit-Queue: Milad Farazmand <mfarazma@ibm.com>
162+
Reviewed-by: Manos Koukoutos <manoskouk@chromium.org>
163+
Cr-Commit-Position: refs/heads/main@{#106082}
164+
165+
Refs: https://github.com/v8/v8/commit/cf1bce40a5ef4c7c1da351754f5bf526c0c96463
166+
PR-URL: http://example.com/123
167+
Reviewed-By: user1 <collab@mail.net>`
168+
);
169+
});
170+
});

0 commit comments

Comments
 (0)