From 6af60158ec40bd15fabdd91f55a571ef3845645e Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Fri, 8 Sep 2017 17:48:48 -0700 Subject: [PATCH] Refactor filename collection for hooks --- pre_commit/commands/run.py | 62 ++++++++++++++++++++++---------------- pre_commit/git.py | 29 ------------------ tests/commands/run_test.py | 44 +++++++++++++++++++++++++++ tests/git_test.py | 51 ------------------------------- 4 files changed, 80 insertions(+), 106 deletions(-) diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py index 505bb54d..e260b662 100644 --- a/pre_commit/commands/run.py +++ b/pre_commit/commands/run.py @@ -3,6 +3,7 @@ from __future__ import unicode_literals import logging import os +import re import subprocess import sys @@ -36,7 +37,19 @@ def _hook_msg_start(hook, verbose): ) -def filter_filenames_by_types(filenames, types, exclude_types): +def _filter_by_include_exclude(filenames, include, exclude): + include_re, exclude_re = re.compile(include), re.compile(exclude) + return { + filename for filename in filenames + if ( + include_re.search(filename) and + not exclude_re.search(filename) and + os.path.lexists(filename) + ) + } + + +def _filter_by_types(filenames, types, exclude_types): types, exclude_types = frozenset(types), frozenset(exclude_types) ret = [] for filename in filenames: @@ -46,34 +59,15 @@ def filter_filenames_by_types(filenames, types, exclude_types): return tuple(ret) -def get_filenames(args, include_expr, exclude_expr): - if args.origin and args.source: - getter = git.get_files_matching( - lambda: git.get_changed_files(args.origin, args.source), - ) - elif args.hook_stage == 'commit-msg': - def getter(*_): - return (args.commit_msg_filename,) - elif args.files: - getter = git.get_files_matching(lambda: args.files) - elif args.all_files: - getter = git.get_all_files_matching - elif git.is_in_merge_conflict(): - getter = git.get_conflicted_files_matching - else: - getter = git.get_staged_files_matching - return getter(include_expr, exclude_expr) - - SKIPPED = 'Skipped' NO_FILES = '(no files to check)' -def _run_single_hook(hook, repo, args, skips, cols): - filenames = get_filenames(args, hook['files'], hook['exclude']) - filenames = filter_filenames_by_types( - filenames, hook['types'], hook['exclude_types'], - ) +def _run_single_hook(filenames, hook, repo, args, skips, cols): + include, exclude = hook['files'], hook['exclude'] + filenames = _filter_by_include_exclude(filenames, include, exclude) + types, exclude_types = hook['types'], hook['exclude_types'] + filenames = _filter_by_types(filenames, types, exclude_types) if hook['id'] in skips: output.write(get_hook_message( _hook_msg_start(hook, args.verbose), @@ -169,13 +163,29 @@ def _compute_cols(hooks, verbose): return max(cols, 80) +def _all_filenames(args): + if args.origin and args.source: + return git.get_changed_files(args.origin, args.source) + elif args.hook_stage == 'commit-msg': + return (args.commit_msg_filename,) + elif args.files: + return args.files + elif args.all_files: + return git.get_all_files() + elif git.is_in_merge_conflict(): + return git.get_conflicted_files() + else: + return git.get_staged_files() + + def _run_hooks(config, repo_hooks, args, environ): """Actually run the hooks.""" skips = _get_skips(environ) cols = _compute_cols([hook for _, hook in repo_hooks], args.verbose) + filenames = _all_filenames(args) retval = 0 for repo, hook in repo_hooks: - retval |= _run_single_hook(hook, repo, args, skips, cols) + retval |= _run_single_hook(filenames, hook, repo, args, skips, cols) if retval and config['fail_fast']: break if ( diff --git a/pre_commit/git.py b/pre_commit/git.py index cdf807b5..1c3191e3 100644 --- a/pre_commit/git.py +++ b/pre_commit/git.py @@ -1,15 +1,12 @@ from __future__ import unicode_literals -import functools import logging import os.path -import re import sys from pre_commit.errors import FatalError from pre_commit.util import CalledProcessError from pre_commit.util import cmd_output -from pre_commit.util import memoize_by_cwd logger = logging.getLogger('pre_commit') @@ -63,7 +60,6 @@ def parse_merge_msg_for_conflicts(merge_msg): ] -@memoize_by_cwd def get_conflicted_files(): logger.info('Checking merge-conflict files only.') # Need to get the conflicted files from the MERGE_MSG because they could @@ -82,7 +78,6 @@ def get_conflicted_files(): return set(merge_conflict_filenames) | set(merge_diff_filenames) -@memoize_by_cwd def get_staged_files(): return zsplit(cmd_output( 'git', 'diff', '--staged', '--name-only', '--no-ext-diff', '-z', @@ -91,7 +86,6 @@ def get_staged_files(): )[1]) -@memoize_by_cwd def get_all_files(): return zsplit(cmd_output('git', 'ls-files', '-z')[1]) @@ -103,29 +97,6 @@ def get_changed_files(new, old): )[1]) -def get_files_matching(all_file_list_strategy): - @functools.wraps(all_file_list_strategy) - @memoize_by_cwd - def wrapper(include_expr, exclude_expr): - include_regex = re.compile(include_expr) - exclude_regex = re.compile(exclude_expr) - return { - filename - for filename in all_file_list_strategy() - if ( - include_regex.search(filename) and - not exclude_regex.search(filename) and - os.path.lexists(filename) - ) - } - return wrapper - - -get_staged_files_matching = get_files_matching(get_staged_files) -get_all_files_matching = get_files_matching(get_all_files) -get_conflicted_files_matching = get_files_matching(get_conflicted_files) - - def check_for_cygwin_mismatch(): """See https://github.com/pre-commit/pre-commit/issues/354""" if sys.platform in ('cygwin', 'win32'): # pragma: no cover (windows) diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index 53e098b0..46d2a7e1 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -12,6 +12,7 @@ import pytest import pre_commit.constants as C from pre_commit.commands.install_uninstall import install from pre_commit.commands.run import _compute_cols +from pre_commit.commands.run import _filter_by_include_exclude from pre_commit.commands.run import _get_skips from pre_commit.commands.run import _has_unmerged_paths from pre_commit.commands.run import run @@ -25,6 +26,7 @@ from testing.fixtures import make_consuming_repo from testing.fixtures import modify_config from testing.fixtures import read_config from testing.util import cmd_output_mocked_pre_commit_home +from testing.util import xfailif_no_symlink @pytest.yield_fixture @@ -744,3 +746,45 @@ def test_fail_fast( ret, printed = _do_run(cap_out, repo_with_failing_hook, _get_opts()) # it should have only run one hook assert printed.count(b'Failing hook') == 1 + + +@pytest.fixture +def some_filenames(): + return ( + '.pre-commit-hooks.yaml', + 'pre_commit/main.py', + 'pre_commit/git.py', + 'im_a_file_that_doesnt_exist.py', + ) + + +def test_include_exclude_base_case(some_filenames): + ret = _filter_by_include_exclude(some_filenames, '', '^$') + assert ret == { + '.pre-commit-hooks.yaml', + 'pre_commit/main.py', + 'pre_commit/git.py', + } + + +@xfailif_no_symlink +def test_matches_broken_symlink(tmpdir): # pramga: no cover (non-windows) + with tmpdir.as_cwd(): + os.symlink('does-not-exist', 'link') + ret = _filter_by_include_exclude({'link'}, '', '^$') + assert ret == {'link'} + + +def test_include_exclude_total_match(some_filenames): + ret = _filter_by_include_exclude(some_filenames, r'^.*\.py$', '^$') + assert ret == {'pre_commit/main.py', 'pre_commit/git.py'} + + +def test_include_exclude_does_search_instead_of_match(some_filenames): + ret = _filter_by_include_exclude(some_filenames, r'\.yaml$', '^$') + assert ret == {'.pre-commit-hooks.yaml'} + + +def test_include_exclude_exclude_removes_files(some_filenames): + ret = _filter_by_include_exclude(some_filenames, '', r'\.py$') + assert ret == {'.pre-commit-hooks.yaml'} diff --git a/tests/git_test.py b/tests/git_test.py index 4fce5ab0..8417523f 100644 --- a/tests/git_test.py +++ b/tests/git_test.py @@ -11,7 +11,6 @@ from pre_commit.errors import FatalError from pre_commit.util import cmd_output from pre_commit.util import cwd from testing.fixtures import git_dir -from testing.util import xfailif_no_symlink def test_get_root_at_root(tempdir_factory): @@ -66,56 +65,6 @@ def test_cherry_pick_conflict(in_merge_conflict): assert git.is_in_merge_conflict() is False -@pytest.fixture -def get_files_matching_func(): - def get_filenames(): - return ( - '.pre-commit-hooks.yaml', - 'pre_commit/main.py', - 'pre_commit/git.py', - 'im_a_file_that_doesnt_exist.py', - ) - - return git.get_files_matching(get_filenames) - - -def test_get_files_matching_base(get_files_matching_func): - ret = get_files_matching_func('', '^$') - assert ret == { - '.pre-commit-hooks.yaml', - 'pre_commit/main.py', - 'pre_commit/git.py', - } - - -@xfailif_no_symlink -def test_matches_broken_symlink(tmpdir): # pragma: no cover (non-windwos) - with tmpdir.as_cwd(): - os.symlink('does-not-exist', 'link') - func = git.get_files_matching(lambda: ('link',)) - assert func('', '^$') == {'link'} - - -def test_get_files_matching_total_match(get_files_matching_func): - ret = get_files_matching_func('^.*\\.py$', '^$') - assert ret == {'pre_commit/main.py', 'pre_commit/git.py'} - - -def test_does_search_instead_of_match(get_files_matching_func): - ret = get_files_matching_func('\\.yaml$', '^$') - assert ret == {'.pre-commit-hooks.yaml'} - - -def test_does_not_include_deleted_fileS(get_files_matching_func): - ret = get_files_matching_func('exist.py', '^$') - assert ret == set() - - -def test_exclude_removes_files(get_files_matching_func): - ret = get_files_matching_func('', '\\.py$') - assert ret == {'.pre-commit-hooks.yaml'} - - def resolve_conflict(): with open('conflict_file', 'w') as conflicted_file: conflicted_file.write('herp\nderp\n')