Add an option to stash changes before running hooks#63
Open
kevinoid wants to merge 2 commits intoobserving:masterfrom
Open
Add an option to stash changes before running hooks#63kevinoid wants to merge 2 commits intoobserving:masterfrom
kevinoid wants to merge 2 commits intoobserving:masterfrom
Conversation
Previously log would always call .exit(), which is intended to be fatal. In order to support non-fatal logging, split out a logOnly method which outputs the formatted lines but does not exit. This preserves the current API completely. Also add a call to .slice() if the lines argument is not a string so that the argument Array is not modified. This codepath is not currently used, but it is no longer a waiting surprise for future callers. Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
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>
Member
|
Thanks for your pull request, @Swaagie Any opinions? |
| , util = require('util') | ||
| , tty = require('tty'); | ||
| , tty = require('tty') | ||
| , bufferedSpawn = require('buffered-spawn') |
Member
There was a problem hiding this comment.
The buffered-spawn isn't really needed. As this a CLI file, there really is no need for async execution. That's why we're using an sync exec throughout the code base. Using https://github.com/observing/pre-commit/blob/master/index.js#L67-L71 would make the code a lot easier to read manage as there would be no need for promises and async flow control.
Contributor
Author
There was a problem hiding this comment.
I tend to think of synchronous execution as an anti-pattern since it prevents the addition of concurrency. But if you feel strongly about it, I can rework the commit to use only synchronous methods.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to ensure any pre-commit tests are run against the files as they will appear in the commit, this PR adds an option to run
git stashto 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. 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 #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 #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.
Thanks for considering,
Kevin
Fixes: #4