Reach and enforce 100% aggregated branch coverage

.github/workflows/tests.yml

@@ -125,8 +125,8 @@ jobs:
           pattern: coverage-*
           path: coverage
 
-      - name: Require 100% combined line coverage
-        run: ruby script/check_coverage.rb coverage/*/lcov.info
+      - name: Require 100% combined line and branch coverage
+        run: ruby script/check_coverage.rb --branches coverage/*/lcov.info
 
       # Shard assignments shift when examples are added. Compare the complete
       # suite's coverage rather than unrelated fractions from individual shards.

AGENTS.md

@@ -1,6 +1,8 @@
 # Repository guidance
 
-- Maintain **100% line coverage** across the complete test suite. Add thorough
+- Maintain **100% line and branch coverage**, aggregated across the complete
+  test suite and all CI platforms, Ruby versions, and shards. Individual shards
+  need not reach 100%; the combined coverage report must. Add thorough
   regression tests for new behavior and failure paths; do not exclude production
   code from coverage or lower the threshold to make checks pass.
 - Preserve existing tests unless they specifically test an implementation that

lib/overcommit/cli.rb

@@ -21,8 +21,6 @@ def run # rubocop:disable Metrics/MethodLength
       parse_arguments
 
       case @options[:action]
-      when :install, :uninstall
-        install_or_uninstall
       when :template_dir
         print_template_directory_path
       when :sign
@@ -31,6 +29,8 @@ def run # rubocop:disable Metrics/MethodLength
         run_all
       when :diff
         diff
+      else
+        install_or_uninstall
       end
     rescue Overcommit::Exceptions::ConfigurationSignatureChanged => e
       puts e

lib/overcommit/hook/pre_commit/chamber_verification.rb

@@ -4,7 +4,7 @@ module Overcommit::Hook::PreCommit
   # Runs `chamber sign --verify`.
   #
   # @see https://github.com/thekompanee/chamber/wiki/Git-Commit-Hooks#chamber-verification-pre-commit-hook
-  # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
+  # rubocop:disable Metrics/CyclomaticComplexity
   class ChamberVerification < Base
     def run
       approver_name  = config.fetch('approver_name') { 'your approver' }
@@ -22,7 +22,7 @@ def run
         compact.
         join("\n\n")
 
-      output = "\n\n#{output}" unless output.empty?
+      output = "\n\n#{output}"
 
       [
         :warn,
@@ -32,5 +32,5 @@ def run
       ]
     end
   end
-  # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
+  # rubocop:enable Metrics/CyclomaticComplexity
 end

lib/overcommit/hook/pre_commit/php_cs_fixer.rb

@@ -17,9 +17,7 @@ def run
         output = result.stdout.chomp
         exit_status_sum += result.status
 
-        if result.status
-          messages = output.lstrip.split("\n")
-        end
+        messages = output.lstrip.split("\n")
       end
 
       unless messages.empty?

lib/overcommit/hook/pre_commit/php_lint.rb

@@ -21,19 +21,14 @@ def run
         result = execute(command, args: [file])
         output = result.stdout.chomp
         exit_status_sum += result.status
-        if result.status
-          # `php -l` returns with a leading newline, and we only need the first
-          # line, there is usually some redundancy
-          messages << output.lstrip.split("\n").first
-        end
+        # `php -l` returns with a leading newline, and we only need the first
+        # line, there is usually some redundancy
+        messages << output.lstrip.split("\n").first
       end
 
       # If the sum of all lint status is zero, then none had exit status
       return :pass if exit_status_sum == 0
 
