Skip to content

Commit

Permalink
Treat non-zero exit code as error by default
Browse files Browse the repository at this point in the history
  • Loading branch information
@niik
niik committed Oct 23, 2024
1 parent 3c08252 commit d0c2a1d
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 82 deletions.
6 changes: 4 additions & 2 deletions lib/errors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,12 @@ export const GitErrorRegexes: { [regexp: string]: GitError } = {
export class ExecError extends Error {
/**
* The error.code property is a string label that identifies the kind of error
* or, in the case of code being a Number it's indicating the exit code of
* the executed process.
*
* See https://nodejs.org/api/errors.html#errorcode
*/
public readonly code?: string
public readonly code?: string | number

/**
* The signal that terminated the process
Expand All @@ -199,7 +201,7 @@ export class ExecError extends Error {

if (cause && typeof cause === 'object') {
if ('code' in cause) {
if (typeof cause.code === 'string') {
if (typeof cause.code === 'string' || typeof cause.code === 'number') {
this.code = cause.code
}
}
Expand Down
76 changes: 46 additions & 30 deletions lib/exec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ import { ChildProcess, execFile, ExecFileOptions } from 'child_process'
import { setupEnvironment } from './git-environment'
import { ExecError } from './errors'
import { ignoreClosedInputStream } from './ignore-closed-input-stream'
import { promisify } from 'util'

const execFileAsync = promisify(execFile)

export interface IGitResult {
/** The standard output from git. */
Expand Down Expand Up @@ -95,6 +98,8 @@ export interface IGitExecutionOptions {
* AbortSignal being triggered. Defaults to 'SIGTERM'
*/
readonly killSignal?: ExecFileOptions['killSignal']

readonly ignoreExitCodes?: ReadonlyArray<number> | true
}

export interface IGitStringExecutionOptions extends IGitExecutionOptions {
Expand All @@ -105,6 +110,17 @@ export interface IGitBufferExecutionOptions extends IGitExecutionOptions {
readonly encoding: 'buffer'
}

const coerceStdio = (
stdio: string | Buffer,
encoding: BufferEncoding | 'buffer'
) => {
if (encoding === 'buffer') {
return Buffer.isBuffer(stdio) ? stdio : Buffer.from(stdio)
}

return Buffer.isBuffer(stdio) ? stdio.toString(encoding) : stdio
}

/**
* Execute a command and read the output using the embedded Git environment.
*
Expand Down Expand Up @@ -141,42 +157,42 @@ export function exec(
killSignal: options?.killSignal,
}

return new Promise<IGitResult>((resolve, reject) => {
const cp = execFile(gitLocation, args, opts, (err, stdout, stderr) => {
if (!err || typeof err.code === 'number') {
const exitCode = typeof err?.code === 'number' ? err.code : 0
resolve({ stdout, stderr, exitCode })
return
}

// If the error's code is a string then it means the code isn't the
// process's exit code but rather an error coming from Node's bowels,
// e.g., ENOENT.
let { message } = err

if (err.code === 'ENOENT') {
message =
`ENOENT: Git failed to execute. This typically means that ` +
`the path provided doesn't exist or that the Git executable ` +
`could not be found which could indicate a problem with the ` +
`packaging of dugite. Verify that resolveGitBinary returns a ` +
`valid path to the git binary.`
}

reject(new ExecError(message, stdout, stderr, err))
})
const promise = execFileAsync(gitLocation, args, opts)

ignoreClosedInputStream(cp)
ignoreClosedInputStream(promise.child)

if (options?.stdin !== undefined && cp.stdin) {
promise.child.on('spawn', () => {
if (options?.stdin !== undefined && promise.child.stdin) {
// See https://github.com/nodejs/node/blob/7b5ffa46fe4d2868c1662694da06eb55ec744bde/test/parallel/test-stdin-pipe-large.js
if (options.stdinEncoding) {
cp.stdin.end(options.stdin, options.stdinEncoding)
promise.child.stdin.end(options.stdin, options.stdinEncoding)
} else {
cp.stdin.end(options.stdin)
promise.child.stdin.end(options.stdin)
}
}

options?.processCallback?.(cp)
})

options?.processCallback?.(promise.child)

return promise
.then(({ stdout, stderr }) => ({ stdout, stderr, exitCode: 0 }))
.catch(e => {
if (typeof e !== 'object') {
const stdio = coerceStdio('', opts.encoding)
throw new ExecError(typeof e === 'string' ? e : `${e}`, stdio, stdio, e)
}

const stdout = coerceStdio('stdout' in e ? e.stdout : '', opts.encoding)
const stderr = coerceStdio('stderr' in e ? e.stderr : '', opts.encoding)

if ('code' in e && typeof e.code === 'number') {
const ignoreExitCodes = options?.ignoreExitCodes
if (ignoreExitCodes === true || ignoreExitCodes?.includes(e.code)) {
return { stdout, stderr, exitCode: e.code }
}
throw new ExecError(`Git failed with code ${e.code}`, stdout, stderr, e)
}

throw new ExecError(e.message, stdout, stderr, e)
})
}
20 changes: 15 additions & 5 deletions test/fast/errors-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ describe('detects errors', () => {

const result = await exec(
['remote', 'add', 'new-remote', 'https://gitlab.com'],
repoPath
repoPath,
{ ignoreExitCodes: true }
)

assertHasGitError(result, GitError.RemoteAlreadyExists)
Expand All @@ -30,15 +31,19 @@ describe('detects errors', () => {
await exec(['tag', 'v0.1'], repoPath)

// try to make the same tag again
const result = await exec(['tag', 'v0.1'], repoPath)
const result = await exec(['tag', 'v0.1'], repoPath, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.TagAlreadyExists)
})
it('BranchAlreadyExists', async () => {
const path = await initialize('branch-already-exists', 'foo')
await exec(['commit', '-m', 'initial', '--allow-empty'], path)

const result = await exec(['branch', 'foo'], path)
const result = await exec(['branch', 'foo'], path, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.BranchAlreadyExists)
})
Expand All @@ -50,6 +55,7 @@ describe('detects errors', () => {
env: {
GIT_TEST_ASSUME_DIFFERENT_OWNER: '1',
},
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.UnsafeDirectory)
Expand All @@ -74,7 +80,9 @@ describe('detects errors', () => {

await exec(['config', 'core.autocrlf', 'nab'], repoPath)

const result = await exec(['commit', '-m', 'add a text file'], repoPath)
const result = await exec(['commit', '-m', 'add a text file'], repoPath, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.BadConfigValue)

Expand All @@ -92,7 +100,9 @@ describe('detects errors', () => {

await exec(['config', 'core.repositoryformatversion', 'nan'], repoPath)

const result = await exec(['commit', '-m', 'add a text file'], repoPath)
const result = await exec(['commit', '-m', 'add a text file'], repoPath, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.BadConfigValue)

Expand Down
63 changes: 40 additions & 23 deletions test/fast/git-process-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,10 @@ describe('git-process', () => {
describe('exitCode', () => {
it('returns exit code when folder is empty', async () => {
const testRepoPath = temp.mkdirSync('desktop-git-test-blank')
const result = await git(['show', 'HEAD'], testRepoPath)
verify(result, r => {
assert.equal(r.exitCode, 128)
const result = await git(['show', 'HEAD'], testRepoPath, {
ignoreExitCodes: [128],
})
assert.equal(result.exitCode, 128)
})

it('handles stdin closed errors', async () => {
Expand All @@ -84,6 +84,7 @@ describe('git-process', () => {
// EPIPE/EOF error thrown from process.stdin
const result = await git(['--trololol'], testRepoPath, {
stdin: '\n'.repeat(1024 * 1024),
ignoreExitCodes: true,
})
verify(result, r => {
assert.equal(r.exitCode, 129)
Expand All @@ -106,7 +107,8 @@ describe('git-process', () => {
'/dev/null',
'new-file.md',
],
testRepoPath
testRepoPath,
{ ignoreExitCodes: true }
)

verify(result, r => {
Expand Down Expand Up @@ -138,7 +140,8 @@ describe('git-process', () => {
'/dev/null',
'new-file.md',
],
testRepoPath
testRepoPath,
{ ignoreExitCodes: true }
)

verify(result, r => {
Expand Down Expand Up @@ -176,7 +179,9 @@ describe('git-process', () => {
it('missing from index', async () => {
const testRepoPath = await initialize('desktop-show-missing-index')

const result = await git(['show', ':missing.txt'], testRepoPath)
const result = await git(['show', ':missing.txt'], testRepoPath, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.PathDoesNotExist)
})
Expand All @@ -190,7 +195,9 @@ describe('git-process', () => {
await git(['add', '.'], testRepoPath)
await git(['commit', '-m', '"added a file"'], testRepoPath)

const result = await git(['show', 'HEAD:missing.txt'], testRepoPath)
const result = await git(['show', 'HEAD:missing.txt'], testRepoPath, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.PathDoesNotExist)
})
Expand All @@ -199,7 +206,9 @@ describe('git-process', () => {
'desktop-show-invalid-object-empty'
)

const result = await git(['show', 'HEAD:missing.txt'], testRepoPath)
const result = await git(['show', 'HEAD:missing.txt'], testRepoPath, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.InvalidObjectName)
})
Expand All @@ -213,7 +222,9 @@ describe('git-process', () => {
await git(['add', '.'], testRepoPath)
await git(['commit', '-m', '"added a file"'], testRepoPath)

const result = await git(['show', '--', '/missing.txt'], testRepoPath)
const result = await git(['show', '--', '/missing.txt'], testRepoPath, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.OutsideRepository)
})
Expand Down Expand Up @@ -241,15 +252,9 @@ describe('git-process', () => {
it('raises error when folder does not exist', async () => {
const testRepoPath = path.join(temp.path(), 'desktop-does-not-exist')

let error: Error | null = null
try {
await git(['show', 'HEAD'], testRepoPath)
} catch (e) {
error = e as Error
}
const e = await git(['show', 'HEAD'], testRepoPath).catch(e => e)

assert.ok(error?.message.includes('Git failed to execute.'))
assert.equal((error as any).code, 'ENOENT')
assert.equal(e.code, 'ENOENT')
})

it('can parse HTTPS auth errors', () => {
Expand Down Expand Up @@ -531,7 +536,9 @@ mark them as resolved using git add`
})

// Execute a merge.
const result = await git(['merge', 'some-other-branch'], repoPath)
const result = await git(['merge', 'some-other-branch'], repoPath, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.MergeWithLocalChanges)
})
Expand Down Expand Up @@ -560,7 +567,9 @@ mark them as resolved using git add`
})

// Execute a rebase.
const result = await git(['rebase', 'some-other-branch'], repoPath)
const result = await git(['rebase', 'some-other-branch'], repoPath, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.RebaseWithLocalChanges)
})
Expand Down Expand Up @@ -602,7 +611,9 @@ mark them as resolved using git add`
})

// Pull from the fork
const result = await git(['pull', 'origin', 'HEAD'], forkRepoPath)
const result = await git(['pull', 'origin', 'HEAD'], forkRepoPath, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.MergeWithLocalChanges)
})
Expand Down Expand Up @@ -644,7 +655,9 @@ mark them as resolved using git add`
})

// Pull from the fork
const result = await git(['pull', 'origin', 'HEAD'], forkRepoPath)
const result = await git(['pull', 'origin', 'HEAD'], forkRepoPath, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.RebaseWithLocalChanges)
})
Expand Down Expand Up @@ -678,7 +691,9 @@ mark them as resolved using git add`
)

// Try to merge the branch.
const result = await git(['merge', 'my-branch'], repoPath)
const result = await git(['merge', 'my-branch'], repoPath, {
ignoreExitCodes: true,
})

assertHasGitError(result, GitError.MergeConflicts)
})
Expand Down Expand Up @@ -712,7 +727,9 @@ mark them as resolved using git add`
)

// Try to merge the branch.
const result = await git(['rebase', 'my-branch'], repoPath)
const result = await git(['rebase', 'my-branch'], repoPath, {
ignoreExitCodes: [1],
})

assertHasGitError(result, GitError.RebaseConflicts)
})
Expand Down
Loading

0 comments on commit d0c2a1d

Please sign in to comment.