Allow execution on Windows

artptr commented 7 years ago

Original pull request is https://github.com/tarmolov/git-hooks-js/pull/29 by @codebling Just resolved a merge conflict.

coveralls commented 7 years ago

Coverage decreased (-8.6%) to 86.614% when pulling 7c8a4b5631c824adadba4b89f7436b0f164bf52f on artptr:master into 7a79c8cc497ac697e5966df4c7b8b4fb9271d495 on tarmolov:master.

coveralls commented 7 years ago

Coverage decreased (-8.6%) to 86.614% when pulling 7c8a4b5631c824adadba4b89f7436b0f164bf52f on artptr:master into 7a79c8cc497ac697e5966df4c7b8b4fb9271d495 on tarmolov:master.

coveralls commented 7 years ago

Coverage decreased (-8.6%) to 86.614% when pulling 7c8a4b5631c824adadba4b89f7436b0f164bf52f on artptr:master into 7a79c8cc497ac697e5966df4c7b8b4fb9271d495 on tarmolov:master.

coveralls commented 7 years ago

Coverage decreased (-8.6%) to 86.614% when pulling 7c8a4b5631c824adadba4b89f7436b0f164bf52f on artptr:master into 7a79c8cc497ac697e5966df4c7b8b4fb9271d495 on tarmolov:master.

coveralls commented 7 years ago

Coverage decreased (-8.6%) to 86.614% when pulling 7c8a4b5631c824adadba4b89f7436b0f164bf52f on artptr:master into 7a79c8cc497ac697e5966df4c7b8b4fb9271d495 on tarmolov:master.

codebling commented 7 years ago

Fixes merge issues with #29

@tarmolov any update on merging Windows compatibility?

tarmolov commented 7 years ago

It'd be a great to add windows support to the library but I have three concerns about this and #29 prs:

platform specific code are mixed through all codebase
fix for spawn looks hacky and doesn't cover all cases (I've compared it with cross-spawn library); adding partial support doesn't look as a good step
I use mac/unix and I cannot test windows specific code for each release; so, it can be broken from time to time (it seems that it can be solved by yet another third-party like https://www.appveyor.com)

coveralls commented 7 years ago

Coverage decreased (-8.6%) to 86.614% when pulling 7c8a4b5631c824adadba4b89f7436b0f164bf52f on artptr:master into 7a79c8cc497ac697e5966df4c7b8b4fb9271d495 on tarmolov:master.

codebling commented 7 years ago

This is good feedback. Looking back at the changes, they can be cleaned up a lot. Respectively,

Checking file permissions is a platform-specific operation, so another conditional is necessary whenever this happens. Testing permissions for executability seems like an anti-pattern to me, however. Simply try to execute the command, then catch-and-ignore any EACCES errors. In this way we could not only clean up the code by removing isExecutable(), but at the same time make it Windows-compatible.
cross-spawn is unlikely to solve the issues related to spawning in the majority of cases. Commands like exec('git check-ignore ') would still require git to be in the PATH, which by default it is not. It may be worth testing to verify those statements.
Correct, appveyor seems to be the way to go for this.

Regaddi commented 7 years ago

Any progress on this?

tarmolov commented 7 years ago

Nope.

I can merge it as an experimental feature but I cannot guarantee that git-hooks will work fine on windows. See my comment above https://github.com/tarmolov/git-hooks-js/pull/38#issuecomment-271422549

tarmolov commented 7 years ago

Sorry for the huge delay with decision about Windows support. I decided keep support only for Unix and macOS. I can guarantee expected behaviour only on this platforms.

And again I want to say sorry about negative decision.

codebling commented 7 years ago

No worries, I completely understand. Windows users can use my fork, I will update and maintain, time allowing.

tarmolov commented 7 years ago

I can add a link to your fork in README file.

codebling commented 7 years ago

Sounds good!

tarmolov commented 7 years ago

Let me know when you publish a first version.

codebling commented 6 years ago

There are a few PRs offering Windows support (see #11, #29, #38, #52 and your own #51) but it's unlikely that these will be merged, see tarmolov's comment here. Check out Husky, it's a very active project which allows you to set up git hooks in your package.json file. It supports OSX, linux and Windows and has support for a few other hooks which git-hooks-js does not.

tarmolov / git-hooks-js

Allow execution on Windows #38