From 22d48599335fdd14cdf0b8b62fc0a948fcd4867d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Tue, 7 Apr 2020 14:57:28 +0200 Subject: [PATCH 1/4] Split conflict resolution and staging into separate steps --- src/services/git.test.ts | 25 ++++++ src/services/git.ts | 21 ++++- src/services/logger.ts | 4 +- ...herrypickAndCreatePullRequest.test.ts.snap | 4 +- src/ui/cherrypickAndCreatePullRequest.test.ts | 14 ++-- src/ui/cherrypickAndCreatePullRequest.ts | 79 ++++++++++++------- 6 files changed, 104 insertions(+), 43 deletions(-) diff --git a/src/services/git.test.ts b/src/services/git.test.ts index f31d12b4..fa48ef37 100644 --- a/src/services/git.test.ts +++ b/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', diff --git a/src/services/git.ts b/src/services/git.ts index 8a05b2c4..59a3f47a 100644 --- a/src/services/git.ts +++ b/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 { try { @@ -139,14 +140,25 @@ 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 @@ -154,6 +166,7 @@ export async function hasConflictMarkers(options: BackportOptions) { } } +// 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`, { diff --git a/src/services/logger.ts b/src/services/logger.ts index 0f512cc2..b2b59901 100644 --- a/src/services/logger.ts +++ b/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'; diff --git a/src/ui/__snapshots__/cherrypickAndCreatePullRequest.test.ts.snap b/src/ui/__snapshots__/cherrypickAndCreatePullRequest.test.ts.snap index 81867b93..3ce63ec3 100644 --- a/src/ui/__snapshots__/cherrypickAndCreatePullRequest.test.ts.snap +++ b/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", }, diff --git a/src/ui/cherrypickAndCreatePullRequest.test.ts b/src/ui/cherrypickAndCreatePullRequest.test.ts index b691c461..02992e82 100644 --- a/src/ui/cherrypickAndCreatePullRequest.test.ts +++ b/src/ui/cherrypickAndCreatePullRequest.test.ts @@ -223,25 +223,25 @@ 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\`: + "Resolve the conflicts in the following files. You do not need to \`git add\` or \`git commit\`:  - /myHomeDir/.backport/repositories/elastic/kibana/conflicting-file.txt - Press ENTER to stage and commit the above files...", + Press ENTER when the conflicts have been resolved", ], Array [ - "Resolve the conflicts in the following files and then return here. You do not need to \`git add\` or \`git commit\`: + "Resolve the conflicts in the following files. You do not need to \`git add\` or \`git commit\`:  - /myHomeDir/.backport/repositories/elastic/kibana/conflicting-file.txt - Press ENTER to stage and commit the above files...", + Press ENTER when the conflicts have been resolved", ], Array [ - "Resolve the conflicts in the following files and then return here. You do not need to \`git add\` or \`git commit\`: + "Resolve the conflicts in the following files. You do not need to \`git add\` or \`git commit\`:  - /myHomeDir/.backport/repositories/elastic/kibana/conflicting-file.txt - Press ENTER to stage and commit the above files...", + Press ENTER when the conflicts have been resolved", ], 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...", diff --git a/src/ui/cherrypickAndCreatePullRequest.ts b/src/ui/cherrypickAndCreatePullRequest.ts index 62ed36d9..cb73bf58 100644 --- a/src/ui/cherrypickAndCreatePullRequest.ts +++ b/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,8 +122,11 @@ 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(); @@ -133,7 +134,7 @@ async function waitForCherrypick( // 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,54 @@ 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 => { + const filesWithConflicts = await getFilesWithConflicts(options); + + if (isEmpty(filesWithConflicts)) { + return; + } - Press ENTER to stage and commit the above files...`); + const res = await confirmPrompt( + dedent(` + ${chalk.reset( + `Resolve the conflicts in the following files. You do not need to \`git add\` or \`git commit\`:` + )} + ${chalk.reset(filesWithConflicts.join('\n'))} - const checkForConflicts = async () => { - const res = await confirmPrompt(text); + Press ENTER when the conflicts have been resolved + `) + ); 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; + } + + const res = await confirmPrompt( + dedent(` + ${chalk.reset(`The following files are unstaged:`)} + ${chalk.reset(unmergedFiles.join('\n'))} + + Press ENTER to stage and commit the above files... + `) + ); + if (!res) { + throw new HandledError('Aborted'); + } +} + function getPullRequestTitle( baseBranch: string, commits: CommitSelected[], From 4446803b324dc6a05bbf1469937f46591af3b918 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Tue, 7 Apr 2020 15:05:40 +0200 Subject: [PATCH 2/4] Add visual linebreaks --- src/ui/cherrypickAndCreatePullRequest.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/ui/cherrypickAndCreatePullRequest.ts b/src/ui/cherrypickAndCreatePullRequest.ts index cb73bf58..46924f46 100644 --- a/src/ui/cherrypickAndCreatePullRequest.ts +++ b/src/ui/cherrypickAndCreatePullRequest.ts @@ -151,6 +151,7 @@ async function listConflictingFiles(options: BackportOptions) { return; } + consoleLog(''); // linebreak const res = await confirmPrompt( dedent(` ${chalk.reset( @@ -178,6 +179,7 @@ async function listUnstagedFiles(options: BackportOptions) { return; } + consoleLog(''); // linebreak const res = await confirmPrompt( dedent(` ${chalk.reset(`The following files are unstaged:`)} @@ -189,6 +191,7 @@ async function listUnstagedFiles(options: BackportOptions) { if (!res) { throw new HandledError('Aborted'); } + consoleLog(''); // linebreak } function getPullRequestTitle( From edd8f3e96004ac1a810172ff0cb2a226b9f34b74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Tue, 7 Apr 2020 15:11:05 +0200 Subject: [PATCH 3/4] Minor copy fixes --- src/ui/cherrypickAndCreatePullRequest.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/ui/cherrypickAndCreatePullRequest.ts b/src/ui/cherrypickAndCreatePullRequest.ts index 46924f46..ecb3a1e4 100644 --- a/src/ui/cherrypickAndCreatePullRequest.ts +++ b/src/ui/cherrypickAndCreatePullRequest.ts @@ -154,12 +154,14 @@ async function listConflictingFiles(options: BackportOptions) { consoleLog(''); // linebreak const res = await confirmPrompt( dedent(` - ${chalk.reset( - `Resolve the conflicts in the following files. You do not need to \`git add\` or \`git commit\`:` - )} + ${chalk.reset(`The following files have conflicts:`)} ${chalk.reset(filesWithConflicts.join('\n'))} - Press ENTER when the conflicts have been resolved + ${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) { @@ -185,7 +187,7 @@ async function listUnstagedFiles(options: BackportOptions) { ${chalk.reset(`The following files are unstaged:`)} ${chalk.reset(unmergedFiles.join('\n'))} - Press ENTER to stage and commit the above files... + Press ENTER to stage them `) ); if (!res) { From 004db64723ba441f5f81687fd1d0a461fe0b3a59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Tue, 7 Apr 2020 15:33:13 +0200 Subject: [PATCH 4/4] update snapshot --- src/ui/cherrypickAndCreatePullRequest.test.ts | 35 +++++++++++++++---- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/src/ui/cherrypickAndCreatePullRequest.test.ts b/src/ui/cherrypickAndCreatePullRequest.test.ts index 02992e82..2046f74f 100644 --- a/src/ui/cherrypickAndCreatePullRequest.test.ts +++ b/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. 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 when the conflicts have been resolved", + 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. 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 when the conflicts have been resolved", + 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. 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 when the conflicts have been resolved", + You do not need to \`git add\` or \`git commit\` the files - simply fix the conflicts. + + Press ENTER to continue", ], Array [ "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(`