Split conflict resolution and staging into separate steps

src/services/git.test.ts

@@ -6,6 +6,7 @@ import {
   deleteRemote,
   createFeatureBranch,
   cherrypick,
+  getFilesWithConflicts,
 } from '../services/git';
 import * as childProcess from '../services/child-process-promisified';
 
@@ -42,6 +43,30 @@ describe('getUnmergedFiles', () => {
   });
 });
 
+describe('getFilesWithConflicts', () => {
+  it('should split by linebreak and remove empty and duplicate items', async () => {
+    const err = {
+      killed: false,
+      code: 2,
+      signal: null,
+      cmd: 'git --no-pager diff --check',
+      stdout:
+        'conflicting-file.txt:1: leftover conflict marker\nconflicting-file.txt:3: leftover conflict marker\nconflicting-file.txt:5: leftover conflict marker\n',
+      stderr: '',
+    };
+    jest.spyOn(childProcess, 'exec').mockRejectedValue(err);
+
+    const options = {
+      repoOwner: 'elastic',
+      repoName: 'kibana',
+    } as BackportOptions;
+
+    expect(await getFilesWithConflicts(options)).toEqual([
+      ' - /myHomeDir/.backport/repositories/elastic/kibana/conflicting-file.txt',
+    ]);
+  });
+});
+
 describe('createFeatureBranch', () => {
   const options = {
     repoOwner: 'elastic',

src/services/git.ts

@@ -7,6 +7,7 @@ import { execAsCallback, exec } from './child-process-promisified';
 import { CommitSelected } from './github/Commit';
 import { logger } from './logger';
 import { resolve as pathResolve } from 'path';
+import uniq from 'lodash.uniq';
 
 async function folderExists(path: string): Promise<boolean> {
   try {
@@ -139,21 +140,33 @@ export async function cherrypickContinue(options: BackportOptions) {
   }
 }
 
-export async function hasConflictMarkers(options: BackportOptions) {
+export async function getFilesWithConflicts(options: BackportOptions) {
+  const repoPath = getRepoPath(options);
   try {
-    await exec(`git --no-pager diff --check`, { cwd: getRepoPath(options) });
-    return false;
+    await exec(`git --no-pager diff --check`, { cwd: repoPath });
+
+    return [];
   } catch (e) {
     const isConflictError = e.cmd && e.code === 2;
     if (isConflictError) {
-      return true;
+      const files = (e.stdout as string)
+        .split('\n')
+        .filter((line: string) => !!line.trim())
+        .map((line: string) => {
+          const posSeparator = line.indexOf(':');
+          const filename = line.slice(0, posSeparator).trim();
+          return ` - ${pathResolve(repoPath, filename)}`;
+        });
+
+      return uniq(files);
     }
 
     // rethrow error since it's unrelated
     throw e;
   }
 }
 
+// retrieve the list of files that could not be cleanly merged
 export async function getUnmergedFiles(options: BackportOptions) {
   const repoPath = getRepoPath(options);
   const res = await exec(`git --no-pager diff --name-only --diff-filter=U`, {

src/services/logger.ts

@@ -9,8 +9,8 @@ const { combine } = format;
 const { argv } = yargs.help(false);
 
 // wrapper around console.log
-export function consoleLog(...args: unknown[]) {
-  console.log(...args);
+export function consoleLog(message: string) {
+  console.log(message);
 }
 
 const level = argv.verbose ? 'verbose' : argv.debug ? 'debug' : 'info';

src/ui/__snapshots__/cherrypickAndCreatePullRequest.test.ts.snap

@@ -21,7 +21,7 @@ Array [
     },
   ],
   Array [
-    "git --no-pager diff --name-only --diff-filter=U",
+    "git --no-pager diff --check",
     Object {
       "cwd": "/myHomeDir/.backport/repositories/elastic/kibana",
     },
@@ -45,7 +45,7 @@ Array [
     },
   ],
   Array [
-    "git --no-pager diff --check",
+    "git --no-pager diff --name-only --diff-filter=U",
     Object {
       "cwd": "/myHomeDir/.backport/repositories/elastic/kibana",
     },

src/ui/cherrypickAndCreatePullRequest.test.ts

@@ -223,28 +223,34 @@ describe('cherrypickAndCreatePullRequest', () => {
       expect(promptSpy.mock.calls).toMatchInlineSnapshot(`
         Array [
           Array [
-            "Resolve the conflicts in the following files and then return here. You do not need to \`git add\` or \`git commit\`:
+            "The following files have conflicts:
          - /myHomeDir/.backport/repositories/elastic/kibana/conflicting-file.txt
 
-        Press ENTER to stage and commit the above files...",
+        You do not need to \`git add\` or \`git commit\` the files - simply fix the conflicts.
+
+        Press ENTER to continue",
           ],
           Array [
-            "Resolve the conflicts in the following files and then return here. You do not need to \`git add\` or \`git commit\`:
+            "The following files have conflicts:
          - /myHomeDir/.backport/repositories/elastic/kibana/conflicting-file.txt
 
-        Press ENTER to stage and commit the above files...",
+        You do not need to \`git add\` or \`git commit\` the files - simply fix the conflicts.
+
+        Press ENTER to continue",
           ],
           Array [
-            "Resolve the conflicts in the following files and then return here. You do not need to \`git add\` or \`git commit\`:
+            "The following files have conflicts:
          - /myHomeDir/.backport/repositories/elastic/kibana/conflicting-file.txt
 
-        Press ENTER to stage and commit the above files...",
+        You do not need to \`git add\` or \`git commit\` the files - simply fix the conflicts.
+
+        Press ENTER to continue",
           ],
           Array [
-            "Resolve the conflicts in the following files and then return here. You do not need to \`git add\` or \`git commit\`:
+            "The following files are unstaged:
          - /myHomeDir/.backport/repositories/elastic/kibana/conflicting-file.txt
 
-        Press ENTER to stage and commit the above files...",
+        Press ENTER to stage them",
           ],
         ]
       `);
@@ -257,6 +263,21 @@ describe('cherrypickAndCreatePullRequest', () => {
          - myCommitMessage
         ",
           ],
+          Array [
+            "",
+          ],
+          Array [
+            "",
+          ],
+          Array [
+            "",
+          ],
+          Array [
+            "",
+          ],
+          Array [
+            "",
+          ],
         ]
       `);
       expect((ora as any).mock.calls).toMatchInlineSnapshot(`

src/ui/cherrypickAndCreatePullRequest.ts

@@ -13,7 +13,7 @@ import {
   getUnmergedFiles,
   addUnstagedFiles,
   cherrypickContinue,
-  hasConflictMarkers,
+  getFilesWithConflicts,
 } from '../services/git';
 import { createPullRequest } from '../services/github/createPullRequest';
 import { getRepoPath } from '../services/env';
@@ -25,6 +25,7 @@ import { withSpinner } from './withSpinner';
 import { confirmPrompt } from '../services/prompts';
 import { HandledError } from '../services/HandledError';
 import dedent = require('dedent');
+import isEmpty = require('lodash.isempty');
 
 export async function cherrypickAndCreatePullRequest({
   options,
@@ -99,22 +100,19 @@ async function waitForCherrypick(
     `Cherry-picking commit ${getShortSha(commit.sha)}`
   ).start();
 
-  let didCherrypick: boolean;
   try {
-    didCherrypick = await cherrypick(options, commit);
-    cherrypickSpinner.succeed();
+    const didCherrypick = await cherrypick(options, commit);
+    if (didCherrypick) {
+      cherrypickSpinner.succeed();
+      return;
+    }
+
+    cherrypickSpinner.fail();
   } catch (e) {
     cherrypickSpinner.fail();
     throw e;
   }
 
-  /*
-   * Commit was cleanly cherrypicked
-   */
-  if (didCherrypick) {
-    return;
-  }
-
   /*
    * Commit could not be cleanly cherrypicked: Initiating conflict resolution
    */
@@ -124,16 +122,19 @@ async function waitForCherrypick(
     await exec(`${options.editor} ${repoPath}`, {});
   }
 
+  // list files with conflict markers and require user to resolve them
+  await listConflictingFiles(options);
+
   // list unmerged files and require user to confirm adding+comitting them
-  await listUnmergedFilesAndWaitForUserConfirmation(options);
+  await listUnstagedFiles(options);
 
   // Conflicts resolved and unstaged files will now be staged and committed
   const stagingSpinner = ora(`Staging and committing files`).start();
   try {
     // add unstaged files
     await addUnstagedFiles(options);
 
-    // continue cherrypick (similar to `git commit`)
+    // Run `cherrypick --continue` (similar to `git commit`)
     await cherrypickContinue(options);
     stagingSpinner.succeed();
   } catch (e) {
@@ -142,32 +143,59 @@ async function waitForCherrypick(
   }
 }
 
-async function listUnmergedFilesAndWaitForUserConfirmation(
-  options: BackportOptions
-) {
-  const unmergedFiles = await getUnmergedFiles(options);
-  const text = dedent(`${chalk.reset(
-    `Resolve the conflicts in the following files and then return here. You do not need to \`git add\` or \`git commit\`:`
-  )}
-  ${chalk.reset(unmergedFiles.join('\n'))}
+async function listConflictingFiles(options: BackportOptions) {
+  const checkForConflicts = async (): Promise<void> => {
+    const filesWithConflicts = await getFilesWithConflicts(options);
+
+    if (isEmpty(filesWithConflicts)) {
+      return;
+    }
 
-  Press ENTER to stage and commit the above files...`);
+    consoleLog(''); // linebreak
+    const res = await confirmPrompt(
+      dedent(`
+        ${chalk.reset(`The following files have conflicts:`)}
+        ${chalk.reset(filesWithConflicts.join('\n'))}
 
-  const checkForConflicts = async () => {
-    const res = await confirmPrompt(text);
+        ${chalk.reset.italic(
+          'You do not need to `git add` or `git commit` the files - simply fix the conflicts.'
+        )}
+
+        Press ENTER to continue
+      `)
+    );
     if (!res) {
       throw new HandledError('Aborted');
     }
 
-    const hasUnresolvedConflicts = await hasConflictMarkers(options);
-    if (hasUnresolvedConflicts) {
-      await checkForConflicts();
-    }
+    await checkForConflicts();
   };
 
   await checkForConflicts();
 }
 
+async function listUnstagedFiles(options: BackportOptions) {
+  const unmergedFiles = await getUnmergedFiles(options);
+
+  if (isEmpty(unmergedFiles)) {
+    return;
+  }
+
+  consoleLog(''); // linebreak
+  const res = await confirmPrompt(
+    dedent(`
+      ${chalk.reset(`The following files are unstaged:`)}
+      ${chalk.reset(unmergedFiles.join('\n'))}
+
+      Press ENTER to stage them
+    `)
+  );
+  if (!res) {
+    throw new HandledError('Aborted');
+  }
+  consoleLog(''); // linebreak
+}
+
 function getPullRequestTitle(
   baseBranch: string,
   commits: CommitSelected[],