From 2633d38a63a923738288fe633b8401649e7e2960 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sat, 12 Oct 2019 13:35:04 -0700 Subject: [PATCH] Fix ordering of mixed stdout / stderr printing --- pre_commit/commands/run.py | 14 ++++------ pre_commit/xargs.py | 13 +++++----- .../stdout_stderr_repo/.pre-commit-hooks.yaml | 4 +++ testing/resources/stdout_stderr_repo/entry | 13 ++++++++++ tests/repository_test.py | 26 +++++++++++++------ tests/xargs_test.py | 17 ++++++------ 6 files changed, 55 insertions(+), 32 deletions(-) create mode 100644 testing/resources/stdout_stderr_repo/.pre-commit-hooks.yaml create mode 100755 testing/resources/stdout_stderr_repo/entry diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py index aee3d9c2..6ab1879d 100644 --- a/pre_commit/commands/run.py +++ b/pre_commit/commands/run.py @@ -118,9 +118,7 @@ def _run_single_hook(classifier, hook, args, skips, cols): sys.stdout.flush() diff_before = cmd_output_b('git', 'diff', '--no-ext-diff', retcode=None) - retcode, stdout, stderr = hook.run( - tuple(filenames) if hook.pass_filenames else (), - ) + retcode, out = hook.run(tuple(filenames) if hook.pass_filenames else ()) diff_after = cmd_output_b('git', 'diff', '--no-ext-diff', retcode=None) file_modifications = diff_before != diff_after @@ -141,7 +139,7 @@ def _run_single_hook(classifier, hook, args, skips, cols): output.write_line(color.format_color(pass_fail, print_color, args.color)) if ( - (stdout or stderr or file_modifications) and + (out or file_modifications) and (retcode or args.verbose or hook.verbose) ): output.write_line('hookid: {}\n'.format(hook.id)) @@ -150,15 +148,13 @@ def _run_single_hook(classifier, hook, args, skips, cols): if file_modifications: output.write('Files were modified by this hook.') - if stdout or stderr: + if out: output.write_line(' Additional output:') output.write_line() - for out in (stdout, stderr): - assert type(out) is bytes, type(out) - if out.strip(): - output.write_line(out.strip(), logfile_name=hook.log_file) + if out.strip(): + output.write_line(out.strip(), logfile_name=hook.log_file) output.write_line() return retcode diff --git a/pre_commit/xargs.py b/pre_commit/xargs.py index 332681d8..44031754 100644 --- a/pre_commit/xargs.py +++ b/pre_commit/xargs.py @@ -6,6 +6,7 @@ import concurrent.futures import contextlib import math import os +import subprocess import sys import six @@ -112,23 +113,24 @@ def xargs(cmd, varargs, **kwargs): max_length = kwargs.pop('_max_length', _get_platform_max_length()) retcode = 0 stdout = b'' - stderr = b'' try: cmd = parse_shebang.normalize_cmd(cmd) except parse_shebang.ExecutableNotFoundError as e: - return e.to_output() + return e.to_output()[:2] partitions = partition(cmd, varargs, target_concurrency, max_length) def run_cmd_partition(run_cmd): - return cmd_output_b(*run_cmd, retcode=None, **kwargs) + return cmd_output_b( + *run_cmd, retcode=None, stderr=subprocess.STDOUT, **kwargs + ) threads = min(len(partitions), target_concurrency) with _thread_mapper(threads) as thread_map: results = thread_map(run_cmd_partition, partitions) - for proc_retcode, proc_out, proc_err in results: + for proc_retcode, proc_out, _ in results: # This is *slightly* too clever so I'll explain it. # First the xor boolean table: # T | F | @@ -141,6 +143,5 @@ def xargs(cmd, varargs, **kwargs): # code. Otherwise, the returncode is unchanged. retcode |= bool(proc_retcode) ^ negate stdout += proc_out - stderr += proc_err - return retcode, stdout, stderr + return retcode, stdout diff --git a/testing/resources/stdout_stderr_repo/.pre-commit-hooks.yaml b/testing/resources/stdout_stderr_repo/.pre-commit-hooks.yaml new file mode 100644 index 00000000..e68174a1 --- /dev/null +++ b/testing/resources/stdout_stderr_repo/.pre-commit-hooks.yaml @@ -0,0 +1,4 @@ +- id: stdout-stderr + name: stdout-stderr + language: script + entry: ./entry diff --git a/testing/resources/stdout_stderr_repo/entry b/testing/resources/stdout_stderr_repo/entry new file mode 100755 index 00000000..e382373d --- /dev/null +++ b/testing/resources/stdout_stderr_repo/entry @@ -0,0 +1,13 @@ +#!/usr/bin/env python +import sys + + +def main(): + for i in range(6): + f = sys.stdout if i % 2 == 0 else sys.stderr + f.write('{}\n'.format(i)) + f.flush() + + +if __name__ == '__main__': + exit(main()) diff --git a/tests/repository_test.py b/tests/repository_test.py index ec09da36..344b3a58 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -177,7 +177,8 @@ def test_run_a_failing_docker_hook(tempdir_factory, store): _test_hook_repo( tempdir_factory, store, 'docker_hooks_repo', 'docker-hook-failing', - ['Hello World from docker'], b'', + ['Hello World from docker'], + mock.ANY, # an error message about `bork` not existing expected_return_code=1, ) @@ -363,6 +364,15 @@ def test_run_hook_with_curly_braced_arguments(tempdir_factory, store): ) +def test_intermixed_stdout_stderr(tempdir_factory, store): + _test_hook_repo( + tempdir_factory, store, 'stdout_stderr_repo', + 'stdout-stderr', + [], + b'0\n1\n2\n3\n4\n5\n', + ) + + def _make_grep_repo(language, entry, store, args=()): config = { 'repo': 'local', @@ -393,20 +403,20 @@ class TestPygrep(object): def test_grep_hook_matching(self, greppable_files, store): hook = _make_grep_repo(self.language, 'ello', store) - ret, out, _ = hook.run(('f1', 'f2', 'f3')) + ret, out = hook.run(('f1', 'f2', 'f3')) assert ret == 1 assert _norm_out(out) == b"f1:1:hello'hi\n" def test_grep_hook_case_insensitive(self, greppable_files, store): hook = _make_grep_repo(self.language, 'ELLO', store, args=['-i']) - ret, out, _ = hook.run(('f1', 'f2', 'f3')) + ret, out = hook.run(('f1', 'f2', 'f3')) assert ret == 1 assert _norm_out(out) == b"f1:1:hello'hi\n" @pytest.mark.parametrize('regex', ('nope', "foo'bar", r'^\[INFO\]')) def test_grep_hook_not_matching(self, regex, greppable_files, store): hook = _make_grep_repo(self.language, regex, store) - ret, out, _ = hook.run(('f1', 'f2', 'f3')) + ret, out = hook.run(('f1', 'f2', 'f3')) assert (ret, out) == (0, b'') @@ -420,7 +430,7 @@ class TestPCRE(TestPygrep): # file to make sure it still fails. This is not the case when naively # using a system hook with `grep -H -n '...'` hook = _make_grep_repo('pcre', 'ello', store) - ret, out, _ = hook.run((os.devnull,) * 15000 + ('f1',)) + ret, out = hook.run((os.devnull,) * 15000 + ('f1',)) assert ret == 1 assert _norm_out(out) == b"f1:1:hello'hi\n" @@ -431,7 +441,7 @@ class TestPCRE(TestPygrep): with mock.patch.object(parse_shebang, 'find_executable', no_grep): hook = _make_grep_repo('pcre', 'ello', store) - ret, out, _ = hook.run(('f1', 'f2', 'f3')) + ret, out = hook.run(('f1', 'f2', 'f3')) assert ret == 1 expected = 'Executable `{}` not found'.format(pcre.GREP).encode() assert out == expected @@ -635,7 +645,7 @@ def test_control_c_control_c_on_install(tempdir_factory, store): # However, it should be perfectly runnable (reinstall after botched # install) install_hook_envs(hooks, store) - retv, stdout, stderr = hook.run(()) + retv, stdout = hook.run(()) assert retv == 0 @@ -657,7 +667,7 @@ def test_invalidated_virtualenv(tempdir_factory, store): cmd_output_b('rm', '-rf', *paths) # pre-commit should rebuild the virtualenv and it should be runnable - retv, stdout, stderr = _get_hook(config, store, 'foo').run(()) + retv, stdout = _get_hook(config, store, 'foo').run(()) assert retv == 0 diff --git a/tests/xargs_test.py b/tests/xargs_test.py index d2d7d7b3..183ab5ad 100644 --- a/tests/xargs_test.py +++ b/tests/xargs_test.py @@ -143,10 +143,9 @@ def test_argument_too_long(): def test_xargs_smoke(): - ret, out, err = xargs.xargs(('echo',), ('hello', 'world')) + ret, out = xargs.xargs(('echo',), ('hello', 'world')) assert ret == 0 assert out.replace(b'\r\n', b'\n') == b'hello world\n' - assert err == b'' exit_cmd = parse_shebang.normalize_cmd(('bash', '-c', 'exit $1', '--')) @@ -155,27 +154,27 @@ max_length = len(' '.join(exit_cmd)) + 3 def test_xargs_negate(): - ret, _, _ = xargs.xargs( + ret, _ = xargs.xargs( exit_cmd, ('1',), negate=True, _max_length=max_length, ) assert ret == 0 - ret, _, _ = xargs.xargs( + ret, _ = xargs.xargs( exit_cmd, ('1', '0'), negate=True, _max_length=max_length, ) assert ret == 1 def test_xargs_negate_command_not_found(): - ret, _, _ = xargs.xargs(('cmd-not-found',), ('1',), negate=True) + ret, _ = xargs.xargs(('cmd-not-found',), ('1',), negate=True) assert ret != 0 def test_xargs_retcode_normal(): - ret, _, _ = xargs.xargs(exit_cmd, ('0',), _max_length=max_length) + ret, _ = xargs.xargs(exit_cmd, ('0',), _max_length=max_length) assert ret == 0 - ret, _, _ = xargs.xargs(exit_cmd, ('0', '1'), _max_length=max_length) + ret, _ = xargs.xargs(exit_cmd, ('0', '1'), _max_length=max_length) assert ret == 1 @@ -184,7 +183,7 @@ def test_xargs_concurrency(): print_pid = ('sleep 0.5 && echo $$',) start = time.time() - ret, stdout, _ = xargs.xargs( + ret, stdout = xargs.xargs( bash_cmd, print_pid * 5, target_concurrency=5, _max_length=len(' '.join(bash_cmd + print_pid)) + 1, @@ -215,6 +214,6 @@ def test_xargs_propagate_kwargs_to_cmd(): cmd = ('bash', '-c', 'echo $PRE_COMMIT_TEST_VAR', '--') cmd = parse_shebang.normalize_cmd(cmd) - ret, stdout, _ = xargs.xargs(cmd, ('1',), env=env) + ret, stdout = xargs.xargs(cmd, ('1',), env=env) assert ret == 0 assert b'Pre commit is awesome' in stdout