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

Suggest usable call when executable files lack shebang on Windows #689

Merged
merged 1 commit into from Dec 7, 2021

Conversation

pujitm
Copy link
Contributor

@pujitm pujitm commented Nov 19, 2021

Resolves #686

Some of the executable error messages suggest using chmod +/-x to toggle the executable bits. This works fine on *nix, but not on windows, and the results range from 'command not found' to doing nothing at all.

Eg:

f'`chmod -x {shlex.quote(path)}`\n'

Instead, this should recommend git add --chmod=+/-x somepath when the hook detects it's running on windows.

@pujitm
Copy link
Contributor Author

pujitm commented Nov 19, 2021

I also noticed that platform checks occur in other functions, like the check_executables function:

if sys.platform == 'win32': # pragma: win32 cover

It might be helpful to refactor those into a consistent call for platform/os checks.

@Kurt-von-Laven

This comment has been minimized.

@asottile

This comment has been minimized.

Comment on lines 66 to 67
f'`chmod -x {shlex.quote(path)}`\n'
f'`{"git add --chmod=-x" if is_windows else "chmod -x"} {shlex.quote(path)}`\n'
Copy link
Member

Choose a reason for hiding this comment

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

ah I mean, without having any branching logic just write:

"chmod -x ..."
" if on windows you may also need to ..."

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

@asottile asottile merged commit ccecdbd into pre-commit:master Dec 7, 2021
jmerdich added a commit to jmerdich/pre-commit-hooks that referenced this pull request Dec 13, 2021
Follow-up for pre-commit#686 and pre-commit#689.

The previous change resolved when a file was executable but lacked
a shebang, but not vice-versa.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Don't suggest chmod +/-x on Windows
3 participants