Repository: okonet/lint-staged
Pull request #780 (2020-01-27 06:20:44)

Fix untracked file handling
This PR adds missing error handling to the untracked files restoration, and uses the same diff arguments as for the unstaged diff.

These omissions when combined possibly lead to new untracked files getting lost when running _lint-staged_. The test didn't detect this because one reason might be the missing `--binary` flag.

~~The test coverage dropped significantly because now when the backup stash is missing, `runAll` fails earlier. Not sure how to test the last error handler at this point.~~

Fixes https://github.com/okonet/lint-staged/issues/779

codecov[bot]: # [Codecov](https://codecov.io/gh/okonet/lint-staged/pull/780?src=pr&el=h1) Report
> Merging [#780](https://codecov.io/gh/okonet/lint-staged/pull/780?src=pr&el=desc) into [master](https://codecov.io/gh/okonet/lint-staged/commit/0f2a1c01ca7bc599ea69744259c0de04b4e223b3?src=pr&el=desc) will **decrease** coverage by `0.22%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/okonet/lint-staged/pull/780/graphs/tree.svg?width=650&token=LSoEgsjk4O&height=150&src=pr)](https://codecov.io/gh/okonet/lint-staged/pull/780?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #780      +/-   ##
==========================================
- Coverage     100%   99.77%   -0.23%     
==========================================
  Files          13       13              
  Lines         449      451       +2     
  Branches      104      103       -1     
==========================================
+ Hits          449      450       +1     
- Misses          0        1       +1
```


| [Impacted Files](https://codecov.io/gh/okonet/lint-staged/pull/780?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [lib/gitWorkflow.js](https://codecov.io/gh/okonet/lint-staged/pull/780/diff?src=pr&el=tree#diff-bGliL2dpdFdvcmtmbG93Lmpz) | `99.14% <100%> (-0.86%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/okonet/lint-staged/pull/780?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/okonet/lint-staged/pull/780?src=pr&el=footer). Last update [0f2a1c0...ee92929](https://codecov.io/gh/okonet/lint-staged/pull/780?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

iiroj: @okonet if you're fine with these changes, please leaven approving review so that the merge button gets enabled for me.

Patch:
diff --git a/.eslintrc.json b/.eslintrc.json
index 14b75a0..81be254 100644
--- a/.eslintrc.json
+++ b/.eslintrc.json
@@ -2,8 +2,9 @@
   "extends": ["okonet/node"],
   "rules": {
     "no-console": "off",
-    "node/no-unsupported-features": ["error", "8.12.0"],
-    "node/no-unsupported-features/es-syntax": ["error", { "version": ">=8.12.0" }],
+    "node/no-unsupported-features/node-builtins": "off",
+    "node/no-unsupported-features/es-syntax": ["error", { "version": ">=10.13.0" }],
+    "node/no-unsupported-features/es-builtins": ["error", { "version": ">=10.13.0" }],
     "prettier/prettier": "off",
     "require-atomic-updates": "off"
   }
diff --git a/lib/file.js b/lib/file.js
index 9a26a44..462b2c2 100644
--- a/lib/file.js
+++ b/lib/file.js
@@ -3,11 +3,26 @@
 const debug = require('debug')('lint-staged:file')
 const fs = require('fs')
 
+const fsPromises = fs.promises
+
+/**
+ * Check if a file exists. Returns the filepath if exists.
+ * @param {string} filepath
+ */
+const exists = async filepath => {
+  try {
+    await fsPromises.access(filepath)
+    return filepath
+  } catch {
+    return false
+  }
+}
+
 /**
  * @param {String} filename
  * @returns {Promise<Buffer|Null>}
  */
-module.exports.readBufferFromFile = (filename, rejectENOENT = false) =>
+const readBufferFromFile = (filename, rejectENOENT = false) =>
   new Promise(resolve => {
     debug('Reading buffer from file `%s`', filename)
     fs.readFile(filename, (error, buffer) => {
@@ -20,12 +35,23 @@ module.exports.readBufferFromFile = (filename, rejectENOENT = false) =>
     })
   })
 
+/**
+ * Unlink a file if it exists
+ * @param {*} filepath
+ */
+const unlink = async filepath => {
+  if (filepath) {
+    await fsPromises.access(filepath)
+    await fsPromises.unlink(filepath)
+  }
+}
+
 /**
  * @param {String} filename
  * @param {Buffer} buffer
  * @returns {Promise<Void>}
  */
-module.exports.writeBufferToFile = (filename, buffer) =>
+const writeBufferToFile = (filename, buffer) =>
   new Promise(resolve => {
     debug('Writing buffer to file `%s`', filename)
     fs.writeFile(filename, buffer, () => {
@@ -33,3 +59,10 @@ module.exports.writeBufferToFile = (filename, buffer) =>
       resolve()
     })
   })
+
+module.exports = {
+  exists,
+  readBufferFromFile,
+  unlink,
+  writeBufferToFile
+}
diff --git a/lib/gitWorkflow.js b/lib/gitWorkflow.js
index 31b1e77..e650ca2 100644
--- 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 { readBufferFromFile, writeBufferToFile } = require('./file')
+const { exists, readBufferFromFile, unlink, writeBufferToFile } = require('./file')
 
 const MERGE_HEAD = 'MERGE_HEAD'
 const MERGE_MODE = 'MERGE_MODE'
@@ -12,7 +12,11 @@ const MERGE_MSG = 'MERGE_MSG'
 
 const STASH = 'lint-staged automatic backup'
 
-const gitApplyArgs = ['apply', '-v', '--whitespace=nowarn', '--recount', '--unidiff-zero']
+const PATCH_UNSTAGED = 'lint-staged_unstaged.patch'
+const PATCH_UNTRACKED = 'lint-staged_untracked.patch'
+
+const GIT_APPLY_ARGS = ['apply', '-v', '--whitespace=nowarn', '--recount', '--unidiff-zero']
+const GIT_DIFF_ARGS = ['--binary', '--unified=0', '--no-color', '--no-ext-diff', '--patch']
 
 /**
  * Delete untracked files using `git clean`
@@ -39,6 +43,7 @@ const handleError = (error, ctx) => {
 class GitWorkflow {
   constructor({ allowEmpty, gitConfigDir, gitDir, stagedFileChunks }) {
     this.execGit = (args, options = {}) => execGit(args, { ...options, cwd: gitDir })
+    this.gitConfigDir = gitConfigDir
     this.unstagedDiff = null
     this.allowEmpty = allowEmpty
     this.stagedFileChunks = stagedFileChunks
@@ -52,6 +57,27 @@ class GitWorkflow {
     this.mergeMsgFilename = path.resolve(gitConfigDir, MERGE_MSG)
   }
 
+  /**
+   * Get absolute path to file hidden inside .git
+   * @param {string} filename
+   */
+  getHiddenFilepath(filename) {
+    return path.resolve(this.gitConfigDir, `./${filename}`)
+  }
+
+  /**
+   * Check if patch file exists and has content.
+   * @param {string} filename
+   */
+  async hasPatch(filename) {
+    const resolved = this.getHiddenFilepath(filename)
+    const pathIfExists = await exists(resolved)
+    if (!pathIfExists) return false
+    const buffer = await readBufferFromFile(pathIfExists)
+    const patch = buffer.toString().trim()
+    return patch.length ? filename : false
+  }
+
   /**
    * Get name of backup stash
    */
@@ -121,13 +147,10 @@ class GitWorkflow {
       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([
+      await this.execGit([
         'diff',
-        '--binary',
-        '--unified=0',
-        '--no-color',
-        '--no-ext-diff',
-        '--patch',
+        ...GIT_DIFF_ARGS,
+        `--output=${this.getHiddenFilepath(PATCH_UNSTAGED)}`,
         await this.getBackupStash(ctx),
         '-R' // Show diff in reverse
       ])
@@ -167,10 +190,14 @@ class GitWorkflow {
       handleError(new Error('Prevented an empty git commit!'), ctx)
     }
 
-    if (this.unstagedDiff) {
+    // Restore unstaged changes by applying the diff back. If it at first fails,
+    // this is probably because of conflicts between task modifications.
+    // 3-way merge usually fixes this, and in case it doesn't we should just give up and throw.
+    if (await this.hasPatch(PATCH_UNSTAGED)) {
       debug('Restoring unstaged changes...')
+      const unstagedPatch = this.getHiddenFilepath(PATCH_UNSTAGED)
       try {
-        await this.execGit(gitApplyArgs, { input: `${this.unstagedDiff}\n` })
+        await this.execGit([...GIT_APPLY_ARGS, unstagedPatch])
       } catch (error) {
         debug('Error while restoring changes:')
         debug(error)
@@ -178,7 +205,7 @@ class GitWorkflow {
 
         try {
           // Retry with `--3way` if normal apply fails
-          await this.execGit([...gitApplyArgs, '--3way'], { input: `${this.unstagedDiff}\n` })
+          await this.execGit([...GIT_APPLY_ARGS, '--3way', unstagedPatch])
         } catch (error2) {
           debug('Error while restoring unstaged changes using 3-way merge:')
           debug(error2)
@@ -193,15 +220,24 @@ class GitWorkflow {
     }
 
     // Restore untracked files by reading from the third commit associated with the backup stash
-    // Git will return with error code if the commit doesn't exist
     // See https://stackoverflow.com/a/52357762
     try {
       const backupStash = await this.getBackupStash(ctx)
-      const output = await this.execGit(['show', '--format=%b', `${backupStash}^3`])
-      const untrackedDiff = typeof output === 'string' && output.trim() // remove empty lines from start of output
-      if (!untrackedDiff) return
-      await this.execGit([...gitApplyArgs], { input: `${untrackedDiff}\n` })
-    } catch (err) {} // eslint-disable-line no-empty
+      const untrackedPatch = this.getHiddenFilepath(PATCH_UNTRACKED)
+      await this.execGit([
+        'show',
+        ...GIT_DIFF_ARGS,
+        '--format=%b',
+        `--output=${untrackedPatch}`,
+        `${backupStash}^3`
+      ])
+      if (await this.hasPatch(PATCH_UNTRACKED)) {
+        await this.execGit([...GIT_APPLY_ARGS, untrackedPatch])
+      }
+    } catch (error) {
+      ctx.gitRestoreUntrackedError = true
+      handleError(error, ctx)
+    }
   }
 
   /**
@@ -228,6 +264,10 @@ class GitWorkflow {
   async dropBackup(ctx) {
     try {
       debug('Dropping backup stash...')
+      await Promise.all([
+        exists(this.getHiddenFilepath(PATCH_UNSTAGED)).then(unlink),
+        exists(this.getHiddenFilepath(PATCH_UNTRACKED)).then(unlink)
+      ])
       const backupStash = await this.getBackupStash(ctx)
       await this.execGit(['stash', 'drop', '--quiet', backupStash])
       debug('Done dropping backup stash!')
diff --git a/lib/runAll.js b/lib/runAll.js
index 735c88d..b48827e 100644
--- a/lib/runAll.js
+++ b/lib/runAll.js
@@ -57,6 +57,12 @@ module.exports = async function runAll(
   if (!files) throw new Error('Unable to get staged files!')
   debugLog('Loaded list of staged files in git:\n%O', files)
 
+  // If there are no files avoid executing any lint-staged logic
+  if (files.length === 0) {
+    logger.log('No staged files found.')
+    return 'No tasks to run.'
+  }
+
   const stagedFileChunks = chunkFiles({ files, gitDir, maxArgLength, relative })
   const chunkCount = stagedFileChunks.length
   if (chunkCount > 1) debugLog(`Chunked staged files into ${chunkCount} part`, chunkCount)


Test patch:
diff --git a/test/__snapshots__/runAll.spec.js.snap b/test/__snapshots__/runAll.spec.js.snap
index 23c713a..6eb4331 100644
--- a/test/__snapshots__/runAll.spec.js.snap
+++ b/test/__snapshots__/runAll.spec.js.snap
@@ -18,7 +18,7 @@ LOG Cleaning up... [completed]"
 
 exports[`runAll should resolve the promise with no files 1`] = `
 "
-LOG No staged files match any of provided globs."
+LOG No staged files found."
 `;
 
 exports[`runAll should skip applying unstaged modifications if there are errors during linting 1`] = `
@@ -85,5 +85,5 @@ LOG {
 
 exports[`runAll should use an injected logger 1`] = `
 "
-LOG No staged files match any of provided globs."
+LOG No staged files found."
 `;
diff --git a/test/gitWorkflow.spec.js b/test/gitWorkflow.spec.js
index f52a0af..c49e2ae 100644
--- a/test/gitWorkflow.spec.js
+++ b/test/gitWorkflow.spec.js
@@ -5,7 +5,7 @@ import path from 'path'
 import nanoid from 'nanoid'
 
 import execGitBase from '../lib/execGit'
-import { cleanUntrackedFiles } from '../lib/gitWorkflow'
+import GitWorkflow, { cleanUntrackedFiles } from '../lib/gitWorkflow'
 
 jest.unmock('execa')
 
@@ -74,4 +74,31 @@ describe('gitWorkflow', () => {
       expect(await fs.exists(testFile)).toEqual(false)
     })
   })
+
+  describe('hasPatch', () => {
+    it('should return false when patch file not found', async () => {
+      const gitWorkflow = new GitWorkflow({
+        gitDir: cwd,
+        gitConfigDir: path.resolve(cwd, './.git')
+      })
+      expect(await gitWorkflow.hasPatch('foo')).toEqual(false)
+    })
+  })
+
+  describe('dropBackup', () => {
+    it('should handle errors', async () => {
+      const gitWorkflow = new GitWorkflow({
+        gitDir: cwd,
+        gitConfigDir: path.resolve(cwd, './.git')
+      })
+      const ctx = {}
+      await expect(gitWorkflow.dropBackup(ctx)).rejects.toThrowErrorMatchingInlineSnapshot(
+        `"lint-staged automatic backup is missing!"`
+      )
+      expect(ctx).toEqual({
+        gitError: true,
+        gitGetBackupStashError: true
+      })
+    })
+  })
 })
diff --git a/test/runAll.unmocked.spec.js b/test/runAll.unmocked.spec.js
index 36d60fc..0c6233f 100644
--- a/test/runAll.unmocked.spec.js
+++ b/test/runAll.unmocked.spec.js
@@ -82,11 +82,6 @@ describe('runAll', () => {
     )
     await removeTempDir(nonGitDir)
   })
-
-  it('should short-circuit with no staged files', async () => {
-    const status = await runAll({ config: { '*.js': 'echo success' }, cwd })
-    expect(status).toEqual('No tasks to run.')
-  })
 })
 
 const globalConsoleTemp = console
@@ -119,6 +114,11 @@ describe('runAll', () => {
     console = globalConsoleTemp
   })
 
+  it('should exit early with no staged files', async () => {
+    const status = await runAll({ config: { '*.js': 'echo success' }, cwd })
+    expect(status).toEqual('No tasks to run.')
+  })
+
   it('Should commit entire staged file when no errors from linter', async () => {
     // Stage pretty file
     await appendFile('test file.js', testJsFilePretty)
@@ -555,8 +555,10 @@ describe('runAll', () => {
     await appendFile('test.js', testJsFilePretty)
     await execGit(['add', 'test.js'])
 
-    // Add another file, but keep it untracked
+    // Add untracked files
     await appendFile('test-untracked.js', testJsFilePretty)
+    await appendFile('.gitattributes', 'binary\n')
+    await writeFile('binary', Buffer.from('Hello, World!', 'binary'))
 
     // Run lint-staged with `prettier --list-different` and commit pretty file
     await gitCommit({ config: { '*.js': 'prettier --list-different' } })
@@ -566,6 +568,7 @@ describe('runAll', () => {
     expect(await execGit(['log', '-1', '--pretty=%B'])).toMatch('test')
     expect(await readFile('test.js')).toEqual(testJsFilePretty)
     expect(await readFile('test-untracked.js')).toEqual(testJsFilePretty)
+    expect(Buffer.from(await readFile('binary'), 'binary').toString()).toEqual('Hello, World!')
   })
 
   it('should work when amending previous commit with unstaged changes', async () => {
@@ -611,12 +614,12 @@ describe('runAll', () => {
   })
 
   it('should handle binary files', async () => {
-    // mark test.js as binary file
-    await appendFile('.gitattributes', 'test.js binary\n')
+    // mark file as binary
+    await appendFile('.gitattributes', 'binary\n')
 
     // Stage pretty file
-    await appendFile('test.js', testJsFilePretty)
-    await execGit(['add', 'test.js'])
+    await writeFile('binary', Buffer.from('Hello, World!', 'binary'))
+    await execGit(['add', 'binary'])
 
     // Run lint-staged with `prettier --list-different` and commit pretty file
     await gitCommit({ config: { '*.js': 'prettier --list-different' } })
@@ -624,7 +627,7 @@ describe('runAll', () => {
     // Nothing is wrong, so a new commit is created
     expect(await execGit(['rev-list', '--count', 'HEAD'])).toEqual('2')
     expect(await execGit(['log', '-1', '--pretty=%B'])).toMatch('test')
-    expect(await readFile('test.js')).toEqual(testJsFilePretty)
+    expect(Buffer.from(await readFile('binary'), 'binary').toString()).toEqual('Hello, World!')
   })
 
   it('should run chunked tasks when necessary', async () => {
@@ -670,10 +673,11 @@ describe('runAll', () => {
       LOG Running tasks for *.js [completed]
       LOG Running tasks... [completed]
       LOG Applying modifications... [started]
-      LOG Applying modifications... [completed]
+      LOG Applying modifications... [failed]
+      LOG → lint-staged automatic backup is missing!
       LOG Cleaning up... [started]
-      LOG Cleaning up... [failed]
-      LOG → lint-staged automatic backup is missing!"
+      LOG Cleaning up... [skipped]
+      LOG → Skipped because of previous git error."
     `)
   })