feat: add NESTED_COMMIT markers when combining multiple source commits

packages/owl-bot/package.json

@@ -18,7 +18,7 @@
     "compile": "tsc -p .",
     "fix": "gts fix",
     "lint": "gts check",
-    "mocha": "c8 mocha build/test",
+    "mocha": "npm run compile && c8 mocha build/test",
     "test": "c8 mocha build/test && node ./build/src/bin/owl-bot.js --help",
     "system-test": "c8 mocha build/system-test",
     "pretest": "npm run compile",

packages/owl-bot/src/bin/commands/copy-code-and-create-pull-request.ts

@@ -19,12 +19,14 @@ import * as cc from '../../copy-code';
 import {octokitFactoryFrom, OctokitParams} from '../../octokit-util';
 import {githubRepoFromOwnerSlashName} from '../../github-repo';
 import {FakeCopyStateStore} from '../../fake-copy-state-store';
+import {WithNestedCommitDelimiters} from '../../create-pr';
 
 interface Args extends OctokitParams {
   'source-repo': string;
   'source-repo-commit-hash': string;
   'dest-repo': string;
   'dest-owlbot-yaml': string;
+  'use-nested-commit-delimiters': boolean;
 }
 
 export const copyCodeAndCreatePullRequestCommand: yargs.CommandModule<
@@ -74,6 +76,13 @@ export const copyCodeAndCreatePullRequestCommand: yargs.CommandModule<
         type: 'string',
         default: '.github/.OwlBot.yaml',
         demand: false,
+      })
+      .option('use-nested-commit-delimiters', {
+        describe:
+          'Whether to use BEGIN_NESTED_COMMIT delimiters when separating multiple commit messages',
+        type: 'boolean',
+        default: false,
+        demand: false,
       });
   },
   async handler(argv) {
@@ -87,8 +96,13 @@ export const copyCodeAndCreatePullRequestCommand: yargs.CommandModule<
       copyStateStore: new FakeCopyStateStore(),
       octokitFactory,
     };
-    await cc.copyCodeAndAppendOrCreatePullRequest(params, [
-      argv['dest-owlbot-yaml'],
-    ]);
+    await cc.copyCodeAndAppendOrCreatePullRequest(
+      params,
+      [argv['dest-owlbot-yaml']],
+      undefined,
+      argv['use-nested-commit-delimiters']
+        ? WithNestedCommitDelimiters.Yes
+        : WithNestedCommitDelimiters.No
+    );
   },
 };

packages/owl-bot/src/copy-code.ts

@@ -29,12 +29,12 @@ import {OWL_BOT_COPY} from './core';
 import {newCmd} from './cmd';
 import {
   createPullRequestFromLastCommit,
-  EMPTY_REGENERATE_CHECKBOX_TEXT,
   Force,
-  REGENERATE_CHECKBOX_TEXT,
   resplit,
   WithRegenerateCheckbox,
   insertApiName,
+  prependCommitMessage,
+  WithNestedCommitDelimiters,
 } from './create-pr';
 import {GithubRepo, githubRepoFromOwnerSlashName} from './github-repo';
 import {CopyStateStore} from './copy-state-store';
