fix: do not always lint matches on negative gitignore patterns

* fixes fallout of #195 * builds on #201, #202 * removes parse-gitignore in favor of node-ignore
xojs · Apr 2, 2017 · eb3500f · eb3500f
1 parent f5b5838
commit eb3500f
Show file tree

Hide file tree

Showing 8 changed files with 119 additions and 112 deletions.
diff --git a/index.js b/index.js
@@ -29,10 +29,10 @@ exports.lintText = (str, opts) => {
 
 	if (opts.filename) {
 		const filename = path.relative(opts.cwd, opts.filename);
-		const gitIgnores = optionsManager.getGitIgnores(opts);
-		const glob = [filename].concat(gitIgnores);
+		const isIgnored = multimatch(filename, opts.ignores).length > 0;
+		const isGitIgnored = !optionsManager.getGitIgnoreFilter(opts)(opts.filename);
 
-		if (multimatch(glob, opts.ignores).length > 0) {
+		if (isIgnored || isGitIgnored) {
 			return {
 				errorCount: 0,
 				warningCount: 0,
@@ -56,17 +56,18 @@ exports.lintFiles = (patterns, opts) => {
 	opts = optionsManager.preprocess(opts);
 	patterns = patterns.length === 0 ? ['**/*'] : arrify(patterns);
 
-	const gitIgnores = optionsManager.getGitIgnores(opts);
-	const glob = patterns.concat(gitIgnores);
+	const ignoreFilter = optionsManager.getGitIgnoreFilter(opts);
 
-	return globby(glob, {ignore: opts.ignores, nodir: true}).then(paths => {
+	return globby(patterns, {ignore: opts.ignores, nodir: true}).then(paths => {
 		// Filter out unwanted file extensions
 		// for silly users that don't specify an extension in the glob pattern
-		paths = paths.filter(x => {
-			// Remove dot before the actual extension
-			const ext = path.extname(x).replace('.', '');
-			return opts.extensions.indexOf(ext) !== -1;
-		});
+		paths = paths
+			.filter(ignoreFilter)
+			.filter(x => {
+				// Remove dot before the actual extension
+				const ext = path.extname(x).replace('.', '');
+				return opts.extensions.indexOf(ext) !== -1;
+			})
 
 		if (!(opts.overrides && opts.overrides.length > 0)) {
 			return runEslint(paths, opts);

diff --git a/options-manager.js b/options-manager.js
@@ -1,14 +1,16 @@
 'use strict';
+const fs = require('fs');
 const os = require('os');
 const path = require('path');
+
 const arrify = require('arrify');
-const pkgConf = require('pkg-conf');
 const deepAssign = require('deep-assign');
+const globby = require('globby');
+const ignore = require('ignore');
 const multimatch = require('multimatch');
-const resolveFrom = require('resolve-from');
 const pathExists = require('path-exists');
-const parseGitignore = require('parse-gitignore');
-const globby = require('globby');
+const pkgConf = require('pkg-conf');
+const resolveFrom = require('resolve-from');
 const slash = require('slash');
 
 const DEFAULT_IGNORE = [
@@ -226,29 +228,34 @@ const getIgnores = opts => {
 	return opts;
 }
 
-const getGitIgnores = opts => {
+const getGitIgnoreFilter = opts => {
 	const ignores = opts.ignores || [];
 	const cwd = opts.cwd || process.cwd();
+	const i = ignore();
+
+	globby.sync('**/.gitignore', {
+		ignore: ignores,
+		cwd: cwd
+	})
+	.map(file => {
+		const fileName = path.join(cwd, file);
+		const base = slash(path.relative(cwd, path.dirname(fileName)));
+
+		const lines = fs.readFileSync(fileName)
+			.toString()
+			.split(/\r\n|\n/)
+			.filter(Boolean)
+			.filter(l => l.charAt(0) !== '#')
+			.map(l => {
+				const negated = l.charAt(0) === '!';
+				const pattern = path.join(base, negated ? l.slice(1) : l);
+				return negated ? '!' + pattern : pattern;
+			});
+
+		i.add(lines)
+	});
 
-	return globby
-		.sync('**/.gitignore', {
-			ignore: ignores,
-			cwd: cwd
-		})
-		.map(filename => {
-			const fullFilename = path.join(cwd, filename);
-			const patterns = parseGitignore(fullFilename);
-			const base = slash(path.relative(cwd, path.dirname(fullFilename)));
-
-			return patterns
-				.map(pattern => {
-					const negate = !pattern.startsWith('!');
-					const patternPath = negate ? pattern : pattern.substr(1);
-					return {negate, pattern: path.posix.join(base, patternPath)};
-				})
-				.map(item => item.negate ? `!${item.pattern}` : item.pattern);
-		})
-		.reduce((a, b) => a.concat(b), []);
+	return p => !i.ignores(path.relative(opts.cwd, p));
 }
 
 const preprocess = opts => {
@@ -271,4 +278,4 @@ exports.groupConfigs = groupConfigs;
 exports.preprocess = preprocess;
 exports.emptyOptions = emptyOptions;
 exports.getIgnores = getIgnores;
-exports.getGitIgnores = getGitIgnores;
+exports.getGitIgnoreFilter = getGitIgnoreFilter;
diff --git a/package.json b/package.json
@@ -80,10 +80,10 @@
     "get-stdin": "^5.0.0",
     "globby": "^6.0.0",
     "has-flag": "^2.0.0",
+    "ignore": "^3.2.6",
     "lodash.isequal": "^4.4.0",
     "meow": "^3.4.2",
     "multimatch": "^2.1.0",
-    "parse-gitignore": "^0.3.1",
     "path-exists": "^3.0.0",
     "pkg-conf": "^2.0.0",
     "resolve-cwd": "^1.0.0",

diff --git a/test/api.js b/test/api.js
@@ -137,6 +137,40 @@ test('lintText() - overrides support', async t => {
 	t.is(indexResults[0].errorCount, 0, indexResults[0]);
 });
 
+test('.lintText() - do not lint gitignored files if filename is given', async t => {
+	const cwd = path.join(__dirname, 'fixtures/gitignore');
+	const ignoredPath = path.resolve('fixtures/gitignore/test/foo.js');
+	const ignored = await readFile(ignoredPath, 'utf-8');
+	const {results} = fn.lintText(ignored, {filename: ignoredPath, cwd});
+	t.is(results[0].errorCount, 0);
+});
+
+test('.lintText() - lint gitignored files if filename is not given', async t => {
+	const cwd = path.join(__dirname, 'fixtures/gitignore');
+	const ignoredPath = path.resolve('fixtures/gitignore/test/foo.js');
+	const ignored = await readFile(ignoredPath, 'utf-8');
+	const {results} = fn.lintText(ignored);
+	t.true(results[0].errorCount > 0);
+});
+
+test('.lintText() - do not lint gitignored files in file with negative gitignores', async t => {
+	const cwd = path.join(__dirname, 'fixtures/negative-gitignore');
+	const ignoredPath = path.resolve('fixtures/negative-gitignore/bar.js');
+	const ignored = await readFile(ignoredPath, 'utf-8');
+	const {results} = fn.lintText(ignored, {filename: ignoredPath, cwd});
+	t.is(results[0].errorCount, 0);
+});
+
+test('.lintText() - lint negatively gitignored files', async t => {
+	const cwd = path.join(__dirname, 'fixtures/negative-gitignore');
+	const glob = path.posix.join(cwd, '*');
+	const negativePath = path.resolve('fixtures/negative-gitignore/bar.js');
+	const negative = await readFile(negativePath, 'utf-8');
+	const {results} = await fn.lintFiles(glob, {cwd});
+
+	t.true(results[0].errorCount > 0);
+});
+
 test('.lintFiles() - only accepts whitelisted extensions', async t => {
 	// Markdown files will always produce linter errors and will not be going away
 	const mdGlob = path.join(__dirname, '..', '*.md');
@@ -175,3 +209,39 @@ test('.lintFiles() - ignores dirs for empty extensions', async t => {
 		t.is(results.errorCount, 1);
 	}
 });
+
+test('.lintFiles() - do not lint gitignored files', async t => {
+	const cwd = path.join(__dirname, 'fixtures/gitignore');
+	const glob = path.posix.join(cwd, '**/*');
+	const ignored = path.resolve('fixtures/gitignore/test/foo.js');
+	const {results} = await fn.lintFiles(glob, {cwd});
+
+	t.is(results.some(r => r.filePath === ignored), false);
+});
+
+test('.lintFiles() - do not lint gitignored files in file with negative gitignores', async t => {
+	const cwd = path.join(__dirname, 'fixtures/negative-gitignore');
+	const glob = path.posix.join(cwd, '*');
+	const ignored = path.resolve('fixtures/negative-gitignore/bar.js');
+	const {results} = await fn.lintFiles(glob, {cwd});
+
+	t.is(results.some(r => r.filePath === ignored), false);
+});
+
+test('.lintFiles() - lint negatively gitignored files', async t => {
+	const cwd = path.join(__dirname, 'fixtures/negative-gitignore');
+	const glob = path.posix.join(cwd, '*');
+	const negative = path.resolve('fixtures/negative-gitignore/foo.js');
+	const {results} = await fn.lintFiles(glob, {cwd});
+
+	t.is(results.some(r => r.filePath === negative), true);
+});
+
+test('.lintFiles() - do not lint inapplicable negatively gitignored files', async t => {
+	const cwd = path.join(__dirname, 'fixtures/negative-gitignore');
+	const glob = path.posix.join(cwd, 'bar.js');
+	const negative = path.resolve('fixtures/negative-gitignore/foo.js');
+	const {results} = await fn.lintFiles(glob, {cwd});
+
+	t.is(results.some(r => r.filePath === negative), false);
+});
diff --git a/test/cli.js b/test/cli.js
@@ -56,11 +56,9 @@ test('ignore files in .gitignore', async t => {
 	t.true(err.stdout.indexOf('bar.js') !== -1);
 });
 
-test.failing('negative gitignores', async t => {
+test('negative gitignores', async t => {
 	const cwd = path.join(__dirname, 'fixtures/negative-gitignore');
-
-	const err = await t.throws(execa(`../../../cli.js`, [`${cwd}/bar.js`, '--no-local'], {cwd}));
-	t.is(err.stdout.indexOf('foo.js'), -1, 'Should not lint foo.js');
+	await execa(`../../../cli.js`, [`${cwd}/bar.js`, '--no-local'], {cwd});
 });
 
 test('supports being extended with a shareable config', async () => {

diff --git a/test/fixtures/gitignore/test/.gitignore b/test/fixtures/gitignore/test/.gitignore
@@ -1 +1,2 @@
 foo.js
+!bar.js
diff --git a/test/fixtures/negative-gitignore/.gitignore b/test/fixtures/negative-gitignore/.gitignore
@@ -1 +1,2 @@
+*.js
 !foo.js
diff --git a/test/options-manager.js b/test/options-manager.js
@@ -171,74 +171,3 @@ test('groupConfigs', t => {
 		return obj;
 	}));
 });
-
-test('gitignore', t => {
-	const ignores = manager.getGitIgnores({});
-	t.not(ignores.indexOf('!foo/**'), -1);
-	t.not(ignores.indexOf('!bar/foo.js'), -1);
-	t.not(ignores.indexOf('bar/bar.js'), -1);
-});
-
-test('ignore ignored .gitignore', t => {
-	const opts = {
-		ignores: [
-			'**/foobar/**'
-		]
-	};
-
-	const ignores = manager.getGitIgnores(opts);
-	t.is(ignores.indexOf('!bar/foobar/bar.js'), -1);
-});
-
-test('positive patterns should be translated to negative patterns', t => {
-	const cwd = path.join(__dirname, 'fixtures/gitignore/test');
-	const result = manager.getGitIgnores({cwd});
-
-	t.deepEqual(result, ['!foo.js', '!foo.js/**']);
-});
-
-test('patterns should be translated according to process.cwd()', t => {
-	const previous = process.cwd();
-	const cwd = path.join(__dirname, 'fixtures/gitignore/test');
-	process.chdir(cwd);
-	const result = manager.getGitIgnores({});
-	const expected = ['!foo.js', '!foo.js/**'];
-
-	t.deepEqual(result, expected);
-	process.chdir(previous);
-});
-
-test('patterns should be translated according to cwd', t => {
-	const cwd = path.join(__dirname, 'fixtures/gitignore');
-	const result = manager.getGitIgnores({cwd});
-	const expected = ['!test/foo.js', '!test/foo.js/**'];
-
-	t.deepEqual(result, expected);
-});
-
-test('mergeWithPkgConf: use child if closest', t => {
-	const cwd = path.resolve('fixtures', 'nested', 'child');
-	const result = manager.mergeWithPkgConf({cwd});
-	const expected = Object.assign({}, childConfig.xo, {cwd});
-	t.deepEqual(result, expected);
-});
-
-test('mergeWithPkgConf: use parent if closest', t => {
-	const cwd = path.resolve('fixtures', 'nested');
-	const result = manager.mergeWithPkgConf({cwd});
-	const expected = Object.assign({}, parentConfig.xo, {cwd});
-	t.deepEqual(result, expected);
-});
-
-test('mergeWithPkgConf: use parent if child is ignored', t => {
-	const cwd = path.resolve('fixtures', 'nested', 'child-ignore');
-	const result = manager.mergeWithPkgConf({cwd});
-	const expected = Object.assign({}, parentConfig.xo, {cwd});
-	t.deepEqual(result, expected);
-});
-
-test('mergeWithPkgConf: use child if child is empty', t => {
-	const cwd = path.resolve('fixtures', 'nested', 'child-empty');
-	const result = manager.mergeWithPkgConf({cwd});
-	t.deepEqual(result, {cwd});
-});