Repository: okonet/lint-staged
Pull request #850 (2020-04-19 17:10:55)

fix: use stash create/store to prevent files from disappearing from disk
The `git stash create` command creates a dangling stash commit without removing any files from the disk, and returns its hash. This when passed to the `git stash store` commit saves it as a regular stash.

What follows is that the files never leave disk, and so don't trigger any side-effects with file watchers or other processes that might be running when lint-staged is running.

Fixes https://github.com/okonet/lint-staged/issues/844

iiroj: @okonet I did not know `git stash create` worked this way, so let this PR sit for a while until I've really verified it. If indeed true, this is a very nice improvement!
iiroj: It might be better to merge this as `fix` instead of `perf`.
codecov[bot]: # [Codecov](https://codecov.io/gh/okonet/lint-staged/pull/850?src=pr&el=h1) Report
> Merging [#850](https://codecov.io/gh/okonet/lint-staged/pull/850?src=pr&el=desc) into [master](https://codecov.io/gh/okonet/lint-staged/commit/e093b1db066ac94d4d72847d24034b217e347849&el=desc) will **decrease** coverage by `1.73%`.
> The diff coverage is `100.00%`.

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

```diff
@@             Coverage Diff             @@
##            master     #850      +/-   ##
===========================================
- Coverage   100.00%   98.26%   -1.74%     
===========================================
  Files           13       13              
  Lines          523      520       -3     
  Branches       108      108              
===========================================
- Hits           523      511      -12     
- Misses           0        9       +9     
```


| [Impacted Files](https://codecov.io/gh/okonet/lint-staged/pull/850?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [lib/gitWorkflow.js](https://codecov.io/gh/okonet/lint-staged/pull/850/diff?src=pr&el=tree#diff-bGliL2dpdFdvcmtmbG93Lmpz) | `94.44% <100.00%> (-5.56%)` | :arrow_down: |
| [lib/runAll.js](https://codecov.io/gh/okonet/lint-staged/pull/850/diff?src=pr&el=tree#diff-bGliL3J1bkFsbC5qcw==) | `99.12% <0.00%> (-0.88%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/okonet/lint-staged/pull/850?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/850?src=pr&el=footer). Last update [e093b1d...3760aae](https://codecov.io/gh/okonet/lint-staged/pull/850?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

iiroj: I guess this is how it works. What do you think, @okonet?
okonet: > I guess this is how it works. What do you think,

About what?
iiroj: well, I couldn’t really find any documentation. Good to merge?

The revert logic still works the same because `git reset --hard HEAD` conveniently cleans things up.

It could be left as a further PR to use checkout instead, and then get rid of special handling of merge state and deleted files.
okonet: Let's go for it!

Patch:
diff --git a/lib/gitWorkflow.js b/lib/gitWorkflow.js
index 2cf02e5..8037318 100644
--- a/lib/gitWorkflow.js
+++ b/lib/gitWorkflow.js
@@ -159,6 +159,7 @@ class GitWorkflow {
         return index !== ' ' && workingTree !== ' ' && index !== '?' && workingTree !== '?'
       })
       .map((line) => line.substr(3)) // Remove first three letters (index, workingTree, and a whitespace)
+      .filter(Boolean) // Filter empty string
     debug('Found partially staged files:', partiallyStaged)
     return partiallyStaged.length ? partiallyStaged : null
   }
@@ -186,24 +187,19 @@ class GitWorkflow {
        */
       if (!shouldBackup) return
 
+      // When backup is enabled, the revert will clear ongoing merge status.
+      await this.backupMergeStatus()
+
       // Get a list of unstaged deleted files, because certain bugs might cause them to reappear:
-      // - in git versions =< 2.13.0 the `--keep-index` flag resurrects deleted files
+      // - in git versions =< 2.13.0 the `git stash --keep-index` option resurrects deleted files
       // - git stash can't infer RD or MD states correctly, and will lose the deletion
       this.deletedFiles = await this.getDeletedFiles()
 
-      // the `git stash` clears metadata about a possible git merge
-      // Manually check and backup if necessary
-      await this.backupMergeStatus()
-
-      // Save stash of original state
-      await this.execGit(['stash', 'save', STASH])
-      await this.execGit(['stash', 'apply', '--quiet', '--index', await this.getBackupStash()])
-
-      // Restore meta information about ongoing git merge, cleared by `git stash`
-      await this.restoreMergeStatus()
-
-      // If stashing resurrected deleted files, clean them out
-      await Promise.all(this.deletedFiles.map((file) => unlink(file)))
+      // Save stash of all staged files.
+      // The `stash create` command creates a dangling commit without removing any files,
+      // and `stash store` saves it as an actual stash.
+      const hash = await this.execGit(['stash', 'create'])
+      await this.execGit(['stash', 'store', '--quiet', '--message', STASH, hash])
 
       debug('Done backing up original state!')
     } catch (error) {


Test patch:
diff --git a/test/gitWorkflow.spec.js b/test/gitWorkflow.spec.js
index 25bd995..30763d6 100644
--- a/test/gitWorkflow.spec.js
+++ b/test/gitWorkflow.spec.js
@@ -65,6 +65,29 @@ describe('gitWorkflow', () => {
     }
   })
 
+  describe('prepare', () => {
+    it('should handle errors', async () => {
+      const gitWorkflow = new GitWorkflow({
+        gitDir: cwd,
+        gitConfigDir: path.resolve(cwd, './.git')
+      })
+      jest.doMock('execa', () => Promise.reject({}))
+      const ctx = {}
+      // mock a simple failure
+      gitWorkflow.getPartiallyStagedFiles = () => ['foo']
+      gitWorkflow.getHiddenFilepath = () => {
+        throw new Error('test')
+      }
+      await expect(gitWorkflow.prepare(ctx, false)).rejects.toThrowErrorMatchingInlineSnapshot(
+        `"test"`
+      )
+      expect(ctx).toEqual({
+        gitError: true,
+        hasPartiallyStagedFiles: true
+      })
+    })
+  })
+
   describe('cleanup', () => {
     it('should handle errors', async () => {
       const gitWorkflow = new GitWorkflow({
diff --git a/test/runAll.spec.js b/test/runAll.spec.js
index fe6009d..0e23032 100644
--- a/test/runAll.spec.js
+++ b/test/runAll.spec.js
@@ -3,19 +3,22 @@ import execa from 'execa'
 import normalize from 'normalize-path'
 import path from 'path'
 
-import resolveGitRepo from '../lib/resolveGitRepo'
 import getStagedFiles from '../lib/getStagedFiles'
+import GitWorkflow from '../lib/gitWorkflow'
+import resolveGitRepo from '../lib/resolveGitRepo'
 import runAll, { shouldSkip } from '../lib/runAll'
 
-jest.mock('../lib/resolveGitRepo')
+jest.mock('../lib/file')
 jest.mock('../lib/getStagedFiles')
 jest.mock('../lib/gitWorkflow')
+jest.mock('../lib/resolveGitRepo')
+
+getStagedFiles.mockImplementation(async () => [])
 
 resolveGitRepo.mockImplementation(async () => {
   const cwd = process.cwd()
   return { gitConfigDir: normalize(path.resolve(cwd, '.git')), gitDir: normalize(cwd) }
 })
-getStagedFiles.mockImplementation(async () => [])
 
 describe('runAll', () => {
   const globalConsoleTemp = console
@@ -77,6 +80,46 @@ describe('runAll', () => {
     `)
   })
 
+  it('should skip tasks if previous git error', async () => {
+    expect.assertions(2)
+    getStagedFiles.mockImplementationOnce(async () => ['sample.js'])
+    GitWorkflow.mockImplementationOnce(() => ({
+      ...jest.requireActual('../lib/gitWorkflow'),
+      prepare: (ctx) => {
+        ctx.gitError = true
+        throw new Error('test')
+      }
+    }))
+
+    await expect(
+      runAll({ config: { '*.js': ['echo "sample"'] } })
+    ).rejects.toThrowErrorMatchingInlineSnapshot(`"Something went wrong"`)
+
+    expect(console.printHistory()).toMatchInlineSnapshot(`
+      "
+      LOG Preparing... [started]
+      LOG Preparing... [failed]
+      LOG → test
+      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 previous git 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.
+      ERROR   Any lost modifications can be restored from a git stash:
+
+          > git stash list
+          stash@{0}: On master: automatic lint-staged backup
+          > git stash apply --index stash@{0}
+      "
+    `)
+  })
+
   it('should skip applying unstaged modifications if there are errors during linting', async () => {
     expect.assertions(2)
     getStagedFiles.mockImplementationOnce(async () => ['sample.js'])
@@ -171,6 +214,13 @@ describe('runAll', () => {
 })
 
 describe('shouldSkip', () => {
+  describe('shouldSkipApplyModifications', () => {
+    it('should return error message when there is an unkown git error', () => {
+      const result = shouldSkip.shouldSkipApplyModifications({ gitError: true })
+      expect(typeof result === 'string').toEqual(true)
+    })
+  })
+
   describe('shouldSkipRevert', () => {
     it('should return error message when there is an unkown git error', () => {
       const result = shouldSkip.shouldSkipRevert({ gitError: true })
diff --git a/test/runAll.unmocked.2.spec.js b/test/runAll.unmocked.2.spec.js
index a505a0a..17698ff 100644
--- a/test/runAll.unmocked.2.spec.js
+++ b/test/runAll.unmocked.2.spec.js
@@ -107,14 +107,21 @@ describe('runAll', () => {
     expect(console.printHistory()).toMatchInlineSnapshot(`
       "
       LOG Preparing... [started]
-      LOG Preparing... [failed]
-      LOG → Merge state could not be restored due to an error!
+      LOG Preparing... [completed]
       LOG Running tasks... [started]
-      LOG Running tasks... [skipped]
-      LOG → Skipped because of previous git error.
+      LOG Running tasks for *.js [started]
+      LOG prettier --list-different [started]
+      LOG prettier --list-different [failed]
+      LOG → 
+      LOG Running tasks for *.js [failed]
+      LOG → 
+      LOG Running tasks... [failed]
       LOG Applying modifications... [started]
       LOG Applying modifications... [skipped]
-      LOG → Skipped because of previous git error.
+      LOG → Skipped because of errors from tasks.
+      LOG Reverting to original state because of errors... [started]
+      LOG Reverting to original state because of errors... [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.
diff --git a/test/runAll.unmocked.spec.js b/test/runAll.unmocked.spec.js
index f76f95f..5f06505 100644
--- a/test/runAll.unmocked.spec.js
+++ b/test/runAll.unmocked.spec.js
@@ -459,7 +459,7 @@ describe('runAll', () => {
 
     // Luckily there is a stash
     expect(await execGit(['stash', 'list'])).toMatchInlineSnapshot(
-      `"stash@{0}: On master: lint-staged automatic backup"`
+      `"stash@{0}: lint-staged automatic backup"`
     )
     await execGit(['reset', '--hard'])
     await execGit(['stash', 'pop', '--index'])