diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 36340642..9cd63760 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -25,3 +25,7 @@ repos: sha: v0.6.4 hooks: - id: add-trailing-comma +- repo: meta + hooks: + - id: check-useless-excludes + - id: check-files-matches-any diff --git a/pre_commit/clientlib.py b/pre_commit/clientlib.py index 11750b74..c94691a5 100644 --- a/pre_commit/clientlib.py +++ b/pre_commit/clientlib.py @@ -98,6 +98,8 @@ def validate_manifest_main(argv=None): _LOCAL_SENTINEL = 'local' +_META_SENTINEL = 'meta' + CONFIG_HOOK_DICT = schema.Map( 'Hook', 'id', @@ -121,7 +123,8 @@ CONFIG_REPO_DICT = schema.Map( schema.Conditional( 'sha', schema.check_string, - condition_key='repo', condition_value=schema.Not(_LOCAL_SENTINEL), + condition_key='repo', + condition_value=schema.NotIn(_LOCAL_SENTINEL, _META_SENTINEL), ensure_absent=True, ), ) @@ -138,6 +141,10 @@ def is_local_repo(repo_entry): return repo_entry['repo'] == _LOCAL_SENTINEL +def is_meta_repo(repo_entry): + return repo_entry['repo'] == _META_SENTINEL + + class InvalidConfigError(FatalError): pass diff --git a/pre_commit/commands/autoupdate.py b/pre_commit/commands/autoupdate.py index a80c6b40..ca0ed5e2 100644 --- a/pre_commit/commands/autoupdate.py +++ b/pre_commit/commands/autoupdate.py @@ -11,6 +11,7 @@ import pre_commit.constants as C from pre_commit import output from pre_commit.clientlib import CONFIG_SCHEMA from pre_commit.clientlib import is_local_repo +from pre_commit.clientlib import is_meta_repo from pre_commit.clientlib import load_config from pre_commit.commands.migrate_config import migrate_config from pre_commit.repository import Repository @@ -115,7 +116,7 @@ def autoupdate(runner, tags_only): input_config = load_config(runner.config_file_path) for repo_config in input_config['repos']: - if is_local_repo(repo_config): + if is_local_repo(repo_config) or is_meta_repo(repo_config): output_repos.append(repo_config) continue output.write('Updating {}...'.format(repo_config['repo'])) diff --git a/pre_commit/meta_hooks/__init__.py b/pre_commit/meta_hooks/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/pre_commit/meta_hooks/check_files_matches_any.py b/pre_commit/meta_hooks/check_files_matches_any.py new file mode 100644 index 00000000..8c9a92d8 --- /dev/null +++ b/pre_commit/meta_hooks/check_files_matches_any.py @@ -0,0 +1,40 @@ +import argparse + +import pre_commit.constants as C +from pre_commit import git +from pre_commit.commands.run import _filter_by_include_exclude +from pre_commit.commands.run import _filter_by_types +from pre_commit.runner import Runner + + +def check_all_hooks_match_files(config_file): + runner = Runner.create(config_file) + files = git.get_all_files() + retv = 0 + + for repo in runner.repositories: + for hook_id, hook in repo.hooks: + include, exclude = hook['files'], hook['exclude'] + filtered = _filter_by_include_exclude(files, include, exclude) + types, exclude_types = hook['types'], hook['exclude_types'] + filtered = _filter_by_types(filtered, types, exclude_types) + if not filtered: + print('{} does not apply to this repository'.format(hook_id)) + retv = 1 + + return retv + + +def main(argv=None): + parser = argparse.ArgumentParser() + parser.add_argument('filenames', nargs='*', default=[C.CONFIG_FILE]) + args = parser.parse_args(argv) + + retv = 0 + for filename in args.filenames: + retv |= check_all_hooks_match_files(filename) + return retv + + +if __name__ == '__main__': + exit(main()) diff --git a/pre_commit/meta_hooks/check_useless_excludes.py b/pre_commit/meta_hooks/check_useless_excludes.py new file mode 100644 index 00000000..189633a8 --- /dev/null +++ b/pre_commit/meta_hooks/check_useless_excludes.py @@ -0,0 +1,64 @@ +from __future__ import print_function + +import argparse +import re + +import pre_commit.constants as C +from pre_commit import git +from pre_commit.clientlib import load_config +from pre_commit.clientlib import MANIFEST_HOOK_DICT +from pre_commit.schema import apply_defaults + + +def exclude_matches_any(filenames, include, exclude): + if exclude == '^$': + return True + include_re, exclude_re = re.compile(include), re.compile(exclude) + for filename in filenames: + if include_re.search(filename) and exclude_re.search(filename): + return True + return False + + +def check_useless_excludes(config_file): + config = load_config(config_file) + files = git.get_all_files() + retv = 0 + + exclude = config['exclude'] + if not exclude_matches_any(files, '', exclude): + print( + 'The global exclude pattern {!r} does not match any files' + .format(exclude), + ) + retv = 1 + + for repo in config['repos']: + for hook in repo['hooks']: + # Not actually a manifest dict, but this more accurately reflects + # the defaults applied during runtime + hook = apply_defaults(hook, MANIFEST_HOOK_DICT) + include, exclude = hook['files'], hook['exclude'] + if not exclude_matches_any(files, include, exclude): + print( + 'The exclude pattern {!r} for {} does not match any files' + .format(exclude, hook['id']), + ) + retv = 1 + + return retv + + +def main(argv=None): + parser = argparse.ArgumentParser() + parser.add_argument('filenames', nargs='*', default=[C.CONFIG_FILE]) + args = parser.parse_args(argv) + + retv = 0 + for filename in args.filenames: + retv |= check_useless_excludes(filename) + return retv + + +if __name__ == '__main__': + exit(main()) diff --git a/pre_commit/repository.py b/pre_commit/repository.py index d7af2e21..2eb62ecb 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -4,7 +4,9 @@ import io import json import logging import os +import pipes import shutil +import sys from collections import defaultdict import pkg_resources @@ -14,6 +16,7 @@ import pre_commit.constants as C from pre_commit import five from pre_commit import git from pre_commit.clientlib import is_local_repo +from pre_commit.clientlib import is_meta_repo from pre_commit.clientlib import load_manifest from pre_commit.clientlib import MANIFEST_HOOK_DICT from pre_commit.languages.all import languages @@ -125,6 +128,12 @@ def _hook(*hook_dicts): return ret +def _hook_from_manifest_dct(dct): + dct = validate(apply_defaults(dct, MANIFEST_HOOK_DICT), MANIFEST_HOOK_DICT) + dct = _hook(dct) + return dct + + class Repository(object): def __init__(self, repo_config, store): self.repo_config = repo_config @@ -135,6 +144,8 @@ class Repository(object): def create(cls, config, store): if is_local_repo(config): return LocalRepository(config, store) + elif is_meta_repo(config): + return MetaRepository(config, store) else: return cls(config, store) @@ -221,14 +232,8 @@ class LocalRepository(Repository): @cached_property def hooks(self): - def _from_manifest_dct(dct): - dct = validate(dct, MANIFEST_HOOK_DICT) - dct = apply_defaults(dct, MANIFEST_HOOK_DICT) - dct = _hook(dct) - return dct - return tuple( - (hook['id'], _from_manifest_dct(hook)) + (hook['id'], _hook_from_manifest_dct(hook)) for hook in self.repo_config['hooks'] ) @@ -246,6 +251,60 @@ class LocalRepository(Repository): return tuple(ret) +class MetaRepository(LocalRepository): + @cached_property + def manifest_hooks(self): + # The hooks are imported here to prevent circular imports. + from pre_commit.meta_hooks import check_files_matches_any + from pre_commit.meta_hooks import check_useless_excludes + + def _make_entry(mod): + """the hook `entry` is passed through `shlex.split()` by the + command runner, so to prevent issues with spaces and backslashes + (on Windows) it must be quoted here. + """ + return '{} -m {}'.format(pipes.quote(sys.executable), mod.__name__) + + meta_hooks = [ + { + 'id': 'check-files-matches-any', + 'name': 'Check hooks match any files', + 'files': '.pre-commit-config.yaml', + 'language': 'system', + 'entry': _make_entry(check_files_matches_any), + }, + { + 'id': 'check-useless-excludes', + 'name': 'Check for useless excludes', + 'files': '.pre-commit-config.yaml', + 'language': 'system', + 'entry': _make_entry(check_useless_excludes), + }, + ] + + return { + hook['id']: _hook_from_manifest_dct(hook) + for hook in meta_hooks + } + + @cached_property + def hooks(self): + for hook in self.repo_config['hooks']: + if hook['id'] not in self.manifest_hooks: + logger.error( + '`{}` is not a valid meta hook. ' + 'Typo? Perhaps it is introduced in a newer version? ' + 'Often `pip install --upgrade pre-commit` fixes this.' + .format(hook['id']), + ) + exit(1) + + return tuple( + (hook['id'], _hook(self.manifest_hooks[hook['id']], hook)) + for hook in self.repo_config['hooks'] + ) + + class _UniqueList(list): def __init__(self): self._set = set() diff --git a/pre_commit/schema.py b/pre_commit/schema.py index e20f74cc..89e1bcfc 100644 --- a/pre_commit/schema.py +++ b/pre_commit/schema.py @@ -101,6 +101,9 @@ def _check_conditional(self, dct): if isinstance(self.condition_value, Not): op = 'is' cond_val = self.condition_value.val + elif isinstance(self.condition_value, NotIn): + op = 'is any of' + cond_val = self.condition_value.values else: op = 'is not' cond_val = self.condition_value @@ -198,14 +201,19 @@ class Array(collections.namedtuple('Array', ('of',))): return [remove_defaults(val, self.of) for val in v] -class Not(object): - def __init__(self, val): - self.val = val - +class Not(collections.namedtuple('Not', ('val',))): def __eq__(self, other): return other is not MISSING and other != self.val +class NotIn(collections.namedtuple('NotIn', ('values',))): + def __new__(cls, *values): + return super(NotIn, cls).__new__(cls, values=values) + + def __eq__(self, other): + return other is not MISSING and other not in self.values + + def check_any(_): pass diff --git a/tests/commands/autoupdate_test.py b/tests/commands/autoupdate_test.py index 9ae70c64..7119c6be 100644 --- a/tests/commands/autoupdate_test.py +++ b/tests/commands/autoupdate_test.py @@ -295,6 +295,24 @@ def test_autoupdate_local_hooks_with_out_of_date_repo( assert new_config_writen['repos'][0] == local_config +def test_autoupdate_meta_hooks(tmpdir, capsys): + cfg = tmpdir.join(C.CONFIG_FILE) + cfg.write( + 'repos:\n' + '- repo: meta\n' + ' hooks:\n' + ' - id: check-useless-excludes\n', + ) + ret = autoupdate(Runner(tmpdir.strpath, C.CONFIG_FILE), tags_only=True) + assert ret == 0 + assert cfg.read() == ( + 'repos:\n' + '- repo: meta\n' + ' hooks:\n' + ' - id: check-useless-excludes\n' + ) + + def test_updates_old_format_to_new_format(tmpdir, capsys): cfg = tmpdir.join(C.CONFIG_FILE) cfg.write( diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index d6812ae5..336222d6 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -645,6 +645,31 @@ def test_local_hook_fails( ) +def test_meta_hook_passes( + cap_out, repo_with_passing_hook, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + )), + ), + ), + )) + add_config_to_repo(repo_with_passing_hook, config) + + _test_run( + cap_out, + repo_with_passing_hook, + opts={}, + expected_outputs=[b'Check for useless excludes'], + expected_ret=0, + stage=False, + ) + + @pytest.yield_fixture def modified_config_repo(repo_with_passing_hook): with modify_config(repo_with_passing_hook, commit=False) as config: diff --git a/tests/meta_hooks/__init__.py b/tests/meta_hooks/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/meta_hooks/hook_matches_any_test.py b/tests/meta_hooks/hook_matches_any_test.py new file mode 100644 index 00000000..005cdf68 --- /dev/null +++ b/tests/meta_hooks/hook_matches_any_test.py @@ -0,0 +1,130 @@ +from collections import OrderedDict + +from pre_commit.meta_hooks import check_files_matches_any +from pre_commit.util import cwd +from testing.fixtures import add_config_to_repo +from testing.fixtures import git_dir + + +def test_hook_excludes_everything( + capsys, tempdir_factory, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('exclude', '.pre-commit-config.yaml'), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_files_matches_any.main(()) == 1 + + out, _ = capsys.readouterr() + assert 'check-useless-excludes does not apply to this repository' in out + + +def test_hook_includes_nothing( + capsys, tempdir_factory, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('files', 'foo'), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_files_matches_any.main(()) == 1 + + out, _ = capsys.readouterr() + assert 'check-useless-excludes does not apply to this repository' in out + + +def test_hook_types_not_matched( + capsys, tempdir_factory, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('types', ['python']), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_files_matches_any.main(()) == 1 + + out, _ = capsys.readouterr() + assert 'check-useless-excludes does not apply to this repository' in out + + +def test_hook_types_excludes_everything( + capsys, tempdir_factory, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('exclude_types', ['yaml']), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_files_matches_any.main(()) == 1 + + out, _ = capsys.readouterr() + assert 'check-useless-excludes does not apply to this repository' in out + + +def test_valid_includes( + capsys, tempdir_factory, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_files_matches_any.main(()) == 0 + + out, _ = capsys.readouterr() + assert out == '' diff --git a/tests/meta_hooks/useless_excludes_test.py b/tests/meta_hooks/useless_excludes_test.py new file mode 100644 index 00000000..08b87aa8 --- /dev/null +++ b/tests/meta_hooks/useless_excludes_test.py @@ -0,0 +1,107 @@ +from collections import OrderedDict + +from pre_commit.meta_hooks import check_useless_excludes +from pre_commit.util import cwd +from testing.fixtures import add_config_to_repo +from testing.fixtures import git_dir + + +def test_useless_exclude_global(capsys, tempdir_factory): + config = OrderedDict(( + ('exclude', 'foo'), + ( + 'repos', [ + OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + )), + ), + ), + )), + ], + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_useless_excludes.main(()) == 1 + + out, _ = capsys.readouterr() + assert "The global exclude pattern 'foo' does not match any files" in out + + +def test_useless_exclude_for_hook(capsys, tempdir_factory): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('exclude', 'foo'), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_useless_excludes.main(()) == 1 + + out, _ = capsys.readouterr() + expected = ( + "The exclude pattern 'foo' for check-useless-excludes " + "does not match any files" + ) + assert expected in out + + +def test_no_excludes(capsys, tempdir_factory): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_useless_excludes.main(()) == 0 + + out, _ = capsys.readouterr() + assert out == '' + + +def test_valid_exclude(capsys, tempdir_factory): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('exclude', '.pre-commit-config.yaml'), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_useless_excludes.main(()) == 0 + + out, _ = capsys.readouterr() + assert out == '' diff --git a/tests/repository_test.py b/tests/repository_test.py index fee76d87..f7c027cd 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -721,6 +721,18 @@ def test_hook_id_not_present(tempdir_factory, store, fake_log_handler): ) +def test_meta_hook_not_present(store, fake_log_handler): + config = {'repo': 'meta', 'hooks': [{'id': 'i-dont-exist'}]} + repo = Repository.create(config, store) + with pytest.raises(SystemExit): + repo.require_installed() + assert fake_log_handler.handle.call_args[0][0].msg == ( + '`i-dont-exist` is not a valid meta hook. ' + 'Typo? Perhaps it is introduced in a newer version? ' + 'Often `pip install --upgrade pre-commit` fixes this.' + ) + + def test_too_new_version(tempdir_factory, store, fake_log_handler): path = make_repo(tempdir_factory, 'script_hooks_repo') with modify_manifest(path) as manifest: diff --git a/tests/schema_test.py b/tests/schema_test.py index c2ecf0fa..565f7e17 100644 --- a/tests/schema_test.py +++ b/tests/schema_test.py @@ -19,6 +19,7 @@ from pre_commit.schema import load_from_filename from pre_commit.schema import Map from pre_commit.schema import MISSING from pre_commit.schema import Not +from pre_commit.schema import NotIn from pre_commit.schema import Optional from pre_commit.schema import OptionalNoDefault from pre_commit.schema import remove_defaults @@ -107,6 +108,16 @@ def test_not(val, expected): assert (compared == val) is expected +@pytest.mark.parametrize( + ('values', 'expected'), + (('bar', True), ('foo', False), (MISSING, False)), +) +def test_not_in(values, expected): + compared = NotIn('baz', 'foo') + assert (values == compared) is expected + assert (compared == values) is expected + + trivial_array_schema = Array(Map('foo', 'id')) @@ -196,6 +207,13 @@ map_conditional_absent_not = Map( condition_key='key', condition_value=Not(True), ensure_absent=True, ), ) +map_conditional_absent_not_in = Map( + 'foo', 'key', + Conditional( + 'key2', check_bool, + condition_key='key', condition_value=NotIn(1, 2), ensure_absent=True, + ), +) @pytest.mark.parametrize('schema', (map_conditional, map_conditional_not)) @@ -248,6 +266,19 @@ def test_ensure_absent_conditional_not(): ) +def test_ensure_absent_conditional_not_in(): + with pytest.raises(ValidationError) as excinfo: + validate({'key': 1, 'key2': True}, map_conditional_absent_not_in) + _assert_exception_trace( + excinfo.value, + ( + 'At foo(key=1)', + 'Expected key2 to be absent when key is any of (1, 2), ' + 'found key2: True', + ), + ) + + def test_no_error_conditional_absent(): validate({}, map_conditional_absent) validate({}, map_conditional_absent_not)