Skip to content

Commit 56f7339

Browse files
committedApr 10, 2015
Protect against lost changes when setup_environment fails
Commit c3d972d fixed an issue where we would not restore changes in the event a plugin failed to load. Unfortunately, it introduced a regression where if `setup_environment` were to raise an error for any reason we would always call `cleanup_environment`. This is not necessarily always safe. For example, in the case of pre-commit, `setup_environment` might fail before it is able to create a stash of the working tree, so calling `cleanup_environment` could result in lost changes. Change the ordering so that we (hopefully) get the best of both worlds: * Call `load_hooks` first outside of the `begin`/`ensure` block so that if it fails we haven't touched the repository yet, so the repo won't be in a bad state if a plugin can't be loaded or a signature is invalid * Call `setup_environment` outside of the `begin`/`ensure` block so that in the worst case we don't accidentally clobber anything with `cleanup_environment`
1 parent 4b784a9 commit 56f7339

File tree

1 file changed

+13
-2
lines changed

1 file changed

+13
-2
lines changed
 

‎lib/overcommit/hook_runner.rb

+13-2
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,20 @@ def run
2222
# these calls to allow some sort of "are you sure?" double-interrupt
2323
# functionality, but until that's deemed necessary let's keep it simple.
2424
InterruptHandler.isolate_from_interrupts do
25+
# Load hooks before setting up the environment so that the repository
26+
# has not been touched yet. This way any load errors at this point don't
27+
# result in Overcommit leaving the repository in a bad state.
28+
load_hooks
29+
30+
# Setup the environment without automatically calling
31+
# `cleanup_environment` on an error. This is because it's possible that
32+
# the `setup_environment` code did not fully complete, so there's no
33+
# guarantee that `cleanup_environment` will be able to accomplish
34+
# anything of value. The safest thing to do is therefore nothing in the
35+
# unlikely case of failure.
36+
@context.setup_environment
37+
2538
begin
26-
@context.setup_environment
27-
load_hooks
2839
run_hooks
2940
ensure
3041
@context.cleanup_environment

0 commit comments

Comments
 (0)