fix: handle git MERGE_* files separately; improve error handling

lint-staged · Dec 20, 2019 · da22cf2 · da22cf2
1 parent 20d5c5d
commit da22cf2
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 85 deletions.
diff --git a/lib/file.js b/lib/file.js
@@ -3,36 +3,20 @@
 const debug = require('debug')('lint-staged:file')
 const fs = require('fs')
 
-/**
- * Check if file exists and is accessible
- * @param {String} filename
- * @returns {Promise<Boolean>}
- */
-module.exports.checkFile = filename =>
-  new Promise(resolve => {
-    debug('Trying to access `%s`', filename)
-    fs.access(filename, fs.constants.R_OK, error => {
-      if (error) {
-        debug('Unable to access file `%s` with error:', filename)
-        debug(error)
-      } else {
-        debug('Successfully accesses file `%s`', filename)
-      }
-
-      resolve(!error)
-    })
-  })
-
 /**
  * @param {String} filename
  * @returns {Promise<Buffer|Null>}
  */
-module.exports.readBufferFromFile = filename =>
+module.exports.readBufferFromFile = (filename, rejectENOENT = false) =>
   new Promise(resolve => {
     debug('Reading buffer from file `%s`', filename)
-    fs.readFile(filename, (error, file) => {
+    fs.readFile(filename, (error, buffer) => {
+      if (!rejectENOENT && error && error.code === 'ENOENT') {
+        debug("File `%s` doesn't exist, ignoring...", filename)
+        return resolve(null) // no-op file doesn't exist
+      }
       debug('Done reading buffer from file `%s`!', filename)
-      resolve(file)
+      resolve(buffer)
     })
   })
 

diff --git a/lib/gitWorkflow.js b/lib/gitWorkflow.js
@@ -4,7 +4,7 @@ const debug = require('debug')('lint-staged:git')
 const path = require('path')
 
 const execGit = require('./execGit')
-const { checkFile, readBufferFromFile, writeBufferToFile } = require('./file')
+const { readBufferFromFile, writeBufferToFile } = require('./file')
 
 const MERGE_HEAD = 'MERGE_HEAD'
 const MERGE_MODE = 'MERGE_MODE'
@@ -31,10 +31,18 @@ const cleanUntrackedFiles = async execGit => {
   }
 }
 
-const handleGitLockError = (error, ctx) => {
-  if (error.message.includes('Another git process seems to be running in this repository')) {
-    ctx.hasErrors = true
+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
   }
   throw error
 }
@@ -69,7 +77,6 @@ class GitWorkflow {
    * Save meta information about ongoing git merge
    */
   async backupMergeStatus() {
-    debug('Detected current merge mode!')
     debug('Backing up merge state...')
     await Promise.all([
       readBufferFromFile(this.mergeHeadFilename).then(buffer => (this.mergeHeadBuffer = buffer)),
@@ -83,55 +90,60 @@ class GitWorkflow {
    * Restore meta information about ongoing git merge
    */
   async restoreMergeStatus() {
-    debug('Detected backup merge state!')
     debug('Restoring merge state...')
-    await Promise.all([
-      writeBufferToFile(this.mergeHeadFilename, this.mergeHeadBuffer),
-      writeBufferToFile(this.mergeModeFilename, this.mergeModeBuffer),
-      writeBufferToFile(this.mergeMsgFilename, this.mergeMsgBuffer)
-    ])
-    debug('Done restoring merge state!')
+    try {
+      await Promise.all([
+        this.mergeHeadBuffer && writeBufferToFile(this.mergeHeadFilename, this.mergeHeadBuffer),
+        this.mergeModeBuffer && writeBufferToFile(this.mergeModeFilename, this.mergeModeBuffer),
+        this.mergeMsgBuffer && writeBufferToFile(this.mergeMsgFilename, this.mergeMsgBuffer)
+      ])
+      debug('Done restoring merge state!')
+    } catch (error) {
+      debug('Failed restoring merge state with error:')
+      debug(error)
+      throw new Error('Merge state could not be restored due to an error!')
+    }
   }
 
   /**
    * Create backup stashes, one of everything and one of only staged changes
    * Staged files are left in the index for running tasks
    */
-  async stashBackup() {
-    debug('Backing up original state...')
+  async stashBackup(ctx) {
+    try {
+      debug('Backing up original state...')
 
-    // the `git stash` clears metadata about a possible git merge
-    // Manually check and backup if necessary
-    if (await checkFile(this.mergeHeadFilename)) {
+      // the `git stash` clears metadata about a possible git merge
+      // Manually check and backup if necessary
       await this.backupMergeStatus()
-    }
 
-    // Save stash of entire original state, including unstaged and untracked changes.
-    // `--keep-index leaves only staged files on disk, for tasks.`
-    await this.execGit(['stash', 'save', '--quiet', '--include-untracked', '--keep-index', STASH])
+      // Save stash of entire original state, including unstaged and untracked changes.
+      // `--keep-index leaves only staged files on disk, for tasks.`
+      await this.execGit(['stash', 'save', '--quiet', '--include-untracked', '--keep-index', STASH])
 
-    // Restore meta information about ongoing git merge
-    if (this.mergeHeadBuffer) {
+      // Restore meta information about ongoing git merge
       await this.restoreMergeStatus()
-    }
-
-    // There is a bug in git =< 2.13.0 where `--keep-index` resurrects deleted files.
-    // These files should be listed and deleted before proceeding.
-    await cleanUntrackedFiles(this.execGit)
-
-    // Get a diff of unstaged changes by diffing the saved stash against what's left on disk.
-    this.unstagedDiff = await this.execGit([
-      'diff',
-      '--binary',
-      '--unified=0',
-      '--no-color',
-      '--no-ext-diff',
-      '--patch',
-      await this.getBackupStash(),
-      '-R' // Show diff in reverse
-    ])
 
-    debug('Done backing up original state!')
+      // There is a bug in git =< 2.13.0 where `--keep-index` resurrects deleted files.
+      // These files should be listed and deleted before proceeding.
+      await cleanUntrackedFiles(this.execGit)
+
+      // Get a diff of unstaged changes by diffing the saved stash against what's left on disk.
+      this.unstagedDiff = await this.execGit([
+        'diff',
+        '--binary',
+        '--unified=0',
+        '--no-color',
+        '--no-ext-diff',
+        '--patch',
+        await this.getBackupStash(),
+        '-R' // Show diff in reverse
+      ])
+
+      debug('Done backing up original state!')
+    } catch (error) {
+      handleError(error, ctx)
+    }
   }
 
   /**
@@ -198,11 +210,9 @@ class GitWorkflow {
       debug('Done restoring original state!')
 
       // Restore meta information about ongoing git merge
-      if (this.mergeHeadBuffer) {
-        await this.restoreMergeStatus()
-      }
+      await this.restoreMergeStatus()
     } catch (error) {
-      handleGitLockError(error, ctx)
+      handleError(error, ctx)
     }
   }
 
@@ -216,7 +226,7 @@ class GitWorkflow {
       await this.execGit(['stash', 'drop', '--quiet', backupStash])
       debug('Done dropping backup stash!')
     } catch (error) {
-      handleGitLockError(error, ctx)
+      handleError(error, ctx)
     }
   }
 }

diff --git a/lib/runAll.js b/lib/runAll.js
@@ -119,10 +119,9 @@ module.exports = async function runAll(
       title:
         chunkCount > 1 ? `Running tasks (chunk ${index + 1}/${chunkCount})...` : 'Running tasks...',
       task: () => new Listr(chunkListrTasks, { ...listrOptions, concurrent }),
-      skip: () => {
-        if (chunkListrTasks.every(task => task.skip())) {
-          return 'No tasks to run.'
-        }
+      skip: ctx => {
+        if (ctx && ctx.hasErrors) return 'Skipped because of previous git error'
+        if (chunkListrTasks.every(task => task.skip())) return 'No tasks to run.'
         return false
       }
     })
@@ -148,7 +147,7 @@ module.exports = async function runAll(
     [
       {
         title: 'Preparing...',
-        task: () => git.stashBackup()
+        task: ctx => git.stashBackup(ctx)
       },
       ...listrTasks,
       {
@@ -163,7 +162,9 @@ module.exports = async function runAll(
       },
       {
         title: 'Cleaning up...',
-        skip: ctx => ctx.hasGitLockError && 'Skipped because of previous git error',
+        skip: ctx =>
+          (ctx.hasGitLockError || ctx.hasGitMergeStateError) &&
+          'Skipped because of previous git error',
         task: ctx => git.dropBackup(ctx)
       }
     ],
@@ -173,7 +174,7 @@ module.exports = async function runAll(
   try {
     await runner.run()
   } catch (error) {
-    if (error.context.hasGitLockError) {
+    if (error.context.hasGitLockError || error.context.hasGitMergeStateError) {
       logger.error(`
   ${symbols.error} ${chalk.red(`lint-staged failed due to a git error.
     Any lost modifications can be restored from a git stash:

diff --git a/test/runAll.unmocked.2.spec.js b/test/runAll.unmocked.2.spec.js
@@ -54,7 +54,7 @@ const execGit = async args => execGitBase(args, { cwd })
 
 // Execute runAll before git commit to emulate lint-staged
 const gitCommit = async (options, args = ['-m test']) => {
-  await runAll({ ...options, cwd, quiet: true })
+  await runAll({ quiet: true, ...options, cwd })
   await execGit(['commit', ...args])
 }
 
@@ -88,8 +88,8 @@ describe('runAll', () => {
     console = globalConsoleTemp
   })
 
-  it('Should throw when restoring untracked files fails', async () => {
-    readBufferFromFile.mockImplementation(async () => [Buffer.from('')])
+  it.only('Should throw when restoring untracked files fails', async () => {
+    readBufferFromFile.mockImplementation(async () => Buffer.from('test'))
     writeBufferToFile.mockImplementation(async () => Promise.reject('test'))
 
     // Stage pretty file
@@ -99,11 +99,36 @@ describe('runAll', () => {
     // Create untracked file
     await appendFile('test-untracked.js', testJsFilePretty)
 
-    try {
-      // Run lint-staged with `prettier --list-different` and commit pretty file
-      await gitCommit({ config: { '*.js': 'prettier --list-different' } })
-    } catch (error) {
-      expect(error.message).toEqual('Untracked changes could not be restored due to an error!')
-    }
+    // Run lint-staged with `prettier --list-different`
+    await expect(
+      gitCommit({ config: { '*.js': 'prettier --list-different' }, quiet: false })
+    ).rejects.toThrowErrorMatchingInlineSnapshot(`"Something went wrong"`)
+
+    expect(console.printHistory()).toMatchInlineSnapshot(`
+      "
+      LOG Preparing... [started]
+      LOG Preparing... [failed]
+      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 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 Cleaning up... [started]
+      LOG Cleaning up... [skipped]
+      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:
+
+          > git stash list
+          stash@{0}: On master: automatic lint-staged backup
+          > git stash pop stash@{0}
+      "
+    `)
   })
 })