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.

Sign up for GitHub

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

Jump to bottom

pre-commit hooks has false positives #4

Open
Raynos opened this issue Feb 25, 2014 · 4 comments
Open

pre-commit hooks has false positives #4

Raynos opened this issue Feb 25, 2014 · 4 comments

Comments

@Raynos
Copy link
Contributor

Raynos commented Feb 25, 2014

When I use git add -p or even git add some files it will run my tests before the commit however the tests pass because of the unstaged changes I have locally.

If I were to stash before running npm test then the tests would have failed.

Is it the responsibility of pre-commit to ensure that the tests pass based on only the staged changes or is that the users responsibility ?
@3rd-Eden
Copy link
Member

As seen in #2 there could be some serious flaws when you programmatically do a git stash before running the tests. If there is a safe way of doing this, or maybe make it opt-in I would be more than happy to implement it.

@3rd-Eden
Copy link
Member

So, i've been thinking about this. We could implement an option which will stash automatically and have it turned off by default.

@vnen
Copy link

vnen commented Feb 26, 2015

I think that the issue with stash as mentioned in #2 is not an issue (at least not anymore). The problem with that mentioned comment is that if you remove a line from a file, git will remove all changes in that file from stage. So the git status command will show that the file is not in the stage. They wouldn't be commited anyway then.

If you run git -p to add only one hunk, the stash, git status will show the same file as staged and not staged. git stash --keep-index will remove the unstaged changes. Then git stash pop will correctly restore the unstaged change and keep the index the same.

I tested it with Git 2.3.0, so I'm not sure how older versions handle it.

@boneskull
Copy link

👍 here. pre-commit runs against the files in your working copy, instead of the files which you are committing. Stashing unstaged changes seems like the way to go.

boneskull added a commit to boneskull/pre-commit that referenced this issue Sep 12, 2015
@boneskull
stash files before running pre-commit hook to avoid false positives; c…
d396815 
…loses observing#4
boneskull added a commit to boneskull/pre-commit that referenced this issue Sep 14, 2015
@boneskull
stash before pre-commit hook to avoid false positives; closes observi…
7b70b43 
…ng#4

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

boneskull added a commit to boneskull/pre-commit that referenced this issue Sep 14, 2015
@boneskull
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
kevinoid added a commit to kevinoid/pre-commit that referenced this issue Feb 6, 2016
@kevinoid
Add an option to stash changes before running hooks
189559c 
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 <boneskull@boneskull.com>
[kevin@kevinlocke.name: Reimplement using Promises and Torek's method]
Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
kevinoid added a commit to kevinoid/pre-commit that referenced this issue Feb 6, 2016
@kevinoid
Add an option to stash changes before running hooks
bc823ae 
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 <boneskull@boneskull.com>
[kevin@kevinlocke.name: Reimplement using Promises and Torek's method]
Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
kevinoid added a commit to kevinoid/pre-commit that referenced this issue Feb 6, 2016
@kevinoid
Add an option to stash changes before running hooks
23b2bb2 
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 <boneskull@boneskull.com>
[kevin@kevinlocke.name: Reimplement using Promises and Torek's method]
Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
kevinoid added a commit to kevinoid/pre-commit that referenced this issue Feb 6, 2016
@kevinoid
Add an option to stash changes before running hooks
3573728 
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 <boneskull@boneskull.com>
[kevin@kevinlocke.name: Reimplement using Promises and Torek's method]
Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
@kevinoid kevinoid mentioned this issue Feb 6, 2016
Open
@climba03003 climba03003 mentioned this issue May 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@3rd-Eden @Raynos @boneskull @vnen