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

solves the windows issue (with symlinks) #84

Merged
merged 2 commits into from
Nov 27, 2016

Conversation

barroudjo
Copy link
Contributor

@barroudjo barroudjo commented Nov 8, 2016

Instead of symlinking to the hook in node-modules/pre-commit, this injects a new pre-commit hook (in .git/hooks) that takes care of launching the hook in node-modules/pre-commit.
This avoids the whole hassle with symlink creation in windows, which up to now prevented using this module when users could not gain elevated privileges (very frequent in enterprise environments).

Instead of symlinking to the hook in node-modules/pre-commit, this injects a new pre-commit hook (in .git/hooks) that takes care of launching the hook in node-modules/pre-commit. 
In addition it stashes the unstaged changes and applies them again after running the pre-commit hook, so that you only run your pre-commit scripts on the commit actual codebase.
@coveralls
Copy link

coveralls commented Nov 8, 2016

Coverage Status

Coverage remained the same at 95.413% when pulling 0d7f290 on barroudjo:solve_windows_issue into 7154dee on observing:master.

@alallier
Copy link

alallier commented Nov 9, 2016

How does the pre-commit file update on changes made inside of package.json?

@barroudjo
Copy link
Contributor Author

It doesn't update the pre-commit file, and it didn't either before this pull request. In both cases the pre-commit hook (directly before, indirectly with this pull request) launches the JS script from this module, which itself launches the scripts defined in the "pre-commit" section of your package.json.

@alallier
Copy link

alallier commented Nov 9, 2016

Duh yeah okay makes plenty of sense, thanks for the clarification.

Stashing automatically is actually a bad idea, it's better to for the user to have control over doing it or not (manually then).
@coveralls
Copy link

coveralls commented Nov 10, 2016

Coverage Status

Coverage remained the same at 95.413% when pulling 1a662e6 on barroudjo:solve_windows_issue into 7154dee on observing:master.

@barroudjo
Copy link
Contributor Author

Can the committers please take a look ? This will make life easier for a lot of people.

@okeydoke
Copy link

👍 Could really do with a working solution on windows

@ZenGuLhe
Copy link

I very much need this as well, could this be looked at ?

@3rd-Eden 3rd-Eden merged commit a9c9732 into observing:master Nov 27, 2016
@3rd-Eden
Copy link
Member

It's in master, can other windows users confirm that it's working for them? Once I get a few +1's i'll release it.

@okeydoke
Copy link

Hey sorry it took so long to get back to you. Just tested master on windows and its working for me now!

@okeydoke
Copy link

okeydoke commented Dec 6, 2016

@3rd-Eden Any idea when release might be?

@bryclee
Copy link

bryclee commented Dec 9, 2016

On my mac, it looks like the pre-commit file generated is not executable, so the hook isn't firing. Anyone else have this issue? Maybe need to specify mode in options here? https://github.com/observing/pre-commit/pull/84/files#diff-f16acefe4b6553580c43edab685f50f3R68

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.

7 participants