Make verbose output less special

asottile · asottile · commit 01a628d96d18 · 2019-12-23T15:51:58.000-08:00
diff --git a/pre_commit/color.py b/pre_commit/color.py
@@ -15,6 +15,7 @@
 GREEN = '\033[42m'
 YELLOW = '\033[43;30m'
 TURQUOISE = '\033[46;30m'
+SUBTLE = '\033[2m'
 NORMAL = '\033[0m'
 
 
diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py
@@ -4,7 +4,6 @@
 import os
 import re
 import subprocess
-import sys
 
 from identify.identify import tags_from_path
 
@@ -71,15 +70,15 @@ def _get_skips(environ):
     return {skip.strip() for skip in skips.split(',') if skip.strip()}
 
 
-def _hook_msg_start(hook, verbose):
-    return '{}{}'.format('[{}] '.format(hook.id) if verbose else '', hook.name)
-
-
 SKIPPED = 'Skipped'
 NO_FILES = '(no files to check)'
 
 
-def _run_single_hook(classifier, hook, args, skips, cols, use_color):
+def _subtle_line(s, use_color):
+    output.write_line(color.format_color(s, color.SUBTLE, use_color))
+
+
+def _run_single_hook(classifier, hook, skips, cols, verbose, use_color):
     filenames = classifier.filenames_for_hook(hook)
 
     if hook.language == 'pcre':
@@ -93,92 +92,78 @@ def _run_single_hook(classifier, hook, args, skips, cols, use_color):
     if hook.id in skips or hook.alias in skips:
         output.write(
             get_hook_message(
-                _hook_msg_start(hook, args.verbose),
+                hook.name,
                 end_msg=SKIPPED,
                 end_color=color.YELLOW,
-                use_color=args.color,
+                use_color=use_color,
                 cols=cols,
             ),
         )
-        return 0
+        retcode = 0
+        files_modified = False
+        out = b''
     elif not filenames and not hook.always_run:
         output.write(
             get_hook_message(
-                _hook_msg_start(hook, args.verbose),
+                hook.name,
                 postfix=NO_FILES,
                 end_msg=SKIPPED,
                 end_color=color.TURQUOISE,
-                use_color=args.color,
+                use_color=use_color,
                 cols=cols,
             ),
         )
-        return 0
-
-    # Print the hook and the dots first in case the hook takes hella long to
-    # run.
-    output.write(
-        get_hook_message(
-            _hook_msg_start(hook, args.verbose), end_len=6, cols=cols,
-        ),
-    )
-    sys.stdout.flush()
+        retcode = 0
+        files_modified = False
+        out = b''
+    else:
+        # print hook and dots first in case the hook takes a while to run
+        output.write(get_hook_message(hook.name, end_len=6, cols=cols))
 
-    diff_before = cmd_output_b('git', 'diff', '--no-ext-diff', retcode=None)
-    filenames = tuple(filenames) if hook.pass_filenames else ()
-    retcode, out = hook.run(filenames, use_color)
-    diff_after = cmd_output_b('git', 'diff', '--no-ext-diff', retcode=None)
+        diff_cmd = ('git', 'diff', '--no-ext-diff')
+        diff_before = cmd_output_b(*diff_cmd, retcode=None)
+        filenames = tuple(filenames) if hook.pass_filenames else ()
+        retcode, out = hook.run(filenames, use_color)
+        diff_after = cmd_output_b(*diff_cmd, retcode=None)
 
-    file_modifications = diff_before != diff_after
+        # if the hook makes changes, fail the commit
+        files_modified = diff_before != diff_after
 
-    # If the hook makes changes, fail the commit
-    if file_modifications:
-        retcode = 1
+        if retcode or files_modified:
+            print_color = color.RED
+            status = 'Failed'
+        else:
+            print_color = color.GREEN
+            status = 'Passed'
 
-    if retcode:
-        retcode = 1
-        print_color = color.RED
-        pass_fail = 'Failed'
-    else:
-        retcode = 0
-        print_color = color.GREEN
-        pass_fail = 'Passed'
+        output.write_line(color.format_color(status, print_color, use_color))
 
-    output.write_line(color.format_color(pass_fail, print_color, args.color))
+    if verbose or hook.verbose or retcode or files_modified:
+        _subtle_line('- hook id: {}'.format(hook.id), use_color)
 
-    if (
-            (out or file_modifications) and
-            (retcode or args.verbose or hook.verbose)
-    ):
-        output.write_line('hookid: {}\n'.format(hook.id))
+        if retcode:
+            _subtle_line('- exit code: {}'.format(retcode), use_color)
 
         # Print a message if failing due to file modifications
-        if file_modifications:
-            output.write('Files were modified by this hook.')
-
-            if out:
-                output.write_line(' Additional output:')
-
-            output.write_line()
+        if files_modified:
+            _subtle_line('- files were modified by this hook', use_color)
 
         if out.strip():
+            output.write_line()
             output.write_line(out.strip(), logfile_name=hook.log_file)
-        output.write_line()
+            output.write_line()
 
