Merge pull request #248 from pre-commit/allow_arbitrary_bytes

Allow arbitrary bytes in output.  Resolves #245

pre_commit/commands/run.py

@@ -102,8 +102,9 @@ def _run_single_hook(hook, repo, args, write, skips=frozenset()):
         write('hookid: {0}\n'.format(hook['id']))
         write('\n')
         for output in (stdout, stderr):
+            assert type(output) is bytes, type(output)
             if output.strip():
-                write(output.strip() + '\n')
+                write(output.strip() + b'\n')
         write('\n')
 
     return retcode

pre_commit/languages/helpers.py

@@ -22,6 +22,7 @@ def run_hook(env, hook, file_args):
         ' '.join(['xargs', '-0', '-s4000', hook['entry']] + quoted_args),
         stdin=file_args_to_stdin(file_args),
         retcode=None,
+        encoding=None,
     )
 
 

pre_commit/languages/pcre.py

@@ -24,4 +24,5 @@ def run_hook(repo_cmd_runner, hook, file_args):
         ],
         stdin=file_args_to_stdin(file_args),
         retcode=None,
+        encoding=None,
     )

pre_commit/languages/script.py

@@ -17,4 +17,5 @@ def run_hook(repo_cmd_runner, hook, file_args):
         # TODO: this is duplicated in pre_commit/languages/helpers.py
         stdin=file_args_to_stdin(file_args),
         retcode=None,
+        encoding=None,
     )

pre_commit/languages/system.py

@@ -18,4 +18,5 @@ def run_hook(repo_cmd_runner, hook, file_args):
         ['xargs', '-0'] + shlex.split(hook['entry']) + hook['args'],
         stdin=file_args_to_stdin(file_args),
         retcode=None,
+        encoding=None,
     )

testing/resources/arbitrary_bytes_repo/hooks.yaml

@@ -0,0 +1,6 @@
+-   id: python3-hook
+    name: Python 3 Hook
+    entry: python3-hook
+    language: python
+    language_version: python3.3
+    files: \.py$

testing/resources/arbitrary_bytes_repo/python3_hook/main.py

@@ -0,0 +1,13 @@
+# -*- coding: utf-8 -*-
+from __future__ import print_function
+from __future__ import unicode_literals
+
+import sys
+
+
+def func():
+    # Intentionally write mixed encoding to the output.  This should not crash
+    # pre-commit and should write bytes to the output.
+    sys.stdout.buffer.write('☃'.encode('UTF-8') + '²'.encode('latin1') + b'\n')
+    # Return 1 to trigger printing
+    return 1

testing/resources/arbitrary_bytes_repo/setup.py

@@ -0,0 +1,11 @@
+from setuptools import find_packages
+from setuptools import setup
+
+setup(
+    name='python3_hook',
+    version='0.0.0',
+    packages=find_packages('.'),
+    entry_points={
+        'console_scripts': ['python3-hook = python3_hook.main:func'],
+    },
+)

tests/commands/run_test.py

@@ -1,6 +1,7 @@
 # -*- coding: UTF-8 -*-
 from __future__ import unicode_literals
 
+import functools
 import io
 import os
 import os.path
@@ -16,6 +17,7 @@ from pre_commit.commands.run import _has_unmerged_paths
 from pre_commit.commands.run import get_changed_files
 from pre_commit.commands.run import run
 from pre_commit.ordereddict import OrderedDict
+from pre_commit.output import sys_stdout_write_wrapper
 from pre_commit.runner import Runner
 from pre_commit.util import cmd_output
 from pre_commit.util import cwd
@@ -44,7 +46,7 @@ def stage_a_file():
 
 
 def get_write_mock_output(write_mock):
