Repository: okonet/lint-staged
Pull request #732 (2019-11-17 10:45:33)

feat: split tasks into chunks to support shells with limited max argument length
This PR adds chunking to running tasks to better support Windows shells with more limited maximum argument string length.

The logic of chunking is to first create the full argument string `filesArg` by joining the list of staged files (and resolving as absolute paths when necessary). This string's length is compared to a safe value of half of the maximum argument length `maxLength`, and the number of chunks is calculated as `Math.min(Math.ceil(filesArg/maxLength), files.length)`. So when the `filesArg` is for example 3.5 times the length of `maxLength`, four chunks are created. If there for whatever reason are less files than chunks, at most one file per chunk is created.

After chunking, instead of a single `Running tasks...`, there will be `Running tasks (1/4)...` and so on.

Fixes https://github.com/okonet/lint-staged/issues/147

codecov[bot]: # [Codecov](https://codecov.io/gh/okonet/lint-staged/pull/732?src=pr&el=h1) Report
> Merging [#732](https://codecov.io/gh/okonet/lint-staged/pull/732?src=pr&el=desc) into [beta](https://codecov.io/gh/okonet/lint-staged/commit/5d989acfe3f88989f02d0168cd983ecb059f4509?src=pr&el=desc) will **decrease** coverage by `0.73%`.
> The diff coverage is `93.33%`.

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

```diff
@@            Coverage Diff            @@
##             beta    #732      +/-   ##
=========================================
- Coverage   99.13%   98.4%   -0.74%     
=========================================
  Files          12      13       +1     
  Lines         346     375      +29     
  Branches       64      70       +6     
=========================================
+ Hits          343     369      +26     
- Misses          3       6       +3
```


| [Impacted Files](https://codecov.io/gh/okonet/lint-staged/pull/732?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [lib/chunkFiles.js](https://codecov.io/gh/okonet/lint-staged/pull/732/diff?src=pr&el=tree#diff-bGliL2NodW5rRmlsZXMuanM=) | `100% <100%> (ø)` | |
| [lib/runAll.js](https://codecov.io/gh/okonet/lint-staged/pull/732/diff?src=pr&el=tree#diff-bGliL3J1bkFsbC5qcw==) | `95.52% <87.5%> (-4.48%)` | :arrow_down: |

------

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

iiroj: What do you think, @okonet?
okonet: Still so much more code to maintain that I’m not sure about that change. Do you think just having a check and throwing a warning wouldn’t suffice?
iiroj: I don't think this is too complicated.

The good thing compared to the previous chunking implementation is that this is completely separate from the generation of tasks, and will simply generate multiple "runs" of tasks from a smaller set of files. It is also run on all platforms, not just on Windows, so the implementation doesn't vary.

I guess the warning wasn't good enough for Windows users, because any team with at least one Windows developer would always have to use function linters and basically implement this feature themselves.
okonet: Okay I’m convinced now. Let’s get it out!
iiroj: Deleted test was for the warning, which no longer exists. There are new tests for the chunking function, and one for `runAll` where it runs two chunks: https://github.com/okonet/lint-staged/pull/732/files#diff-850db98aaae9bd6f45e2f5cbae2b9199R542
okonet: Okay call sgtm

Patch:
diff --git a/README.md b/README.md
index a171e46..fe953ea 100644
--- a/README.md
+++ b/README.md
@@ -394,6 +394,8 @@ Parameters to `lintStaged` are equivalent to their CLI counterparts:
 ```js
 const success = await lintStaged({
   configPath: './path/to/configuration/file',
+  maxArgLength: null,
+  relative: false,
   shell: false,
   quiet: false,
   debug: false
@@ -407,12 +409,16 @@ const success = await lintStaged({
   config: {
     '*.js': 'eslint --fix'
   },
+  maxArgLength: null,
+  relative: false,
   shell: false,
   quiet: false,
   debug: false
 })
 ```
 
+The `maxArgLength` option configures chunking of tasks into multiple parts that are run one after the other. This is to avoid issues on Windows platforms where the maximum length of the command line argument string is limited to 8192 characters. Lint-staged might generate a very long argument string when there are many staged files. This option is set automatically from the cli, but not via the Node.js API by default.
+
 ### Using with JetBrains IDEs _(WebStorm, PyCharm, IntelliJ IDEA, RubyMine, etc.)_
 
 _**Update**_: The latest version of JetBrains IDEs now support running hooks as you would expect.
diff --git a/bin/lint-staged b/bin/lint-staged
index 277f1b6..eea0d69 100755
--- a/bin/lint-staged
+++ b/bin/lint-staged
@@ -42,8 +42,27 @@ if (cmdline.debug) {
 
 debug('Running `lint-staged@%s`', pkg.version)
 
+/**
+ * Get the maximum length of a command-line argument string based on current platform
+ *
+ * https://serverfault.com/questions/69430/what-is-the-maximum-length-of-a-command-line-in-mac-os-x
+ * https://support.microsoft.com/en-us/help/830473/command-prompt-cmd-exe-command-line-string-limitation
+ * https://unix.stackexchange.com/a/120652
+ */
+const getMaxArgLength = () => {
+  switch (process.platform) {
+    case 'darwin':
+      return 262144
+    case 'win32':
+      return 8191
+    default:
+      return 131072
+  }
+}
+
 lintStaged({
   configPath: cmdline.config,
+  maxArgLength: getMaxArgLength(),
   relative: !!cmdline.relative,
   shell: !!cmdline.shell,
   quiet: !!cmdline.quiet,
diff --git a/lib/chunkFiles.js b/lib/chunkFiles.js
new file mode 100644
index 0000000..824c383
--- /dev/null
+++ b/lib/chunkFiles.js
@@ -0,0 +1,40 @@
+'use strict'
+
+const normalize = require('normalize-path')
+const path = require('path')
+
+/**
+ * Chunk array into sub-arrays
+ * @param {Array} arr
+ * @param {Number} chunkCount
+ * @retuns {Array<Array>}
+ */
+function chunkArray(arr, chunkCount) {
+  if (chunkCount === 1) return [arr]
+  const chunked = []
+  let position = 0
+  for (let i = 0; i < chunkCount; i++) {
+    const chunkLength = Math.ceil((arr.length - position) / (chunkCount - i))
+    chunked.push([])
+    chunked[i] = arr.slice(position, chunkLength + position)
+    position += chunkLength
+  }
+  return chunked
+}
+
+/**
+ * Chunk files into sub-arrays based on the length of the resulting argument string
+ * @param {Object} opts
+ * @param {Array<String>} opts.files
+ * @param {String} opts.gitDir
+ * @param {number} [opts.maxArgLength] the maximum argument string length
+ * @param {Boolean} [opts.relative] whether files are relative to `gitDir` or should be resolved as absolute
+ * @returns {Array<Array<String>>}
+ */
+module.exports = function chunkFiles({ files, gitDir, maxArgLength = null, relative = false }) {
+  if (!maxArgLength) return [files]
+  const normalizedFiles = files.map(file => normalize(relative ? file : path.resolve(gitDir, file)))
+  const fileListLength = normalizedFiles.join(' ').length
+  const chunkCount = Math.min(Math.ceil(fileListLength / maxArgLength), files.length)
+  return chunkArray(files, chunkCount)
+}
diff --git a/lib/index.js b/lib/index.js
index bdfd92b..53827a4 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -44,6 +44,7 @@ function loadConfig(configPath) {
  * @param {object} options
  * @param {string} [options.configPath] - Path to configuration file
  * @param {object}  [options.config] - Object with configuration for programmatic API
+ * @param {number} [options.maxArgLength] - Maximum argument string length
  * @param {boolean} [options.relative] - Pass relative filepaths to tasks
  * @param {boolean} [options.shell] - Skip parsing of tasks for better shell support
  * @param {boolean} [options.quiet] - Disable lint-staged’s own console output
@@ -53,7 +54,15 @@ function loadConfig(configPath) {
  * @returns {Promise<boolean>} Promise of whether the linting passed or failed
  */
 module.exports = function lintStaged(
-  { configPath, config, relative = false, shell = false, quiet = false, debug = false } = {},
+  {
+    configPath,
+    config,
+    maxArgLength,
+    relative = false,
+    shell = false,
+    quiet = false,
+    debug = false
+  } = {},
   logger = console
 ) {
   debugLog('Loading config using `cosmiconfig`')
@@ -76,7 +85,7 @@ module.exports = function lintStaged(
         debugLog('lint-staged config:\n%O', config)
       }
 
-      return runAll({ config, relative, shell, quiet, debug }, logger)
+      return runAll({ config, maxArgLength, relative, shell, quiet, debug }, logger)
         .then(() => {
           debugLog('tasks were executed successfully!')
           return Promise.resolve(true)
diff --git a/lib/runAll.js b/lib/runAll.js
index 21c1cef..cbc6ada 100644
--- a/lib/runAll.js
+++ b/lib/runAll.js
@@ -6,6 +6,7 @@ const chalk = require('chalk')
 const Listr = require('listr')
 const symbols = require('log-symbols')
 
+const chunkFiles = require('./chunkFiles')
 const generateTasks = require('./generateTasks')
 const getStagedFiles = require('./getStagedFiles')
 const GitWorkflow = require('./gitWorkflow')
@@ -14,20 +15,13 @@ const resolveGitDir = require('./resolveGitDir')
 
 const debugLog = require('debug')('lint-staged:run')
 
-/**
- * https://serverfault.com/questions/69430/what-is-the-maximum-length-of-a-command-line-in-mac-os-x
- * https://support.microsoft.com/en-us/help/830473/command-prompt-cmd-exe-command-line-string-limitation
- * https://unix.stackexchange.com/a/120652
- */
-const MAX_ARG_LENGTH =
-  (process.platform === 'darwin' && 262144) || (process.platform === 'win32' && 8191) || 131072
-
 /**
  * Executes all tasks and either resolves or rejects the promise
  *
  * @param {object} options
  * @param {Object} [options.config] - Task configuration
  * @param {Object} [options.cwd] - Current working directory
+ * @param {number} [options.maxArgLength] - Maximum argument string length
  * @param {boolean} [options.relative] - Pass relative filepaths to tasks
  * @param {boolean} [options.shell] - Skip parsing of tasks for better shell support
  * @param {boolean} [options.quiet] - Disable lint-staged’s own console output
@@ -36,7 +30,15 @@ const MAX_ARG_LENGTH =
  * @returns {Promise}
  */
 module.exports = async function runAll(
-  { config, cwd = process.cwd(), debug = false, quiet = false, relative = false, shell = false },
+  {
+    config,
+    cwd = process.cwd(),
+    debug = false,
+    maxArgLength,
+    quiet = false,
+    relative = false,
+    shell = false
+  },
   logger = console
 ) {
   debugLog('Running all linter scripts')
@@ -57,48 +59,70 @@ module.exports = async function runAll(
 
   debugLog('Loaded list of staged files in git:\n%O', files)
 
-  const argLength = files.join(' ').length
-  if (argLength > MAX_ARG_LENGTH) {
-    logger.warn(`
- ${symbols.warning}  ${chalk.yellow(
-      `lint-staged generated an argument string of ${argLength} characters, and commands might not run correctly on your platform.
-    It is recommended to use functions as linters and split your command based on the number of staged files. For more info, please visit:
-    https://github.com/okonet/lint-staged#using-js-functions-to-customize-linter-commands`
-    )}
-`)
+  const chunkedFiles = chunkFiles({ files, gitDir, maxArgLength, relative })
+  const chunkCount = chunkedFiles.length
+  if (chunkCount > 1) {
+    debugLog(`Chunked staged files into ${chunkCount} part`, chunkCount)
   }
 
-  const tasks = generateTasks({ config, cwd, gitDir, files, relative })
-
   // lint-staged 10 will automatically add modifications to index
   // Warn user when their command includes `git add`
   let hasDeprecatedGitAdd = false
 
-  const listrTasks = tasks.map(task => {
-    const subTasks = makeCmdTasks({ commands: task.commands, files: task.fileList, gitDir, shell })
+  const listrOptions = {
+    dateFormat: false,
+    renderer: (quiet && 'silent') || (debug && 'verbose') || 'update'
+  }
 
-    if (subTasks.some(subTask => subTask.command.includes('git add'))) {
-      hasDeprecatedGitAdd = true
-    }
+  const listrTasks = []
+
+  for (const [index, files] of chunkedFiles.entries()) {
+    const chunkTasks = generateTasks({ config, cwd, gitDir, files, relative })
+    const chunkListrTasks = chunkTasks.map(task => {
+      const subTasks = makeCmdTasks({
+        commands: task.commands,
+        files: task.fileList,
+        gitDir,
+        shell
+      })
 
-    return {
-      title: `Running tasks for ${task.pattern}`,
-      task: async () =>
-        new Listr(subTasks, {
-          // In sub-tasks we don't want to run concurrently
-          // and we want to abort on errors
-          dateFormat: false,
-          concurrent: false,
-          exitOnError: true
-        }),
+      if (subTasks.some(subTask => subTask.command.includes('git add'))) {
+        hasDeprecatedGitAdd = true
+      }
+
+      return {
+        title: `Running tasks for ${task.pattern}`,
+        task: async () =>
+          new Listr(subTasks, {
+            // In sub-tasks we don't want to run concurrently
+            // and we want to abort on errors
+            dateFormat: false,
+            concurrent: false,
+            exitOnError: true
+          }),
+        skip: () => {
+          if (task.fileList.length === 0) {
+            return `No staged files match ${task.pattern}`
+          }
+          return false
+        }
+      }
+    })
+
+    listrTasks.push({
+      // No need to show number of task chunks when there's only one
+      title:
+        chunkCount > 1 ? `Running tasks (chunk ${index}/${chunkCount})...` : 'Running tasks...',
+      task: () =>
+        new Listr(chunkListrTasks, { ...listrOptions, concurrent: false, exitOnError: false }),
       skip: () => {
-        if (task.fileList.length === 0) {
-          return `No staged files match ${task.pattern}`
+        if (chunkListrTasks.every(task => task.skip())) {
+          return 'No tasks to run'
         }
         return false
       }
-    }
-  })
+    })
+  }
 
   if (hasDeprecatedGitAdd) {
     logger.warn(`${symbols.warning} ${chalk.yellow(
@@ -114,11 +138,6 @@ module.exports = async function runAll(
     return 'No tasks to run.'
   }
 
-  const listrOptions = {
-    dateFormat: false,
-    renderer: (quiet && 'silent') || (debug && 'verbose') || 'update'
-  }
-
   const git = new GitWorkflow(gitDir)
 
   const runner = new Listr(
@@ -127,10 +146,7 @@ module.exports = async function runAll(
         title: 'Preparing...',
         task: () => git.stashBackup()
       },
-      {
-        title: 'Running tasks...',
-        task: () => new Listr(listrTasks, { ...listrOptions, concurrent: true, exitOnError: false })
-      },
+      ...listrTasks,
       {
         title: 'Applying modifications...',
         skip: ctx => ctx.hasErrors && 'Skipped because of errors from tasks',


Test patch:
diff --git a/test/__snapshots__/runAll.spec.js.snap b/test/__snapshots__/runAll.spec.js.snap
index 130ed3b..3ce7f0e 100644
--- a/test/__snapshots__/runAll.spec.js.snap
+++ b/test/__snapshots__/runAll.spec.js.snap
@@ -87,24 +87,3 @@ exports[`runAll should use an injected logger 1`] = `
 "
 LOG No staged files match any of provided globs."
 `;
-
-exports[`runAll should warn if the argument length is longer than what the platform can handle 1`] = `
-"
-WARN 
- ‼  lint-staged generated an argument string of 999999 characters, and commands might not run correctly on your platform.
-    It is recommended to use functions as linters and split your command based on the number of staged files. For more info, please visit:
-    https://github.com/okonet/lint-staged#using-js-functions-to-customize-linter-commands
-
-LOG Preparing... [started]
-LOG Preparing... [completed]
-LOG Running tasks... [started]
-LOG Running tasks for *.js [started]
-LOG echo \\"sample\\" [started]
-LOG echo \\"sample\\" [completed]
-LOG Running tasks for *.js [completed]
-LOG Running tasks... [completed]
-LOG Applying modifications... [started]
-LOG Applying modifications... [completed]
-LOG Cleaning up... [started]
-LOG Cleaning up... [completed]"
-`;
diff --git a/test/chunkFiles.spec.js b/test/chunkFiles.spec.js
new file mode 100644
index 0000000..7686d44
--- /dev/null
+++ b/test/chunkFiles.spec.js
@@ -0,0 +1,29 @@
+import chunkFiles from '../lib/chunkFiles'
+
+describe('chunkFiles', () => {
+  const files = ['example.js', 'foo.js', 'bar.js', 'foo/bar.js']
+  const gitDir = '/opt/git/example.git'
+
+  it('should default to sane value', () => {
+    const chunkedFiles = chunkFiles({ files: ['foo.js'], gitDir, relative: true })
+    expect(chunkedFiles).toEqual([['foo.js']])
+  })
+
+  it('should not chunk short argument string', () => {
+    const chunkedFiles = chunkFiles({ files, gitDir, maxArgLength: 1000 })
+    expect(chunkedFiles).toEqual([files])
+  })
+
+  it('should chunk too long argument string', () => {
+    const chunkedFiles = chunkFiles({ files, gitDir, maxArgLength: 20 })
+    expect(chunkedFiles).toEqual(files.map(file => [file]))
+  })
+
+  it('should take into account relative setting', () => {
+    const chunkedFiles = chunkFiles({ files, gitDir, maxArgLength: 20, relative: true })
+    expect(chunkedFiles).toEqual([
+      [files[0], files[1]],
+      [files[2], files[3]]
+    ])
+  })
+})
diff --git a/test/runAll.spec.js b/test/runAll.spec.js
index c729399..c5d32b9 100644
--- a/test/runAll.spec.js
+++ b/test/runAll.spec.js
@@ -38,18 +38,6 @@ describe('runAll', () => {
     expect(console.printHistory()).toMatchSnapshot()
   })
 
-  it('should warn if the argument length is longer than what the platform can handle', async () => {
-    getStagedFiles.mockImplementationOnce(async () => new Array(100000).fill('sample.js'))
-
-    try {
-      await runAll({ config: { '*.js': () => 'echo "sample"' } })
-    } catch (err) {
-      console.log(err)
-    }
-
-    expect(console.printHistory()).toMatchSnapshot()
-  })
-
   it('should use an injected logger', async () => {
     expect.assertions(1)
     const logger = makeConsoleMock()
diff --git a/test/runAll.unmocked.spec.js b/test/runAll.unmocked.spec.js
index affb642..52c6f90 100644
--- a/test/runAll.unmocked.spec.js
+++ b/test/runAll.unmocked.spec.js
@@ -82,6 +82,11 @@ 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
@@ -533,4 +538,22 @@ describe('runAll', () => {
     expect(await execGit(['log', '-1', '--pretty=%B'])).toMatch('test')
     expect(await readFile('test.js')).toEqual(testJsFilePretty)
   })
+
+  it('should run chunked tasks when necessary', async () => {
+    // Stage two files
+    await appendFile('test.js', testJsFilePretty)
+    await execGit(['add', 'test.js'])
+    await appendFile('test2.js', testJsFilePretty)
+    await execGit(['add', 'test2.js'])
+
+    // Run lint-staged with `prettier --list-different` and commit pretty file
+    // Set maxArgLength low enough so that chunking is used
+    await gitCommit({ config: { '*.js': 'prettier --list-different' }, maxArgLength: 10 })
+
+    // 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(await readFile('test2.js')).toEqual(testJsFilePretty)
+  })
 })