diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py index 8b80bef0..99232585 100644 --- a/pre_commit/commands/run.py +++ b/pre_commit/commands/run.py @@ -192,17 +192,14 @@ def get_repo_hooks(runner): yield (repo, hook) -def _has_unmerged_paths(runner): - _, stdout, _ = runner.cmd_runner.run(['git', 'ls-files', '--unmerged']) +def _has_unmerged_paths(): + _, stdout, _ = cmd_output('git', 'ls-files', '--unmerged') return bool(stdout.strip()) def _has_unstaged_config(runner): - retcode, _, _ = runner.cmd_runner.run( - ( - 'git', 'diff', '--no-ext-diff', '--exit-code', - runner.config_file_path, - ), + retcode, _, _ = cmd_output( + 'git', 'diff', '--no-ext-diff', '--exit-code', runner.config_file_path, retcode=None, ) # be explicit, other git errors don't mean it has an unstaged config. @@ -213,7 +210,7 @@ def run(runner, args, environ=os.environ): no_stash = args.all_files or bool(args.files) # Check if we have unresolved merge conflict files and fail fast. - if _has_unmerged_paths(runner): + if _has_unmerged_paths(): logger.error('Unmerged files. Resolve before committing.') return 1 if bool(args.source) != bool(args.origin): @@ -234,7 +231,7 @@ def run(runner, args, environ=os.environ): if no_stash: ctx = noop_context() else: - ctx = staged_files_only(runner.cmd_runner) + ctx = staged_files_only(runner.store.directory) with ctx: repo_hooks = list(get_repo_hooks(runner)) diff --git a/pre_commit/runner.py b/pre_commit/runner.py index c7455d71..21707cb4 100644 --- a/pre_commit/runner.py +++ b/pre_commit/runner.py @@ -57,11 +57,6 @@ class Runner(object): def pre_push_path(self): return self.get_hook_path('pre-push') - @cached_property - def cmd_runner(self): - # TODO: remove this and inline runner.store.cmd_runner - return self.store.cmd_runner - @cached_property def store(self): return Store() diff --git a/pre_commit/staged_files_only.py b/pre_commit/staged_files_only.py index 4d233924..cfd63815 100644 --- a/pre_commit/staged_files_only.py +++ b/pre_commit/staged_files_only.py @@ -3,44 +3,41 @@ from __future__ import unicode_literals import contextlib import io import logging +import os.path import time from pre_commit.util import CalledProcessError +from pre_commit.util import cmd_output logger = logging.getLogger('pre_commit') -def _git_apply(cmd_runner, patch): +def _git_apply(patch): args = ('apply', '--whitespace=nowarn', patch) try: - cmd_runner.run(('git',) + args, encoding=None) + cmd_output('git', *args, encoding=None) except CalledProcessError: # Retry with autocrlf=false -- see #570 - cmd = ('git', '-c', 'core.autocrlf=false') + args - cmd_runner.run(cmd, encoding=None) + cmd_output('git', '-c', 'core.autocrlf=false', *args, encoding=None) @contextlib.contextmanager -def staged_files_only(cmd_runner): +def staged_files_only(patch_dir): """Clear any unstaged changes from the git working directory inside this context. - - Args: - cmd_runner - PrefixedCommandRunner """ # Determine if there are unstaged files - tree = cmd_runner.run(('git', 'write-tree'))[1].strip() - retcode, diff_stdout_binary, _ = cmd_runner.run( - ( - 'git', 'diff-index', '--ignore-submodules', '--binary', - '--exit-code', '--no-color', '--no-ext-diff', tree, '--', - ), + tree = cmd_output('git', 'write-tree')[1].strip() + retcode, diff_stdout_binary, _ = cmd_output( + 'git', 'diff-index', '--ignore-submodules', '--binary', + '--exit-code', '--no-color', '--no-ext-diff', tree, '--', retcode=None, encoding=None, ) if retcode and diff_stdout_binary.strip(): - patch_filename = cmd_runner.path('patch{}'.format(int(time.time()))) + patch_filename = 'patch{}'.format(int(time.time())) + patch_filename = os.path.join(patch_dir, patch_filename) logger.warning('Unstaged files detected.') logger.info( 'Stashing unstaged files to {}.'.format(patch_filename), @@ -50,13 +47,13 @@ def staged_files_only(cmd_runner): patch_file.write(diff_stdout_binary) # Clear the working directory of unstaged changes - cmd_runner.run(('git', 'checkout', '--', '.')) + cmd_output('git', 'checkout', '--', '.') try: yield finally: # Try to apply the patch we saved try: - _git_apply(cmd_runner, patch_filename) + _git_apply(patch_filename) except CalledProcessError: logger.warning( 'Stashed changes conflicted with hook auto-fixes... ' @@ -65,8 +62,8 @@ def staged_files_only(cmd_runner): # We failed to apply the patch, presumably due to fixes made # by hooks. # Roll back the changes made by hooks. - cmd_runner.run(('git', 'checkout', '--', '.')) - _git_apply(cmd_runner, patch_filename) + cmd_output('git', 'checkout', '--', '.') + _git_apply(patch_filename) logger.info('Restored changes from {}.'.format(patch_filename)) else: # There weren't any staged files so we don't need to do anything diff --git a/pre_commit/store.py b/pre_commit/store.py index 365ed9a1..263b315b 100644 --- a/pre_commit/store.py +++ b/pre_commit/store.py @@ -11,7 +11,6 @@ from cached_property import cached_property import pre_commit.constants as C from pre_commit import file_lock -from pre_commit.prefixed_command_runner import PrefixedCommandRunner from pre_commit.util import clean_path_on_failure from pre_commit.util import cmd_output from pre_commit.util import copy_tree_to_path @@ -162,10 +161,6 @@ class Store(object): make_local_strategy, ) - @cached_property - def cmd_runner(self): - return PrefixedCommandRunner(self.directory) - @cached_property def db_path(self): return os.path.join(self.directory, 'db.db') diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index 5ed0ad8a..b544eb75 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -7,7 +7,6 @@ import subprocess import sys from collections import OrderedDict -import mock import pytest import pre_commit.constants as C @@ -327,11 +326,10 @@ def test_origin_source_error_msg( assert warning_msg not in printed -@pytest.mark.parametrize(('output', 'expected'), (('some', True), ('', False))) -def test_has_unmerged_paths(output, expected): - mock_runner = mock.Mock() - mock_runner.cmd_runner.run.return_value = (1, output, '') - assert _has_unmerged_paths(mock_runner) is expected +def test_has_unmerged_paths(in_merge_conflict): + assert _has_unmerged_paths() is True + cmd_output('git', 'add', '.') + assert _has_unmerged_paths() is False def test_merge_conflict(cap_out, in_merge_conflict, mock_out_store_directory): diff --git a/tests/runner_test.py b/tests/runner_test.py index 0201156c..cfca44f3 100644 --- a/tests/runner_test.py +++ b/tests/runner_test.py @@ -133,9 +133,3 @@ def test_pre_push_path(in_tmpdir): runner = Runner(path, C.CONFIG_FILE) expected_path = os.path.join(path, '.git', 'hooks', 'pre-push') assert runner.pre_push_path == expected_path - - -def test_cmd_runner(mock_out_store_directory): - runner = Runner(os.path.join('foo', 'bar'), C.CONFIG_FILE) - ret = runner.cmd_runner - assert ret.prefix_dir == os.path.join(mock_out_store_directory) + os.sep diff --git a/tests/staged_files_only_test.py b/tests/staged_files_only_test.py index 78926d05..aec55f5d 100644 --- a/tests/staged_files_only_test.py +++ b/tests/staged_files_only_test.py @@ -4,11 +4,9 @@ from __future__ import unicode_literals import io import itertools -import logging import os.path import shutil -import mock import pytest from pre_commit.staged_files_only import staged_files_only @@ -22,6 +20,11 @@ from testing.util import get_resource_path FOO_CONTENTS = '\n'.join(('1', '2', '3', '4', '5', '6', '7', '8', '')) +@pytest.fixture +def patch_dir(tempdir_factory): + return tempdir_factory.get() + + def get_short_git_status(): git_status = cmd_output('git', 'status', '-s')[1] return dict(reversed(line.split()) for line in git_status.splitlines()) @@ -54,43 +57,43 @@ def test_foo_staged(foo_staged): _test_foo_state(foo_staged) -def test_foo_nothing_unstaged(foo_staged, cmd_runner): - with staged_files_only(cmd_runner): +def test_foo_nothing_unstaged(foo_staged, patch_dir): + with staged_files_only(patch_dir): _test_foo_state(foo_staged) _test_foo_state(foo_staged) -def test_foo_something_unstaged(foo_staged, cmd_runner): +def test_foo_something_unstaged(foo_staged, patch_dir): with io.open(foo_staged.foo_filename, 'w') as foo_file: foo_file.write('herp\nderp\n') _test_foo_state(foo_staged, 'herp\nderp\n', 'AM') - with staged_files_only(cmd_runner): + with staged_files_only(patch_dir): _test_foo_state(foo_staged) _test_foo_state(foo_staged, 'herp\nderp\n', 'AM') -def test_something_unstaged_ext_diff_tool(foo_staged, cmd_runner, tmpdir): +def test_something_unstaged_ext_diff_tool(foo_staged, patch_dir, tmpdir): diff_tool = tmpdir.join('diff-tool.sh') diff_tool.write('#!/usr/bin/env bash\necho "$@"\n') cmd_output('git', 'config', 'diff.external', diff_tool.strpath) - test_foo_something_unstaged(foo_staged, cmd_runner) + test_foo_something_unstaged(foo_staged, patch_dir) -def test_foo_something_unstaged_diff_color_always(foo_staged, cmd_runner): +def test_foo_something_unstaged_diff_color_always(foo_staged, patch_dir): cmd_output('git', 'config', '--local', 'color.diff', 'always') - test_foo_something_unstaged(foo_staged, cmd_runner) + test_foo_something_unstaged(foo_staged, patch_dir) -def test_foo_both_modify_non_conflicting(foo_staged, cmd_runner): +def test_foo_both_modify_non_conflicting(foo_staged, patch_dir): with io.open(foo_staged.foo_filename, 'w') as foo_file: foo_file.write(FOO_CONTENTS + '9\n') _test_foo_state(foo_staged, FOO_CONTENTS + '9\n', 'AM') - with staged_files_only(cmd_runner): + with staged_files_only(patch_dir): _test_foo_state(foo_staged) # Modify the file as part of the "pre-commit" @@ -102,13 +105,13 @@ def test_foo_both_modify_non_conflicting(foo_staged, cmd_runner): _test_foo_state(foo_staged, FOO_CONTENTS.replace('1', 'a') + '9\n', 'AM') -def test_foo_both_modify_conflicting(foo_staged, cmd_runner): +def test_foo_both_modify_conflicting(foo_staged, patch_dir): with io.open(foo_staged.foo_filename, 'w') as foo_file: foo_file.write(FOO_CONTENTS.replace('1', 'a')) _test_foo_state(foo_staged, FOO_CONTENTS.replace('1', 'a'), 'AM') - with staged_files_only(cmd_runner): + with staged_files_only(patch_dir): _test_foo_state(foo_staged) # Modify in the same place as the stashed diff @@ -144,30 +147,30 @@ def test_img_staged(img_staged): _test_img_state(img_staged) -def test_img_nothing_unstaged(img_staged, cmd_runner): - with staged_files_only(cmd_runner): +def test_img_nothing_unstaged(img_staged, patch_dir): + with staged_files_only(patch_dir): _test_img_state(img_staged) _test_img_state(img_staged) -def test_img_something_unstaged(img_staged, cmd_runner): +def test_img_something_unstaged(img_staged, patch_dir): shutil.copy(get_resource_path('img2.jpg'), img_staged.img_filename) _test_img_state(img_staged, 'img2.jpg', 'AM') - with staged_files_only(cmd_runner): + with staged_files_only(patch_dir): _test_img_state(img_staged) _test_img_state(img_staged, 'img2.jpg', 'AM') -def test_img_conflict(img_staged, cmd_runner): +def test_img_conflict(img_staged, patch_dir): """Admittedly, this shouldn't happen, but just in case.""" shutil.copy(get_resource_path('img2.jpg'), img_staged.img_filename) _test_img_state(img_staged, 'img2.jpg', 'AM') - with staged_files_only(cmd_runner): + with staged_files_only(patch_dir): _test_img_state(img_staged) shutil.copy(get_resource_path('img3.jpg'), img_staged.img_filename) _test_img_state(img_staged, 'img3.jpg', 'AM') @@ -220,77 +223,48 @@ def test_sub_staged(sub_staged): _test_sub_state(sub_staged) -def test_sub_nothing_unstaged(sub_staged, cmd_runner): - with staged_files_only(cmd_runner): +def test_sub_nothing_unstaged(sub_staged, patch_dir): + with staged_files_only(patch_dir): _test_sub_state(sub_staged) _test_sub_state(sub_staged) -def test_sub_something_unstaged(sub_staged, cmd_runner): +def test_sub_something_unstaged(sub_staged, patch_dir): checkout_submodule(sub_staged.submodule.sha2) _test_sub_state(sub_staged, 'sha2', 'AM') - with staged_files_only(cmd_runner): + with staged_files_only(patch_dir): # This is different from others, we don't want to touch subs _test_sub_state(sub_staged, 'sha2', 'AM') _test_sub_state(sub_staged, 'sha2', 'AM') -@pytest.yield_fixture -def fake_logging_handler(): - class FakeHandler(logging.Handler): - def __init__(self): - logging.Handler.__init__(self) - self.logs = [] - - def emit(self, record): - self.logs.append(record) # pragma: no cover (only hit in failure) - - pre_commit_logger = logging.getLogger('pre_commit') - original_level = pre_commit_logger.getEffectiveLevel() - handler = FakeHandler() - pre_commit_logger.addHandler(handler) - pre_commit_logger.setLevel(logging.WARNING) - yield handler - pre_commit_logger.setLevel(original_level) - pre_commit_logger.removeHandler(handler) - - -def test_diff_returns_1_no_diff_though(fake_logging_handler, foo_staged): - cmd_runner = mock.Mock() - cmd_runner.run.return_value = (1, '', '') - cmd_runner.path.return_value = '.pre-commit-files_patch' - with staged_files_only(cmd_runner): - pass - assert not fake_logging_handler.logs - - -def test_stage_utf8_changes(foo_staged, cmd_runner): +def test_stage_utf8_changes(foo_staged, patch_dir): contents = '\u2603' with io.open('foo', 'w', encoding='UTF-8') as foo_file: foo_file.write(contents) _test_foo_state(foo_staged, contents, 'AM') - with staged_files_only(cmd_runner): + with staged_files_only(patch_dir): _test_foo_state(foo_staged) _test_foo_state(foo_staged, contents, 'AM') -def test_stage_non_utf8_changes(foo_staged, cmd_runner): +def test_stage_non_utf8_changes(foo_staged, patch_dir): contents = 'รบ' # Produce a latin-1 diff with io.open('foo', 'w', encoding='latin-1') as foo_file: foo_file.write(contents) _test_foo_state(foo_staged, contents, 'AM', encoding='latin-1') - with staged_files_only(cmd_runner): + with staged_files_only(patch_dir): _test_foo_state(foo_staged) _test_foo_state(foo_staged, contents, 'AM', encoding='latin-1') -def test_non_utf8_conflicting_diff(foo_staged, cmd_runner): +def test_non_utf8_conflicting_diff(foo_staged, patch_dir): """Regression test for #397""" # The trailing whitespace is important here, this triggers git to produce # an error message which looks like: @@ -307,7 +281,7 @@ def test_non_utf8_conflicting_diff(foo_staged, cmd_runner): foo_file.write(contents) _test_foo_state(foo_staged, contents, 'AM', encoding='latin-1') - with staged_files_only(cmd_runner): + with staged_files_only(patch_dir): _test_foo_state(foo_staged) # Create a conflicting diff that will need to be rolled back with io.open('foo', 'w') as foo_file: @@ -337,7 +311,7 @@ bool_product = tuple(itertools.product((True, False), repeat=2)) @pytest.mark.parametrize(('crlf_before', 'crlf_after'), bool_product) @pytest.mark.parametrize('autocrlf', ('true', 'false', 'input')) -def test_crlf(in_git_dir, cmd_runner, crlf_before, crlf_after, autocrlf): +def test_crlf(in_git_dir, patch_dir, crlf_before, crlf_after, autocrlf): cmd_output('git', 'config', '--local', 'core.autocrlf', autocrlf) before, after = b'1\n2\n', b'3\n4\n\n' @@ -347,16 +321,16 @@ def test_crlf(in_git_dir, cmd_runner, crlf_before, crlf_after, autocrlf): _write(before) cmd_output('git', 'add', 'foo') _write(after) - with staged_files_only(cmd_runner): + with staged_files_only(patch_dir): assert_no_diff() -def test_whitespace_errors(in_git_dir, cmd_runner): +def test_whitespace_errors(in_git_dir, patch_dir): cmd_output('git', 'config', '--local', 'apply.whitespace', 'error') - test_crlf(in_git_dir, cmd_runner, True, True, 'true') + test_crlf(in_git_dir, patch_dir, True, True, 'true') -def test_autocrlf_commited_crlf(in_git_dir, cmd_runner): +def test_autocrlf_commited_crlf(in_git_dir, patch_dir): """Regression test for #570""" cmd_output('git', 'config', '--local', 'core.autocrlf', 'false') _write(b'1\r\n2\r\n') @@ -366,5 +340,5 @@ def test_autocrlf_commited_crlf(in_git_dir, cmd_runner): cmd_output('git', 'config', '--local', 'core.autocrlf', 'true') _write(b'1\r\n2\r\n\r\n\r\n\r\n') - with staged_files_only(cmd_runner): + with staged_files_only(patch_dir): assert_no_diff() diff --git a/tests/store_test.py b/tests/store_test.py index eab4b009..106a4645 100644 --- a/tests/store_test.py +++ b/tests/store_test.py @@ -125,11 +125,6 @@ def test_clone_cleans_up_on_checkout_failure(store): assert things_starting_with_repo == [] -def test_has_cmd_runner_at_directory(store): - ret = store.cmd_runner - assert ret.prefix_dir == store.directory + os.sep - - def test_clone_when_repo_already_exists(store): # Create an entry in the sqlite db that makes it look like the repo has # been cloned.