diff --git a/pre_commit/color.py b/pre_commit/color.py index 1fb6acce..7a138f47 100644 --- a/pre_commit/color.py +++ b/pre_commit/color.py @@ -15,6 +15,7 @@ RED = '\033[41m' 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 index f5a5b1e6..4ea55ffc 100644 --- a/pre_commit/commands/run.py +++ b/pre_commit/commands/run.py @@ -4,7 +4,6 @@ import logging 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() - - 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) - - file_modifications = diff_before != diff_after - - # If the hook makes changes, fail the commit - if file_modifications: - retcode = 1 - - if retcode: - retcode = 1 - print_color = color.RED - pass_fail = 'Failed' - else: retcode = 0 - print_color = color.GREEN - pass_fail = 'Passed' + 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)) - output.write_line(color.format_color(pass_fail, print_color, args.color)) + 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) - if ( - (out or file_modifications) and - (retcode or args.verbose or hook.verbose) - ): - output.write_line('hookid: {}\n'.format(hook.id)) + # if the hook makes changes, fail the commit + files_modified = diff_before != diff_after + + if retcode or files_modified: + print_color = color.RED + status = 'Failed' + else: + print_color = color.GREEN + status = 'Passed' + + output.write_line(color.format_color(status, print_color, use_color)) + + if verbose or hook.verbose or retcode or files_modified: + _subtle_line('- hook id: {}'.format(hook.id), use_color) + + 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 index f48f3136..5e405903 100644 --- a/pre_commit/xargs.py +++ b/pre_commit/xargs.py @@ -135,17 +135,9 @@ def xargs(cmd, varargs, **kwargs): 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 index 52f6e4e5..28bf66d1 100644 --- 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 index 63d09254..4c75e62a 100644 --- 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 index d9a0401a..6e9db9db 100644 --- 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 index 5f2ed1cb..8f001384 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -221,7 +221,7 @@ def test_run_a_failing_docker_hook(tempdir_factory, store): 'docker-hook-failing', ['Hello World from docker'], mock.ANY, # an error message about `bork` not existing - expected_return_code=1, + expected_return_code=127, ) diff --git a/tests/xargs_test.py b/tests/xargs_test.py index a6772804..65b1d495 100644 --- a/tests/xargs_test.py +++ b/tests/xargs_test.py @@ -178,6 +178,10 @@ def test_xargs_retcode_normal(): ret, _ = xargs.xargs(exit_cmd, ('0', '1'), _max_length=max_length) assert ret == 1 + # takes the maximum return code + ret, _ = xargs.xargs(exit_cmd, ('0', '5', '1'), _max_length=max_length) + assert ret == 5 + def test_xargs_concurrency(): bash_cmd = parse_shebang.normalize_cmd(('bash', '-c'))