From 66b1d39c6e7aeba75586bbfcb66cb6cb1d6e8663 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Thu, 23 Jul 2015 12:59:13 -0700 Subject: [PATCH] Allow arbitrary bytes in output. Resolves #245 --- pre_commit/commands/run.py | 3 +- pre_commit/languages/helpers.py | 1 + pre_commit/languages/pcre.py | 1 + pre_commit/languages/script.py | 1 + pre_commit/languages/system.py | 1 + .../resources/arbitrary_bytes_repo/hooks.yaml | 6 ++ .../python3_hook/__init__.py | 0 .../arbitrary_bytes_repo/python3_hook/main.py | 13 ++++ .../resources/arbitrary_bytes_repo/setup.py | 11 +++ tests/commands/run_test.py | 70 ++++++++++++------- tests/repository_test.py | 47 +++++++------ 11 files changed, 104 insertions(+), 50 deletions(-) create mode 100644 testing/resources/arbitrary_bytes_repo/hooks.yaml create mode 100644 testing/resources/arbitrary_bytes_repo/python3_hook/__init__.py create mode 100644 testing/resources/arbitrary_bytes_repo/python3_hook/main.py create mode 100644 testing/resources/arbitrary_bytes_repo/setup.py diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py index 0e0e16e9..cdd11f53 100644 --- a/pre_commit/commands/run.py +++ b/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 diff --git a/pre_commit/languages/helpers.py b/pre_commit/languages/helpers.py index ffbe66ac..b0add575 100644 --- a/pre_commit/languages/helpers.py +++ b/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, ) diff --git a/pre_commit/languages/pcre.py b/pre_commit/languages/pcre.py index c66aae6d..248dd7de 100644 --- a/pre_commit/languages/pcre.py +++ b/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, ) diff --git a/pre_commit/languages/script.py b/pre_commit/languages/script.py index cb72e986..1b357e4d 100644 --- a/pre_commit/languages/script.py +++ b/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, ) diff --git a/pre_commit/languages/system.py b/pre_commit/languages/system.py index d7287e41..3de48aac 100644 --- a/pre_commit/languages/system.py +++ b/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, ) diff --git a/testing/resources/arbitrary_bytes_repo/hooks.yaml b/testing/resources/arbitrary_bytes_repo/hooks.yaml new file mode 100644 index 00000000..becc2837 --- /dev/null +++ b/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$ diff --git a/testing/resources/arbitrary_bytes_repo/python3_hook/__init__.py b/testing/resources/arbitrary_bytes_repo/python3_hook/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/testing/resources/arbitrary_bytes_repo/python3_hook/main.py b/testing/resources/arbitrary_bytes_repo/python3_hook/main.py new file mode 100644 index 00000000..c6a5547c --- /dev/null +++ b/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 diff --git a/testing/resources/arbitrary_bytes_repo/setup.py b/testing/resources/arbitrary_bytes_repo/setup.py new file mode 100644 index 00000000..bf7690c0 --- /dev/null +++ b/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'], + }, +) diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index 747493dc..fc7037fc 100644 --- a/tests/commands/run_test.py +++ b/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 diff --git a/tests/repository_test.py b/tests/repository_test.py index e7ad2276..66bab931 100644 --- a/tests/repository_test.py +++ b/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():