Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mainline option #179

Merged
merged 6 commits into from
Apr 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
9 changes: 9 additions & 0 deletions docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<space>` to select, and `<enter>` to confirm you selection.
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
19 changes: 19 additions & 0 deletions src/options/cliArgs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
1 change: 1 addition & 0 deletions src/options/options.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ describe('validateRequiredOptions', () => {
fork: true,
gitHostname: 'github.com',
labels: [],
mainline: undefined,
multiple: false,
multipleBranches: true,
multipleCommits: false,
Expand Down
1 change: 1 addition & 0 deletions src/runWithOptions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ describe('runWithOptions', () => {
fork: true,
gitHostname: 'github.com',
labels: [],
mainline: undefined,
multiple: false,
multipleBranches: false,
multipleCommits: false,
Expand Down
5 changes: 2 additions & 3 deletions src/services/child-process-promisified.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
136 changes: 116 additions & 20 deletions src/services/git.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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 <parent-number>

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)
Expand All @@ -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(
Expand Down
26 changes: 19 additions & 7 deletions src/services/git.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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 <parent-number>\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;
}
}

Expand Down
21 changes: 9 additions & 12 deletions src/test/ExecError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading