Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support checking executable bit without Git. #750

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions pre_commit_hooks/check_shebang_scripts_are_executable.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,38 @@
from __future__ import annotations

import argparse
import os
import shlex
import sys
from typing import Sequence

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:
Expand Down
64 changes: 59 additions & 5 deletions tests/check_shebang_scripts_are_executable_test.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from __future__ import annotations

import os
import sys

import pytest

Expand All @@ -9,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():
Expand Down Expand Up @@ -83,7 +139,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)]
Comment on lines -86 to -87
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this bit was important to demonstrate the windows behaviour

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purely considering the -x case, I don't understand how assert main([]) == expected demonstrates behavior on Windows as opposed to demonstrating behavior when running the hook with pass_filenames: false. check-shebang-scripts-are-executable parses the mode from the output of git ls-files -z --stage -- in this case, which is why it still sees the one file. Maybe a more convincing test of behavior on Windows would be to set the filesystem mode to +x and the Git mode to -x to prove that it's reading from the latter. I believe the fact that your test matrix includes Windows already achieves the same effect though. In the +x case, the line above simplifies to filenames = [str(path)].


assert main(filenames) == expected
files = (str(path),)
assert main(files) == expected