From 91eb7fb01fe466468537621cb94b9f932026506e Mon Sep 17 00:00:00 2001 From: Steve King Date: Sun, 23 Jan 2022 09:31:06 +0000 Subject: [PATCH] fix: support parsing empty responses fix: support parsing empty responses - parsers should treat `undefined` in either `stdout` or `stderr` the same as an empty string fix: detect fatal exceptions in `git pull` - In the case that a `git pull` fails with a recognised fatal error (eg: a `--ff-only` pull cannot be fast-forwarded), the exception thrown will be a `GitResponseError` with summary details of the exception rather than a standard process terminated exception Closes #713 --- simple-git/src/lib/parsers/parse-pull.ts | 21 ++++- simple-git/src/lib/responses/PullSummary.ts | 18 +++- simple-git/src/lib/tasks/pull.ts | 12 ++- simple-git/src/lib/utils/util.ts | 2 +- simple-git/test/__fixtures__/setup-init.ts | 2 +- .../test/integration/pull-fails-ff.spec.ts | 87 +++++++++++++++++++ simple-git/test/unit/utils.spec.ts | 7 ++ simple-git/typings/response.d.ts | 17 ++++ simple-git/typings/simple-git.d.ts | 3 +- 9 files changed, 162 insertions(+), 7 deletions(-) create mode 100644 simple-git/test/integration/pull-fails-ff.spec.ts diff --git a/simple-git/src/lib/parsers/parse-pull.ts b/simple-git/src/lib/parsers/parse-pull.ts index 606a30b9..9076fdb2 100644 --- a/simple-git/src/lib/parsers/parse-pull.ts +++ b/simple-git/src/lib/parsers/parse-pull.ts @@ -1,5 +1,5 @@ -import { PullDetail, PullResult, RemoteMessages } from '../../../typings'; -import { PullSummary } from '../responses/PullSummary'; +import { PullDetail, PullFailedResult, PullResult, RemoteMessages } from '../../../typings'; +import { PullFailedSummary, PullSummary } from '../responses/PullSummary'; import { TaskParser } from '../types'; import { append, LineParser, parseStringResponse } from '../utils'; import { parseRemoteMessages } from './parse-remote-messages'; @@ -35,6 +35,17 @@ const parsers: LineParser[] = [ }), ]; +const errorParsers: LineParser[] = [ + new LineParser(/^from\s(.+)$/i, (result, [remote]) => void (result.remote = remote)), + new LineParser(/^fatal:\s(.+)$/, (result, [message]) => void (result.message = message)), + new LineParser(/([a-z0-9]+)\.\.([a-z0-9]+)\s+(\S+)\s+->\s+(\S+)$/, (result, [hashLocal, hashRemote, branchLocal, branchRemote]) => { + result.branch.local = branchLocal; + result.hash.local = hashLocal; + result.branch.remote = branchRemote; + result.hash.remote = hashRemote; + }), +]; + export const parsePullDetail: TaskParser = (stdOut, stdErr) => { return parseStringResponse(new PullSummary(), parsers, stdOut, stdErr); } @@ -46,3 +57,9 @@ export const parsePullResult: TaskParser = (stdOut, stdErr) parseRemoteMessages(stdOut, stdErr), ); } + +export function parsePullErrorResult(stdOut: string, stdErr: string) { + const pullError = parseStringResponse(new PullFailedSummary(), errorParsers, stdOut, stdErr); + + return pullError.message && pullError; +} diff --git a/simple-git/src/lib/responses/PullSummary.ts b/simple-git/src/lib/responses/PullSummary.ts index 928709e1..b943406a 100644 --- a/simple-git/src/lib/responses/PullSummary.ts +++ b/simple-git/src/lib/responses/PullSummary.ts @@ -1,4 +1,4 @@ -import { PullDetailFileChanges, PullDetailSummary, PullResult } from '../../../typings'; +import { PullDetailFileChanges, PullDetailSummary, PullFailedResult, PullResult } from '../../../typings'; export class PullSummary implements PullResult { public remoteMessages = { @@ -16,4 +16,20 @@ export class PullSummary implements PullResult { }; } +export class PullFailedSummary implements PullFailedResult { + remote = ''; + hash = { + local: '', + remote: '', + }; + branch = { + local: '', + remote: '', + }; + message = ''; + + toString() { + return this.message; + } +} diff --git a/simple-git/src/lib/tasks/pull.ts b/simple-git/src/lib/tasks/pull.ts index a8efaee8..0fa4c6f3 100644 --- a/simple-git/src/lib/tasks/pull.ts +++ b/simple-git/src/lib/tasks/pull.ts @@ -1,6 +1,8 @@ import { PullResult } from '../../../typings'; -import { parsePullResult } from '../parsers/parse-pull'; +import { GitResponseError } from '../errors/git-response-error'; +import { parsePullErrorResult, parsePullResult } from '../parsers/parse-pull'; import { Maybe, StringTask } from '../types'; +import { bufferToString } from '../utils'; export function pullTask(remote: Maybe, branch: Maybe, customArgs: string[]): StringTask { const commands: string[] = ['pull', ...customArgs]; @@ -13,6 +15,14 @@ export function pullTask(remote: Maybe, branch: Maybe, customArg format: 'utf-8', parser(stdOut, stdErr): PullResult { return parsePullResult(stdOut, stdErr); + }, + onError(result, _error, _done, fail) { + const pullError = parsePullErrorResult(bufferToString(result.stdOut), bufferToString(result.stdErr)); + if (pullError) { + return fail(new GitResponseError(pullError)); + } + + fail(_error); } } } diff --git a/simple-git/src/lib/utils/util.ts b/simple-git/src/lib/utils/util.ts index 594b2d37..6ffb2749 100644 --- a/simple-git/src/lib/utils/util.ts +++ b/simple-git/src/lib/utils/util.ts @@ -55,7 +55,7 @@ function isArrayLike(input: any): input is ArrayLike { return !!(input && typeof input.length === 'number'); } -export function toLinesWithContent(input: string, trimmed = true, separator = '\n'): string[] { +export function toLinesWithContent(input = '', trimmed = true, separator = '\n'): string[] { return input.split(separator) .reduce((output, line) => { const lineContent = trimmed ? line.trim() : line; diff --git a/simple-git/test/__fixtures__/setup-init.ts b/simple-git/test/__fixtures__/setup-init.ts index b93c9733..75691e5f 100644 --- a/simple-git/test/__fixtures__/setup-init.ts +++ b/simple-git/test/__fixtures__/setup-init.ts @@ -4,7 +4,7 @@ import { SimpleGitTestContext } from './create-test-context'; export const GIT_USER_NAME = 'Simple Git Tests'; export const GIT_USER_EMAIL = 'tests@simple-git.dev'; -export async function setUpInit ({git}: SimpleGitTestContext) { +export async function setUpInit({git}: Pick) { await git.raw('-c', 'init.defaultbranch=master', 'init'); await configureGitCommitter(git); } diff --git a/simple-git/test/integration/pull-fails-ff.spec.ts b/simple-git/test/integration/pull-fails-ff.spec.ts new file mode 100644 index 00000000..1c963923 --- /dev/null +++ b/simple-git/test/integration/pull-fails-ff.spec.ts @@ -0,0 +1,87 @@ +import { promiseError } from '@kwsites/promise-result'; +import { GitResponseError, PullFailedResult } from '../../typings'; +import { createTestContext, like, newSimpleGit, setUpInit, SimpleGitTestContext } from '../__fixtures__'; + +describe('pull --ff-only', () => { + let context: SimpleGitTestContext; + + beforeEach(async () => context = await createTestContext()); + beforeEach(async () => { + const upstream = await context.dir('upstream'); + const local = context.path('local'); + await context.file(['upstream', 'file']); + + await givenRemote(upstream); + await givenLocal(upstream, local); + }); + + async function givenLocal(upstream: string, local: string) { + await newSimpleGit(context.root).clone(upstream, local); + await setUpInit({git: newSimpleGit(local)}); + } + + async function givenRemote(upstream: string) { + const git = newSimpleGit(upstream); + await setUpInit({git}); + await git.add('.'); + await git.commit('first'); + } + + async function givenRemoteFileChanged() { + await context.file(['upstream', 'file'], 'new remote file content'); + await newSimpleGit(context.path('upstream')).add('.').commit('remote updated'); + } + + async function givenLocalFileChanged() { + await context.file(['local', 'file'], 'new local file content'); + await newSimpleGit(context.path('local')).add('.').commit('local updated'); + } + + it('allows fast-forward when there are no changes local or remote', async () => { + const git = newSimpleGit(context.path('local')); + const result = await git.pull(['--ff-only']); + + expect(result.files).toEqual([]); + }); + + it('allows fast-forward when there are some remote but no local changes', async () => { + await givenRemoteFileChanged(); + + const git = newSimpleGit(context.path('local')); + const result = await git.pull(['--ff-only']); + + expect(result.files).toEqual(['file']); + }); + + it('allows fast-forward when there are no remote but some local changes', async () => { + await givenLocalFileChanged(); + + const git = newSimpleGit(context.path('local')); + const result = await git.pull(['--ff-only']); + + expect(result.files).toEqual([]); + }); + + it('fails fast-forward when there are both remote and local changes', async () => { + await givenLocalFileChanged(); + await givenRemoteFileChanged(); + + const git = newSimpleGit(context.path('local')); + const err = await promiseError>(git.pull(['--ff-only'])); + + expect(err?.git.message).toMatch('Not possible to fast-forward, aborting'); + expect(err?.git).toEqual(like({ + remote: context.path('upstream'), + hash: { + local: expect.any(String), + remote: expect.any(String), + }, + branch: { + local: expect.any(String), + remote: expect.any(String), + }, + message: String(err?.git), + })) + }); + +}); diff --git a/simple-git/test/unit/utils.spec.ts b/simple-git/test/unit/utils.spec.ts index 694d3af2..cec5824a 100644 --- a/simple-git/test/unit/utils.spec.ts +++ b/simple-git/test/unit/utils.spec.ts @@ -60,6 +60,13 @@ describe('utils', () => { describe('content', () => { + it('caters for empty values', () => { + expect(toLinesWithContent()).toEqual([]); + expect(toLinesWithContent(undefined, false)).toEqual([]); + expect(toLinesWithContent('')).toEqual([]); + expect(toLinesWithContent('', false)).toEqual([]); + }); + it('filters lines with content', () => { expect(toLinesWithContent(' \n content \n\n')).toEqual(['content']); expect(toLinesWithContent(' \n content \n\n', false)).toEqual([' ', ' content ']); diff --git a/simple-git/typings/response.d.ts b/simple-git/typings/response.d.ts index d115eeed..ab619126 100644 --- a/simple-git/typings/response.d.ts +++ b/simple-git/typings/response.d.ts @@ -256,6 +256,23 @@ export interface PullDetail { export interface PullResult extends PullDetail, RemoteMessageResult { } +/** + * Wrapped with the `GitResponseError` as the exception thrown from a `git.pull` task + * to provide additional detail as to what failed. + */ +export interface PullFailedResult { + remote: string, + hash: { + local: string; + remote: string; + }, + branch: { + local: string; + remote: string; + }, + message: string; +} + /** * Represents file name changes in a StatusResult */ diff --git a/simple-git/typings/simple-git.d.ts b/simple-git/typings/simple-git.d.ts index 84062948..af54d773 100644 --- a/simple-git/typings/simple-git.d.ts +++ b/simple-git/typings/simple-git.d.ts @@ -442,7 +442,8 @@ export interface SimpleGit extends SimpleGitBase { mv(from: string | string[], to: string, callback?: types.SimpleGitTaskCallback): Response; /** - * Fetch from and integrate with another repository or a local branch. + * Fetch from and integrate with another repository or a local branch. In the case that the `git pull` fails with a + * recognised fatal error, the exception thrown by this function will be a `GitResponseError`. */ pull(remote?: string, branch?: string, options?: types.TaskOptions, callback?: types.SimpleGitTaskCallback): Response;