-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix(shell): double quote params #824
Conversation
Thanks for the PR. |
Any application that parses the git hooks directory itself without respecting a custom directory set through git config, like GitKraken for example. I suggested respecting the config setting to them earlier today, but until then the many users of GitKraken won't see husky v5 hooks executed as long as they don't add symlinks to the default git hook directory. |
Why is the CI failing btw? :0 |
I resolved the merge conflicts. @typicode what's your status on this PR? :) |
Hmm, I'd prefer to keep the script as it is and not rely on However, if users want to edit their As for the rest LGTM, good catch on the |
src/commands/add.ts
Outdated
@@ -15,7 +15,12 @@ function createHookFile(file: string, cmd: string) { | |||
throw new Error(`${file} already exists`) | |||
} | |||
|
|||
const data = ['#!/bin/sh', '. "$(dirname $0)/_/husky.sh"', '', cmd].join('\n') | |||
const data = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const data = [ | |
const data = ['#!/bin/sh', '. "$(dirname "$0")/_/husky.sh"', '', cmd].join('\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, except for the additional indentation (?) :)
@@ -1,4 +1,4 @@ | |||
. $(dirname $0)/_functions.sh | |||
. $(dirname "$0")/_functions.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 could you change test/config-dir.sh
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/default.sh
Outdated
@@ -1,4 +1,7 @@ | |||
. $(dirname $0)/_functions.sh | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the shebang can be removed since these scripts are not executable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced that with the shellcheck comment.
I don't really see it as a fix for external tools but as broadened functionality for husky even if the "fix" happened to be the reason for this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! :)
I've set
test/default.sh
's shebang tobash
becauseecho -e
is used in this file.