Repository: pre-commit/pre-commit
Pull request #608 (2017-09-05 21:53:56)

Remove Runner.cmd_runner and Store.cmd_runner


Patch:
diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py
index 8b80bef..9923258 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 c7455d7..21707cb 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 4d23392..cfd6381 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 365ed9a..263b315 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')


Test patch:
diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py
index 5ed0ad8..b544eb7 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 0201156..cfca44f 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 78926d0..aec55f5 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 eab4b00..106a464 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.