stash files before running pre-commit hook to avoid false positives; closes #4 #47

boneskull · 2015-09-12T20:55:57Z

Let me know what you think.

3rd-Eden · 2015-09-12T23:19:16Z

Code it self looks solid, but would love to see a configuration option for this to be disabled. If people do happen to run in to issues, they could just ignore something pre-commit.stash: false

boneskull · 2015-09-13T02:46:37Z

k. haven't had a chance to look at the CI failures; it passed for me locally.

boneskull · 2015-09-13T19:36:26Z

Looks like some reformats crept in too

3rd-Eden · 2015-09-13T21:49:40Z

index.js

+    env: process.env,
+    cwd: hooked.root,
+    stdio: [0, 1, 2]
+  }).once('close', function(code) {


You might as well supply the done method here directly to eliminate this function call.

not exactly sure what you mean. will rebase in a sec anyway

You have .once('close, function (code) { if (code) done(code); done() }) Couldn't you just write it as

.once('close', done);

Maybe? I'm not sure if async would interpret 0 as an error.

I left it this way to be safe

…ng#4 - `precommit.stash` is now configurable in `package.json`; set to `false` to avoid stashing

boneskull · 2015-09-14T05:24:54Z

ok, I've added it as an option

fhemberger · 2015-10-20T08:39:45Z

@boneskull Could you please have a look why the tests are failing? Would be really nice to have this feature on board.

3rd-Eden · 2015-10-20T09:07:16Z

Yup, travis-ci shouldn't fail. You can safely ignore the appveyor for now.

atorian · 2015-11-30T16:35:58Z

Why is it treated as an issue?
Stashing files will lead to issues in custom scripts which rely on git commands.
And if you modify the tree - it might lead to unnecessary issues.

Example:

git ci -am 'message'
git stash  --keep-index
git diff --name-only --cached // empty file list - all in the stash
git stash apply
// bad code in the commit

In order to ensure any pre-commit tests are run against the files as they will appear in the commit, this commit adds an option to run `git stash` to save any changes to the working tree before running the scripts, then restore any changes after the scripts finish. The save/restore procedure is based on Chris Torek's StackOverflow answer to a question asking how to do exactly this.[1] This implementation does not do a hard reset before restoring the stash by default, since it assumes that if scripts modify tracked files the changes should be kept. But it does provide this behavior, and `git clean`, as configurable options. It follows the convention requested in observing#4 by making the new stash option default to off. Although there are no known implementation issues (with the exception of the git bug noted in Torek's SO answer), current scripts may expect modified/untracked files to exist or modify untracked files in a way which prevents applying the stash, making default-on behavior backwards-incompatible. The tests are split into a separate file. Since each stash option is tested against a project repository and a clean/reset is done between each test, the tests are somewhat slow. This way the tests are not run by default, but are run as test-stash, as part of test-all, and as part of test-travis. This commit is based off of the work of Christopher Hiller in observing#47, although the implementation differs significantly due to the use of Promises in place of async, which I found to be significantly clearer, more flexible, and they make the tests significantly more concise. Fixes: observing#4 1. https://stackoverflow.com/a/20480591 Signed-off-by: Christopher Hiller <[email protected]> [[email protected]: Reimplement using Promises and Torek's method] Signed-off-by: Kevin Locke <[email protected]>

In order to ensure any pre-commit tests are run against the files as they will appear in the commit, this commit adds an option to run `git stash` to save any changes to the working tree before running the scripts, then restore any changes after the scripts finish. The save/restore procedure is based on Chris Torek's StackOverflow answer to a question asking how to do exactly this.[1] This implementation does not do a hard reset before restoring the stash by default, since it assumes that if scripts modify tracked files the changes should be kept. But it does provide this behavior, and `git clean`, as configurable options. It follows the convention requested in observing#4 by making the new stash option default to off. Although there are no known implementation issues (with the exception of the git bug noted in Torek's SO answer), current scripts may expect modified/untracked files to exist or modify untracked files in a way which prevents applying the stash, making default-on behavior backwards-incompatible. The tests are split into a separate file. Since each stash option is tested against a project repository and a clean/reset is done between each test, the tests are somewhat slow. By splitting the tests into a separate file, we can avoid running them by default. They can instead be run as test-stash, as part of test-all, and as part of test-travis. This commit is based off of the work of Christopher Hiller in observing#47, although the implementation differs significantly due to the use of Promises in place of async, which I found to be significantly clearer, more flexible, and they make the tests significantly more concise. Fixes: observing#4 1. https://stackoverflow.com/a/20480591 Signed-off-by: Christopher Hiller <[email protected]> [[email protected]: Reimplement using Promises and Torek's method] Signed-off-by: Kevin Locke <[email protected]>

3rd-Eden reviewed Sep 13, 2015
View reviewed changes

boneskull force-pushed the issue/4 branch from d396815 to 7b70b43 Compare September 14, 2015 05:23

stash before pre-commit hook to avoid false positives; closes observi…

4c76ce7

…ng#4 - `precommit.stash` is now configurable in `package.json`; set to `false` to avoid stashing

boneskull force-pushed the issue/4 branch from 7b70b43 to 4c76ce7 Compare September 14, 2015 05:24

kevinoid mentioned this pull request Feb 6, 2016

Add an option to stash changes before running hooks #63

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stash files before running pre-commit hook to avoid false positives; closes #4 #47

stash files before running pre-commit hook to avoid false positives; closes #4 #47

boneskull commented Sep 12, 2015

3rd-Eden commented Sep 12, 2015

boneskull commented Sep 13, 2015

boneskull commented Sep 13, 2015

3rd-Eden Sep 13, 2015

boneskull Sep 14, 2015

3rd-Eden Sep 14, 2015

boneskull Sep 14, 2015

boneskull Sep 14, 2015

boneskull commented Sep 14, 2015

fhemberger commented Oct 20, 2015

3rd-Eden commented Oct 20, 2015

atorian commented Nov 30, 2015

stash files before running pre-commit hook to avoid false positives; closes #4 #47

Are you sure you want to change the base?

stash files before running pre-commit hook to avoid false positives; closes #4 #47

Conversation

boneskull commented Sep 12, 2015

3rd-Eden commented Sep 12, 2015

boneskull commented Sep 13, 2015

boneskull commented Sep 13, 2015

3rd-Eden Sep 13, 2015

Choose a reason for hiding this comment

boneskull Sep 14, 2015

Choose a reason for hiding this comment

3rd-Eden Sep 14, 2015

Choose a reason for hiding this comment

boneskull Sep 14, 2015

Choose a reason for hiding this comment

boneskull Sep 14, 2015

Choose a reason for hiding this comment

boneskull commented Sep 14, 2015

fhemberger commented Oct 20, 2015

3rd-Eden commented Oct 20, 2015

atorian commented Nov 30, 2015