Skip to content

Commit

Permalink
fix: make sure deleted files aren't restored due to git bugs (#778)
Browse files Browse the repository at this point in the history
* fix: make sure deleted files aren't restored due to git bugs

* refactor(file): use fs.promises

* perf: run file removals in parallel

* fix: filter out empty string for when there is no output

* fix: correct file handling

* test: add test for complex additions and deletions

* fix: make cleaning untracked files resurrected due to git bug testable
  • Loading branch information
iiroj authored Jan 30, 2020
1 parent b98a5ed commit 6bfbe6c
Show file tree
Hide file tree
Showing 9 changed files with 161 additions and 85 deletions.
81 changes: 44 additions & 37 deletions lib/file.js
Original file line number Diff line number Diff line change
@@ -1,68 +1,75 @@
'use strict'

const debug = require('debug')('lint-staged:file')
const fs = require('fs')

const fsPromises = fs.promises
const fs = require('fs').promises

/**
* Check if a file exists. Returns the filepath if exists.
* @param {string} filepath
* Check if a file exists. Returns the filename if exists.
* @param {String} filename
* @returns {String|Boolean}
*/
const exists = async filepath => {
const exists = async filename => {
try {
await fsPromises.access(filepath)
return filepath
await fs.access(filename)
return filename
} catch {
return false
}
}

/**
* Read contents of a file to buffer
* @param {String} filename
* @returns {Promise<Buffer|Null>}
* @param {Boolean} [ignoreENOENT=true] — Whether to throw if the file doesn't exist
* @returns {Promise<Buffer>}
*/
const readBufferFromFile = (filename, rejectENOENT = false) =>
new Promise(resolve => {
debug('Reading buffer from file `%s`', filename)
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(buffer)
})
})
const readFile = async (filename, ignoreENOENT = true) => {
debug('Reading file `%s`', filename)
try {
return await fs.readFile(filename)
} catch (error) {
if (ignoreENOENT && error.code === 'ENOENT') {
debug("File `%s` doesn't exist, ignoring...", filename)
return null // no-op file doesn't exist
} else {
throw error
}
}
}