-    return ''.join(call[0][0] for call in write_mock.call_args_list)
+    return b''.join(call[0][0] for call in write_mock.write.call_args_list)
 
 
 def _get_opts(
@@ -76,7 +78,8 @@ def _get_opts(
 def _do_run(repo, args, environ={}):
     runner = Runner(repo)
     write_mock = mock.Mock()
-    ret = run(runner, args, write=write_mock, environ=environ)
+    write_fn = functools.partial(sys_stdout_write_wrapper, stream=write_mock)
+    ret = run(runner, args, write=write_fn, environ=environ)
     printed = get_write_mock_output(write_mock)
     return ret, printed
 
@@ -97,32 +100,43 @@ def test_run_all_hooks_failing(
     _test_run(
         repo_with_failing_hook,
         {},
-        ('Failing hook', 'Failed', 'hookid: failing_hook', 'Fail\nfoo.py\n'),
+        (
+            b'Failing hook',
+            b'Failed',
+            b'hookid: failing_hook',
+            b'Fail\nfoo.py\n',
+        ),
         1,
         True,
     )
 
 
+def test_arbitrary_bytes_hook(tmpdir_factory, mock_out_store_directory):
+    git_path = make_consuming_repo(tmpdir_factory, 'arbitrary_bytes_repo')
+    with cwd(git_path):
+        _test_run(git_path, {}, (b'\xe2\x98\x83\xb2\n',), 1, True)
+
+
 @pytest.mark.parametrize(
     ('options', 'outputs', 'expected_ret', 'stage'),
     (
-        ({}, ('Bash hook', 'Passed'), 0, True),
-        ({'verbose': True}, ('foo.py\nHello World',), 0, True),
-        ({'hook': 'bash_hook'}, ('Bash hook', 'Passed'), 0, True),
-        ({'hook': 'nope'}, ('No hook with id `nope`',), 1, True),
+        ({}, (b'Bash hook', b'Passed'), 0, True),
+        ({'verbose': True}, (b'foo.py\nHello World',), 0, True),
+        ({'hook': 'bash_hook'}, (b'Bash hook', b'Passed'), 0, True),
+        ({'hook': 'nope'}, (b'No hook with id `nope`',), 1, True),
         (
             {'all_files': True, 'verbose': True},
-            ('foo.py'),
+            (b'foo.py',),
             0,
             True,
         ),
         (
             {'files': ('foo.py',), 'verbose': True},
-            ('foo.py'),
+            (b'foo.py',),
             0,
             True,
         ),
-        ({}, ('Bash hook', '(no files to check)', 'Skipped'), 0, False),
+        ({}, (b'Bash hook', b'(no files to check)', b'Skipped'), 0, False),
     )
 )
 def test_run(
@@ -150,7 +164,7 @@ def test_origin_source_error_msg(
 ):
     args = _get_opts(origin=origin, source=source)
     ret, printed = _do_run(repo_with_passing_hook, args)
-    warning_msg = 'Specify both --origin and --source.'
+    warning_msg = b'Specify both --origin and --source.'
     if expect_failure:
         assert ret == 1
         assert warning_msg in printed
@@ -183,7 +197,7 @@ def test_no_stash(
     args = _get_opts(no_stash=no_stash, all_files=all_files)
     ret, printed = _do_run(repo_with_passing_hook, args)
     assert ret == 0
-    warning_msg = '[WARNING] Unstaged files detected.'
+    warning_msg = b'[WARNING] Unstaged files detected.'
     if expect_stash:
         assert warning_msg in printed
     else:
@@ -200,7 +214,7 @@ def test_has_unmerged_paths(output, expected):
 def test_merge_conflict(in_merge_conflict, mock_out_store_directory):
     ret, printed = _do_run(in_merge_conflict, _get_opts())
     assert ret == 1
-    assert 'Unmerged files.  Resolve before committing.' in printed
+    assert b'Unmerged files.  Resolve before committing.' in printed
 
 
 def test_merge_conflict_modified(in_merge_conflict, mock_out_store_directory):
@@ -211,13 +225,15 @@ def test_merge_conflict_modified(in_merge_conflict, mock_out_store_directory):
 
     ret, printed = _do_run(in_merge_conflict, _get_opts())
     assert ret == 1
-    assert 'Unmerged files.  Resolve before committing.' in printed
+    assert b'Unmerged files.  Resolve before committing.' in printed
 
 
 def test_merge_conflict_resolved(in_merge_conflict, mock_out_store_directory):
     cmd_output('git', 'add', '.')
     ret, printed = _do_run(in_merge_conflict, _get_opts())
-    for msg in ('Checking merge-conflict files only.', 'Bash hook', 'Passed'):
+    for msg in (
+            b'Checking merge-conflict files only.', b'Bash hook', b'Passed',
+    ):
         assert msg in printed
 
 
@@ -242,7 +258,7 @@ def test_skip_hook(repo_with_passing_hook, mock_out_store_directory):
     ret, printed = _do_run(
         repo_with_passing_hook, _get_opts(), {'SKIP': 'bash_hook'},
     )
-    for msg in ('Bash hook', 'Skipped'):
+    for msg in (b'Bash hook', b'Skipped'):
         assert msg in printed
 
 
@@ -250,14 +266,14 @@ def test_hook_id_not_in_non_verbose_output(
         repo_with_passing_hook, mock_out_store_directory
 ):
     ret, printed = _do_run(repo_with_passing_hook, _get_opts(verbose=False))
-    assert '[bash_hook]' not in printed
+    assert b'[bash_hook]' not in printed
 
 
 def test_hook_id_in_verbose_output(
         repo_with_passing_hook, mock_out_store_directory,
 ):
     ret, printed = _do_run(repo_with_passing_hook, _get_opts(verbose=True))
-    assert '[bash_hook] Bash hook' in printed
+    assert b'[bash_hook] Bash hook' in printed
 
 
 def test_multiple_hooks_same_id(
@@ -272,7 +288,7 @@ def test_multiple_hooks_same_id(
 
     ret, output = _do_run(repo_with_passing_hook, _get_opts())
     assert ret == 0
-    assert output.count('Bash hook') == 2
+    assert output.count(b'Bash hook') == 2
 
 
 def test_non_ascii_hook_id(
@@ -383,7 +399,7 @@ def test_local_hook_passes(
     _test_run(
         repo_with_passing_hook,
         options={},
-        expected_outputs=[''],
+        expected_outputs=[b''],
         expected_ret=0,
         stage=False
     )
@@ -412,7 +428,7 @@ def test_local_hook_fails(
     _test_run(
         repo_with_passing_hook,
         options={},
-        expected_outputs=[''],
+        expected_outputs=[b''],
         expected_ret=1,
         stage=False
     )
@@ -428,8 +444,8 @@ def test_allow_unstaged_config_option(
 
     args = _get_opts(allow_unstaged_config=True)
     ret, printed = _do_run(repo_with_passing_hook, args)
-    assert 'You have an unstaged config file' in printed
-    assert 'have specified the --allow-unstaged-config option.' in printed
+    assert b'You have an unstaged config file' in printed
+    assert b'have specified the --allow-unstaged-config option.' in printed
     assert ret == 0
 
 
@@ -446,7 +462,7 @@ def test_no_allow_unstaged_config_option(
     modify_config(repo_with_passing_hook)
     args = _get_opts(allow_unstaged_config=False)
     ret, printed = _do_run(repo_with_passing_hook, args)
-    assert 'Your .pre-commit-config.yaml is unstaged.' in printed
+    assert b'Your .pre-commit-config.yaml is unstaged.' in printed
     assert ret == 1
 
 
@@ -456,7 +472,7 @@ def test_no_stash_suppresses_allow_unstaged_config_option(
     modify_config(repo_with_passing_hook)
     args = _get_opts(allow_unstaged_config=False, no_stash=True)
     ret, printed = _do_run(repo_with_passing_hook, args)
-    assert 'Your .pre-commit-config.yaml is unstaged.' not in printed
+    assert b'Your .pre-commit-config.yaml is unstaged.' not in printed
 
 
 def test_all_files_suppresses_allow_unstaged_config_option(
@@ -465,7 +481,7 @@ def test_all_files_suppresses_allow_unstaged_config_option(
     modify_config(repo_with_passing_hook)
     args = _get_opts(all_files=True)
     ret, printed = _do_run(repo_with_passing_hook, args)
-    assert 'Your .pre-commit-config.yaml is unstaged.' not in printed
+    assert b'Your .pre-commit-config.yaml is unstaged.' not in printed
 
 
 def test_files_suppresses_allow_unstaged_config_option(
@@ -474,4 +490,4 @@ def test_files_suppresses_allow_unstaged_config_option(
     modify_config(repo_with_passing_hook)
     args = _get_opts(files=['.pre-commit-config.yaml'])
     ret, printed = _do_run(repo_with_passing_hook, args)
-    assert 'Your .pre-commit-config.yaml is unstaged.' not in printed
+    assert b'Your .pre-commit-config.yaml is unstaged.' not in printed

tests/repository_test.py

@@ -8,6 +8,7 @@ import shutil
 import mock
 import pytest
 
+from pre_commit import five
 from pre_commit.clientlib.validate_config import CONFIG_JSON_SCHEMA
 from pre_commit.clientlib.validate_config import validate_config_extra
 from pre_commit.jsonschema_extensions import apply_defaults
@@ -43,14 +44,15 @@ def _test_hook_repo(
     ][0]
     ret = repo.run_hook(hook_dict, args)
     assert ret[0] == expected_return_code
-    assert ret[1].replace('\r\n', '\n') == expected
+    assert ret[1].replace(b'\r\n', b'\n') == expected
 
 
 @pytest.mark.integration
 def test_python_hook(tmpdir_factory, store):
     _test_hook_repo(
         tmpdir_factory, store, 'python_hooks_repo',
-        'foo', [os.devnull], "['{0}']\nHello World\n".format(os.devnull),
+        'foo', [os.devnull],
+        b"['" + five.to_bytes(os.devnull) + b"']\nHello World\n"
     )
 
 
@@ -60,8 +62,8 @@ def test_python_hook_args_with_spaces(tmpdir_factory, store):
         tmpdir_factory, store, 'python_hooks_repo',
         'foo',
         [],
-        "['i have spaces', 'and\"\\'quotes', '$and !this']\n"
-        'Hello World\n',
+        b"['i have spaces', 'and\"\\'quotes', '$and !this']\n"
+        b'Hello World\n',
         config_kwargs={
             'hooks': [{
                 'id': 'foo',
@@ -88,10 +90,10 @@ def test_switch_language_versions_doesnt_clobber(tmpdir_factory, store):
         ]
         ret = repo.run_hook(hook_dict, [])
         assert ret[0] == 0
-        assert ret[1].replace('\r\n', '\n') == expected_output
+        assert ret[1].replace(b'\r\n', b'\n') == expected_output
 
-    run_on_version('python3.4', '3.4\n[]\nHello World\n')
-    run_on_version('python3.3', '3.3\n[]\nHello World\n')
+    run_on_version('python3.4', b'3.4\n[]\nHello World\n')
+    run_on_version('python3.3', b'3.3\n[]\nHello World\n')
 
 
 @pytest.mark.integration
@@ -100,7 +102,7 @@ def test_versioned_python_hook(tmpdir_factory, store):
         tmpdir_factory, store, 'python3_hooks_repo',
         'python3-hook',
         [os.devnull],
-        "3.3\n['{0}']\nHello World\n".format(os.devnull),
+        b"3.3\n['" + five.to_bytes(os.devnull) + b"']\nHello World\n",
     )
 
 
@@ -110,7 +112,7 @@ def test_versioned_python_hook(tmpdir_factory, store):
 def test_run_a_node_hook(tmpdir_factory, store):
     _test_hook_repo(
         tmpdir_factory, store, 'node_hooks_repo',
-        'foo', ['/dev/null'], 'Hello World\n',
+        'foo', ['/dev/null'], b'Hello World\n',
     )
 
 
@@ -120,7 +122,7 @@ def test_run_a_node_hook(tmpdir_factory, store):
 def test_run_versioned_node_hook(tmpdir_factory, store):
     _test_hook_repo(
         tmpdir_factory, store, 'node_0_11_8_hooks_repo',
-        'node-11-8-hook', ['/dev/null'], 'v0.11.8\nHello World\n',
+        'node-11-8-hook', ['/dev/null'], b'v0.11.8\nHello World\n',
     )
 
 
@@ -130,7 +132,7 @@ def test_run_versioned_node_hook(tmpdir_factory, store):
 def test_run_a_ruby_hook(tmpdir_factory, store):
     _test_hook_repo(
         tmpdir_factory, store, 'ruby_hooks_repo',
-        'ruby_hook', ['/dev/null'], 'Hello world from a ruby hook\n',
+        'ruby_hook', ['/dev/null'], b'Hello world from a ruby hook\n',
     )
 
 
@@ -142,7 +144,7 @@ def test_run_versioned_ruby_hook(tmpdir_factory, store):
         tmpdir_factory, store, 'ruby_1_9_3_hooks_repo',
         'ruby_hook',
         ['/dev/null'],
-        '1.9.3\n484\nHello world from a ruby hook\n',
+        b'1.9.3\n484\nHello world from a ruby hook\n',
     )
 
 
@@ -150,7 +152,7 @@ def test_run_versioned_ruby_hook(tmpdir_factory, store):
 def test_system_hook_with_spaces(tmpdir_factory, store):
     _test_hook_repo(
         tmpdir_factory, store, 'system_hook_with_spaces_repo',
-        'system-hook-with-spaces', ['/dev/null'], 'Hello World\n',
+        'system-hook-with-spaces', ['/dev/null'], b'Hello World\n',
     )
 
 
@@ -158,7 +160,7 @@ def test_system_hook_with_spaces(tmpdir_factory, store):
 def test_run_a_script_hook(tmpdir_factory, store):
     _test_hook_repo(
         tmpdir_factory, store, 'script_hooks_repo',
-        'bash_hook', ['bar'], 'bar\nHello World\n',
+        'bash_hook', ['bar'], b'bar\nHello World\n',
     )
 
 
@@ -168,7 +170,7 @@ def test_run_hook_with_spaced_args(tmpdir_factory, store):
         tmpdir_factory, store, 'arg_per_line_hooks_repo',
         'arg-per-line',
         ['foo bar', 'baz'],
-        'arg: hello\narg: world\narg: foo bar\narg: baz\n',
+        b'arg: hello\narg: world\narg: foo bar\narg: baz\n',
     )
 
 
@@ -185,12 +187,12 @@ def test_pcre_hook_no_match(tmpdir_factory, store):
 
         _test_hook_repo(
             tmpdir_factory, store, 'pcre_hooks_repo',
-            'regex-with-quotes', ['herp', 'derp'], '',
+            'regex-with-quotes', ['herp', 'derp'], b'',
         )
 
         _test_hook_repo(
             tmpdir_factory, store, 'pcre_hooks_repo',
-            'other-regex', ['herp', 'derp'], '',
+            'other-regex', ['herp', 'derp'], b'',
         )
 
 
@@ -207,13 +209,13 @@ def test_pcre_hook_matching(tmpdir_factory, store):
 
         _test_hook_repo(
             tmpdir_factory, store, 'pcre_hooks_repo',
-            'regex-with-quotes', ['herp', 'derp'], "herp:2:herpfoo'bard\n",
+            'regex-with-quotes', ['herp', 'derp'], b"herp:2:herpfoo'bard\n",
             expected_return_code=123,
         )
 
         _test_hook_repo(
             tmpdir_factory, store, 'pcre_hooks_repo',
-            'other-regex', ['herp', 'derp'], 'derp:1:[INFO] information yo\n',
+            'other-regex', ['herp', 'derp'], b'derp:1:[INFO] information yo\n',
             expected_return_code=123,
         )
 
@@ -233,7 +235,7 @@ def test_pcre_many_files(tmpdir_factory, store):
             tmpdir_factory, store, 'pcre_hooks_repo',
             'other-regex',
             ['/dev/null'] * 15000 + ['herp'],
-            'herp:1:[INFO] info\n',
+            b'herp:1:[INFO] info\n',
             expected_return_code=123,
         )
 
@@ -243,6 +245,7 @@ def _norm_pwd(path):
     # This normalizes to the bash /tmp
     return cmd_output(
         'bash', '-c', "cd '{0}' && pwd".format(path),
+        encoding=None,
     )[1].strip()
 
 
@@ -253,7 +256,7 @@ def test_cwd_of_hook(tmpdir_factory, store):
     with cwd(path):
         _test_hook_repo(
             tmpdir_factory, store, 'prints_cwd_repo',
-            'prints_cwd', ['-L'], _norm_pwd(path) + '\n',
+            'prints_cwd', ['-L'], _norm_pwd(path) + b'\n',
         )
 
 
@@ -400,7 +403,7 @@ def test_tags_on_repositories(in_tmpdir, tmpdir_factory, store):
     )
     ret = repo_2.run_hook(repo_2.hooks[0][1], ['bar'])
     assert ret[0] == 0
-    assert ret[1] == 'bar\nHello World\n'
+    assert ret[1] == b'bar\nHello World\n'
 
 
 def test_local_repository():