Skip to content

Commit a5bdc87

Browse files
authored
Support hook side effects without stashing repository changes (#939)
Pre-commit and commit-msg hooks can now modify, create, remove, and stage files without losing their changes during cleanup. Unstaged edits are saved to a durable binary patch and hidden with a reverse application; cleanup reapplies that patch without changing HEAD, the index, the stash, or merge/cherry-pick metadata. Hooks retain native Git behavior: only edits they explicitly stage enter the commit, and their edits survive failed or interrupted runs. If restoration conflicts with hook edits, the commit fails while preserving both the hook changes and a patch with recovery instructions. Patches survive garbage collection and forced termination, and successful cleanup removes them. File modification times are restored for unchanged files while hook edits remain visible to watchers. The existing behavior tests remain, including deletion/rename and configuration-error recovery regressions. Tests tied to stash commands and legacy stash emulation were replaced with patch failure and recovery tests. New regression coverage includes actual commits, first commits, hook failures/exceptions/interruptions, index changes, alternate indexes, worktrees, binary files, unusual paths, symlinks/type changes, CRLF conversion, executable modes, conflicting edits, and shell-safe recovery commands. Interrupt messages now describe patch recovery instead of pointing to the stash. `AGENTS.md` and contributor guidance require 100% line coverage; CI now enforces it after combining all shard/platform reports. Validation: - Complete local suite: **2,169 examples, 0 failures**. - Local exact coverage gate: **3,844/3,844 executable lines (100%)**. - `bundle exec overcommit --run`: passed, including RuboCop. - All **21 GitHub checks passed**, including Ruby 3.3/3.4/4.0 on Linux and Windows, the final CI gate, and Coveralls. Combined CI coverage: **3,846/3,846 executable lines (100%)**. This removes the stash/reset mechanisms behind reports such as #65, #80, #135, #168, #372, #524, and #669. lint-staged's current backup/revert workflow still uses stashes and hard resets, so it was not copied. Closes #238.
1 parent de6a144 commit a5bdc87

19 files changed

Lines changed: 832 additions & 384 deletions

‎.github/workflows/tests.yml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,9 @@ jobs:
122122
pattern: coverage-*
123123
path: coverage
124124

125+
- name: Require 100% combined line coverage
126+
run: ruby script/check_coverage.rb coverage/*/lcov.info
127+
125128
# Shard assignments shift when examples are added. Compare the complete
126129
# suite's coverage rather than unrelated fractions from individual shards.
127130
- name: Report combined code coverage

‎AGENTS.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
# Repository guidance
2+
3+
- Maintain **100% line coverage** across the complete test suite. Add thorough
4+
regression tests for new behavior and failure paths; do not exclude production
5+
code from coverage or lower the threshold to make checks pass.
6+
- Preserve existing tests unless they specifically test an implementation that
7+
is being replaced. Prefer updating those tests to assert observable behavior.
8+
- Run `bundle exec ruby script/parallel_rspec.rb` for the full suite and
9+
`bundle exec overcommit --run` for lint checks before submitting changes.
10+
- Keep iterating on pull requests until all CI checks pass. Do not merge unless
11+
explicitly instructed to do so.

‎CHANGELOG.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
# Overcommit Changelog
22

3+
## Unreleased
4+
5+
* Support hooks that modify files and the index, preserving their changes while
6+
hiding/restoring unstaged edits without Git stash or hard resets
7+
* Retain a recovery patch when unstaged edits conflict with hook changes
8+
39
## 0.76.0
410

511
* Add configurable line parsing for command hooks

‎CONTRIBUTING.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,11 @@ The best way to see a feature added, however, is to submit a pull request.
6161
The final `CI` check depends on all test shards and lint, and reports their combined coverage.
6262
Repository rulesets can require this single check instead of individual matrix jobs.
6363

64+
* Maintain **100% line coverage** across the complete suite, including failure
65+
paths. CI merges coverage across all test shards and platforms and rejects any
66+
uncovered lines. Do not lower coverage thresholds or exclude production code
67+
to make a check pass.
68+
6469
* Submit your pull request!
6570

6671
All pull requests will be tested against [Travis CI][3], where the following

‎README.md‎

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -555,20 +555,27 @@ commit message editor is displayed. If a hook fails, the commit will not be
555555
created. These hooks are ideal for syntax checkers, linters, and other checks
556556
that you want to run before you allow a commit to even be created.
557557

558-
#### WARNING: pre-commit hooks cannot have side effects
559-
560-
`pre-commit` hooks currently do not support hooks with side effects (such as
561-
modifying files and adding them to the index with `git add`). This is a
562-
consequence of Overcommit's pre-commit hook stashing behavior to ensure hooks
563-
are run against _only the changes you are about to commit_.
564-
565-
Without Overcommit, the proper way to write a `pre-commit` hook would be to
566-
extract the staged changes into temporary files and lint those files
567-
instead of whatever contents are in your working tree (as you don't want
568-
unstaged changes to taint your results). Overcommit takes care
569-
of this for you, but to do it in a generalized way introduces this
570-
limitation. See the [thread tracking this
571-
issue](https://github.com/sds/overcommit/issues/238) for more details.
558+
#### Hooks that modify files
559+
560+
`pre-commit` hooks can modify, create, delete, and stage files. Overcommit
561+
preserves these changes even if a hook fails. Hooks must explicitly use `git add`
562+
or `git rm` to include their changes in the commit; Overcommit does not stage
563+
hook edits automatically. Set `parallelize: false` for hooks that modify files
564+
or the Git index so they run without other hooks accessing those resources.
565+
566+
Overcommit temporarily hides unstaged changes to tracked files, so hooks see
567+
only staged contents at their original paths. Untracked files and submodule
568+
working trees remain available. This also works for the initial commit.
569+
Unstaged changes are saved as a patch in the current worktree's Git directory,
570+
then reapplied after the hooks finish. Overcommit does not use the Git stash or
571+
reset the index, HEAD, or merge/cherry-pick state.
572+
573+
If hook edits conflict with the saved patch, Overcommit aborts the commit and
574+
keeps both the hook edits and the recovery patch. Follow the error's recovery
575+
instructions after resolving the conflict. The patch is removed only after
576+
successful restoration. If the process is forcibly terminated, recovery patches
577+
remain as `overcommit-unstaged-*.patch` beside the worktree's Git index; inspect
578+
and apply the patch from the repository root, then delete it when recovered.
572579

573580
* [`*`AuthorEmail](lib/overcommit/hook/pre_commit/author_email.rb)
574581
* [`*`AuthorName](lib/overcommit/hook/pre_commit/author_name.rb)

‎lib/overcommit/hook_context/commit_msg.rb‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
# frozen_string_literal: true
22

33
require_relative 'pre_commit'
4-
require_relative 'helpers/stash_unstaged_changes'
4+
require_relative 'helpers/hide_unstaged_changes'
55
require_relative 'helpers/file_modifications'
66

77
module Overcommit::HookContext
88
# Contains helpers related to contextual information used by commit-msg hooks.
99
class CommitMsg < Base
10-
include Overcommit::HookContext::Helpers::StashUnstagedChanges
10+
include Overcommit::HookContext::Helpers::HideUnstagedChanges
1111
include Overcommit::HookContext::Helpers::FileModifications
1212

1313
def empty_message?
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
# frozen_string_literal: true
2+
3+
require 'tempfile'
4+
require 'digest'
5+
require 'shellwords'
6+
7+
module Overcommit::HookContext
8+
module Helpers
9+
# Hide unstaged changes without moving HEAD, rewriting the index, or using
10+
# the stash. Hooks retain the same file paths and can change the index.
11+
module HideUnstagedChanges
12+
def setup_environment
13+
store_modified_times
14+
save_unstaged_changes
15+
return unless @unstaged_patch
16+
17+
apply_unstaged_changes(reverse: true)
18+
restore_modified_times
19+
@hook_contents = file_contents
20+
end
21+
22+
# Returns whether the current git branch is empty (has no commits).
23+
def initial_commit?
24+
return @initial_commit unless @initial_commit.nil?
25+
26+
@initial_commit = Overcommit::GitRepo.initial_commit?
27+
end
28+
29+
def cleanup_environment
30+
return unless @unstaged_patch
31+
32+
# Retain modification times for hook edits so file watchers see them.
33+
unchanged_files = file_contents.select { |file, contents| @hook_contents[file] == contents }
34+
apply_unstaged_changes
35+
restore_modified_times(unchanged_files.keys)
36+
File.unlink(@unstaged_patch)
37+
@unstaged_patch = nil
38+
end
39+
40+
private
41+
42+
def save_unstaged_changes
43+
result = Overcommit::Utils.execute(%w[
44+
git diff --binary --full-index --no-color --no-ext-diff --no-textconv
45+
--no-renames --ignore-submodules=all --src-prefix=a/ --dst-prefix=b/ --unified=3 --
46+
])
47+
check_patch_result(result, Overcommit::Exceptions::HookSetupFailed)
48+
return if result.stdout.empty?
49+
50+
# Tempfile.create returns a regular File: neither GC nor process exit
51+
# removes the recovery patch if setup, hooks, or cleanup are interrupted.
52+
file = Tempfile.create(['overcommit-unstaged-', '.patch'],
53+
File.dirname(Overcommit::Utils.git_path('index')))
54+
begin
55+
file.binmode
56+
file.write(result.stdout)
57+
file.flush
58+
file.fsync
59+
@unstaged_patch = file.path
60+
ensure
61+
file.close
62+
end
63+
end
64+
65+
def apply_unstaged_changes(reverse: false)
66+
command = %w[git apply --whitespace=nowarn]
67+
command << '--reverse' if reverse
68+
result = Overcommit::Utils.execute(command + ['--', @unstaged_patch])
69+
error = if reverse
70+
Overcommit::Exceptions::HookSetupFailed
71+
else
72+
Overcommit::Exceptions::HookCleanupFailed
73+
end
74+
check_patch_result(result, error)
75+
end
76+
77+
def check_patch_result(result, error)
78+
return if result.success?
79+
80+
recovery = if @unstaged_patch
81+
"\nYour unstaged changes are preserved in #{@unstaged_patch}.\n" \
82+
'Hook changes are preserved. Resolve any conflicts, then run' \
83+
"#{Overcommit::OS.windows? ? ' in PowerShell' : ''}:\n" \
84+
' git apply --whitespace=nowarn -- ' \
85+
"#{quoted_patch_path}\n"
86+
end
87+
raise error,
88+
"Unable to restore or hide unstaged changes for #{hook_script_name} hooks:" \
89+
"\nSTDOUT:#{result.stdout}\nSTDERR:#{result.stderr}#{recovery}"
90+
end
91+
92+
def quoted_patch_path
93+
if Overcommit::OS.windows?
94+
# Quote for PowerShell, including literal apostrophes in directory names.
95+
"'#{@unstaged_patch.gsub("'", "''")}'"
96+
else
97+
Shellwords.escape(@unstaged_patch)
98+
end
99+
end
100+
101+
def store_modified_times
102+
@modified_times = {}
103+
files = modified_files + Overcommit::GitRepo.modified_files(staged: false)
104+
files.each do |file|
105+
next if Overcommit::Utils::FileUtils.symlink?(file)
106+
next unless File.file?(file)
107+
108+
@modified_times[file] = File.mtime(file)
109+
end
110+
end
111+
112+
def file_contents
113+
@modified_times.keys.each_with_object({}) do |file, contents|
114+
contents[file] = if Overcommit::Utils::FileUtils.symlink?(file)
115+
[:symlink, Overcommit::Utils::FileUtils.readlink(file)]
116+
elsif File.file?(file)
117+
[:file, Digest::SHA256.file(file).hexdigest]
118+
end
119+
end
120+
end
121+
122+
def restore_modified_times(files = @modified_times.keys)
123+
files.each do |file|
124+
next if Overcommit::Utils::FileUtils.symlink?(file)
125+
next unless File.file?(file)
126+
127+
File.utime(@modified_times.fetch(file), @modified_times.fetch(file), file)
128+
end
129+
end
130+
end
131+
end
132+
end

0 commit comments

Comments
 (0)