-    return retcode
+    return files_modified or bool(retcode)
 
 
-def _compute_cols(hooks, verbose):
+def _compute_cols(hooks):
     """Compute the number of columns to display hook messages.  The widest
     that will be displayed is in the no files skipped case:
 
         Hook name...(no files to check) Skipped
-
-    or in the verbose case
-
-        Hook name [hookid]...(no files to check) Skipped
     """
     if hooks:
-        name_len = max(len(_hook_msg_start(hook, verbose)) for hook in hooks)
+        name_len = max(len(hook.name) for hook in hooks)
     else:
         name_len = 0
 
@@ -204,7 +189,7 @@ def _all_filenames(args):
 def _run_hooks(config, hooks, args, environ):
     """Actually run the hooks."""
     skips = _get_skips(environ)
-    cols = _compute_cols(hooks, args.verbose)
+    cols = _compute_cols(hooks)
     filenames = _all_filenames(args)
     filenames = filter_by_include_exclude(
         filenames, config['files'], config['exclude'],
@@ -213,7 +198,8 @@ def _run_hooks(config, hooks, args, environ):
     retval = 0
     for hook in hooks:
         retval |= _run_single_hook(
-            classifier, hook, args, skips, cols, args.color,
+            classifier, hook, skips, cols,
+            verbose=args.verbose, use_color=args.color,
         )
         if retval and config['fail_fast']:
             break
diff --git a/pre_commit/xargs.py b/pre_commit/xargs.py
@@ -135,17 +135,9 @@ def run_cmd_partition(run_cmd):
         results = thread_map(run_cmd_partition, partitions)
 
         for proc_retcode, proc_out, _ in results:
-            # This is *slightly* too clever so I'll explain it.
-            # First the xor boolean table:
-            #     T | F |
-            #   +-------+
-            # T | F | T |
-            # --+-------+
-            # F | T | F |
-            # --+-------+
-            # When negate is True, it has the effect of flipping the return
-            # code. Otherwise, the returncode is unchanged.
-            retcode |= bool(proc_retcode) ^ negate
+            if negate:
+                proc_retcode = not proc_retcode
+            retcode = max(retcode, proc_retcode)
             stdout += proc_out
 
     return retcode, stdout
diff --git a/tests/commands/install_uninstall_test.py b/tests/commands/install_uninstall_test.py
@@ -288,7 +288,8 @@ def test_environment_not_sourced(tempdir_factory, store):
 FAILING_PRE_COMMIT_RUN = re.compile(
     r'^\[INFO\] Initializing environment for .+\.\r?\n'
     r'Failing hook\.+Failed\r?\n'
-    r'hookid: failing_hook\r?\n'
+    r'- hook id: failing_hook\r?\n'
+    r'- exit code: 1\r?\n'
     r'\r?\n'
     r'Fail\r?\n'
     r'foo\r?\n'
@@ -548,7 +549,7 @@ def test_pre_push_integration_failing(tempdir_factory, store):
         assert 'Failing hook' in output
         assert 'Failed' in output
         assert 'foo zzz' in output  # both filenames should be printed
-        assert 'hookid: failing_hook' in output
+        assert 'hook id: failing_hook' in output
 
 
 def test_pre_push_integration_accepted(tempdir_factory, store):
@@ -647,8 +648,11 @@ def test_commit_msg_integration_failing(
     install(C.CONFIG_FILE, store, hook_types=['commit-msg'])
     retc, out = _get_commit_output(tempdir_factory)
     assert retc == 1
-    assert out.startswith('Must have "Signed off by:"...')
-    assert out.strip().endswith('...Failed')
+    assert out.replace('\r', '') == '''\
+Must have "Signed off by:"...............................................Failed
+- hook id: must-have-signoff
+- exit code: 1
+'''
 
 
 def test_commit_msg_integration_passing(
@@ -691,16 +695,18 @@ def test_prepare_commit_msg_integration_failing(
     install(C.CONFIG_FILE, store, hook_types=['prepare-commit-msg'])
     retc, out = _get_commit_output(tempdir_factory)
     assert retc == 1
-    assert out.startswith('Add "Signed off by:"...')
-    assert out.strip().endswith('...Failed')
+    assert out.replace('\r', '') == '''\
+Add "Signed off by:".....................................................Failed
+- hook id: add-signoff
+- exit code: 1
+'''
 
 
 def test_prepare_commit_msg_integration_passing(
         prepare_commit_msg_repo, tempdir_factory, store,
 ):
     install(C.CONFIG_FILE, store, hook_types=['prepare-commit-msg'])
-    msg = 'Hi'
-    retc, out = _get_commit_output(tempdir_factory, msg=msg)
+    retc, out = _get_commit_output(tempdir_factory, msg='Hi')
     assert retc == 0
     first_line = out.splitlines()[0]
     assert first_line.startswith('Add "Signed off by:"...')
@@ -730,8 +736,7 @@ def test_prepare_commit_msg_legacy(
 
     install(C.CONFIG_FILE, store, hook_types=['prepare-commit-msg'])
 
-    msg = 'Hi'
-    retc, out = _get_commit_output(tempdir_factory, msg=msg)
+    retc, out = _get_commit_output(tempdir_factory, msg='Hi')
     assert retc == 0
     first_line, second_line = out.splitlines()[:2]
     assert first_line == 'legacy'
diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py
@@ -94,7 +94,7 @@ def test_run_all_hooks_failing(cap_out, store, repo_with_failing_hook):
         (
             b'Failing hook',
             b'Failed',
-            b'hookid: failing_hook',
+            b'hook id: failing_hook',
             b'Fail\nfoo.py\n',
         ),
         expected_ret=1,
@@ -125,14 +125,14 @@ def test_hook_that_modifies_but_returns_zero(cap_out, store, tempdir_factory):
                 # The first should fail
                 b'Failed',
                 # With a modified file (default message + the hook's output)
-                b'Files were modified by this hook. Additional output:\n\n'
+                b'- files were modified by this hook\n\n'
                 b'Modified: foo.py',
                 # The next hook should pass despite the first modifying
                 b'Passed',
                 # The next hook should fail
                 b'Failed',
                 # bar.py was modified, but provides no additional output
-                b'Files were modified by this hook.\n',
+                b'- files were modified by this hook\n',
             ),
             1,
             True,
@@ -176,7 +176,7 @@ def test_global_exclude(cap_out, store, tempdir_factory):
         ret, printed = _do_run(cap_out, store, git_path, opts)
         assert ret == 0
         # Does not contain foo.py since it was excluded
-        expected = b'hookid: bash_hook\n\nbar.py\nHello World\n\n'
+        expected = b'- hook id: bash_hook\n\nbar.py\nHello World\n\n'
         assert printed.endswith(expected)
 
 
@@ -192,7 +192,7 @@ def test_global_files(cap_out, store, tempdir_factory):
         ret, printed = _do_run(cap_out, store, git_path, opts)
         assert ret == 0
         # Does not contain foo.py since it was not included
-        expected = b'hookid: bash_hook\n\nbar.py\nHello World\n\n'
+        expected = b'- hook id: bash_hook\n\nbar.py\nHello World\n\n'
         assert printed.endswith(expected)
 
 
@@ -422,23 +422,21 @@ def test_merge_conflict_resolved(cap_out, store, in_merge_conflict):
 
 
 @pytest.mark.parametrize(
-    ('hooks', 'verbose', 'expected'),
+    ('hooks', 'expected'),
     (
-        ([], True, 80),
-        ([auto_namedtuple(id='a', name='a' * 51)], False, 81),
-        ([auto_namedtuple(id='a', name='a' * 51)], True, 85),
+        ([], 80),
+        ([auto_namedtuple(id='a', name='a' * 51)], 81),
         (
             [
                 auto_namedtuple(id='a', name='a' * 51),
                 auto_namedtuple(id='b', name='b' * 52),
             ],
-            False,
             82,
         ),
     ),
 )
-def test_compute_cols(hooks, verbose, expected):
-    assert _compute_cols(hooks, verbose) == expected
+def test_compute_cols(hooks, expected):
+    assert _compute_cols(hooks) == expected
 
 
 @pytest.mark.parametrize(
@@ -492,7 +490,7 @@ def test_hook_id_in_verbose_output(cap_out, store, repo_with_passing_hook):
     ret, printed = _do_run(
         cap_out, store, repo_with_passing_hook, run_opts(verbose=True),
     )
-    assert b'[bash_hook] Bash hook' in printed
+    assert b'- hook id: bash_hook' in printed
 
 
 def test_multiple_hooks_same_id(cap_out, store, repo_with_passing_hook):
diff --git a/tests/commands/try_repo_test.py b/tests/commands/try_repo_test.py
@@ -54,15 +54,17 @@ def test_try_repo_repo_only(cap_out, tempdir_factory):
         '    -   id: bash_hook3\n$',
         config,
     )
-    assert rest == (
-        '[bash_hook] Bash hook................................(no files to check)Skipped\n'  # noqa: E501
-        '[bash_hook2] Bash hook...................................................Passed\n'  # noqa: E501
-        'hookid: bash_hook2\n'
-        '\n'
-        'test-file\n'
-        '\n'
-        '[bash_hook3] Bash hook...............................(no files to check)Skipped\n'  # noqa: E501
-    )
+    assert rest == '''\
+Bash hook............................................(no files to check)Skipped
+- hook id: bash_hook
+Bash hook................................................................Passed
+- hook id: bash_hook2
+
+test-file
+
+Bash hook............................................(no files to check)Skipped
+- hook id: bash_hook3
+'''
 
 
 def test_try_repo_with_specific_hook(cap_out, tempdir_factory):
@@ -77,7 +79,10 @@ def test_try_repo_with_specific_hook(cap_out, tempdir_factory):
         '    -   id: bash_hook\n$',
         config,
     )
-    assert rest == '[bash_hook] Bash hook................................(no files to check)Skipped\n'  # noqa: E501
+    assert rest == '''\
+Bash hook............................................(no files to check)Skipped
+- hook id: bash_hook
+'''
 
 
 def test_try_repo_relative_path(cap_out, tempdir_factory):
diff --git a/tests/repository_test.py b/tests/repository_test.py
diff --git a/tests/xargs_test.py b/tests/xargs_test.py
