Fail gracefully on undecodable install output.

asottile · asottile · commit 2aaaddb5cc41 · 2016-01-12T10:08:59.000-08:00
diff --git a/pre_commit/error_handler.py b/pre_commit/error_handler.py
@@ -18,8 +18,19 @@ class PreCommitSystemExit(SystemExit):
     pass
 
 
+def _to_bytes(exc):
+    try:
+        return bytes(exc)
+    except Exception:
+        return five.text(exc).encode('UTF-8')
+
+
 def _log_and_exit(msg, exc, formatted, write_fn=sys_stdout_write_wrapper):
-    error_msg = '{0}: {1}: {2}\n'.format(msg, type(exc).__name__, exc)
+    error_msg = b''.join((
+        five.to_bytes(msg), b': ',
+        five.to_bytes(type(exc).__name__), b': ',
+        _to_bytes(exc), b'\n',
+    ))
     write_fn(error_msg)
     write_fn('Check the log at ~/.pre-commit/pre-commit.log\n')
     store = Store()
diff --git a/pre_commit/languages/node.py b/pre_commit/languages/node.py
@@ -45,13 +45,14 @@ def install_environment(
         repo_cmd_runner.run(cmd)
 
         with in_env(repo_cmd_runner, version) as node_env:
-            node_env.run("cd '{prefix}' && npm install -g")
+            node_env.run("cd '{prefix}' && npm install -g", encoding=None)
             if additional_dependencies:
                 node_env.run(
                     "cd '{prefix}' && npm install -g " +
                     ' '.join(
                         shell_escape(dep) for dep in additional_dependencies
-                    )
+                    ),
+                    encoding=None,
                 )
 
 
diff --git a/pre_commit/languages/python.py b/pre_commit/languages/python.py
@@ -63,13 +63,14 @@ def install_environment(
             venv_cmd.extend(['-p', norm_version(version)])
         repo_cmd_runner.run(venv_cmd)
         with in_env(repo_cmd_runner, version) as env:
-            env.run("cd '{prefix}' && pip install .")
+            env.run("cd '{prefix}' && pip install .", encoding=None)
             if additional_dependencies:
                 env.run(
                     "cd '{prefix}' && pip install " +
                     ' '.join(
                         shell_escape(dep) for dep in additional_dependencies
-                    )
+                    ),
+                    encoding=None,
                 )
 
 
diff --git a/pre_commit/languages/ruby.py b/pre_commit/languages/ruby.py
@@ -95,13 +95,15 @@ def install_environment(
             ruby_env.run(
                 'cd {prefix} && gem build *.gemspec && '
                 'gem install --no-ri --no-rdoc *.gem',
+                encoding=None,
             )
             if additional_dependencies:
                 ruby_env.run(
                     'cd {prefix} && gem install --no-ri --no-rdoc ' +
                     ' '.join(
                         shell_escape(dep) for dep in additional_dependencies
-                    )
+                    ),
+                    encoding=None,
                 )
 
 
diff --git a/pre_commit/util.py b/pre_commit/util.py
@@ -124,26 +124,38 @@ def __init__(self, returncode, cmd, expected_returncode, output=None):
         self.expected_returncode = expected_returncode
         self.output = output
 
-    def __str__(self):
+    def to_bytes(self):
         output = []
-        for text in self.output:
-            if text:
-                output.append('\n    ' + text.replace('\n', '\n    '))
+        for maybe_text in self.output:
+            if maybe_text:
+                output.append(
+                    b'\n    ' +
+                    five.to_bytes(maybe_text).replace(b'\n', b'\n    ')
+                )
             else:
-                output.append('(none)')
-
-        return (
-            'Command: {0!r}\n'
-            'Return code: {1}\n'
-            'Expected return code: {2}\n'
-            'Output: {3}\n'
-            'Errors: {4}\n'.format(
-                self.cmd,
-                self.returncode,
-                self.expected_returncode,
-                *output
-            )
-        )
+                output.append(b'(none)')
+
+        return b''.join((
+            five.to_bytes(
+                'Command: {0!r}\n'
+                'Return code: {1}\n'
+                'Expected return code: {2}\n'.format(
+                    self.cmd, self.returncode, self.expected_returncode
+                )
+            ),
+            b'Output: ', output[0], b'\n',
+            b'Errors: ', output[1], b'\n',
+        ))
+
+    def to_text(self):
+        return self.to_bytes().decode('UTF-8')
+
+    if five.PY3:  # pragma: no cover
+        __bytes__ = to_bytes
+        __str__ = to_text
+    else:
+        __str__ = to_bytes
+        __unicode__ = to_text
 
 
 def cmd_output(*cmd, **kwargs):
diff --git a/testing/resources/not_installable_repo/hooks.yaml b/testing/resources/not_installable_repo/hooks.yaml
@@ -0,0 +1,6 @@
+-   id: foo
+    name: Foo
+    entry: foo
+    language: python
+    language_version: python2.7
+    files: \.py$
diff --git a/testing/resources/not_installable_repo/setup.py b/testing/resources/not_installable_repo/setup.py
@@ -0,0 +1,17 @@
+# -*- coding: utf-8 -*-
+from __future__ import print_function
+from __future__ import unicode_literals
+
+import sys
+
+
+def main():
+    # Intentionally write mixed encoding to the output.  This should not crash
+    # pre-commit and should write bytes to the output.
+    sys.stderr.write('☃'.encode('UTF-8') + '²'.encode('latin1') + b'\n')
+    # Return 1 to indicate failures
+    return 1
+
+
+if __name__ == '__main__':
+    exit(main())
diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py
@@ -371,6 +371,33 @@ def test_stdout_write_bug_py26(
         assert 'UnicodeDecodeError' not in stdout
 
 
+def test_hook_install_failure(mock_out_store_directory, tempdir_factory):
+    git_path = make_consuming_repo(tempdir_factory, 'not_installable_repo')
+    with cwd(git_path):
+        install(Runner(git_path))
+
+        # Don't want to write to home directory
+        env = dict(os.environ, PRE_COMMIT_HOME=tempdir_factory.get())
+        _, stdout, _ = cmd_output(
+            'git', 'commit', '-m', 'Commit!',
+            # git commit puts pre-commit to stderr
+            stderr=subprocess.STDOUT,
+            env=env,
+            retcode=None,
+            encoding=None,
+        )
+        assert b'UnicodeDecodeError' not in stdout
+        # Doesn't actually happen, but a reasonable assertion
+        assert b'UnicodeEncodeError' not in stdout
+
+        # Sanity check our output
+        assert (
+            b'An unexpected error has occurred: CalledProcessError: ' in
+            stdout
+        )
+        assert '☃'.encode('UTF-8') + '²'.encode('latin1') in stdout
+
+
 def test_get_changed_files():
     files = get_changed_files(
         '78c682a1d13ba20e7cb735313b9314a74365cd3a',
diff --git a/tests/error_handler_test.py b/tests/error_handler_test.py
@@ -11,6 +11,7 @@
 import pytest
 
 from pre_commit import error_handler
+from pre_commit import five
 from pre_commit.errors import FatalError
 from pre_commit.util import cmd_output
 
@@ -82,7 +83,9 @@ def test_log_and_exit(mock_out_store_directory):
             write_fn=mocked_write,
         )
 
-    printed = ''.join(call[0][0] for call in mocked_write.call_args_list)
+    printed = ''.join(
+        five.to_text(call[0][0]) for call in mocked_write.call_args_list
+    )
     assert printed == (
         'msg: FatalError: hai\n'
         'Check the log at ~/.pre-commit/pre-commit.log\n'
