feat(git-node): refactor metadata generation

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();

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) {

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');
   }

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 <foo@example.com>
+Reviewed-By: Quux User <quux@example.com>
+Reviewed-By: Baz User <baz@example.com>
+Reviewed-By: Bar User <bar@example.com>
 `;
 
 const selfRefData = Object.assign({}, data, { pr: selfRefPR });