Merge pull request #287 from pre-commit/fail_modifying_hooks_285

Fail a hook if it makes modifications.  Resolves #285

pre_commit/commands/run.py

@@ -85,7 +85,13 @@ def _run_single_hook(hook, repo, args, write, skips=frozenset()):
     write(get_hook_message(_hook_msg_start(hook, args.verbose), end_len=6))
     sys.stdout.flush()
 
+    diff_before = cmd_output('git', 'diff', retcode=None)
     retcode, stdout, stderr = repo.run_hook(hook, filenames)
+    diff_after = cmd_output('git', 'diff', retcode=None)
+
+    # If the hook makes changes, fail the commit
+    if diff_before != diff_after:
+        retcode = 1
 
     if retcode:
         retcode = 1

testing/resources/modified_file_returns_zero_repo/bin/hook.sh

@@ -0,0 +1,6 @@
+#!/usr/bin/env bash
+
+for f in $@; do
+    echo modified > "$f"
+    echo "Modified: $f!"
+done

testing/resources/modified_file_returns_zero_repo/bin/hook2.sh

@@ -0,0 +1,2 @@
+#!/usr/bin/env bash
+echo $@

testing/resources/modified_file_returns_zero_repo/hooks.yaml

@@ -0,0 +1,10 @@
+-   id: bash_hook
+    name: Bash hook
+    entry: bin/hook.sh
+    language: script
+    files: ''
+-   id: bash_hook2
+    name: Bash hook
+    entry: bin/hook2.sh
+    language: script
+    files: ''

tests/commands/run_test.py

@@ -120,6 +120,29 @@ def test_arbitrary_bytes_hook(tempdir_factory, mock_out_store_directory):
         _test_run(git_path, {}, (b'\xe2\x98\x83\xb2\n',), 1, True)
 
 
+def test_hook_that_modifies_but_returns_zero(
+        tempdir_factory, mock_out_store_directory,
+):
+    git_path = make_consuming_repo(
+        tempdir_factory, 'modified_file_returns_zero_repo',
+    )
+    with cwd(git_path):
+        _test_run(
+            git_path,
+            {},
+            (
+                # The first should fail
+                b'Failed',
+                # With a modified file (the hook's output)
+                b'Modified: foo.py',
+                # The next hook should pass despite the first modifying
+                b'Passed',
+            ),
+            1,
+            True,
+        )
+
+
 @pytest.mark.parametrize(
     ('options', 'outputs', 'expected_ret', 'stage'),
     (