Skip to content

Commit eabfbde

Browse files
committed
fix: cherry-pick the landedCommitSha for security releases
Sometimes we need to include a commit that already landed on `main` in a security release
1 parent 9ccbe8e commit eabfbde

3 files changed

Lines changed: 64 additions & 16 deletions

File tree

lib/cherry_pick.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,12 @@ export default class CherryPick {
9393
}, false, cli);
9494
this.expectedCommitShas =
9595
metadata.data.commits.map(({ commit }) => commit.oid);
96+
// In security releases the fix may have already landed publicly, so the
97+
// PR is merged and refs/pull/<prid>/merge no longer exists - pick the
98+
// commits as they landed on the base branch instead.
99+
this.landedCommitSha = metadata.data.pr.merged
100+
? metadata.data.pr.mergeCommit?.oid
101+
: undefined;
96102

97103
if (this.promptAmend) {
98104
const amend = await cli.prompt(

lib/landing_session.js

Lines changed: 54 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,9 @@ export default class LandingSession extends Session {
8282
}
8383

8484
async downloadAndPatch() {
85-
const { cli, upstream, prid, expectedCommitShas, crossRepoPR } = this;
85+
const {
86+
cli, upstream, prid, expectedCommitShas, crossRepoPR, landedCommitSha
87+
} = this;
8688

8789
if (crossRepoPR) {
8890
const { owner, repo } = this;
@@ -107,33 +109,70 @@ export default class LandingSession extends Session {
107109
{ ignoreFailure: false });
108110
}
109111
} while (!isHeadAMergeCommit());
112+
} else if (landedCommitSha) {
113+
// The PR already landed (e.g. a security fix released publicly), so
114+
// its merge ref is gone - fetch the commits from the base branch.
115+
cli.startSpinner(`Downloading landed commits for ${prid}`);
116+
await forceRunAsync('git', ['fetch', upstream, landedCommitSha],
117+
{ ignoreFailure: false });
110118
} else {
111119
cli.startSpinner(`Downloading patch for ${prid}`);
112120
await runAsync('git', [
113121
'fetch', upstream,
114122
`refs/pull/${prid}/merge`]);
115123
}
116124

117-
// We fetched the commit that would result if we used `git merge`.
118-
// ^1 and ^2 refer to the PR base and the PR head, respectively.
119-
const [base, head, rebaseHead] = await runAsync('git',
120-
['rev-parse', 'FETCH_HEAD^1', 'FETCH_HEAD^2', 'HEAD'],
121-
{ captureStdout: 'lines' });
125+
let base, head, rebaseHead;
126+
if (landedCommitSha && !crossRepoPR) {
127+
// FETCH_HEAD is the last commit of the PR as landed on the base branch.
128+
// Walk back while the PR-URL trailer still points to this PR to find
129+
// the base of the landed series (landing may have squashed commits).
130+
const prUrlRe = new RegExp(`^PR-URL: \\S+/pull/${prid}$`, 'm');
131+
let count = 0;
132+
while (count < expectedCommitShas.length) {
133+
const message = await runAsync('git',
134+
['log', '-1', '--format=%B', `FETCH_HEAD~${count}`],
135+
{ captureStdout: true });
136+
if (!prUrlRe.test(message)) break;
137+
count++;
138+
}
139+
if (count === 0) {
140+
cli.error(`Landed commit ${landedCommitSha} does not have a ` +
141+
`PR-URL trailer pointing to pull request ${prid}`);
142+
process.exit(1);
143+
}
144+
[base, head, rebaseHead] = await runAsync('git',
145+
['rev-parse', `FETCH_HEAD~${count}`, 'FETCH_HEAD', 'HEAD'],
146+
{ captureStdout: 'lines' });
147+
} else {
148+
// We fetched the commit that would result if we used `git merge`.
149+
// ^1 and ^2 refer to the PR base and the PR head, respectively.
150+
[base, head, rebaseHead] = await runAsync('git',
151+
['rev-parse', 'FETCH_HEAD^1', 'FETCH_HEAD^2', 'HEAD'],
152+
{ captureStdout: 'lines' });
153+
}
122154
const commitShas = await runAsync('git',
123155
['rev-list', `${base}..${head}`],
124156
{ captureStdout: 'lines' });
125157
cli.stopSpinner(`Fetched commits as ${shortSha(base)}..${shortSha(head)}`);
126158
cli.separator();
127159

128-
const mismatchedCommits = [
129-
...commitShas.filter((sha) => !expectedCommitShas.includes(sha))
130-
.map((sha) => `Unexpected commit ${sha}`),
131-
...expectedCommitShas.filter((sha) => !commitShas.includes(sha))
132-
.map((sha) => `Missing commit ${sha}`)
133-
].join('\n');
134-
if (mismatchedCommits.length > 0) {
135-
cli.error(`Mismatched commits:\n${mismatchedCommits}`);
136-
process.exit(1);
160+
if (landedCommitSha && !crossRepoPR) {
161+
// The landed commits have different SHAs from the ones in the PR, so
162+
// they cannot be compared - they were validated by their PR-URL
163+
// trailers above.
164+
cli.ok(`Using ${commitShas.length} commit(s) as landed for PR ${prid}`);
165+
} else {
166+
const mismatchedCommits = [
167+
...commitShas.filter((sha) => !expectedCommitShas.includes(sha))
168+
.map((sha) => `Unexpected commit ${sha}`),
169+
...expectedCommitShas.filter((sha) => !commitShas.includes(sha))
170+
.map((sha) => `Missing commit ${sha}`)
171+
].join('\n');
172+
if (mismatchedCommits.length > 0) {
173+
cli.error(`Mismatched commits:\n${mismatchedCommits}`);
174+
process.exit(1);
175+
}
137176
}
138177

139178
const commitInfo = { base, head, shas: commitShas, rebaseHead };

lib/queries/PR.gql

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,10 @@ query PR($prid: Int!, $owner: String!, $repo: String!) {
3131
closed,
3232
closedAt,
3333
merged,
34-
mergedAt
34+
mergedAt,
35+
mergeCommit {
36+
oid
37+
}
3538
}
3639
}
3740
}

0 commit comments

Comments
 (0)