chore: add Trop annotations to release notes. (#24644)

* chore: add Trop annotations to release notes.

Trop annotations are in the form of "(Also in 7.3, 8, 9)"
with links to the sibling branches.

script/release/notes/index.js

@@ -120,7 +120,7 @@ const getPreviousPoint = async (point) => {
   }
 };
 
-async function getReleaseNotes (range, newVersion, explicitLinks) {
+async function getReleaseNotes (range, newVersion) {
   const rangeList = range.split('..') || ['HEAD'];
   const to = rangeList.pop();
   const from = rangeList.pop() || (await getPreviousPoint(to));
@@ -129,10 +129,9 @@ async function getReleaseNotes (range, newVersion, explicitLinks) {
     newVersion = to;
   }
 
-  console.log(`Generating release notes between ${from} and ${to} for version ${newVersion}`);
   const notes = await notesGenerator.get(from, to, newVersion);
   const ret = {
-    text: notesGenerator.render(notes, explicitLinks)
+    text: notesGenerator.render(notes)
   };
 
   if (notes.unknown.length) {
@@ -144,7 +143,7 @@ async function getReleaseNotes (range, newVersion, explicitLinks) {
 
 async function main () {
   const opts = minimist(process.argv.slice(2), {
-    boolean: ['explicit-links', 'help'],
+    boolean: ['help'],
     string: ['version']
   });
   opts.range = opts._.shift();
@@ -153,14 +152,13 @@ async function main () {
     console.log(`
 easy usage: ${name} version
 
-full usage: ${name} [begin..]end [--version version] [--explicit-links]
+full usage: ${name} [begin..]end [--version version]
 
  * 'begin' and 'end' are two git references -- tags, branches, etc --
    from which the release notes are generated.
  * if omitted, 'begin' defaults to the previous tag in end's branch.
  * if omitted, 'version' defaults to 'end'. Specifying a version is
    useful if you're making notes on a new version that isn't tagged yet.
- * 'explicit-links' makes every note's issue, commit, or pull an MD link
 
 For example, these invocations are equivalent:
   ${process.argv[1]} v4.0.1
@@ -169,7 +167,7 @@ For example, these invocations are equivalent:
     return 0;
   }
 
-  const notes = await getReleaseNotes(opts.range, opts.version, opts['explicit-links']);
+  const notes = await getReleaseNotes(opts.range, opts.version);
   console.log(notes.text);
   if (notes.warning) {
     throw new Error(notes.warning);

script/release/notes/notes.js

@@ -2,24 +2,22 @@
 
 'use strict';
 
-const childProcess = require('child_process');
 const fs = require('fs');
-const os = require('os');
 const path = require('path');
 
 const { GitProcess } = require('dugite');
 const octokit = require('@octokit/rest')({
   auth: process.env.ELECTRON_GITHUB_TOKEN
 });
-const semver = require('semver');
 
-const { ELECTRON_VERSION, SRC_DIR } = require('../../lib/utils');
+const { SRC_DIR } = require('../../lib/utils');
 
 const MAX_FAIL_COUNT = 3;
 const CHECK_INTERVAL = 5000;
 
+const TROP_LOGIN = 'trop[bot]';
+
 const NO_NOTES = 'No notes';
-const FOLLOW_REPOS = ['electron/electron', 'electron/node'];
 
 const docTypes = new Set(['doc', 'docs']);
 const featTypes = new Set(['feat', 'feature']);
@@ -57,6 +55,11 @@ class Commit {
 
     this.isBreakingChange = false;
     this.note = null; // string
+
+    // A set of branches to which this change has been merged.
+    // '8-x-y' => GHKey { owner: 'electron', repo: 'electron', number: 23714 }
+    this.trops = new Map(); // Map<string,GHKey>
+
     this.prKeys = new Set(); // GHKey
     this.revertHash = null; // string
     this.semanticType = null; // string
@@ -107,7 +110,7 @@ const getNoteFromClerk = async (ghKey) => {
     if (comment.body.startsWith(PERSIST_LEAD)) {
       return comment.body
         .slice(PERSIST_LEAD.length).trim() // remove PERSIST_LEAD
-        .split('\r?\n') // break into lines
+        .split(/\r?\n/) // split into lines
         .map(line => line.trim())
         .filter(line => line.startsWith(QUOTE_LEAD)) // notes are quoted
         .map(line => line.slice(QUOTE_LEAD.length)) // unquote the lines
@@ -117,18 +120,6 @@ const getNoteFromClerk = async (ghKey) => {
   }
 };
 
-// copied from https://github.com/electron/clerk/blob/master/src/index.ts#L4-L13
-const OMIT_FROM_RELEASE_NOTES_KEYS = [
-  'no-notes',
-  'no notes',
-  'no_notes',
-  'none',
-  'no',
-  'nothing',
-  'empty',
-  'blank'
-];
-
 /**
  * Looks for our project's conventions in the commit message:
  *
@@ -206,7 +197,9 @@ const parsePullText = (pull, commit) => parseCommitMessage(`${pull.data.title}\n
 
 const getLocalCommitHashes = async (dir, ref) => {
   const args = ['log', '--format=%H', ref];
-  return (await runGit(dir, args)).split(/[\r\n]+/).map(hash => hash.trim());
+  return (await runGit(dir, args))
+    .split(/\r?\n/) // split into lines
+    .map(hash => hash.trim());
 };
 
 // return an array of Commits
@@ -216,7 +209,9 @@ const getLocalCommits = async (module, point1, point2) => {
   const fieldSep = ',';
   const format = ['%H', '%s'].join(fieldSep);
   const args = ['log', '--cherry-pick', '--right-only', '--first-parent', `--format=${format}`, `${point1}..${point2}`];
-  const logs = (await runGit(dir, args)).split(/[\r\n]+/).map(field => field.trim());
+  const logs = (await runGit(dir, args))
+    .split(/\r?\n/) // split into lines
+    .map(field => field.trim());
 
   const commits = [];
   for (const log of logs) {
@@ -332,92 +327,50 @@ const addRepoToPool = async (pool, repo, from, to) => {
   }
 };
 
-/***
-****  Other Repos
-***/
-
-// other repos - gn
-
-const getDepsVariable = async (ref, key) => {
-  // get a copy of that reference point's DEPS file
-  const deps = await runGit(ELECTRON_VERSION, ['show', `${ref}:DEPS`]);
-  const filename = path.resolve(os.tmpdir(), 'DEPS');
-  fs.writeFileSync(filename, deps);
-
-  // query the DEPS file
-  const response = childProcess.spawnSync(
-    'gclient',
-    ['getdep', '--deps-file', filename, '--var', key],
-    { encoding: 'utf8' }
-  );
-
-  // cleanup
-  fs.unlinkSync(filename);
-  return response.stdout.trim();
-};
-
-const getDependencyCommitsGN = async (pool, fromRef, toRef) => {
-  const repos = [{ // just node
-    owner: 'electron',
-    repo: 'node',
-    dir: path.resolve(SRC_DIR, 'third_party', 'electron_node'),
-    deps_variable_name: 'node_version'
-  }];
-
-  for (const repo of repos) {
-    // the 'DEPS' file holds the dependency reference point
-    const key = repo.deps_variable_name;
-    const from = await getDepsVariable(fromRef, key);
-    const to = await getDepsVariable(toRef, key);
-    await addRepoToPool(pool, repo, from, to);
+// returns Map<string,GHKey>
+// where the key is a branch name (e.g. '7-1-x' or '8-x-y')
+// and the value is a GHKey to the PR
+async function getBranchesOfCommit (commit, pool) {
+  const branches = new Map();
+
+  for (const prKey of commit.prKeys.values()) {
+    const pull = pool.pulls[prKey.number];
+    const mergedBranches = new Set((pull?.data?.labels || [])
+      .map(label => (label?.name ?? '').match(/merged\/([0-9]+-[x0-9]-[xy0-9])/))
+      .filter(match => match)
+      .map(match => match[1])
+    );
+
+    if (mergedBranches.size > 0) {
+      const isTropComment = (comment) => (comment?.user?.login ?? '') === TROP_LOGIN;
+
+      const ghKey = GHKey.NewFromPull(pull.data);
+      const backportRegex = /backported this PR to "(.*)",\s+please check out #(\d+)/;
+      const getBranchNameAndPullKey = (comment) => {
+        const match = (comment?.body ?? '').match(backportRegex);
+        return match ? [match[1], new GHKey(ghKey.owner, ghKey.repo, parseInt(match[2]))] : null;
+      };
+
+      ((await getComments(ghKey))?.data ?? [])
+        .filter(isTropComment)
+        .map(getBranchNameAndPullKey)
+        .filter(pair => pair)
+        .filter(([branch]) => mergedBranches.has(branch))
+        .forEach(([branch, key]) => branches.set(branch, key));
+    }
   }
-};
-
-// Changes are interesting if they make a change relative to a previous
-// release in the same series. For example if you fix a Y.0.0 bug, that
-// should be included in the Y.0.1 notes even if it's also tropped back
-// to X.0.1.
-//
-// The phrase 'previous release' is important: if this is the first
-// prerelease or first stable release in a series, we omit previous
-// branches' changes. Otherwise we will have an overwhelmingly long
-// list of mostly-irrelevant changes.
-const shouldIncludeMultibranchChanges = (version) => {
-  let show = true;
-
-  if (semver.valid(version)) {
-    const prerelease = semver.prerelease(version);
-    show = prerelease
-      ? parseInt(prerelease.pop()) > 1
-      : semver.patch(version) > 0;
-  }
-
-  return show;
-};
-
-function getOldestMajorBranchOfPull (pull) {
-  return pull.data.labels
-    .map(label => label.name.match(/merged\/(\d+)-(\d+)-x/) || label.name.match(/merged\/(\d+)-x-y/))
-    .filter(label => !!label)
-    .map(label => parseInt(label[1]))
-    .filter(major => !!major)
-    .sort()
-    .shift();
-}
 
-function getOldestMajorBranchOfCommit (commit, pool) {
-  return [...commit.prKeys.values()]
-    .map(prKey => pool.pulls[prKey.number])
-    .filter(pull => !!pull)
-    .map(pull => getOldestMajorBranchOfPull(pull))
-    .filter(major => !!major)
-    .sort()
-    .shift();
+  return branches;
 }
 
-function commitExistsBeforeMajor (commit, pool, major) {
-  const firstAppearance = getOldestMajorBranchOfCommit(commit, pool);
-  return firstAppearance && (firstAppearance < major);
+// @return the shorthand name of the branch that `ref` is on.
+// e.g. a ref of '10.0.0-beta.1' will return '10-x-y'
+async function getBranchNameOfRef (ref, dir) {
+  return (await runGit(dir, ['branch', '--all', '--contains', ref, '--sort', 'version:refname']))
+    .split(/\r?\n/) // split into lines
+    .shift() // we sorted by refname and want the first result
+    .match(/(?:.*\/)?(.*)/)[1] // 'remote/origins/10-x-y' -> '10-x-y'
+    .trim();
 }
 
 /***
@@ -431,21 +384,15 @@ const getNotes = async (fromRef, toRef, newVersion) => {
   }
 
   const pool = new Pool();
+  const electronDir = path.resolve(SRC_DIR, 'electron');
+  const toBranch = await getBranchNameOfRef(toRef, electronDir);
+
+  console.log(`Generating release notes between ${fromRef} and ${toRef} for version ${newVersion} in branch ${toBranch}`);
 
   // get the electron/electron commits
-  const electron = { owner: 'electron', repo: 'electron', dir: path.resolve(SRC_DIR, 'electron') };
+  const electron = { owner: 'electron', repo: 'electron', dir: electronDir };
   await addRepoToPool(pool, electron, fromRef, toRef);
 
-  // Don't include submodules if comparing across major versions;
-  // there's just too much churn otherwise.
-  const includeDeps = semver.valid(fromRef) &&
-                      semver.valid(toRef) &&
-                      semver.major(fromRef) === semver.major(toRef);
-
-  if (includeDeps) {
-    await getDependencyCommitsGN(pool, fromRef, toRef);
-  }
-
   // remove any old commits
   pool.commits = pool.commits.filter(commit => !pool.processedHashes.has(commit.hash));
 
@@ -482,9 +429,8 @@ const getNotes = async (fromRef, toRef, newVersion) => {
     .filter(commit => commit.note && (commit.note !== NO_NOTES))
     .filter(commit => !((commit.note || commit.subject).match(/^[Bb]ump v\d+\.\d+\.\d+/)));
 
-  if (!shouldIncludeMultibranchChanges(newVersion)) {
-    const { major } = semver.parse(newVersion);
-    pool.commits = pool.commits.filter(commit => !commitExistsBeforeMajor(commit, pool, major));
+  for (const commit of pool.commits) {
+    commit.trops = await getBranchesOfCommit(commit, pool);
   }
 
   pool.commits = removeSupercededStackUpdates(pool.commits);
@@ -496,7 +442,8 @@ const getNotes = async (fromRef, toRef, newVersion) => {
     fix: [],
     other: [],
     unknown: [],
-    name: newVersion
+    name: newVersion,
+    toBranch
   };
 
   pool.commits.forEach(commit => {
@@ -545,31 +492,42 @@ const removeSupercededStackUpdates = (commits) => {
 ****  Render
 ***/
 
-const renderLink = (commit, explicitLinks) => {
-  let link;
-  const { owner, repo } = commit;
-  const keyIt = commit.prKeys.values().next();
-  if (keyIt.done) /* no PRs */ {
-    const { hash } = commit;
-    const url = `https://github.com/${owner}/${repo}/commit/${hash}`;
-    const text = owner === 'electron' && repo === 'electron'
-      ? `${hash.slice(0, 8)}`
-      : `${owner}/${repo}@${hash.slice(0, 8)}`;
-    link = explicitLinks ? `[${text}](${url})` : text;
-  } else {
-    const { number } = keyIt.value;
-    const url = `https://github.com/${owner}/${repo}/pull/${number}`;
-    const text = owner === 'electron' && repo === 'electron'
-      ? `#${number}`
-      : `${owner}/${repo}#${number}`;
-    link = explicitLinks ? `[${text}](${url})` : text;
-  }
-  return link;
-};
+// @return the pull request's GitHub URL
+const buildPullURL = ghKey => `https://github.com/${ghKey.owner}/${ghKey.repo}/pull/${ghKey.number}`;
+
+const renderPull = ghKey => `[#${ghKey.number}](${buildPullURL(ghKey)})`;
 
-const renderCommit = (commit, explicitLinks) => {
-  // clean up the note
-  let note = commit.note || commit.subject;
+// @return the commit's GitHub URL
+const buildCommitURL = commit => `https://github.com/${commit.owner}/${commit.repo}/commit/${commit.hash}`;
+
+const renderCommit = commit => `[${commit.hash.slice(0, 8)}](${buildCommitURL(commit)})`;
+
+// @return a markdown link to the PR if available; otherwise, the git commit
+function renderLink (commit) {
+  const maybePull = commit.prKeys.values().next();
+  return maybePull.value ? renderPull(maybePull.value) : renderCommit(commit);
+}
+
+// @return a terser branch name,
+//   e.g. '7-2-x' -> '7.2' and '8-x-y' -> '8'
+const renderBranchName = name => name.replace(/-[a-zA-Z]/g, '').replace('-', '.');
+
+const renderTrop = (branch, ghKey) => `[${renderBranchName(branch)}](${buildPullURL(ghKey)})`;
+
+// @return markdown-formatted links to other branches' trops,
+//   e.g. "(Also in 7.2, 8, 9)"
+function renderTrops (commit, excludeBranch) {
+  const body = [...commit.trops.entries()]
+    .filter(([branch]) => branch !== excludeBranch)
+    .sort(([branchA], [branchB]) => parseInt(branchA) - parseInt(branchB)) // sort by semver major
+    .map(([branch, key]) => renderTrop(branch, key))
+    .join(', ');
+  return body ? `<span style="font-size:small;">(Also in ${body})</span>` : body;
+}
+
+// @return a slightly cleaned-up human-readable change description
+function renderDescription (commit) {
+  let note = commit.note || commit.subject || '';
   note = note.trim();
   if (note.length !== 0) {
     note = note[0].toUpperCase() + note.substr(1);
@@ -606,30 +564,24 @@ const renderCommit = (commit, explicitLinks) => {
     }
   }
 
-  const link = renderLink(commit, explicitLinks);
+  return note;
+}
 
-  return { note, link };
-};
+// @return markdown-formatted release note line item,
+//   e.g. '* Fixed a foo. #12345 (Also in 7.2, 8, 9)'
+const renderNote = (commit, excludeBranch) =>
+  `* ${renderDescription(commit)} ${renderLink(commit)} ${renderTrops(commit, excludeBranch)}\n`;
 
-const renderNotes = (notes, explicitLinks) => {
+const renderNotes = (notes) => {
   const rendered = [`# Release Notes for ${notes.name}\n\n`];
 
   const renderSection = (title, commits) => {
-    if (commits.length === 0) {
-      return;
-    }
-    const notes = new Map();
-    for (const note of commits.map(commit => renderCommit(commit, explicitLinks))) {
-      if (!notes.has(note.note)) {
-        notes.set(note.note, [note.link]);
-      } else {
-        notes.get(note.note).push(note.link);
-      }
+    if (commits.length > 0) {
+      rendered.push(
+        `## ${title}\n\n`,
+        ...(commits.map(commit => renderNote(commit, notes.toBranch)).sort())
+      );
     }
-    rendered.push(`## ${title}\n\n`);
-    const lines = [];
-    notes.forEach((links, key) => lines.push(` * ${key} ${links.map(link => link.toString()).sort().join(', ')}\n`));
-    rendered.push(...lines.sort(), '\n');
   };
 
   renderSection('Breaking Changes', notes.breaking);
@@ -638,7 +590,7 @@ const renderNotes = (notes, explicitLinks) => {
   renderSection('Other Changes', notes.other);
 
   if (notes.docs.length) {
-    const docs = notes.docs.map(commit => renderLink(commit, explicitLinks)).sort();
+    const docs = notes.docs.map(commit => renderLink(commit)).sort();
     rendered.push('## Documentation\n\n', ` * Documentation changes: ${docs.join(', ')}\n`, '\n');
   }
 

spec-main/release-notes-spec.ts

@@ -67,6 +67,14 @@ class GitFake {
         }
       }
       stdout = lines.join('\n');
+    } else if (args.length === 6 &&
+               args[0] === 'branch' &&
+               args[1] === '--all' &&
+               args[2] === '--contains' &&
+               args[3].endsWith('-x-y')) {
+      // "what branch is this tag in?"
+      // git branch --all --contains ${ref} --sort version:refname
+      stdout = args[3];
     } else {
       console.error('unhandled GitProcess.exec():', args);
     }
@@ -115,25 +123,15 @@ describe('release notes', () => {
     sandbox.restore();
   });
 
-  describe('changes that exist in older branches', () => {
-    // use case: this fix is NOT news because it was already fixed
-    // while oldBranch was the latest stable release
-    it('are skipped if the target version is a new major line (x.0.0)', async function () {
+  describe('trop annotations', () => {
+    it('shows sibling branches', async function () {
       const version = 'v9.0.0';
       gitFake.setBranch(oldBranch, [...sharedHistory, oldTropFix]);
       gitFake.setBranch(newBranch, [...sharedHistory, newTropFix]);
       const results: any = await notes.get(oldBranch, newBranch, version);
-      expect(results.fix).to.have.lengthOf(0);
-    });
-
-    // use case: this fix IS news because it's being fixed in
-    // multiple stable branches at once, including newBranch.
-    it('are included if the target version is a minor or patch bump', async function () {
-      const version = 'v9.0.1';
-      gitFake.setBranch(oldBranch, [...sharedHistory, oldTropFix]);
-      gitFake.setBranch(newBranch, [...sharedHistory, newTropFix]);
-      const results: any = await notes.get(oldBranch, newBranch, version);
       expect(results.fix).to.have.lengthOf(1);
+      console.log(results.fix);
+      expect(results.fix[0].trops).to.have.keys('8-x-y', '9-x-y');
     });
   });