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

fix prepare-commit-msg on windows #737

Merged
merged 3 commits into from
Dec 13, 2020
Merged

Conversation

kirkoman
Copy link

Should fix issue #735

@kirkoman kirkoman changed the title Rest git params capture all git args with rest param syntax Jun 22, 2020
@kirkoman
Copy link
Author

Note that the fix (8f05f8a) still works even if you keep the quotes. I removed the quotes in husky.sh (as a separate commit - afac6c4) because it seemed better to have all environments uniformly using the rest param syntax.

If the husky.sh change seems to disruptive it can be dropped. For environments where the quoting is working as expected, those arguments will just come through as a single string and the join() will pass it through unmolested.

Copy link
Author

@kirkoman kirkoman left a comment

Choose a reason for hiding this comment

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

squeak

@whschultz
Copy link

This approach (hand-modified local js to match) is working for me so long as my hooks in package.json look like this:

  "husky": {
    "hooks": {
      "pre-commit": ".githooks/pre-commit.sh ${HUSKY_GIT_PARAMS[@]}",
      "post-commit": ".githooks/post-commit.sh ${HUSKY_GIT_PARAMS[@]}",
      "post-checkout": ".githooks/post-checkout.sh ${HUSKY_GIT_PARAMS[@]}",
      "post-merge": ".githooks/post-merge.sh ${HUSKY_GIT_PARAMS[@]}"
    }
  }

@kirkoman
Copy link
Author

Interesting, I am not finding the array expansion syntax to be necessary.

Did you include the .join(' ') change on line 87 when making your local js edits? This should be serializing the array to a string.

@kirkoman
Copy link
Author

@whschultz I've added some tests that contradict your results. The shell syntax for array-parameter expansion should not be needed. Let me know if I missed something though.

The tests also illustrate that the old behavior was wrong for multi-parameter hooks (viz. prepare-commit-msg) as they do fail without the fix.

Hoping a maintainer will weigh in some week soon...

@kirkoman
Copy link
Author

@typicode got any feels on a husky release any time soon?

@steelbrain
Copy link

Just got bit by this. Applied the patch and it worked well. Can a maintainer please have a look at this?

@rikoe
Copy link

rikoe commented Nov 18, 2020

I can confirm that this change does indeed magically fix issue #735 with git-commit-template. I have been struggling for a while with this issue, thinking there was something wrong with git-commit-template, but the issue is that all the git params aren't being passed through correctly by husky.

Is there any plan to approve and merge this any time soon?

@kirkoman is there a workaround for getting git-commit-template to work without this change? I can't really use it as is at the moment...

@kirkoman
Copy link
Author

kirkoman commented Dec 1, 2020

@rikoe I wasn't aware of that tool until you referenced it. Taking a closer look yeah I would not expect it to function properly on Windows based on #735 and the need for this PR.

What I do is to apply a this patch locally when I want to work around this issue. Ultimately it seems related to a platform difference in how shell is parsing arguments so you might find a workaround by playing around with the quotation-escaping on line 17 of husky.sh (but I lost patience looking for a solution with that approach).

  "$@" husky-run $hookName "$gitParams"

@kirkoman kirkoman changed the title capture all git args with rest param syntax fix prepare-commit-msg on windows Dec 11, 2020
@kirkoman
Copy link
Author

bump

Updated title to reflect that this is a bug fix.

Just realized from the tag history that v5 is in progress, and it seems to take a very different approach to managing the hooks.

@typicode this is still an ongoing nuisance for myself and presumably the others in the thread. Hoping to get this PR noticed and merged (or at least closed with some feedback).

@typicode
Copy link
Owner

Sorry for the late reply. Out of the curiosity, I added your test to master and it seems like the current version is correctly working (https://travis-ci.org/github/typicode/husky/jobs/749056907 search for sixth tests).

So I guess, it doesn't need to be merged?

PS: thanks for improving the tests

@kirkoman
Copy link
Author

Cool, thanks for the response. It's a Windows-only bug so the test only fails on Windows. I just ran them locally on the tip of master again to confirm, this is what it looks like for me:

---
hook should run and pass all HUSKY_GIT_PARAMS
---

husky:debug husky v4.3.5 - pre-commit
husky:debug Current working directory is /tmp/husky-project
husky:debug pre-commit config not found, skipping hook
husky:debug husky v4.3.5 - prepare-commit-msg
husky:debug Current working directory is /tmp/husky-project
husky > prepare-commit-msg (node v12.13.1)
prepare-commit-msg hook from Husky
husky:debug npx --no-install husky-run exited with 0 exit code
husky:debug husky v4.3.5 - commit-msg
husky:debug Current working directory is /tmp/husky-project
husky:debug npx --no-install husky-run exited with 0 exit code
husky:debug husky v4.3.5 - post-commit
husky:debug Current working directory is /tmp/husky-project
husky:debug post-commit config not found, skipping hook
[master 1d3c28e] sixth msg
 3 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 fifth
 create mode 100644 fourth
 create mode 100644 sixth
Fail: HUSKY_GIT_PARAMS weren't set correctly
.git/COMMIT_EDITMSG != .git/COMMIT_EDITMSG message

C:\Users\kirkoman\Source\husky>

I think it is a difference in how the shell escaping or argument parsing works on Windows. The gitParams are wrapped in quotes in the husky.sh which seems to encode all the params as a single string - on all other platforms besides Windows :).

@kirkoman
Copy link
Author

rebased to fix merge conflict and correct my commiter info

@kirkoman
Copy link
Author

@typicode note that your appveyor windows build did fail that test: https://ci.appveyor.com/project/typicode/husky/builds/36785480

@typicode
Copy link
Owner

I see, thanks for the details. I'll have another look.

@typicode
Copy link
Owner

Just gave it a try on Windows, really weird... thanks for catching it and finding a fix! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants