fix: try to automatically fix and warn about invalid brace patterns (#…

…992)

* refactor: use arrow function

* fix: correctly handle incorrect patterns with braces of only single value

Technically brace patterns like `*.{js}` are invalid because braces should
always contain at least one comma or a sequence,
for example `*.{js,ts}` or `file{1..10}.js`.

The `micromatch` library used to match patterns to files will silently ignore
such invalid braces and thus lead to the pattern always matching zero files.
This is a unintuitive, so lint-staged should normalize such cases
by simply removing the unnecessary braces before matching files.

* test: add test for matching pattern with bracket sequence

* refactor: less nesting in validateConfig

* refactor: move pattern fixing to validateConfig

* refactor: move error to messages

* refactor: throw early for empty configuration

* fix: improve configuration error output

* refactor: move formatConfig into validateConfig

* test: update snapshot serializer to remove absolute file path

* fix: do not pass unused logger argument

* refactor: move config validation logging to validateConfig

* fix: do not match escaped curly braces (`\{` or `\}`)

* test: better error message in snapshot

* test: add missing assertions

* fix: match braces with escaped comma inside

* fix: do not match braces when they start with the dollar sign (`$`)

* docs: update BRACES_REGEXP comment

* refactor: move brace pattern validation to separate file

* fix: correctly handle nested braces

* refactor: clean up code

* test: add some more tests

lib/generateTasks.js

@@ -16,43 +16,41 @@ const debug = require('debug')('lint-staged:gen-tasks')
  * @param {boolean} [options.files] - Staged filepaths
  * @param {boolean} [options.relative] - Whether filepaths to should be relative to gitDir
  */
-module.exports = function generateTasks({
-  config,
-  cwd = process.cwd(),
-  gitDir,
-  files,
-  relative = false,
-}) {
+const generateTasks = ({ config, cwd = process.cwd(), gitDir, files, relative = false }) => {
   debug('Generating linter tasks')
 
   const absoluteFiles = files.map((file) => normalize(path.resolve(gitDir, file)))
   const relativeFiles = absoluteFiles.map((file) => normalize(path.relative(cwd, file)))
 
-  return Object.entries(config).map(([pattern, commands]) => {
+  return Object.entries(config).map(([rawPattern, commands]) => {
+    let pattern = rawPattern
+
     const isParentDirPattern = pattern.startsWith('../')
 
-    const fileList = micromatch(
-      relativeFiles
-        // Only worry about children of the CWD unless the pattern explicitly
-        // specifies that it concerns a parent directory.
-        .filter((file) => {
-          if (isParentDirPattern) return true
-          return !file.startsWith('..') && !path.isAbsolute(file)
-        }),
-      pattern,
-      {
-        cwd,
-        dot: true,
-        // If pattern doesn't look like a path, enable `matchBase` to
-        // match against filenames in every directory. This makes `*.js`
-        // match both `test.js` and `subdirectory/test.js`.
-        matchBase: !pattern.includes('/'),
-      }
-    ).map((file) => normalize(relative ? file : path.resolve(cwd, file)))
+    // Only worry about children of the CWD unless the pattern explicitly
+    // specifies that it concerns a parent directory.
+    const filteredFiles = relativeFiles.filter((file) => {
+      if (isParentDirPattern) return true
+      return !file.startsWith('..') && !path.isAbsolute(file)
+    })
+
+    const matches = micromatch(filteredFiles, pattern, {
+      cwd,
+      dot: true,
+      // If the pattern doesn't look like a path, enable `matchBase` to
+      // match against filenames in every directory. This makes `*.js`
+      // match both `test.js` and `subdirectory/test.js`.
+      matchBase: !pattern.includes('/'),
+      strictBrackets: true,
+    })
+
+    const fileList = matches.map((file) => normalize(relative ? file : path.resolve(cwd, file)))
 
     const task = { pattern, commands, fileList }
     debug('Generated task: \n%O', task)
 
     return task
   })
 }
+
+module.exports = generateTasks

lib/index.js

@@ -1,6 +1,5 @@
 'use strict'
 
-const dedent = require('dedent')
 const { cosmiconfig } = require('cosmiconfig')
 const debugLog = require('debug')('lint-staged')
 const stringifyObject = require('stringify-object')
@@ -13,9 +12,7 @@ const {
   ConfigNotFoundError,
   GetBackupStashError,
   GitError,
-  InvalidOptionsError,
 } = require('./symbols')
-const formatConfig = require('./formatConfig')
 const validateConfig = require('./validateConfig')
 const validateOptions = require('./validateOptions')
 
@@ -85,105 +82,78 @@ const lintStaged = async (
   } = {},
   logger = console
 ) => {
-  try {
-    await validateOptions({ shell }, logger)
+  await validateOptions({ shell }, logger)
 
-    debugLog('Loading config using `cosmiconfig`')
+  debugLog('Loading config using `cosmiconfig`')
 
-    const resolved = configObject
-      ? { config: configObject, filepath: '(input)' }
-      : await loadConfig(configPath)
+  const resolved = configObject
+    ? { config: configObject, filepath: '(input)' }
+    : await loadConfig(configPath)
 
-    if (resolved == null) {
-      throw ConfigNotFoundError
-    }
+  if (resolved == null) {
+    logger.error(`${ConfigNotFoundError.message}.`)
+    throw ConfigNotFoundError
+  }
 
-    debugLog('Successfully loaded config from `%s`:\n%O', resolved.filepath, resolved.config)
-
-    // resolved.config is the parsed configuration object
-    // resolved.filepath is the path to the config file that was found
-    const formattedConfig = formatConfig(resolved.config)
-    const config = validateConfig(formattedConfig)
-
-    if (debug) {
-      // Log using logger to be able to test through `consolemock`.
-      logger.log('Running lint-staged with the following config:')
-      logger.log(stringifyObject(config, { indent: '  ' }))
-    } else {
-      // We might not be in debug mode but `DEBUG=lint-staged*` could have
-      // been set.
-      debugLog('lint-staged config:\n%O', config)
-    }
+  debugLog('Successfully loaded config from `%s`:\n%O', resolved.filepath, resolved.config)
 
-    // Unset GIT_LITERAL_PATHSPECS to not mess with path interpretation
-    debugLog('Unset GIT_LITERAL_PATHSPECS (was `%s`)', process.env.GIT_LITERAL_PATHSPECS)
-    delete process.env.GIT_LITERAL_PATHSPECS
-
-    try {
-      const ctx = await runAll(
-        {
-          allowEmpty,
-          concurrent,
-          config,
-          cwd,
-          debug,
-          maxArgLength,
-          quiet,
-          relative,
-          shell,
-          stash,
-          verbose,
-        },
-        logger
-      )
-      debugLog('Tasks were executed successfully!')
-      printTaskOutput(ctx, logger)
-      return true
-    } catch (runAllError) {
-      if (runAllError && runAllError.ctx && runAllError.ctx.errors) {
-        const { ctx } = runAllError
-        if (ctx.errors.has(ApplyEmptyCommitError)) {
-          logger.warn(PREVENTED_EMPTY_COMMIT)
-        } else if (ctx.errors.has(GitError) && !ctx.errors.has(GetBackupStashError)) {
-          logger.error(GIT_ERROR)
-          if (ctx.shouldBackup) {
-            // No sense to show this if the backup stash itself is missing.
-            logger.error(RESTORE_STASH_EXAMPLE)
-          }
-        }
+  // resolved.config is the parsed configuration object
+  // resolved.filepath is the path to the config file that was found
+  const config = validateConfig(resolved.config, logger)
 
-        printTaskOutput(ctx, logger)
-        return false
-      }
+  if (debug) {
+    // Log using logger to be able to test through `consolemock`.
+    logger.log('Running lint-staged with the following config:')
+    logger.log(stringifyObject(config, { indent: '  ' }))
+  } else {
+    // We might not be in debug mode but `DEBUG=lint-staged*` could have
+    // been set.
+    debugLog('lint-staged config:\n%O', config)
+  }
 
-      // Probably a compilation error in the config js file. Pass it up to the outer error handler for logging.
-      throw runAllError
-    }
-  } catch (lintStagedError) {
-    /** throw early because `validateOptions` options contains own logging */
-    if (lintStagedError === InvalidOptionsError) {
-      throw InvalidOptionsError
-    }
+  // Unset GIT_LITERAL_PATHSPECS to not mess with path interpretation
+  debugLog('Unset GIT_LITERAL_PATHSPECS (was `%s`)', process.env.GIT_LITERAL_PATHSPECS)
+  delete process.env.GIT_LITERAL_PATHSPECS
 
-    /** @todo move logging to `validateConfig` and remove this try/catch block */
-    if (lintStagedError === ConfigNotFoundError) {
-      logger.error(`${lintStagedError.message}.`)
-    } else {
-      // It was probably a parsing error
-      logger.error(dedent`
-        Could not parse lint-staged config.
+  try {
+    const ctx = await runAll(
+      {
+        allowEmpty,
+        concurrent,
+        config,
+        cwd,
+        debug,
+        maxArgLength,
+        quiet,
+        relative,
+        shell,
+        stash,
+        verbose,
+      },
+      logger
+    )
+    debugLog('Tasks were executed successfully!')
+    printTaskOutput(ctx, logger)
+    return true
+  } catch (runAllError) {
+    if (runAllError && runAllError.ctx && runAllError.ctx.errors) {
+      const { ctx } = runAllError
+      if (ctx.errors.has(ApplyEmptyCommitError)) {
+        logger.warn(PREVENTED_EMPTY_COMMIT)
+      } else if (ctx.errors.has(GitError) && !ctx.errors.has(GetBackupStashError)) {
+        logger.error(GIT_ERROR)
+        if (ctx.shouldBackup) {
+          // No sense to show this if the backup stash itself is missing.
+          logger.error(RESTORE_STASH_EXAMPLE)
+        }
+      }
 
-        ${lintStagedError}
-      `)
+      printTaskOutput(ctx, logger)
+      return false
     }
-    logger.error() // empty line
-    // Print helpful message for all errors
-    logger.error(dedent`
-      Please make sure you have created it correctly.
-      See https://github.com/okonet/lint-staged#configuration.
-    `)
-
-    throw lintStagedError
+
+    // Probably a compilation error in the config js file. Pass it up to the outer error handler for logging.
+    throw runAllError
   }
 }
 

lib/makeCmdTasks.js

@@ -3,8 +3,8 @@
 const cliTruncate = require('cli-truncate')
 const debug = require('debug')('lint-staged:make-cmd-tasks')
 
+const { configurationError } = require('./messages')
 const resolveTaskFn = require('./resolveTaskFn')
-const { createError } = require('./validateConfig')
 
 const STDOUT_COLUMNS_DEFAULT = 80
 
@@ -51,7 +51,7 @@ const makeCmdTasks = async ({ commands, files, gitDir, renderer, shell, verbose
       // Do the validation here instead of `validateConfig` to skip evaluating the function multiple times
       if (isFn && typeof command !== 'string') {
         throw new Error(
-          createError(
+          configurationError(
             '[Function]',
             'Function task should return a string or an array of strings',
             resolved

lib/messages.js

@@ -2,11 +2,26 @@
 
 const chalk = require('chalk')
 const { error, info, warning } = require('log-symbols')
+const format = require('stringify-object')
+
+const configurationError = (opt, helpMsg, value) =>
+  `${chalk.redBright(`${error} Validation Error:`)}
+
+  Invalid value for '${chalk.bold(opt)}': ${chalk.bold(
+    format(value, { inlineCharacterLimit: Number.POSITIVE_INFINITY })
+  )}
+
+  ${helpMsg}`
 
 const NOT_GIT_REPO = chalk.redBright(`${error} Current directory is not a git directory!`)
 
 const FAILED_GET_STAGED_FILES = chalk.redBright(`${error} Failed to get staged files!`)
 
+const incorrectBraces = (before, after) => `${warning} ${chalk.yellow(
+  `Detected incorrect braces with only single value: \`${before}\`. Reformatted as: \`${after}\``
+)}
+`
+
 const NO_STAGED_FILES = `${info} No staged files found.`
 
 const NO_TASKS = `${info} No staged files match any configured task.`
@@ -29,7 +44,7 @@ const GIT_ERROR = `\n  ${error} ${chalk.red(`lint-staged failed due to a git err
 
 const invalidOption = (name, value, message) => `${chalk.redBright(`${error} Validation Error:`)}
 
-  Invalid value for option ${chalk.bold(name)}: ${chalk.bold(value)}
+  Invalid value for option '${chalk.bold(name)}': ${chalk.bold(value)}
 
   ${message}
 
@@ -51,9 +66,11 @@ const CONFIG_STDIN_ERROR = 'Error: Could not read config from stdin.'
 
 module.exports = {
   CONFIG_STDIN_ERROR,
+  configurationError,
   DEPRECATED_GIT_ADD,
   FAILED_GET_STAGED_FILES,
   GIT_ERROR,
+  incorrectBraces,
   invalidOption,
   NO_STAGED_FILES,
   NO_TASKS,

lib/validateBraces.js

@@ -0,0 +1,71 @@
+const { incorrectBraces } = require('./messages')
+
+/**
+ * A correctly-formed brace expansion must contain unquoted opening and closing braces,
+ * and at least one unquoted comma or a valid sequence expression.
+ * Any incorrectly formed brace expansion is left unchanged.
+ *
+ * @see https://www.gnu.org/software/bash/manual/html_node/Brace-Expansion.html
+ *
+ * Lint-staged uses `micromatch` for brace expansion, and its behavior is to treat
+ * invalid brace expansions as literal strings, which means they (typically) do not match
+ * anything.
+ *
+ * This RegExp tries to match most cases of invalid brace expansions, so that they can be
+ * detected, warned about, and re-formatted by removing the braces and thus hopefully
+ * matching the files as intended by the user. The only real fix is to remove the incorrect
+ * braces from user configuration, but this is left to the user (after seeing the warning).
+ *
+ * @example <caption>Globs with brace expansions</caption>
+ * - *.{js,tx}         // expanded as *.js, *.ts
+ * - *.{{j,t}s,css}    // expanded as *.js, *.ts, *.css
+ * - file_{1..10}.css  // expanded as file_1.css, file_2.css, …, file_10.css
+ *
+ * @example <caption>Globs with incorrect brace expansions</caption>
+ * - *.{js}       // should just be *.js
+ * - *.{js,{ts}}  // should just be *.{js,ts}
+ * - *.\{js\}     // escaped braces, so they're treated literally
+ * - *.${js}      // dollar-sign inhibits expansion, so treated literally
+ * - *.{js\,ts}   // the comma is escaped, so treated literally
+ */
+const BRACES_REGEXP = /(?<![\\$])({)(?:(?!(?<!\\),|\.\.|\{|\}).)*?(?<!\\)(})/g
+
+/**
+ * @param {string} pattern
+ * @returns {string}
+ */
+const withoutIncorrectBraces = (pattern) => {
+  let output = `${pattern}`
+  let match = null
+
+  while ((match = BRACES_REGEXP.exec(pattern))) {
+    const fullMatch = match[0]
+    const withoutBraces = fullMatch.replace(/{/, '').replace(/}/, '')
+    output = output.replace(fullMatch, withoutBraces)
+  }
+
+  return output
+}
+
+/**
+ * Validate and remove incorrect brace expansions from glob pattern.
+ * For example `*.{js}` is incorrect because it doesn't contain a `,` or `..`,
+ * and will be reformatted as `*.js`.
+ *
+ * @param {string} pattern the glob pattern
+ * @param {*} logger
+ * @returns {string}
+ */
+const validateBraces = (pattern, logger) => {
+  const fixedPattern = withoutIncorrectBraces(pattern)
+
+  if (fixedPattern !== pattern) {
+    logger.warn(incorrectBraces(pattern, fixedPattern))
+  }
+
+  return fixedPattern
+}
+
+module.exports = validateBraces
+
+module.exports.BRACES_REGEXP = BRACES_REGEXP