From 24be0791da79ff7e9e0b07789dc33cf38809bee8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Wed, 15 Apr 2020 21:36:32 +0200 Subject: [PATCH] Add `mainline` option (#179) --- README.md | 1 + docs/configuration.md | 9 ++ package.json | 2 +- src/options/cliArgs.ts | 19 +++ src/options/options.test.ts | 1 + src/runWithOptions.test.ts | 1 + src/services/child-process-promisified.ts | 5 +- src/services/git.test.ts | 136 +++++++++++++++--- src/services/git.ts | 26 +++- src/test/ExecError.ts | 21 ++- .../__snapshots__/integration.test.ts.snap | 60 ++------ ...herrypickAndCreatePullRequest.test.ts.snap | 6 + src/ui/cherrypickAndCreatePullRequest.test.ts | 17 +-- src/ui/cherrypickAndCreatePullRequest.ts | 4 +- 14 files changed, 199 insertions(+), 109 deletions(-) diff --git a/README.md b/README.md index b0e44377..e176a778 100644 --- a/README.md +++ b/README.md @@ -106,6 +106,7 @@ The above commands will start an interactive prompt. You can use the `arrow keys | --github-api-base-url-v3 | Base url for Github's Rest (v3) API | https://api.github.com | string | | --github-api-base-url-v4 | Base url for Github's GraphQL (v4) API | https://api.github.com/graphql | string | | --labels | Pull request labels | | string | +| --mainline | Parent id of merge commit | 1 (when no parent id given) | number | | --multiple | Select multiple commits/branches | false | boolean | | --path | Only list commits touching files under a specific path | | string | | --pr-description | Pull request description suffix | | string | diff --git a/docs/configuration.md b/docs/configuration.md index 5ebb9ad3..f33b93fa 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -117,6 +117,15 @@ Config: } ``` +#### `mainline` + +When backporting a merge commit the parent id must be specified. This is directly passed to `git cherry-pick` and additional details can be read on the [Git Docs](https://git-scm.com/docs/git-cherry-pick#Documentation/git-cherry-pick.txt---mainlineparent-number) + +**Examples:** + +- Defaults to 1 when no parent id is given: `backport --mainline` +- Specifying parent id: `backport --mainline 2` + #### `multipleCommits` `true`: you will be able to select multiple commits to backport. You will use `` to select, and `` to confirm you selection. diff --git a/package.json b/package.json index 0c8fbd5b..29b993ae 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,7 @@ "branches", "branching" ], - "version": "5.1.5", + "version": "5.1.6-beta.1", "main": "./dist/index.js", "bin": { "backport": "./dist/index.js" diff --git a/src/options/cliArgs.ts b/src/options/cliArgs.ts index 8cae05b1..3d5714b5 100644 --- a/src/options/cliArgs.ts +++ b/src/options/cliArgs.ts @@ -75,6 +75,25 @@ export function getOptionsFromCliArgs( description: 'Pull request labels for the resulting backport PRs', type: 'array', }) + .option('mainline', { + description: + 'Parent id of merge commit. Defaults to 1 when supplied without arguments', + type: 'number', + coerce: (mainline) => { + // `--mainline` (default to 1 when no parent is given) + if (mainline === undefined) { + return 1; + } + + // use specified mainline parent + if (Number.isInteger(mainline)) { + return mainline as number; + } + + // Invalid value provided + throw new Error(`--mainline must be an integer. Received: ${mainline}`); + }, + }) .option('multiple', { default: configOptions.multiple, description: 'Select multiple branches/commits', diff --git a/src/options/options.test.ts b/src/options/options.test.ts index e90702ea..b07c2850 100644 --- a/src/options/options.test.ts +++ b/src/options/options.test.ts @@ -115,6 +115,7 @@ describe('validateRequiredOptions', () => { fork: true, gitHostname: 'github.com', labels: [], + mainline: undefined, multiple: false, multipleBranches: true, multipleCommits: false, diff --git a/src/runWithOptions.test.ts b/src/runWithOptions.test.ts index de5003dd..3d462585 100644 --- a/src/runWithOptions.test.ts +++ b/src/runWithOptions.test.ts @@ -39,6 +39,7 @@ describe('runWithOptions', () => { fork: true, gitHostname: 'github.com', labels: [], + mainline: undefined, multiple: false, multipleBranches: false, multipleCommits: false, diff --git a/src/services/child-process-promisified.ts b/src/services/child-process-promisified.ts index 477a4d3c..839411ec 100644 --- a/src/services/child-process-promisified.ts +++ b/src/services/child-process-promisified.ts @@ -3,17 +3,16 @@ import { promisify } from 'util'; import { logger } from './logger'; export async function exec(cmd: string, options: child_process.ExecOptions) { - logger.info(`exec: \`${cmd}\``); const execPromisified = promisify(child_process.exec); try { const res = await execPromisified(cmd, { maxBuffer: 100 * 1024 * 1024, ...options, }); - logger.verbose(`exec response (success):`, res); + logger.verbose(`exec success '${cmd}':`, res); return res; } catch (e) { - logger.info(`exec response (error):`, e); + logger.info(`exec error '${cmd}':`, e); throw e; } } diff --git a/src/services/git.test.ts b/src/services/git.test.ts index 936f44d7..05527808 100644 --- a/src/services/git.test.ts +++ b/src/services/git.test.ts @@ -9,6 +9,11 @@ import { cherrypick, getFilesWithConflicts, } from '../services/git'; +import { ExecError } from '../test/ExecError'; + +beforeEach(() => { + jest.clearAllMocks(); +}); describe('getUnmergedFiles', () => { it('should split lines and remove empty', async () => { @@ -54,7 +59,7 @@ describe('getFilesWithConflicts', () => { '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); + jest.spyOn(childProcess, 'exec').mockRejectedValueOnce(err); const options = { repoOwner: 'elastic', @@ -88,7 +93,7 @@ describe('createFeatureBranch', () => { stderr: "fatal: couldn't find remote ref 4.x\n", }; - jest.spyOn(childProcess, 'exec').mockRejectedValue(err); + jest.spyOn(childProcess, 'exec').mockRejectedValueOnce(err); await expect( createFeatureBranch(options, baseBranch, featureBranch) ).rejects.toThrowErrorMatchingInlineSnapshot( @@ -99,7 +104,7 @@ describe('createFeatureBranch', () => { it('should rethrow normal error', async () => { expect.assertions(1); const err = new Error('just a normal error'); - jest.spyOn(childProcess, 'exec').mockRejectedValue(err); + jest.spyOn(childProcess, 'exec').mockRejectedValueOnce(err); expect.assertions(1); await expect( @@ -125,13 +130,13 @@ describe('deleteRemote', () => { stderr: "fatal: No such remote: 'origin'\n", }; - jest.spyOn(childProcess, 'exec').mockRejectedValue(err); + jest.spyOn(childProcess, 'exec').mockRejectedValueOnce(err); await expect(await deleteRemote(options, remoteName)).toBe(undefined); }); it('should rethrow normal error', async () => { const err = new Error('just a normal error'); - jest.spyOn(childProcess, 'exec').mockRejectedValue(err); + jest.spyOn(childProcess, 'exec').mockRejectedValueOnce(err); expect.assertions(1); await expect( @@ -152,29 +157,120 @@ describe('cherrypick', () => { sha: 'abcd', }; - it('should swallow cherrypick error', async () => { + it('should return `needsResolving: false` when no errors are encountered', async () => { jest .spyOn(childProcess, 'exec') + + // mock git fetch + .mockResolvedValueOnce({ stderr: '', stdout: '' }) + + // mock cherry pick command .mockResolvedValueOnce({ stderr: '', stdout: '' }); - jest.spyOn(childProcess, 'exec').mockRejectedValueOnce({ - killed: false, - code: 128, - signal: null, - cmd: 'git cherry-pick abcd', - stdout: '', - stderr: '', + expect(await cherrypick(options, commit)).toEqual({ + needsResolving: false, }); - await expect(await cherrypick(options, commit)).toBe(false); }); - it('should re-throw other errors', async () => { - const err = new Error('non-cherrypick error'); - jest + it('should use mainline option when specified', async () => { + const execSpy = jest .spyOn(childProcess, 'exec') + + // mock git fetch + .mockResolvedValueOnce({ stderr: '', stdout: '' }) + + // mock cherry pick command .mockResolvedValueOnce({ stderr: '', stdout: '' }); - jest.spyOn(childProcess, 'exec').mockRejectedValueOnce(err); + await cherrypick({ ...options, mainline: 1 }, commit); + + expect(execSpy.mock.calls[1][0]).toBe('git cherry-pick --mainline 1 abcd'); + }); + + it('should return `needsResolving: true` upon cherrypick error', async () => { + jest + .spyOn(childProcess, 'exec') + + // mock git fetch + .mockResolvedValueOnce({ stderr: '', stdout: '' }) + + // mock cherry pick command + .mockRejectedValueOnce( + new ExecError({ + killed: false, + code: 128, + cmd: 'git cherry-pick abcd', + stdout: '', + stderr: '', + }) + ) + + // mock getFilesWithConflicts + .mockRejectedValueOnce( + new ExecError({ + code: 2, + 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: '', + }) + ) + + // mock getUnmergedFiles + .mockResolvedValueOnce({ stdout: '', stderr: '' }); + + expect(await cherrypick(options, commit)).toEqual({ + needsResolving: true, + }); + }); + + it('it should let the user know about the "--mainline" argument when cherry-picking a merge commit without specifying it', async () => { + jest + .spyOn(childProcess, 'exec') + + // mock git fetch + .mockResolvedValueOnce({ stderr: '', stdout: '' }) + + // mock cherry pick command + .mockRejectedValueOnce( + new ExecError({ + killed: false, + code: 128, + signal: null, + cmd: 'git cherry-pick 381c7b604110257437a289b1f1742685eb8d79c5', + stdout: '', + stderr: + 'error: commit 381c7b604110257437a289b1f1742685eb8d79c5 is a merge but no -m option was given.\nfatal: cherry-pick failed\n', + }) + ); + + await expect(cherrypick(options, commit)).rejects + .toThrowError(`Failed to cherrypick because the selected commit was a merge. Please try again by specifying the parent with the \`mainline\` argument: + +> backport --mainline + +or: + +> backport --mainline + +Or refer to the git documentation for more information: https://git-scm.com/docs/git-cherry-pick#Documentation/git-cherry-pick.txt---mainlineparent-number`); + }); + + it('should re-throw non-cherrypick errors', async () => { + jest + .spyOn(childProcess, 'exec') + + // mock git fetch + .mockResolvedValueOnce({ stderr: '', stdout: '' }) + + // mock cherry pick command + .mockRejectedValueOnce(new Error('non-cherrypick error')) + + // getFilesWithConflicts + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + + // getUnmergedFiles + .mockResolvedValueOnce({ stdout: '', stderr: '' }); await expect( cherrypick(options, commit) @@ -199,13 +295,13 @@ describe('cherrypickContinue', () => { 'error: no cherry-pick or revert in progress\nfatal: cherry-pick failed\n', }; - jest.spyOn(childProcess, 'exec').mockRejectedValue(err); + jest.spyOn(childProcess, 'exec').mockRejectedValueOnce(err); await expect(await cherrypickContinue(options)).toBe(undefined); }); it('should re-throw other errors', async () => { const err = new Error('non-cherrypick error'); - jest.spyOn(childProcess, 'exec').mockRejectedValue(err); + jest.spyOn(childProcess, 'exec').mockRejectedValueOnce(err); expect.assertions(1); await expect( diff --git a/src/services/git.ts b/src/services/git.ts index e111c6a0..9a3aad42 100644 --- a/src/services/git.ts +++ b/src/services/git.ts @@ -1,5 +1,6 @@ import { resolve as pathResolve } from 'path'; import del from 'del'; +import isEmpty from 'lodash.isempty'; import uniq from 'lodash.uniq'; import { BackportOptions } from '../options/options'; import { CommitSelected } from '../types/Commit'; @@ -105,19 +106,30 @@ export async function cherrypick( `git fetch ${options.repoOwner} ${commit.branch}:${commit.branch} --force`, { cwd: getRepoPath(options) } ); + const mainline = + options.mainline != undefined ? ` --mainline ${options.mainline}` : ''; - const cmd = `git cherry-pick ${commit.sha}`; + const cmd = `git cherry-pick${mainline} ${commit.sha}`; try { await exec(cmd, { cwd: getRepoPath(options) }); - return true; + return { needsResolving: false }; } catch (e) { - // re-throw unknown errors - if (e.cmd !== cmd) { - throw e; + if (e.message.includes('is a merge but no -m option was given')) { + throw new HandledError( + 'Failed to cherrypick because the selected commit was a merge. Please try again by specifying the parent with the `mainline` argument:\n\n> backport --mainline\n\nor:\n\n> backport --mainline \n\nOr refer to the git documentation for more information: https://git-scm.com/docs/git-cherry-pick#Documentation/git-cherry-pick.txt---mainlineparent-number' + ); } - // handle cherrypick-related error - return false; + const isCherryPickError = e.cmd === cmd; + const hasConflicts = !isEmpty(await getFilesWithConflicts(options)); + const hasUnmergedFiles = !isEmpty(await getUnmergedFiles(options)); + + if (isCherryPickError && (hasConflicts || hasUnmergedFiles)) { + return { needsResolving: true }; + } + + // re-throw error if there are no conflicts to solve + throw e; } } diff --git a/src/test/ExecError.ts b/src/test/ExecError.ts index 64e22d06..567bc756 100644 --- a/src/test/ExecError.ts +++ b/src/test/ExecError.ts @@ -3,18 +3,15 @@ export class ExecError extends Error { cmd: string | undefined; stdout: string | undefined; stderr: string | undefined; - constructor( - public message: string, - error: { - cmd?: string; - killed?: boolean; - code?: number; - signal?: NodeJS.Signals; - stdout?: string; - stderr?: string; - } - ) { - super(message); + constructor(error: { + cmd?: string; + killed?: boolean; + code?: number; + signal?: NodeJS.Signals | null; + stdout?: string; + stderr?: string; + }) { + super(error.stderr); this.name = 'ExecError'; this.code = error.code; this.cmd = error.cmd; diff --git a/src/test/integration/__snapshots__/integration.test.ts.snap b/src/test/integration/__snapshots__/integration.test.ts.snap index 3b856031..67ac9751 100644 --- a/src/test/integration/__snapshots__/integration.test.ts.snap +++ b/src/test/integration/__snapshots__/integration.test.ts.snap @@ -430,47 +430,27 @@ Array [ }, ], Array [ - "[INFO] exec: \`git remote rm origin\`", - undefined, - ], - Array [ - "[VERBOSE] exec response (success):", + "[VERBOSE] exec success 'git remote rm origin':", "", ], Array [ - "[INFO] exec: \`git remote rm sqren\`", - undefined, - ], - Array [ - "[INFO] exec response (error):", + "[INFO] exec error 'git remote rm sqren':", [Error: Command failed: git remote rm sqren fatal: No such remote: 'sqren' ], ], Array [ - "[INFO] exec: \`git remote add sqren https://myAccessToken@github.com/sqren/backport-demo.git\`", - undefined, - ], - Array [ - "[VERBOSE] exec response (success):", + "[VERBOSE] exec success 'git remote add sqren https://myAccessToken@github.com/sqren/backport-demo.git':", "", ], Array [ - "[INFO] exec: \`git remote rm elastic\`", - undefined, - ], - Array [ - "[INFO] exec response (error):", + "[INFO] exec error 'git remote rm elastic':", [Error: Command failed: git remote rm elastic fatal: No such remote: 'elastic' ], ], Array [ - "[INFO] exec: \`git remote add elastic https://myAccessToken@github.com/elastic/backport-demo.git\`", - undefined, - ], - Array [ - "[VERBOSE] exec response (success):", + "[VERBOSE] exec success 'git remote add elastic https://myAccessToken@github.com/elastic/backport-demo.git':", "", ], Array [ @@ -482,28 +462,16 @@ fatal: No such remote: 'elastic' undefined, ], Array [ - "[INFO] exec: \`git reset --hard && git clean -d --force && git fetch elastic 6.0 && git checkout -B backport/6.0/pr-85 elastic/6.0 --no-track\`", - undefined, - ], - Array [ - "[VERBOSE] exec response (success):", + "[VERBOSE] exec success 'git reset --hard && git clean -d --force && git fetch elastic 6.0 && git checkout -B backport/6.0/pr-85 elastic/6.0 --no-track':", "HEAD is now at Add 👻 ", ], Array [ - "[INFO] exec: \`git fetch elastic master:master --force\`", - undefined, - ], - Array [ - "[VERBOSE] exec response (success):", + "[VERBOSE] exec success 'git fetch elastic master:master --force':", "", ], Array [ - "[INFO] exec: \`git cherry-pick f3b618b9421fdecdb36862f907afbdd6344b361d\`", - undefined, - ], - Array [ - "[VERBOSE] exec response (success):", + "[VERBOSE] exec success 'git cherry-pick f3b618b9421fdecdb36862f907afbdd6344b361d':", "[backport/6.0/pr-85 ] Add witch (#85) Date: Thu Apr 11 00:46:08 2019 +0200 1 file changed, 1 insertion(+), 1 deletion(-) @@ -514,19 +482,11 @@ fatal: No such remote: 'elastic' undefined, ], Array [ - "[INFO] exec: \`git push sqren backport/6.0/pr-85:backport/6.0/pr-85 --force\`", - undefined, - ], - Array [ - "[VERBOSE] exec response (success):", + "[VERBOSE] exec success 'git push sqren backport/6.0/pr-85:backport/6.0/pr-85 --force':", "", ], Array [ - "[INFO] exec: \`git checkout master && git branch -D backport/6.0/pr-85\`", - undefined, - ], - Array [ - "[VERBOSE] exec response (success):", + "[VERBOSE] exec success 'git checkout master && git branch -D backport/6.0/pr-85':", "Deleted branch backport/6.0/pr-85 (was ). ", ], diff --git a/src/ui/__snapshots__/cherrypickAndCreatePullRequest.test.ts.snap b/src/ui/__snapshots__/cherrypickAndCreatePullRequest.test.ts.snap index 3ce63ec3..edb57d4f 100644 --- a/src/ui/__snapshots__/cherrypickAndCreatePullRequest.test.ts.snap +++ b/src/ui/__snapshots__/cherrypickAndCreatePullRequest.test.ts.snap @@ -26,6 +26,12 @@ Array [ "cwd": "/myHomeDir/.backport/repositories/elastic/kibana", }, ], + Array [ + "git --no-pager diff --name-only --diff-filter=U", + Object { + "cwd": "/myHomeDir/.backport/repositories/elastic/kibana", + }, + ], Array [ "git --no-pager diff --check", Object { diff --git a/src/ui/cherrypickAndCreatePullRequest.test.ts b/src/ui/cherrypickAndCreatePullRequest.test.ts index fe04dfc8..cb4b1bb1 100644 --- a/src/ui/cherrypickAndCreatePullRequest.test.ts +++ b/src/ui/cherrypickAndCreatePullRequest.test.ts @@ -237,14 +237,6 @@ describe('cherrypickAndCreatePullRequest', () => { You do not need to \`git add\` or \`git commit\` the files - simply fix the conflicts. - Press ENTER to continue", - ], - Array [ - "The following files have conflicts: - - /myHomeDir/.backport/repositories/elastic/kibana/conflicting-file.txt - - You do not need to \`git add\` or \`git commit\` the files - simply fix the conflicts. - Press ENTER to continue", ], Array [ @@ -276,9 +268,6 @@ describe('cherrypickAndCreatePullRequest', () => { Array [ "", ], - Array [ - "", - ], ] `); expect((ora as any).mock.calls).toMatchInlineSnapshot(` @@ -324,17 +313,17 @@ function setupExecSpy() { // cherrypick if (cmd === 'git cherry-pick mySha') { - throw new ExecError('cherrypick failed', { cmd }); + throw new ExecError({ cmd }); } - // filesWithConflicts + // getFilesWithConflicts if (cmd === 'git --no-pager diff --check') { conflictCheckCounts++; if (conflictCheckCounts >= 4) { return { stderr: '', stdout: '' }; } - throw new ExecError('Not all conflicts resolved', { + throw new ExecError({ code: 2, cmd, stdout: `conflicting-file.txt:1: leftover conflict marker\nconflicting-file.txt:3: leftover conflict marker\nconflicting-file.txt:5: leftover conflict marker\n`, diff --git a/src/ui/cherrypickAndCreatePullRequest.ts b/src/ui/cherrypickAndCreatePullRequest.ts index 1f48dfbf..a0cbd9d3 100644 --- a/src/ui/cherrypickAndCreatePullRequest.ts +++ b/src/ui/cherrypickAndCreatePullRequest.ts @@ -101,8 +101,8 @@ async function waitForCherrypick( ).start(); try { - const didCherrypick = await cherrypick(options, commit); - if (didCherrypick) { + const { needsResolving } = await cherrypick(options, commit); + if (!needsResolving) { cherrypickSpinner.succeed(); return; }