-      # No messages is great news for us
-      return :pass if messages.empty?
-
       # Return the list of message objects
       extract_messages(
         messages,

lib/overcommit/hook_context/pre_push.rb

@@ -79,7 +79,7 @@ def overwritten_commits
 
         result = Overcommit::Subprocess.spawn(%W[git rev-list #{remote_sha1} ^#{local_sha1}])
         if result.success?
-          result.stdout.split("\n")
+          @overwritten_commits = result.stdout.split("\n")
         else
           raise Overcommit::Exceptions::GitRevListError,
                 "Unable to check if commits on the remote ref will be overwritten: #{result.stderr}"

lib/overcommit/installer.rb

@@ -129,8 +129,6 @@ def can_replace_file?(file)
     end
 
     def preserve_old_hooks
-      return unless File.directory?(hooks_path)
-
       ensure_directory(old_hooks_path)
       Overcommit::Utils.supported_hook_types.each do |hook_type|
         hook_file = File.join(hooks_path, hook_type)

lib/overcommit/utils/messages_utils.rb

@@ -65,8 +65,6 @@ def extract_file(match, message)
       end
 
       def extract_line(match, message)
-        return unless match.names.include?('line')
-
         Integer(match[:line])
       rescue ArgumentError, TypeError
         raise Overcommit::Exceptions::MessageProcessingError,

script/check_coverage.rb

@@ -1,9 +1,11 @@
 # frozen_string_literal: true
 
-# Merge LCOV line hits across platforms and shards before enforcing coverage.
+# Merge LCOV hits across Ruby versions, platforms, and shards before enforcing coverage.
 # Individual shards need not cover every line, and platform-specific lines may
 # only run on one operating system.
+require_branches = ARGV.delete('--branches')
 lines = Hash.new(0)
+branches = Hash.new(0)
 ARGV.each do |path|
   source = nil
   File.foreach(path) do |record|
@@ -13,6 +15,10 @@
     when /^DA:(\d+),(\d+)/
       abort "Line coverage without a source file in #{path}" unless source
       lines[[source, Regexp.last_match(1).to_i]] += Regexp.last_match(2).to_i
+    when /^BRDA:(\d+),(\d+),(\d+),(-|\d+)/
+      abort "Branch coverage without a source file in #{path}" unless source
+      line, block, branch, hits = Regexp.last_match.captures
+      branches[[source, line.to_i, block.to_i, branch.to_i]] += hits == '-' ? 0 : hits.to_i
     when /^end_of_record/
       source = nil
     end
@@ -26,3 +32,16 @@
   abort '100% line coverage is required'
 end
 puts "Line coverage: 100% (#{lines.size}/#{lines.size})"
+
+if require_branches
+  abort 'No branch coverage found' if branches.empty?
+
+  uncovered_branches = branches.select { |_, hits| hits == 0 }.keys
+  unless uncovered_branches.empty?
+    warn "Uncovered branches:\n#{uncovered_branches.sort.map do |file, line, block, branch|
+      "#{file}:#{line} (block #{block}, branch #{branch})"
+    end.join("\n")}"
+    abort '100% branch coverage is required'
+  end
+  puts "Branch coverage: 100% (#{branches.size}/#{branches.size})"
+end

spec/coverage_check_spec.rb

@@ -59,3 +59,72 @@ def check_coverage(*reports)
     result.status.should_not == 0
   end
 end
+
+describe 'combined branch coverage check' do
+  let(:script) { File.expand_path('../script/check_coverage.rb', __dir__) }
+
+  def check_branches(*reports)
+    result = nil
+    directory do
+      paths = reports.each_with_index.map do |report, index|
+        path = "coverage-#{index}.info"
+        File.write(path, report)
+        path
+      end
+      result = shell([Gem.ruby, script, '--branches'] + paths)
+    end
+    result
+  end
+
+  it 'combines complementary branch hits across platforms and shards' do
+    result = check_branches(
+      "SF:a.rb\nDA:1,1\nBRDA:1,0,0,1\nBRDA:1,0,1,-\nend_of_record\n",
+      "SF:a.rb\nDA:1,0\nBRDA:1,0,0,0\nBRDA:1,0,1,2\nend_of_record\n",
+    )
+    expect(result.status).to eq(0)
+    expect(result.stdout).to include('Line coverage: 100% (1/1)', 'Branch coverage: 100% (2/2)')
+  end
+
+  ['0', '-'].each do |hits|
+    it "fails for an uncovered branch recorded as #{hits.inspect}" do
+      result = check_branches("SF:a.rb\nDA:1,1\nBRDA:1,0,0,99999\nBRDA:1,0,1,#{hits}\n")
+      expect(result.status).not_to eq(0)
+      expect(result.stderr).to include('a.rb:1 (block 0, branch 1)',
+                                       '100% branch coverage is required')
+    end
+  end
+
+  it 'keeps files, lines, blocks, and branch identities separate' do
+    reports = [
+      "SF:a.rb\nDA:1,1\nBRDA:1,0,0,1\nBRDA:1,0,1,1\nBRDA:1,1,0,1\nBRDA:2,0,0,1\n",
+      "SF:b.rb\nDA:1,1\nBRDA:1,0,0,0\n",
+    ]
+    result = check_branches(*reports)
+    expect(result.status).not_to eq(0)
+    expect(result.stderr).to include('b.rb:1 (block 0, branch 0)')
+  end
+
+  it 'requires branch records when enforcing branch coverage' do
+    result = check_branches("SF:a.rb\nDA:1,1\nend_of_record\n")
+    expect(result.status).not_to eq(0)
+    expect(result.stderr).to include('No branch coverage found')
+  end
+
+  it 'rejects branch records without a source file' do
+    result = check_branches("BRDA:1,0,0,1\n")
+    expect(result.status).not_to eq(0)
+    expect(result.stderr).to include('Branch coverage without a source file')
+  end
+
+  it 'resets the branch source at the end of each record' do
+    result = check_branches("SF:a.rb\nDA:1,1\nend_of_record\nBRDA:1,0,0,1\n")
+    expect(result.status).not_to eq(0)
+    expect(result.stderr).to include('Branch coverage without a source file')
+  end
+
+  it 'continues enforcing line coverage when every branch is covered' do
+    result = check_branches("SF:a.rb\nDA:1,0\nBRDA:1,0,0,1\n")
+    expect(result.status).not_to eq(0)
+    expect(result.stderr).to include('100% line coverage is required')
+  end
+end

spec/overcommit/cli_errors_spec.rb

@@ -91,4 +91,27 @@
     expect { cli.run }.to raise_error(SystemExit)
     expect(output.string).to include('Example: enabled (plugin)', 'Other: disabled')
   end
+
+  it 'runs an explicitly selected hook context' do
+    arguments.concat(['--run', 'pre-push'])
+    config = double('config', apply_environment!: nil)
+    context = double('context')
+    allow(cli).to receive(:config).and_return(config)
+    expect(Overcommit::HookContext).to receive(:create).
+      with('pre-push', config, [], an_instance_of(File)).and_return(context)
+    allow(Overcommit::Printer).to receive(:new).and_return(double('printer'))
+    allow(Overcommit::HookRunner).to receive(:new).and_return(double(run: true))
+    expect { cli.run }.to raise_error(SystemExit) { |error| expect(error.status).to eq(0) }
+  end
+
+  it 'exits successfully after diff hooks pass' do
+    arguments << '--diff'
+    config = double('config', apply_environment!: nil)
+    context = double('context')
+    allow(cli).to receive(:config).and_return(config)
+    allow(Overcommit::HookContext).to receive(:create).and_return(context)
+    allow(Overcommit::Printer).to receive(:new).and_return(double('printer'))
+    allow(Overcommit::HookRunner).to receive(:new).and_return(double(run: true))
+    expect { cli.run }.to raise_error(SystemExit) { |error| expect(error.status).to eq(0) }
+  end
 end

spec/overcommit/configuration_validator_spec.rb

@@ -124,4 +124,15 @@
       end
     end
   end
+
+  context 'when processor counts are invalid and no logger is supplied' do
+    let(:options) { {} }
+    let(:config_hash) do
+      { 'concurrency' => 1, 'PreCommit' => { 'Example' => { 'processors' => 2 } } }
+    end
+
+    it 'still raises a configuration error' do
+      expect { subject }.to raise_error(Overcommit::Exceptions::ConfigurationError, /processor/)
+    end
+  end
 end

spec/overcommit/gem_plugin_spec.rb

@@ -253,4 +253,10 @@ def changed_config(hash)
                                                 /Unable to activate company-hooks/)
     end
   end
+
+  it 'does not sign Overcommit itself as a plugin dependency' do
+    specification.add_runtime_dependency('overcommit')
+    expect(Gem::Specification).not_to receive(:find_by_name).with('overcommit', anything)
+    expect { plugin.update_signature! }.not_to raise_error
+  end
 end

spec/overcommit/git_config_spec.rb

@@ -237,4 +237,13 @@
       it { should == '--worktree' }
     end
   end
+
+  it 'resolves commit cleanup mode when no logger is configured' do
+    repo do
+      shell(%w[git config commit.cleanup strip])
+      allow(Overcommit::Utils).to receive(:parent_command).and_return('git commit')
+      allow(Overcommit::Utils).to receive(:log).and_return(nil)
+      expect(described_class.commit_cleanup_mode).to eq('strip')
+    end
+  end
 end

spec/overcommit/git_repo_spec.rb

@@ -595,4 +595,56 @@ def stage_files(paths)
       it { should be_empty }
     end
   end
+
+  describe 'restoring absent operation state' do
+    around do |example|
+      repo do
+        %w[merge_head merge_mode merge_msg cherry_head].each do |name|
+          described_class.instance_variable_set(:"@#{name}", nil)
+        end
+        example.run
+      ensure
+        %w[merge_head merge_mode merge_msg cherry_head].each do |name|
+          described_class.instance_variable_set(:"@#{name}", nil)
+        end
+      end
+    end
+
+    it 'leaves a repository without merge or cherry-pick state unchanged' do
+      described_class.store_merge_state
+      described_class.store_cherry_pick_state
+      described_class.restore_merge_state
+      described_class.restore_cherry_pick_state
+      %w[MERGE_HEAD MERGE_MODE MERGE_MSG CHERRY_PICK_HEAD].each do |name|
+        expect(File.exist?(Overcommit::Utils.git_path(name))).to eq(false)
+      end
+    end
+
+    it 'restores a merge head without requiring a merge mode or message' do
+      File.write(Overcommit::Utils.git_path('MERGE_HEAD'), 'a' * 40)
+      described_class.store_merge_state
+      FileUtils.rm(Overcommit::Utils.git_path('MERGE_HEAD'))
+      described_class.restore_merge_state
+      expect(File.read(Overcommit::Utils.git_path('MERGE_HEAD'))).to eq('a' * 40)
+      expect(File.exist?(Overcommit::Utils.git_path('MERGE_MODE'))).to eq(false)
+    end
+
+    it 'has no staged submodule removals without a gitmodules file' do
+      expect(described_class.staged_submodule_removals).to eq([])
+    end
+  end
+
+  it 'includes existing submodules when using Git older than 1.8.5' do
+    stub_const('Overcommit::GitRepo::GIT_VERSION', Overcommit::Utils::Version.new('1.8.4'))
+    allow(described_class).to receive(:`).and_return(<<~INI)
+      [submodule "example"]
+        path = example
+        url = https://example.com/example.git
+    INI
+    expect(Overcommit::Utils).to receive(:execute).with(%w[git submodule status example]).
+      and_return(double(success?: true))
+    expect(described_class.submodules).to eq([
+      described_class::Submodule.new('example', 'https://example.com/example.git')
+])
+  end
 end

spec/overcommit/hook/post_commit/git_guilt_spec.rb

@@ -49,4 +49,11 @@
 
     it { should pass }
   end
+
+  it 'colorizes blame changes when output is a terminal' do
+    allow($stdout).to receive(:tty?).and_return(true)
+    allow(subject).to receive(:initial_commit?).and_return(false)
+    allow(subject).to receive(:execute).and_return(double(success?: true, stdout: 'Tester +++'))
+    expect(subject.run).to eq([:warn, "Tester \e[32m+++\e[0m\e[31m\e[0m"])
+  end
 end

spec/overcommit/hook/pre_commit/bundle_check_spec.rb

@@ -60,4 +60,14 @@
       end
     end
   end
+
+  it 'preserves an existing lockfile when bundle check does not change it' do
+    repo do
+      File.write('Gemfile.lock', 'locked dependencies')
+      allow(subject).to receive(:execute).with(%w[git ls-files -o -i --exclude-standard]).
+        and_return(double(stdout: ''))
+      allow(subject).to receive(:execute).with(subject.command).and_return(double(success?: true))
+      expect(subject.run).to eq(:pass)
+    end
+  end
 end