-
Notifications
You must be signed in to change notification settings - Fork 30
Windows support #11
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
Windows support #11
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,10 @@ | ||
#!/usr/bin/env node | ||
|
||
var spawn = require('child_process').spawn; | ||
var exec = require('child_process').execSync; | ||
var gitCachedFiles = exec('git diff --cached --name-only --diff-filter=ACMR').toString().trim(); | ||
var CP = require('child_process'); | ||
var gitCachedFiles = CP.execSync('git diff --cached --name-only --diff-filter=ACMR').toString().trim(); | ||
var opts = {stdio: 'inherit'}; | ||
|
||
if (gitCachedFiles) { | ||
spawn('npm', ['test'], {stdio: 'inherit'}).on('close', process.exit.bind(process)); | ||
(process.platform === 'win32' ? | ||
CP.exec('npm test', opts) : CP.spawn('npm', ['test'], opts)).on('close', process.exit.bind(process)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ var util = require('util'); | |
var spawn = require('child_process').spawn; | ||
var fs = require('fs'); | ||
var fsHelpers = require('./fs-helpers'); | ||
var isWin32 = process.platform === 'win32'; | ||
|
||
var HOOKS_DIRNAME = 'hooks'; | ||
var HOOKS_OLD_DIRNAME = 'hooks.old'; | ||
|
@@ -51,7 +52,12 @@ module.exports = { | |
} | ||
|
||
var hookTemplate = fs.readFileSync(__dirname + '/' + HOOKS_TEMPLATE_FILE_NAME); | ||
var pathToGitHooks = path.relative(hooksPath, __dirname); | ||
var pathToGitHooks = path.join(path.relative(hooksPath, __dirname), 'git-hooks'); | ||
|
||
if (isWin32) { | ||
pathToGitHooks = pathToGitHooks.replace(/\\/g, '\\\\'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does it need for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. backslash, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String |
||
} | ||
|
||
var hook = util.format(hookTemplate.toString(), pathToGitHooks); | ||
|
||
fsHelpers.makeDir(hooksPath); | ||
|
@@ -160,11 +166,22 @@ function isExecutable(stats) { | |
*/ | ||
function spawnHook(hookName, args) { | ||
var stats = fs.statSync(hookName); | ||
var isHookExecutable = stats && stats.isFile() && isExecutable(stats); | ||
if (!isHookExecutable) { | ||
var command = hookName; | ||
var opts = args; | ||
var hook; | ||
|
||
if (isWin32) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's better to move all windows specific code into a separate module. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. explain more detail |
||
hook = fs.readFileSync(hookName).toString(); | ||
if (!require('shebang-regex').test(hook)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't merge the pr with extra modules. If you really want to help, let's list all current problems with windows version and find out the best solution for each of them. Discussion first, then code. We can start with your description: #10 (comment) |
||
throw new Error('Cannot find shebang in hook: ' + hookName + '.'); | ||
} | ||
command = require('shebang-command')(require('shebang-regex').exec(hook)[0]); | ||
opts = [hookName].concat(opts); | ||
} else if (!(stats && stats.isFile() && isExecutable(stats))) { | ||
throw new Error('Cannot execute hook: ' + hookName + '. Please check file permissions.'); | ||
} | ||
return spawn(hookName, args, {stdio: 'inherit'}); | ||
|
||
return spawn(command, opts, {stdio: 'inherit'}); | ||
} | ||
|
||
/** | ||
|
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.
Using
execSync
for executingnpm test
is a bad idea because the script can reach buffer limit. Moreover,execSync
doesn't proxy any output during execution; so, a user watches nothing but an empty screen. It's a bad UX.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.
exec().stdout.on('data') resolves problem. There is error occur if i spawn npm(npm enoent).