@@ -280,7 +280,8 @@ export function branchNameForCopies(yamlPaths: string[]): string {
 async function findAndAppendPullRequest(
   params: WorkingCopyParams,
   yamlPaths: string[],
-  logger: Logger = console
+  logger: Logger = console,
+  withNestedCommitDelimiters: WithNestedCommitDelimiters = WithNestedCommitDelimiters.No
 ): Promise<boolean> {
   const cmd = newCmd(logger);
   const octokit = await params.octokitFactory.getShortLivedOctokit();
@@ -372,14 +373,11 @@ async function findAndAppendPullRequest(
   })
     .toString('utf8')
     .trim();
-  const {title, body} = resplit(
-    `${commitBody}\n\n` +
-      `${pull.title}\n` +
-      pull.body
-        ?.replace(REGENERATE_CHECKBOX_TEXT, '')
-        .replace(EMPTY_REGENERATE_CHECKBOX_TEXT, '')
-        .trim() ?? '',
-    WithRegenerateCheckbox.Yes
+  const {title, body} = prependCommitMessage(
+    commitBody,
+    {title: pull.title, body: pull.body || ''},
+    WithRegenerateCheckbox.Yes,
+    withNestedCommitDelimiters
   );
   const apiNames = copiedYamls.map(tag => tag.yaml['api-name']).filter(Boolean);
   const apiList = abbreviateApiListForTitle(apiNames as string[]);
@@ -422,7 +420,8 @@ interface WorkingCopyParams extends CopyParams {
 export async function copyCodeAndAppendOrCreatePullRequest(
   params: CopyParams,
   yamlPaths: string[],
-  logger: Logger = console
+  logger: Logger = console,
+  withNestedCommitDelimiters: WithNestedCommitDelimiters = WithNestedCommitDelimiters.No
 ): Promise<void> {
   const workDir = tmp.dirSync().name;
   logger.info(`Working in ${workDir}`);
@@ -449,7 +448,14 @@ export async function copyCodeAndAppendOrCreatePullRequest(
   const leftOvers: string[] = [];
   const wparams: WorkingCopyParams = {...params, destDir, workDir};
   for (const yamlPath of yamlPaths) {
-    if (!(await findAndAppendPullRequest(wparams, [yamlPath], logger))) {
+    if (
+      !(await findAndAppendPullRequest(
+        wparams,
+        [yamlPath],
+        logger,
+        withNestedCommitDelimiters
+      ))
+    ) {
       leftOvers.push(yamlPath);
     }
   }
@@ -459,7 +465,14 @@ export async function copyCodeAndAppendOrCreatePullRequest(
   if (isDeepStrictEqual(yamlPaths, leftOvers)) {
     // Don't repeat exactly the same search
   } else {
-    if (await findAndAppendPullRequest(wparams, leftOvers, logger)) {
+    if (
+      await findAndAppendPullRequest(
+        wparams,
+        leftOvers,
+        logger,
+        withNestedCommitDelimiters
+      )
+    ) {
       return; // Appended a pull request for all the left-over APIs.  Done.
     }
   }

packages/owl-bot/src/create-pr.ts

@@ -27,6 +27,11 @@ export const EMPTY_REGENERATE_CHECKBOX_TEXT = REGENERATE_CHECKBOX_TEXT.replace(
   '[ ]'
 );
 
+interface PullRequestContent {
+  title: string;
+  body: string;
+}
+
 /***
  * Github will reject the pull request if the title is longer than 255
  * characters.  This function will move characters from the title to the body
@@ -37,7 +42,7 @@ export const EMPTY_REGENERATE_CHECKBOX_TEXT = REGENERATE_CHECKBOX_TEXT.replace(
 export function resplit(
   rawBody: string,
   withRegenerateCheckbox: WithRegenerateCheckbox
-): {title: string; body: string} {
+): PullRequestContent {
   const regexp = /([^\r\n]*)([\r\n]*)((.|\r|\n)*)/;
   const match = regexp.exec(rawBody)!;
   let title = match[1];
@@ -53,6 +58,72 @@ export function resplit(
   return {title, body: body.substring(0, MAX_BODY_LENGTH)};
 }
 
+const NESTED_COMMIT_SEPARATOR = 'BEGIN_NESTED_COMMIT';
+
+/**
+ * Given pull request content and a new commit message. Rewrite the pull request
+ * title and body using the newest message as the title. If the initial pull
+ * request title was truncated with ellipses, rejoin the title to the remaining part.
+ *
+ * For example, if the existing pull request is something like:
+ * Title: `feat: original feature`
+ * Body: `Copy-Tag: 1234`
+ *
+ * and we prepend a new message of `feat: another new feature\nCopy-Tag: 2345`, the
+ * output will be:
+ * Title: `feat: another new feature`
+ * Body: `Copy-Tag: 2345\nBEGIN_NESTED_COMMIT\nfeat: original feature\nCopy-Tag:1234\nEND_NESTED_COMMIT`
+ *
+ * @param {string} newCommitMessage the new commit message
+ * @param {PullRequestContent} existingContent exisiting pull request title and body
+ * @param {WithRegenerateCheckbox} withRegenerateCheckbox whether to include the
+ *   checkbox to regenerate the pull request
+ */
+export function prependCommitMessage(
+  newCommitMessage: string,
+  existingContent: PullRequestContent,
+  withRegenerateCheckbox: WithRegenerateCheckbox,
+  withNestedCommitDelimiters: WithNestedCommitDelimiters = WithNestedCommitDelimiters.No
+): PullRequestContent {
+  // remove any regenerate checkbox content and leading/trailing whitespace
+  const oldStrippedBody = existingContent.body
+    .replace(EMPTY_REGENERATE_CHECKBOX_TEXT, '')
+    .replace(REGENERATE_CHECKBOX_TEXT, '')
+    .trim();
+  // if title was truncated, re-add it to the beginning of the commit message
+  const oldBody = existingContent.title.endsWith('...')
+    ? `${existingContent.title.substring(
+        0,
+        existingContent.title.length - 3
+      )}${oldStrippedBody}`
+    : `${existingContent.title}\n${oldStrippedBody}`;
+  if (withNestedCommitDelimiters === WithNestedCommitDelimiters.Yes) {
+    // anything before the first BEGIN_NESTED_COMMIT marker, is considered part of
+    // the previous commit
+    const bodyParts = oldBody
+      .split(NESTED_COMMIT_SEPARATOR)
+      .map(part => part.trim());
+    const oldBodyWithNestedCommitMarkers =
+      bodyParts.length === 1
+        ? // there is a single commit -- wrap the old body in the nested commit tags
+          `${NESTED_COMMIT_SEPARATOR}\n${oldBody}\nEND_NESTED_COMMIT`
+        : // there are already existing nested commit tags, content before the first
+          // one is wrapped in a new nested commit tag
+          `${NESTED_COMMIT_SEPARATOR}\n${
+            bodyParts[0]
+          }\nEND_NESTED_COMMIT\n${NESTED_COMMIT_SEPARATOR}\n${bodyParts
+            .slice(1)
+            .join(NESTED_COMMIT_SEPARATOR)}`;
+
+    // prepend the new commit message and use original title truncation logic
+    return resplit(
+      `${newCommitMessage}\n\n${oldBodyWithNestedCommitMarkers}`,
+      withRegenerateCheckbox
+    );
+  }
+  return resplit(`${newCommitMessage}\n\n${oldBody}`, withRegenerateCheckbox);
+}
+
 // Exported for testing only.
 /**
  * Inserts an API name into a pr title after the first colon.
@@ -101,6 +172,17 @@ export enum WithRegenerateCheckbox {
   No = 'no',
 }
 
+/**
+ * Should createPullRequestFromLastCommit() separate multiple commit message
+ * bodies with `BEGIN_NESTED_COMMIT`/`END_NESTED_COMMIT`?
+ *
+ * More type safe and readable than a boolean.
+ */
+export enum WithNestedCommitDelimiters {
+  Yes = 'yes',
+  No = 'no',
+}
+
 /**
  * Creates a pull request using the title and commit message from the most
  * recent commit.

packages/owl-bot/src/scan-googleapis-gen-and-create-pull-requests.ts

@@ -27,6 +27,7 @@ import {newCmd} from './cmd';
 import {CopyStateStore} from './copy-state-store';
 import {GithubRepo} from './github-repo';
 import {Logger, LoggerWithTimestamp} from './logger';
+import {WithNestedCommitDelimiters} from './create-pr';
 
 interface Todo {
   repo: GithubRepo;
@@ -79,7 +80,8 @@ export async function scanGoogleapisGenAndCreatePullRequests(
   cloneDepth = 100,
   copyStateStore: CopyStateStore,
   combinePullsThreshold = Number.MAX_SAFE_INTEGER,
-  logger: Logger = console
+  logger: Logger = console,
+  withNestedCommitDelimiters: WithNestedCommitDelimiters = WithNestedCommitDelimiters.No
 ): Promise<number> {
   logger = new LoggerWithTimestamp(logger);
   // Clone the source repo.
@@ -184,7 +186,12 @@ export async function scanGoogleapisGenAndCreatePullRequests(
       copyStateStore,
       octokitFactory,
     };
-    await copyCodeAndAppendOrCreatePullRequest(params, todo.yamlPaths, logger);
+    await copyCodeAndAppendOrCreatePullRequest(
+      params,
+      todo.yamlPaths,
+      logger,
+      withNestedCommitDelimiters
+    );
   }
   return todoStack.length;
 }