Support checking executable bit without Git. #750
Conversation
| @@ -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.skipif( | |||
There was a problem hiding this comment.
these should be xfail if anything -- then you shouldn't need the no cover
There was a problem hiding this comment.
Neat function. I tried removing the no cover in the hook and the no covers in the test and got test coverage failures though. Did I miss something here?
| # simulate how identify chooses that something is executable | ||
| filenames = [path for path in [str(path)] if os.access(path, os.X_OK)] |
There was a problem hiding this comment.
this bit was important to demonstrate the windows behaviour
There was a problem hiding this comment.
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)].
| return _check_git_filemode(paths) \ | ||
| if fs_tracks_executable_bit == 'false' \ | ||
| else _check_fs_filemode(paths) |
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.
| # simulate how identify chooses that something is executable | ||
| filenames = [path for path in [str(path)] if os.access(path, os.X_OK)] |
There was a problem hiding this comment.
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)].
| return _check_git_filemode(paths) \ | ||
| if fs_tracks_executable_bit == 'false' \ | ||
| else _check_fs_filemode(paths) |
| @@ -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.skipif( | |||
There was a problem hiding this comment.
Neat function. I tried removing the no cover in the hook and the no covers in the test and got test coverage failures though. Did I miss something here?
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.
|
@asotille, please let me know if there's anything I should change about this pull request. |
The
check-shebang-scripts-are-executablehook 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 itscore.fileModeconfig variable.Also, fix previously asymptomatic copy/paste bug in
check_shebang_scripts_are_executable_test.test_git_executable_shebangexposed by the other commit. This test manually filtered executable files out before callingcheck_shebang_scripts_are_executable. This makes sense in the test it was copied from,check_executables_have_shebangs.test_git_executable_shebang, because thecheck-executables-have-shebangshook only runs on executable files. However,check-shebang-scripts-are-executablecorrectly runs on all text files, so the test shouldn't filter executable files out. The test still passed because whengit ls-filesis passed no files in particular, itlists all files in the Git repository that satisfy the given filters.
Closes #749.