diff --git a/components/git/land.js b/components/git/land.js index f2fa5b69..f88a29e8 100644 --- a/components/git/land.js +++ b/components/git/land.js @@ -202,12 +202,6 @@ async function main(state, argv, cli, dir) { } session = new LandingSession(cli, req, dir, argv); const metadata = await getMetadata(session.argv, argv.skipRefs, cli); - if (argv.backport) { - const split = metadata.metadata.split('\n')[0]; - if (split === 'PR-URL: ') { - cli.error('Commit message is missing PR-URL'); - } - } return session.start(metadata); } else if (state === APPLY) { return session.apply(); diff --git a/lib/landing_session.js b/lib/landing_session.js index 15b67ed7..3d06e25e 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -286,6 +286,12 @@ export default class LandingSession extends Session { cli.warn('Not yet ready to amend, run `git node land --abort`'); return; } + + 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+)$/i; + this.startAmending(); const rev = this.getCurrentRev(); @@ -303,7 +309,12 @@ export default class LandingSession extends Session { }).trim().length !== 0; const metadata = this.metadata.trim().split('\n'); - const amended = original.split('\n'); + // Filtering out metadata that we will add ourselves: + const isFiltered = line => + (!PR_RE.test(line) || this.backport) && + !BACKPORT_RE.test(line) && + !REVIEW_RE.test(line); + const amended = original.split('\n').filter(isFiltered); // If the original commit message already contains trailers (such as // "Co-authored-by"), we simply add our own metadata after those. Otherwise, @@ -313,35 +324,29 @@ export default class LandingSession extends Session { amended.push(''); } - 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+)/i; - let containCVETrailer = false; for (const line of metadata) { - if (line.length !== 0 && original.includes(line)) { - if (line.match(CVE_RE)) { - containCVETrailer = true; - } + if (line.length !== 0 && original.includes(line) && !isFiltered(line)) { + containCVETrailer ||= CVE_RE.test(line); if (originalHasTrailers) { cli.warn(`Found ${line}, skipping..`); + continue; } else { cli.error('Git found no trailers in the original commit message, ' + `but '${line}' is present and should be a trailer.`); process.exit(1); // make it work with git rebase -x } - } else { - if (line.match(BACKPORT_RE)) { - let prIndex = amended.findIndex(datum => datum.match(PR_RE)); - if (prIndex === -1) { - prIndex = amended.findIndex(datum => datum.match(REVIEW_RE)) - 1; - } - amended.splice(prIndex + 1, 0, line); - } else { - amended.push(line); + } + if (BACKPORT_RE.test(line)) { + const prIndex = + (amended.findIndex(datum => PR_RE.test(datum)) + 1) || + amended.findIndex(datum => REVIEW_RE.test(datum)); + if (prIndex !== -1) { + amended.splice(prIndex, 0, line); + continue; } } + amended.push(line); } if (!containCVETrailer && this.includeCVE) { diff --git a/lib/metadata_gen.js b/lib/metadata_gen.js index 9a100791..82a633cc 100644 --- a/lib/metadata_gen.js +++ b/lib/metadata_gen.js @@ -31,7 +31,6 @@ export default class MetadataGenerator { const parser = new LinkParser(owner, repo, op); const fixes = parser.getFixes(); const refs = parser.getRefs().filter(f => f !== prUrl); - const altPrUrl = parser.getAltPrUrl(); const meta = []; @@ -44,17 +43,10 @@ export default class MetadataGenerator { meta.push(...refs.map((ref) => `Refs: ${ref}`)); } const backport = this.argv ? this.argv.backport : undefined; - if (backport) { - meta.unshift(`Backport-PR-URL: ${prUrl}`); - meta.unshift(`PR-URL: ${altPrUrl}`); - } else { - // Reviews are only added here as backports should not contain reviews - // for the backport itself in the metadata - meta.unshift(`PR-URL: ${prUrl}`); - meta.push( - ...reviewedBy.map((r) => `Reviewed-By: ${r.reviewer.getContact()}`) - ); - } + meta.unshift(`${backport ? 'Backport-' : ''}PR-URL: ${prUrl}`); + meta.push( + ...reviewedBy.map((r) => `Reviewed-By: ${r.reviewer.getContact()}`) + ); meta.push(''); // creates final EOL return meta.join('\n'); } diff --git a/test/unit/metadata_gen.test.js b/test/unit/metadata_gen.test.js index 8eda702d..b5cd7722 100644 --- a/test/unit/metadata_gen.test.js +++ b/test/unit/metadata_gen.test.js @@ -41,10 +41,13 @@ const backportArgv = { } }; const backportData = Object.assign({}, data, { pr: backportPR }, backportArgv); -const backportExpected = `PR-URL: https://github.com/nodejs/node/pull/29995 -Backport-PR-URL: https://github.com/nodejs/node/pull/30072 +const backportExpected = `Backport-PR-URL: https://github.com/nodejs/node/pull/30072 Fixes: https://github.com/nodejs/build/issues/1961 Refs: https://github.com/nodejs/node/commit/53ca0b9ae145c430842bf78e553e3b6cbd2823aa#commitcomment-35494896 +Reviewed-By: Foo User +Reviewed-By: Quux User +Reviewed-By: Baz User +Reviewed-By: Bar User `; const selfRefData = Object.assign({}, data, { pr: selfRefPR });