Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fix: error handling skips dropping backup stash after internal git er…
…rors
  • Loading branch information
iiroj committed Dec 24, 2019
1 parent da22cf2 commit 30b4809
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 47 deletions.
35 changes: 15 additions & 20 deletions lib/gitWorkflow.js
Expand Up @@ -31,19 +31,8 @@ const cleanUntrackedFiles = async execGit => {
}
}

const isGitLockError = error =>
error.message.includes('Another git process seems to be running in this repository')

const isGitMergeStateError = error =>
error.message.includes('Merge state could not be restored due to an error!')

const handleError = (error, ctx) => {
ctx.hasErrors = true
if (isGitLockError(error)) {
ctx.hasGitLockError = true
} else if (isGitMergeStateError(error)) {
ctx.hasGitMergeStateError = true
}
ctx.gitError = true
throw error
}

Expand All @@ -66,10 +55,13 @@ class GitWorkflow {
/**
* Get name of backup stash
*/
async getBackupStash() {
async getBackupStash(ctx) {
const stashes = await this.execGit(['stash', 'list'])
const index = stashes.split('\n').findIndex(line => line.includes(STASH))
if (index === -1) throw new Error('lint-staged automatic backup is missing!')
if (index === -1) {
ctx.gitGetBackupStashError = true
throw new Error('lint-staged automatic backup is missing!')
}
return `stash@{${index}}`
}

Expand Down Expand Up @@ -136,7 +128,7 @@ class GitWorkflow {
'--no-color',
'--no-ext-diff',
'--patch',
await this.getBackupStash(),
await this.getBackupStash(ctx),
'-R' // Show diff in reverse
])

Expand Down Expand Up @@ -179,8 +171,11 @@ class GitWorkflow {
} catch (error2) {
debug('Error while restoring unstaged changes using 3-way merge:')
debug(error2)
ctx.hasErrors = true
throw new Error('Unstaged changes could not be restored due to a merge conflict!')
ctx.gitApplyModificationsError = true
handleError(
new Error('Unstaged changes could not be restored due to a merge conflict!'),
ctx
)
}
}
debug('Done restoring unstaged changes!')
Expand All @@ -190,7 +185,7 @@ class GitWorkflow {
// Git will return with error code if the commit doesn't exist
// See https://stackoverflow.com/a/52357762
try {
const backupStash = await this.getBackupStash()
const backupStash = await this.getBackupStash(ctx)
const output = await this.execGit(['show', '--format=%b', `${backupStash}^3`])
const untrackedDiff = output.replace(/^\n*/, '') // remove empty lines from start of output
if (!untrackedDiff) return
Expand All @@ -204,7 +199,7 @@ class GitWorkflow {
async restoreOriginalState(ctx) {
try {
debug('Restoring original state...')
const backupStash = await this.getBackupStash()
const backupStash = await this.getBackupStash(ctx)
await this.execGit(['reset', '--hard', 'HEAD'])
await this.execGit(['stash', 'apply', '--quiet', '--index', backupStash])
debug('Done restoring original state!')
Expand All @@ -222,7 +217,7 @@ class GitWorkflow {
async dropBackup(ctx) {
try {
debug('Dropping backup stash...')
const backupStash = await this.getBackupStash()
const backupStash = await this.getBackupStash(ctx)
await this.execGit(['stash', 'drop', '--quiet', backupStash])
debug('Done dropping backup stash!')
} catch (error) {
Expand Down
3 changes: 1 addition & 2 deletions lib/resolveTaskFn.js
Expand Up @@ -38,8 +38,7 @@ function throwError(message) {
* @returns {Error}
*/
function makeErr(linter, result, context = {}) {
// Indicate that some linter will fail so we don't update the index with formatting changes
context.hasErrors = true // eslint-disable-line no-param-reassign
context.taskError = true
const { stdout, stderr, killed, signal } = result
if (killed || (signal && signal !== '')) {
return throwError(
Expand Down
31 changes: 22 additions & 9 deletions lib/runAll.js
Expand Up @@ -106,6 +106,7 @@ module.exports = async function runAll(
exitOnError: true
}),
skip: () => {
// Skip task when no files matched
if (task.fileList.length === 0) {
return `No staged files match ${task.pattern}`
}
Expand All @@ -119,8 +120,10 @@ module.exports = async function runAll(
title:
chunkCount > 1 ? `Running tasks (chunk ${index + 1}/${chunkCount})...` : 'Running tasks...',
task: () => new Listr(chunkListrTasks, { ...listrOptions, concurrent }),
skip: ctx => {
if (ctx && ctx.hasErrors) return 'Skipped because of previous git error'
skip: (ctx = {}) => {
// Skip if the first step (backup) failed
if (ctx.gitError) return 'Skipped because of previous git error.'
// Skip chunk when no every task is skipped (due to no matches)
if (chunkListrTasks.every(task => task.skip())) return 'No tasks to run.'
return false
}
Expand All @@ -143,6 +146,12 @@ module.exports = async function runAll(

const git = new GitWorkflow({ gitDir, stagedFileChunks })

// Running git reset or dropping the backup stash should be skipped
// when there are git errors NOT related to applying unstaged modifications.
// In the latter case, the original state is restored.
const cleanupNotSafe = ctx =>
ctx.gitError && !ctx.gitApplyModificationsError && 'Skipped because of previous git error.'

const runner = new Listr(
[
{
Expand All @@ -152,29 +161,33 @@ module.exports = async function runAll(
...listrTasks,
{
title: 'Applying modifications...',
skip: ctx => ctx.hasErrors && 'Skipped because of errors from tasks',
skip: ctx => {
if (ctx.gitError) return 'Skipped because of previous git error.'
if (ctx.taskError) return 'Skipped because of errors from tasks.'
},
task: ctx => git.applyModifications(ctx)
},
{
title: 'Reverting to original state...',
enabled: ctx => ctx.hasErrors,
enabled: ctx => ctx.taskError || ctx.gitApplyModificationsError,
skip: cleanupNotSafe,
task: ctx => git.restoreOriginalState(ctx)
},
{
title: 'Cleaning up...',
skip: ctx =>
(ctx.hasGitLockError || ctx.hasGitMergeStateError) &&
'Skipped because of previous git error',
skip: cleanupNotSafe,
task: ctx => git.dropBackup(ctx)
}
],
listrOptions
)

try {
await runner.run()
await runner.run({})
} catch (error) {
if (error.context.hasGitLockError || error.context.hasGitMergeStateError) {
// Show help text about manual restore in case of git errors.
// No sense to show this if the backup stash itself is missing.
if (error.context.gitError && !error.context.gitGetBackupStashError) {
logger.error(`
${symbols.error} ${chalk.red(`lint-staged failed due to a git error.
Any lost modifications can be restored from a git stash:
Expand Down
12 changes: 6 additions & 6 deletions test/__snapshots__/runAll.spec.js.snap
Expand Up @@ -35,7 +35,7 @@ LOG →
LOG Running tasks... [failed]
LOG Applying modifications... [started]
LOG Applying modifications... [skipped]
LOG → Skipped because of errors from tasks
LOG → Skipped because of errors from tasks.
LOG Reverting to original state... [started]
LOG Reverting to original state... [completed]
LOG Cleaning up... [started]
Expand All @@ -45,10 +45,10 @@ LOG {
errors: [
{
privateMsg: '\\\\n\\\\n\\\\n× echo \\"sample\\" found some errors. Please fix them and try committing again.\\\\n\\\\nLinter finished with error',
context: {hasErrors: true}
context: {taskError: true}
}
],
context: {hasErrors: true}
context: {taskError: true}
}"
`;

Expand All @@ -66,7 +66,7 @@ LOG →
LOG Running tasks... [failed]
LOG Applying modifications... [started]
LOG Applying modifications... [skipped]
LOG → Skipped because of errors from tasks
LOG → Skipped because of errors from tasks.
LOG Reverting to original state... [started]
LOG Reverting to original state... [completed]
LOG Cleaning up... [started]
Expand All @@ -76,10 +76,10 @@ LOG {
errors: [
{
privateMsg: '\\\\n\\\\n\\\\n‼ echo \\"sample\\" was terminated with SIGINT',
context: {hasErrors: true}
context: {taskError: true}
}
],
context: {hasErrors: true}
context: {taskError: true}
}"
`;

Expand Down
8 changes: 4 additions & 4 deletions test/resolveTaskFn.spec.js
Expand Up @@ -175,15 +175,15 @@ describe('resolveTaskFn', () => {
}
})

it('should not set hasErrors on context if no error occur', async () => {
it('should not set taskError on context if no error occur', async () => {
expect.assertions(1)
const context = {}
const taskFn = resolveTaskFn({ ...defaultOpts, command: 'jest', gitDir: '../' })
await taskFn(context)
expect(context.hasErrors).toBeUndefined()
expect(context.taskError).toBeUndefined()
})

it('should set hasErrors on context to true on error', async () => {
it('should set taskError on context to true on error', async () => {
execa.mockResolvedValueOnce({
stdout: 'Mock error',
stderr: '',
Expand All @@ -197,7 +197,7 @@ describe('resolveTaskFn', () => {
try {
await taskFn(context)
} catch (err) {
expect(context.hasErrors).toEqual(true)
expect(context.taskError).toEqual(true)
}
})
})
9 changes: 3 additions & 6 deletions test/runAll.unmocked.2.spec.js
Expand Up @@ -111,16 +111,13 @@ describe('runAll', () => {
LOG → Merge state could not be restored due to an error!
LOG Running tasks... [started]
LOG Running tasks... [skipped]
LOG → Skipped because of previous git error
LOG → Skipped because of previous git error.
LOG Applying modifications... [started]
LOG Applying modifications... [skipped]
LOG → Skipped because of errors from tasks
LOG Reverting to original state... [started]
LOG Reverting to original state... [failed]
LOG → Merge state could not be restored due to an error!
LOG → Skipped because of previous git error.
LOG Cleaning up... [started]
LOG Cleaning up... [skipped]
LOG → Skipped because of previous git error
LOG → Skipped because of previous git error.
ERROR
× lint-staged failed due to a git error.
Any lost modifications can be restored from a git stash:
Expand Down

0 comments on commit 30b4809

Please sign in to comment.