Skip to content

Commit 257c949

Browse files
sdssds
authored
Keep deleted and renamed paths absent while stashing changes (sds#928)
Older Git versions can resurrect staged deletions and old rename paths as untracked files while commit hooks run. The scripted `git stash --keep-index` resets the working tree to HEAD, then restores the index without removing those paths, so tools such as RSpec can discover obsolete files. Stash normally and restore the staged tree with `git read-tree --reset -u stash@{0}^2`. This updates the index and working tree without moving HEAD. If restoring the staged tree fails, stop before running hooks and retain the stash for recovery. Only restore it when this run actually created a stash. Fixes the original normal-commit report in sds#699; the separate `overcommit --run` case was addressed in sds#905. The added examples exposed a coverage-reporting problem: round-robin shard assignments shift when tests are added, so per-shard coverage comparisons can fail despite higher total coverage. CI now collects every shard's LCOV report and submits them together for one complete-suite coverage comparison. All test matrix jobs still run, and missing coverage artifacts fail the build. Validation: - Full suite: 1,750 examples, zero failures. - All pre-commit checks and RuboCop pass. - Added 12 examples covering pre-commit and commit-message contexts, current and emulated legacy stash behavior, complete state restoration, existing stashes, and setup failure recovery. No existing tests were removed. - Confirmed setup and cleanup against Git 2.20's actual stash script, with its legacy text-output helper restored for compatibility with current Git. - Confirmed the old-name regression tests fail before the fix. Fixes sds#699.
1 parent 0b1e50e commit 257c949

4 files changed

Lines changed: 165 additions & 9 deletions

File tree

‎.github/workflows/tests.yml‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -69,12 +69,14 @@ jobs:
6969
git config --global user.name "GitHub Actions"
7070
bundle exec ruby script/parallel_rspec.rb
7171
72-
- name: Code coverage reporting
73-
uses: coverallsapp/github-action@v2
72+
- name: Save shard coverage
73+
uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2
7474
with:
75-
github-token: ${{ secrets.github_token }}
76-
flag-name: ruby${{ matrix.ruby-version }}-${{ matrix.target.os }}-shard${{ matrix.target.shard }}of${{ matrix.target.shards }}
77-
parallel: true
75+
name: coverage-ruby${{ matrix.ruby-version }}-${{ matrix.target.os }}-shard${{ matrix.target.shard }}of${{ matrix.target.shards }}
76+
path: coverage/lcov.info
77+
if-no-files-found: error
78+
overwrite: true
79+
retention-days: 1
7880

7981
overcommit:
8082
timeout-minutes: 10
@@ -112,8 +114,18 @@ jobs:
112114
JOB_RESULTS: ${{ toJSON(needs) }}
113115
run: echo "$JOB_RESULTS" | jq -e 'all(.[]; .result == "success")'
114116

115-
- name: Finalize code coverage report
117+
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
118+
119+
- name: Collect all shard coverage
120+
uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2
121+
with:
122+
pattern: coverage-*
123+
path: coverage
124+
125+
# Shard assignments shift when examples are added. Compare the complete
126+
# suite's coverage rather than unrelated fractions from individual shards.
127+
- name: Report combined code coverage
116128
uses: coverallsapp/github-action@v2
117129
with:
118130
github-token: ${{ secrets.github_token }}
119-
parallel-finished: true
131+
files: coverage/*/lcov.info

‎CONTRIBUTING.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ The best way to see a feature added, however, is to submit a pull request.
5858

5959
GitHub Actions runs one Linux shard and five Windows shards per Ruby version.
6060

61-
The final `CI` check depends on all test shards and lint, and finalizes coverage reporting.
61+
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

6464
* Submit your pull request!

‎lib/overcommit/hook_context/helpers/stash_unstaged_changes.rb‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ def setup_environment
1717
# editors/tools which watch for file changes.
1818
if !initial_commit? && unstaged_changes?
1919
stash_changes
20+
restore_staged_changes if @changes_stashed
2021

2122
# While running hooks make it appear as if nothing changed
2223
restore_modified_times
@@ -82,7 +83,7 @@ def stash_changes
8283
stash_message = "Overcommit: Stash of repo state before hook run at #{Time.now}"
8384
result = Overcommit::Utils.with_environment('GIT_LITERAL_PATHSPECS' => '0') do
8485
Overcommit::Utils.execute(
85-
%w[git -c commit.gpgsign=false stash save --keep-index --quiet] + [stash_message]
86+
%w[git -c commit.gpgsign=false stash save --quiet] + [stash_message]
8687
)
8788
end
8889

@@ -97,6 +98,18 @@ def stash_changes
9798
@changes_stashed = `git stash list -1`.include?(stash_message)
9899
end
99100

101+
# Restore the staged tree explicitly: older Git versions' --keep-index
102+
# leave deleted files and the old paths of renamed files in the working tree.
103+
# read-tree updates both the index and working tree without moving HEAD.
104+
def restore_staged_changes
105+
result = Overcommit::Utils.execute(%w[git read-tree --reset -u stash@{0}^2])
106+
unless result.success?
107+
raise Overcommit::Exceptions::HookSetupFailed,
108+
"Unable to restore staged changes for #{hook_script_name} hook run:" \
109+
"\nSTDOUT:#{result.stdout}\nSTDERR:#{result.stderr}"
110+
end
111+
end
112+
100113
# Restores the file modification times for all modified files to make it
101114
# appear like they never changed.
102115
def restore_modified_times

‎spec/overcommit/hook_context/helpers/stash_unstaged_changes_spec.rb‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
# frozen_string_literal: true
2+
3+
require 'spec_helper'
4+
require 'overcommit/hook_context/pre_commit'
5+
require 'overcommit/hook_context/commit_msg'
6+
7+
[Overcommit::HookContext::PreCommit, Overcommit::HookContext::CommitMsg].each do |hook_context|
8+
describe hook_context do
9+
describe 'stashing staged deletions and renames' do
10+
let(:context) { described_class.new(nil, [], nil) }
11+
let(:renamed_file) { 'renamed file.rb' }
12+
13+
around do |example|
14+
repo do
15+
echo('Renamed content', 'original.rb')
16+
echo('Deleted content', 'deleted.rb')
17+
echo('Partially staged content', 'partial.rb')
18+
echo('Unchanged content', 'other.rb')
19+
shell(%w[git add .]).status.should == 0
20+
shell(%w[git commit -m Initial]).status.should == 0
21+
22+
echo('Previous stash', 'other.rb')
23+
shell(%w[git stash save --quiet previous-stash]).status.should == 0
24+
shell(['git', 'mv', 'original.rb', renamed_file]).status.should == 0
25+
shell(%w[git rm deleted.rb]).status.should == 0
26+
echo('Staged addition', 'partial.rb', append: true)
27+
shell(%w[git add partial.rb]).status.should == 0
28+
echo('Unstaged addition', 'partial.rb', append: true)
29+
echo('Unstaged rename edit', renamed_file, append: true)
30+
echo('Unstaged other edit', 'other.rb', append: true)
31+
echo('Untracked content', 'untracked.rb')
32+
example.run
33+
end
34+
end
35+
36+
before do
37+
allow(context).to receive(:amendment?).and_return(false)
38+
end
39+
40+
def repository_state
41+
{
42+
head: shell(%w[git rev-parse HEAD]).stdout,
43+
index: shell(%w[git write-tree]).stdout,
44+
status: shell(%w[git status --porcelain -z]).stdout,
45+
staged_diff: shell(%w[git diff --cached --binary]).stdout,
46+
unstaged_diff: shell(%w[git diff --binary]).stdout,
47+
files: Dir['*.rb'].sort.map { |path| [path, File.binread(path)] },
48+
stashes: shell(%w[git stash list]).stdout,
49+
}
50+
end
51+
52+
# Git's scripted stash restored the index without updating the working tree,
53+
# then checked out modified index entries. Deleted paths stayed on disk.
54+
# Emulate those operations with real Git commands so this regression is
55+
# exercised even on Git versions whose --keep-index has since been fixed.
56+
def emulate_legacy_stash
57+
allow(Overcommit::Utils).to receive(:execute).and_wrap_original do |original, args, *rest|
58+
if args.include?('stash') && args.include?('--keep-index')
59+
result = original.call(args - ['--keep-index'], *rest)
60+
if result.success?
61+
shell(%w[git read-tree --reset stash@{0}^2]).status.should == 0
62+
modified = shell(%w[git ls-files -z --modified]).stdout
63+
Overcommit::Subprocess.spawn(%w[git checkout-index -z --force --stdin],
64+
input: modified).status.should == 0
65+
end
66+
result
67+
else
68+
original.call(args, *rest)
69+
end
70+
end
71+
end
72+
73+
['current Git', 'legacy Git stash'].each do |git_implementation|
74+
context "with #{git_implementation}" do
75+
before { emulate_legacy_stash if git_implementation == 'legacy Git stash' }
76+
77+
it 'exposes the staged tree without resurrecting deleted paths or moving HEAD' do
78+
head = shell(%w[git rev-parse HEAD]).stdout
79+
index = shell(%w[git write-tree]).stdout
80+
context.setup_environment
81+
82+
File.exist?('original.rb').should == false
83+
File.exist?('deleted.rb').should == false
84+
File.read(renamed_file).should == "Renamed content\n"
85+
File.read('partial.rb').should == "Partially staged content\nStaged addition\n"
86+
File.read('other.rb').should == "Unchanged content\n"
87+
File.read('untracked.rb').should == "Untracked content\n"
88+
shell(%w[git rev-parse HEAD]).stdout.should == head
89+
shell(%w[git write-tree]).stdout.should == index
90+
end
91+
92+
it 'restores all staged and unstaged changes and preserves the previous stash' do
93+
state = repository_state
94+
context.setup_environment
95+
context.cleanup_environment
96+
97+
repository_state.should == state
98+
end
99+
end
100+
end
101+
102+
it 'preserves the stash for recovery if restoring the staged tree fails' do
103+
state = repository_state
104+
result = Overcommit::Subprocess::Result.new(128, '', 'fatal: unable to restore index')
105+
allow(Overcommit::Utils).to receive(:execute).and_call_original
106+
allow(Overcommit::Utils).to receive(:execute).
107+
with(%w[git read-tree --reset -u stash@{0}^2]).and_return(result)
108+
109+
expect { context.setup_environment }.
110+
to raise_error(Overcommit::Exceptions::HookSetupFailed, /fatal: unable to restore index/)
111+
112+
shell(%w[git stash list]).stdout.should include 'Overcommit: Stash of repo state'
113+
shell(%w[git stash pop --index]).status.should == 0
114+
repository_state.should == state
115+
end
116+
117+
it 'leaves the previous stash alone if there are no longer any changes to stash' do
118+
shell(%w[git reset --hard]).status.should == 0
119+
state = repository_state
120+
# Simulate changes disappearing between the initial check and git stash.
121+
allow(context).to receive(:unstaged_changes?).and_return(true)
122+
expect(context).not_to receive(:restore_staged_changes)
123+
124+
context.setup_environment
125+
context.cleanup_environment
126+
127+
repository_state.should == state
128+
end
129+
end
130+
end
131+
end

0 commit comments

Comments
 (0)