Merge pull request pre-commit#1168 from pre-commit/fix_stdout_stderr_splicing

asottile · web-flow · commit 2048fed1106d · 2019-10-12T13:54:11.000-07:00
Fix ordering of mixed stdout / stderr printing
diff --git 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
@@ -6,6 +6,7 @@
 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 run_cmd_partition(run_cmd):
             # 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
@@ -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
@@ -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
@@ -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 @@ def test_pcre_hook_many_files(self, greppable_files, store):
         # 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 @@ def no_grep(exe, **kwargs):
 
         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 @@ class MyKeyboardInterrupt(KeyboardInterrupt):
     # 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
@@ -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 @@ def test_xargs_smoke():
 
 
 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
