Skip to content

Commit b3f36ba

Browse files
sdssds
committed
Avoid stashing changes when all changes are staged
It was suggested in #135 that a potential way to avoid unnecessary file change detection and general stashing woes would be to stash changes only when there were unstaged changes that needed to be excluded from the run. This presents a nice optimization that I wish we had considered long ago. We can simplify the logic now that we see the purpose of this feature a little more clearly: stash only when you need to. Issue #669 raised an interesting point about the difficulty to detect if a stash entry was created, so we modified the tests to check if a method call was made. This isn't great as it's peeking deep into the implementation, so we added some additional tests that ensured the methods in question actually existed. Not perfect, but gives us some confidence in the implementation. While here, we were able to: * Extract the stashing logic into a separate `stash_changes` helper method. * Simplify the `cleanup_environment` logic to just use whether changes were stashed in order to decide whether to restore. Closes #669.
1 parent 985873e commit b3f36ba

2 files changed

Lines changed: 74 additions & 35 deletions

File tree

‎lib/overcommit/hook_context/pre_commit.rb‎

Lines changed: 34 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -49,29 +49,15 @@ def setup_environment
4949
Overcommit::GitRepo.store_merge_state
5050
Overcommit::GitRepo.store_cherry_pick_state
5151

52-
if !initial_commit? && any_changes?
53-
@stash_attempted = true
54-
55-
stash_message = "Overcommit: Stash of repo state before hook run at #{Time.now}"
56-
result = Overcommit::Utils.with_environment('GIT_LITERAL_PATHSPECS' => '0') do
57-
Overcommit::Utils.execute(
58-
%w[git -c commit.gpgsign=false stash save --keep-index --quiet] + [stash_message]
59-
)
60-
end
61-
62-
unless result.success?
63-
# Failure to stash in this case is likely due to a configuration
64-
# issue (e.g. author/email not set or GPG signing key incorrect)
65-
raise Overcommit::Exceptions::HookSetupFailed,
66-
"Unable to setup environment for #{hook_script_name} hook run:" \
67-
"\nSTDOUT:#{result.stdout}\nSTDERR:#{result.stderr}"
68-
end
52+
# Don't attempt to stash changes if all changes are staged, as this
53+
# prevents us from modifying files at all, which plays better with
54+
# editors/tools which watch for file changes.
55+
if !initial_commit? && unstaged_changes?
56+
stash_changes
6957

70-
@changes_stashed = `git stash list -1`.include?(stash_message)
58+
# While running hooks make it appear as if nothing changed
59+
restore_modified_times
7160
end
72-
73-
# While running the hooks make it appear as if nothing changed
74-
restore_modified_times
7561
end
7662

7763
# Restore unstaged changes and reset file modification times so it appears
@@ -82,19 +68,14 @@ def setup_environment
8268
# modification time on the file was newer. This helps us play more nicely
8369
# with file watchers.
8470
def cleanup_environment
85-
unless initial_commit? || (@stash_attempted && !@changes_stashed)
86-
clear_working_tree # Ensure working tree is clean before restoring it
87-
restore_modified_times
88-
end
89-
9071
if @changes_stashed
72+
clear_working_tree
9173
restore_working_tree
9274
restore_modified_times
9375
end
9476

9577
Overcommit::GitRepo.restore_merge_state
9678
Overcommit::GitRepo.restore_cherry_pick_state
97-
restore_modified_times
9879
end
9980

10081
# Get a list of added, copied, or modified files that have been staged.
@@ -140,6 +121,27 @@ def initial_commit?
140121

141122
private
142123

124+
def stash_changes
125+
@stash_attempted = true
126+
127+
stash_message = "Overcommit: Stash of repo state before hook run at #{Time.now}"
128+
result = Overcommit::Utils.with_environment('GIT_LITERAL_PATHSPECS' => '0') do
129+
Overcommit::Utils.execute(
130+
%w[git -c commit.gpgsign=false stash save --keep-index --quiet] + [stash_message]
131+
)
132+
end
133+
134+
unless result.success?
135+
# Failure to stash in this case is likely due to a configuration
136+
# issue (e.g. author/email not set or GPG signing key incorrect)
137+
raise Overcommit::Exceptions::HookSetupFailed,
138+
"Unable to setup environment for #{hook_script_name} hook run:" \
139+
"\nSTDOUT:#{result.stdout}\nSTDERR:#{result.stderr}"
140+
end
141+
142+
@changes_stashed = `git stash list -1`.include?(stash_message)
143+
end
144+
143145
# Clears the working tree so that the stash can be applied.
144146
def clear_working_tree
145147
removed_submodules = Overcommit::GitRepo.staged_submodule_removals
@@ -172,14 +174,11 @@ def restore_working_tree
172174
end
173175
end
174176

175-
# Returns whether there are any changes to the working tree, staged or
176-
# otherwise.
177-
def any_changes?
178-
modified_files = `git status -z --untracked-files=no`.
179-
split("\0").
180-
map { |line| line.gsub(/[^\s]+\s+(.+)/, '\\1') }
181-
182-
modified_files.any?
177+
# Returns whether there are any changes to tracked files which have not yet
178+
# been staged.
179+
def unstaged_changes?
180+
result = Overcommit::Utils.execute(%w[git --no-pager diff --quiet])
181+
!result.success?
183182
end
184183

185184
# Stores the modification times for all modified files to make it appear like

‎spec/overcommit/hook_context/pre_commit_spec.rb‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,25 @@
158158
end
159159
end
160160

161+
context 'when all changes have been staged' do
162+
around do |example|
163+
repo do
164+
echo('Hello World', 'tracked-file')
165+
`git add tracked-file`
166+
`git commit -m "Add tracked-file"`
167+
echo('Hello Other World', 'other-tracked-file')
168+
`git add other-tracked-file`
169+
example.run
170+
end
171+
end
172+
173+
it 'does not stash changes' do
174+
expect(context.private_methods).to include :stash_changes
175+
expect(context).not_to receive(:stash_changes)
176+
subject
177+
end
178+
end
179+
161180
context 'when renaming a file during an amendment' do
162181
around do |example|
163182
repo do
@@ -319,6 +338,27 @@
319338
end
320339
end
321340

341+
context 'when all changes were staged' do
342+
around do |example|
343+
repo do
344+
echo('Hello World', 'tracked-file')
345+
`git add tracked-file`
346+
`git commit -m "Add tracked-file"`
347+
echo('Hello Other World', 'other-tracked-file')
348+
`git add other-tracked-file`
349+
example.run
350+
end
351+
end
352+
353+
it 'does not touch the working tree' do
354+
expect(context.private_methods).to include :clear_working_tree
355+
expect(context.private_methods).to include :restore_working_tree
356+
expect(context).not_to receive(:clear_working_tree)
357+
expect(context).not_to receive(:restore_working_tree)
358+
subject
359+
end
360+
end
361+
322362
context 'when there were deleted files' do
323363
around do |example|
324364
repo do

0 commit comments

Comments
 (0)