/**
* Unlink a file if it exists
* @param {*} filepath
* @param {String} filename
* @param {Boolean} [ignoreENOENT=true] — Whether to throw if the file doesn't exist
*/
const unlink = async filepath => {
if (filepath) {
await fsPromises.access(filepath)
await fsPromises.unlink(filepath)
const unlink = async (filename, ignoreENOENT = true) => {
if (filename) {
debug('Unlinking file `%s`', filename)
try {
await fs.unlink(filename)
} catch (error) {
if (ignoreENOENT && error.code === 'ENOENT') {
debug("File `%s` doesn't exist, ignoring...", filename)
} else {
throw error
}
}
}
}

/**
* Write buffer to file
* @param {String} filename
* @param {Buffer} buffer
* @returns {Promise<Void>}
*/
const writeBufferToFile = (filename, buffer) =>
new Promise(resolve => {
debug('Writing buffer to file `%s`', filename)
fs.writeFile(filename, buffer, () => {
debug('Done writing buffer to file `%s`!', filename)
resolve()
})
})
const writeFile = async (filename, buffer) => {
debug('Writing file `%s`', filename)
await fs.writeFile(filename, buffer)
}

module.exports = {
exists,
readBufferFromFile,
readFile,
unlink,
writeBufferToFile
writeFile
}
2 changes: 1 addition & 1 deletion lib/getStagedFiles.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ module.exports = async function getStagedFiles(options) {
try {
const lines = await execGit(['diff', '--staged', '--diff-filter=ACMR', '--name-only'], options)
return lines ? lines.split('\n') : []
} catch (error) {
} catch {
return null
}
}
63 changes: 36 additions & 27 deletions lib/gitWorkflow.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const debug = require('debug')('lint-staged:git')
const path = require('path')

const execGit = require('./execGit')
const { exists, readBufferFromFile, unlink, writeBufferToFile } = require('./file')
const { exists, readFile, unlink, writeFile } = require('./file')

const MERGE_HEAD = 'MERGE_HEAD'
const MERGE_MODE = 'MERGE_MODE'
Expand All @@ -18,23 +18,6 @@ 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`
* @param {Function} execGit function for executing git commands using execa
* @returns {Promise<void>}
*/
const cleanUntrackedFiles = async execGit => {
const untrackedFiles = await execGit(['ls-files', '--others', '--exclude-standard'])
if (untrackedFiles) {
debug('Detected unstaged, untracked files: ', untrackedFiles)
debug(
'This is probably due to a bug in git =< 2.13.0 where `git stash --keep-index` resurrects deleted files.'
)
debug('Deleting the files using `git clean`...')
await execGit(['clean', '--force', ...untrackedFiles.split('\n')])
}
}

const handleError = (error, ctx) => {
ctx.gitError = true
throw error
Expand All @@ -44,6 +27,7 @@ class GitWorkflow {
constructor({ allowEmpty, gitConfigDir, gitDir, stagedFileChunks }) {
this.execGit = (args, options = {}) => execGit(args, { ...options, cwd: gitDir })
this.gitConfigDir = gitConfigDir
this.gitDir = gitDir
this.unstagedDiff = null
this.allowEmpty = allowEmpty
this.stagedFileChunks = stagedFileChunks
Expand Down Expand Up @@ -73,7 +57,7 @@ class GitWorkflow {
const resolved = this.getHiddenFilepath(filename)
const pathIfExists = await exists(resolved)
if (!pathIfExists) return false
const buffer = await readBufferFromFile(pathIfExists)
const buffer = await readFile(pathIfExists)
const patch = buffer.toString().trim()
return patch.length ? filename : false
}
Expand All @@ -97,9 +81,9 @@ class GitWorkflow {
async backupMergeStatus() {
debug('Backing up merge state...')
await Promise.all([
readBufferFromFile(this.mergeHeadFilename).then(buffer => (this.mergeHeadBuffer = buffer)),
readBufferFromFile(this.mergeModeFilename).then(buffer => (this.mergeModeBuffer = buffer)),
readBufferFromFile(this.mergeMsgFilename).then(buffer => (this.mergeMsgBuffer = buffer))
readFile(this.mergeHeadFilename).then(buffer => (this.mergeHeadBuffer = buffer)),
readFile(this.mergeModeFilename).then(buffer => (this.mergeModeBuffer = buffer)),
readFile(this.mergeMsgFilename).then(buffer => (this.mergeMsgBuffer = buffer))
])
debug('Done backing up merge state!')
}
Expand All @@ -111,9 +95,9 @@ class GitWorkflow {
debug('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)
this.mergeHeadBuffer && writeFile(this.mergeHeadFilename, this.mergeHeadBuffer),
this.mergeModeBuffer && writeFile(this.mergeModeFilename, this.mergeModeBuffer),
this.mergeMsgBuffer && writeFile(this.mergeMsgFilename, this.mergeMsgBuffer)
])
debug('Done restoring merge state!')
} catch (error) {
Expand All @@ -123,6 +107,18 @@ class GitWorkflow {
}
}

/**
* List and delete untracked files
*/
async cleanUntrackedFiles() {
const lsFiles = await this.execGit(['ls-files', '--others', '--exclude-standard'])
const untrackedFiles = lsFiles
.split('\n')
.filter(Boolean)
.map(file => path.resolve(this.gitDir, file))
await Promise.all(untrackedFiles.map(file => unlink(file)))
}

/**
* Create backup stashes, one of everything and one of only staged changes
* Staged files are left in the index for running tasks
Expand All @@ -135,6 +131,14 @@ class GitWorkflow {
// Manually check and backup if necessary
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
// - git stash can't infer RD or MD states correctly, and will lose the deletion
this.deletedFiles = (await this.execGit(['ls-files', '--deleted']))
.split('\n')
.filter(Boolean)
.map(file => path.resolve(this.gitDir, file))

// 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', '--include-untracked', '--keep-index', STASH])
Expand All @@ -144,7 +148,7 @@ class GitWorkflow {

// 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)
await this.cleanUntrackedFiles()

// Get a diff of unstaged changes by diffing the saved stash against what's left on disk.
await this.execGit([
Expand Down Expand Up @@ -238,6 +242,9 @@ class GitWorkflow {
ctx.gitRestoreUntrackedError = true
handleError(error, ctx)
}

// If stashing resurrected deleted files, clean them out
await Promise.all(this.deletedFiles.map(file => unlink(file)))
}

/**
Expand All @@ -251,6 +258,9 @@ class GitWorkflow {
await this.execGit(['stash', 'apply', '--quiet', '--index', backupStash])
debug('Done restoring original state!')

// If stashing resurrected deleted files, clean them out
await Promise.all(this.deletedFiles.map(file => unlink(file)))

// Restore meta information about ongoing git merge
await this.restoreMergeStatus()
} catch (error) {
Expand Down Expand Up @@ -278,4 +288,3 @@ class GitWorkflow {
}

module.exports = GitWorkflow
module.exports.cleanUntrackedFiles = cleanUntrackedFiles
2 changes: 1 addition & 1 deletion lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const errConfigNotFound = new Error('Config could not be found')
function resolveConfig(configPath) {
try {
return require.resolve(configPath)
} catch (ignore) {
} catch {
return configPath
}
}
Expand Down
4 changes: 2 additions & 2 deletions lib/resolveGitRepo.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const path = require('path')
const debugLog = require('debug')('lint-staged:resolveGitRepo')

const execGit = require('./execGit')
const { readBufferFromFile } = require('./file')
const { readFile } = require('./file')

/**
* Resolve path to the .git directory, with special handling for
Expand All @@ -19,7 +19,7 @@ const resolveGitConfigDir = async gitDir => {
// If .git is a directory, use it
if (stats.isDirectory()) return defaultDir
// Otherwise .git is a file containing path to real location
const file = (await readBufferFromFile(defaultDir)).toString()
const file = (await readFile(defaultDir)).toString()
return path.resolve(gitDir, file.replace(/^gitdir: /, '')).trim()
}

Expand Down
13 changes: 13 additions & 0 deletions test/file.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { unlink, readFile } from '../lib/file'

describe('unlink', () => {
it('should throw when second argument is false and file is not found', async () => {
await expect(unlink('example', false)).rejects.toThrow('ENOENT')
})
})

describe('readFile', () => {
it('should throw when second argument is false and file is not found', async () => {
await expect(readFile('example', false)).rejects.toThrow('ENOENT')
})
})
27 changes: 16 additions & 11 deletions test/gitWorkflow.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import path from 'path'
import nanoid from 'nanoid'

import execGitBase from '../lib/execGit'
import GitWorkflow, { cleanUntrackedFiles } from '../lib/gitWorkflow'
import GitWorkflow from '../lib/gitWorkflow'

jest.unmock('execa')

Expand Down Expand Up @@ -65,16 +65,6 @@ describe('gitWorkflow', () => {
}
})

describe('cleanUntrackedFiles', () => {
it('should delete untracked, unstaged files', async () => {
const testFile = path.resolve(cwd, 'test.js')
await appendFile(testFile, 'test')
expect(await fs.exists(testFile)).toEqual(true)
await cleanUntrackedFiles(execGit)
expect(await fs.exists(testFile)).toEqual(false)
})
})

describe('hasPatch', () => {
it('should return false when patch file not found', async () => {
const gitWorkflow = new GitWorkflow({
Expand All @@ -101,4 +91,19 @@ describe('gitWorkflow', () => {
})
})
})

describe('cleanUntrackedFiles', () => {
it('should remove untracked files', async () => {
const tempFile = path.resolve(cwd, 'tempFile')
await fs.writeFile(tempFile, 'Hello')

const gitWorkflow = new GitWorkflow({
gitDir: cwd,
gitConfigDir: path.resolve(cwd, './.git')
})

await gitWorkflow.cleanUntrackedFiles()
await expect(fs.access(tempFile)).rejects.toThrow('ENOENT')
})
})
})
6 changes: 3 additions & 3 deletions test/runAll.unmocked.2.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import nanoid from 'nanoid'
jest.mock('../lib/file')

import execGitBase from '../lib/execGit'
import { readBufferFromFile, writeBufferToFile } from '../lib/file'
import { readFile, writeFile } from '../lib/file'
import runAll from '../lib/runAll'

jest.unmock('execa')
Expand Down Expand Up @@ -89,8 +89,8 @@ describe('runAll', () => {
})

it.only('Should throw when restoring untracked files fails', async () => {
readBufferFromFile.mockImplementation(async () => Buffer.from('test'))
writeBufferToFile.mockImplementation(async () => Promise.reject('test'))
readFile.mockImplementation(async () => Buffer.from('test'))
writeFile.mockImplementation(async () => Promise.reject('test'))

// Stage pretty file
await appendFile('test.js', testJsFilePretty)
Expand Down
Loading

0 comments on commit 6bfbe6c

Please sign in to comment.