From 9245e07a3e4c0c32489075474ff66ebb6bb90c72 Mon Sep 17 00:00:00 2001 From: Kurt von Laven Date: Wed, 27 Apr 2022 16:57:53 -0700 Subject: [PATCH 1/2] Fix asymptomatic copy/paste bug in test. check_shebang_scripts_are_executable_test.test_git_executable_shebang manually filtered executable files out before calling check_shebang_scripts_are_executable. This makes sense in check_executables_have_shebangs.test_git_executable_shebang, because the check-executables-have-shebangs hook only runs on executable files. However, check-shebang-scripts-are-executable correctly runs on all text files, so the test shouldn't filter executable files out. The test still passed because when git ls-files is passed no files in particular, it lists all files in the Git repository that satisfy the given filters. --- tests/check_shebang_scripts_are_executable_test.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/check_shebang_scripts_are_executable_test.py b/tests/check_shebang_scripts_are_executable_test.py index e4bd07ca..7c2696dd 100644 --- a/tests/check_shebang_scripts_are_executable_test.py +++ b/tests/check_shebang_scripts_are_executable_test.py @@ -1,7 +1,5 @@ from __future__ import annotations -import os - import pytest from pre_commit_hooks.check_shebang_scripts_are_executable import \ @@ -83,7 +81,5 @@ def test_git_executable_shebang(temp_git_dir, content, mode, expected): cmd_output('chmod', mode, str(path)) cmd_output('git', 'update-index', f'--chmod={mode}', str(path)) - # simulate how identify chooses that something is executable - filenames = [path for path in [str(path)] if os.access(path, os.X_OK)] - - assert main(filenames) == expected + files = (str(path),) + assert main(files) == expected From 314fa5366d88305a4b23b10a97c56d9247e1f806 Mon Sep 17 00:00:00 2001 From: Kurt von Laven Date: Sun, 10 Apr 2022 12:52:36 -0700 Subject: [PATCH 2/2] Support checking executable bit without Git. The check-shebang-scripts-are-executable hook already avoided false negatives in a Git repository by looking up the Git file mode rather than relying on the file mode in the file system. Git already automatically probes the file system for executable bit support. Use the file mode in the file system when we are not in a Git clone or it is trusted by Git according to its core.fileMode config variable. --- .../check_shebang_scripts_are_executable.py | 26 +++++++-- ...eck_shebang_scripts_are_executable_test.py | 58 +++++++++++++++++++ 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/pre_commit_hooks/check_shebang_scripts_are_executable.py b/pre_commit_hooks/check_shebang_scripts_are_executable.py index 621696ce..0436df0c 100644 --- a/pre_commit_hooks/check_shebang_scripts_are_executable.py +++ b/pre_commit_hooks/check_shebang_scripts_are_executable.py @@ -2,6 +2,7 @@ from __future__ import annotations import argparse +import os import shlex import sys from typing import Sequence @@ -9,13 +10,30 @@ from pre_commit_hooks.check_executables_have_shebangs import EXECUTABLE_VALUES from pre_commit_hooks.check_executables_have_shebangs import git_ls_files from pre_commit_hooks.check_executables_have_shebangs import has_shebang +from pre_commit_hooks.util import cmd_output def check_shebangs(paths: list[str]) -> int: - # Cannot optimize on non-executability here if we intend this check to - # work on win32 -- and that's where problems caused by non-executability - # (elsewhere) are most likely to arise from. - return _check_git_filemode(paths) + fs_tracks_executable_bit = cmd_output( + 'git', 'config', 'core.fileMode', retcode=None, + ).strip() + return ( + _check_git_filemode(paths) + if fs_tracks_executable_bit == 'false' + else _check_fs_filemode(paths) + ) + + +def _check_fs_filemode( + paths: list[str], +) -> int: # pragma: win32 no cover + retv = 0 + for path in paths: + if not os.access(path, os.X_OK) and has_shebang(path): + _message(path) + retv = 1 + + return retv def _check_git_filemode(paths: Sequence[str]) -> int: diff --git a/tests/check_shebang_scripts_are_executable_test.py b/tests/check_shebang_scripts_are_executable_test.py index 7c2696dd..9c204fbb 100644 --- a/tests/check_shebang_scripts_are_executable_test.py +++ b/tests/check_shebang_scripts_are_executable_test.py @@ -1,5 +1,7 @@ from __future__ import annotations +import sys + import pytest from pre_commit_hooks.check_shebang_scripts_are_executable import \ @@ -7,6 +9,62 @@ from pre_commit_hooks.check_shebang_scripts_are_executable import main from pre_commit_hooks.util import cmd_output +skip_win32 = pytest.mark.xfail( + sys.platform == 'win32', + reason="non-git checks aren't relevant on windows", +) + + +@skip_win32 # pragma: win32 no cover +@pytest.mark.parametrize( + 'content', ( + b'#!/bin/bash\nhello world\n', + b'#!/usr/bin/env python3.10', + b'#!python', + '#!☃'.encode(), + ), +) +def test_executable_shebang(content, tmpdir): + path = tmpdir.join('path') + path.write(content, 'wb') + cmd_output('chmod', '+x', path) + assert main((str(path),)) == 0 + + +@skip_win32 # pragma: win32 no cover +@pytest.mark.parametrize( + 'content', ( + b'#!/bin/bash\nhello world\n', + b'#!/usr/bin/env python3.10', + b'#!python', + '#!☃'.encode(), + ), +) +def test_not_executable_shebang(content, tmpdir, capsys): + path = tmpdir.join('path') + path.write(content, 'wb') + assert main((str(path),)) == 1 + _, stderr = capsys.readouterr() + assert stderr.startswith( + f'{path}: has a shebang but is not marked executable!', + ) + + +@skip_win32 # pragma: win32 no cover +@pytest.mark.parametrize( + 'content', ( + b'', + b' #!python\n', + b'\n#!python\n', + b'python\n', + '☃'.encode(), + ), +) +def test_not_executable_no_shebang(content, tmpdir, capsys): + path = tmpdir.join('path') + path.write(content, 'wb') + assert main((str(path),)) == 0 + def test_check_git_filemode_passing(tmpdir): with tmpdir.as_cwd():