From b4d4ce0e819dad926dfd0073540dd73a5217e2e9 Mon Sep 17 00:00:00 2001 From: benmelz Date: Wed, 29 Jan 2025 19:26:01 -0500 Subject: [PATCH 1/2] Add `-diff` cli option for running precommit hooks against diffs (#860) For example, running `overcommit --diff main` from a feature branch will run pre-commit hooks against the diff between the two branches. I was able to very easily leverage existing code for the bulk of the feature - this is mainly just adding the cli option, a hook context to do the execution and some tests based on the existing `--run-all` test. --- For background, my team is responsible for a couple of really old, really large rails apps. Getting them completely in compliance with our various linters is a huge task that isn't getting done anytime soon (things are funky to the point that we've even observed breakages with "safe" auto-correct functions). I introduced/started heavily encouraging overcommit so that we at least don't add _new_ linting offenses and things will naturally improve over time. It's been great, but offenses still slip through though here and there, especially with juniors who might be getting away with not having a local install (and/or abusing `OVERCOMMIT_DISABLE=1`). An option like this would allow me to leverage the very useful "only apply to changed lines" logic within a ci environment and help enforce my desired "no new linting offenses" policy. --- lib/overcommit/cli.rb | 23 +++- lib/overcommit/hook_context.rb | 4 +- lib/overcommit/hook_context/base.rb | 4 +- lib/overcommit/hook_context/diff.rb | 37 ++++++ spec/integration/diff_flag_spec.rb | 39 ++++++ spec/overcommit/cli_spec.rb | 26 ++++ spec/overcommit/hook_context/diff_spec.rb | 143 ++++++++++++++++++++++ 7 files changed, 272 insertions(+), 4 deletions(-) create mode 100644 lib/overcommit/hook_context/diff.rb create mode 100644 spec/integration/diff_flag_spec.rb create mode 100644 spec/overcommit/hook_context/diff_spec.rb diff --git a/lib/overcommit/cli.rb b/lib/overcommit/cli.rb index f8127125..dafc545a 100644 --- a/lib/overcommit/cli.rb +++ b/lib/overcommit/cli.rb @@ -9,6 +9,7 @@ module Overcommit class CLI # rubocop:disable Metrics/ClassLength def initialize(arguments, input, logger) @arguments = arguments + @cli_options = {} @input = input @log = logger @options = {} @@ -28,6 +29,8 @@ def run sign when :run_all run_all + when :diff + diff end rescue Overcommit::Exceptions::ConfigurationSignatureChanged => e puts e @@ -45,7 +48,7 @@ def parse_arguments @parser = create_option_parser begin - @parser.parse!(@arguments) + @parser.parse!(@arguments, into: @cli_options) # Default action is to install @options[:action] ||= :install @@ -98,6 +101,11 @@ def add_installation_options(opts) @options[:action] = :run_all @options[:hook_to_run] = arg ? arg.to_s : 'run-all' end + + opts.on('--diff [ref]', 'Run pre_commit hooks against the diff between a given ref. Defaults to `main`.') do |arg| # rubocop:disable Layout/LineLength + @options[:action] = :diff + arg + end end def add_other_options(opts) @@ -209,6 +217,19 @@ def run_all halt(status ? 0 : 65) end + def diff + empty_stdin = File.open(File::NULL) # pre-commit hooks don't take input + context = Overcommit::HookContext.create('diff', config, @arguments, empty_stdin, **@cli_options) # rubocop:disable Layout/LineLength + config.apply_environment!(context, ENV) + + printer = Overcommit::Printer.new(config, log, context) + runner = Overcommit::HookRunner.new(config, log, context, printer) + + status = runner.run + + halt(status ? 0 : 65) + end + # Used for ease of stubbing in tests def halt(status = 0) exit status diff --git a/lib/overcommit/hook_context.rb b/lib/overcommit/hook_context.rb index acdff1bb..5863ed94 100644 --- a/lib/overcommit/hook_context.rb +++ b/lib/overcommit/hook_context.rb @@ -2,13 +2,13 @@ # Utility module which manages the creation of {HookContext}s. module Overcommit::HookContext - def self.create(hook_type, config, args, input) + def self.create(hook_type, config, args, input, **cli_options) hook_type_class = Overcommit::Utils.camel_case(hook_type) underscored_hook_type = Overcommit::Utils.snake_case(hook_type) require "overcommit/hook_context/#{underscored_hook_type}" - Overcommit::HookContext.const_get(hook_type_class).new(config, args, input) + Overcommit::HookContext.const_get(hook_type_class).new(config, args, input, **cli_options) rescue LoadError, NameError => e # Could happen when a symlink was created for a hook type Overcommit does # not yet support. diff --git a/lib/overcommit/hook_context/base.rb b/lib/overcommit/hook_context/base.rb index 077394fb..b50698c9 100644 --- a/lib/overcommit/hook_context/base.rb +++ b/lib/overcommit/hook_context/base.rb @@ -18,10 +18,12 @@ class Base # @param config [Overcommit::Configuration] # @param args [Array] # @param input [IO] standard input stream - def initialize(config, args, input) + # @param options [Hash] cli options + def initialize(config, args, input, **options) @config = config @args = args @input = input + @options = options end # Executes a command as if it were a regular git hook, passing all diff --git a/lib/overcommit/hook_context/diff.rb b/lib/overcommit/hook_context/diff.rb new file mode 100644 index 00000000..0c4b97e0 --- /dev/null +++ b/lib/overcommit/hook_context/diff.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'overcommit/git_repo' + +module Overcommit::HookContext + # Simulates a pre-commit context based on the diff with another git ref. + # + # This results in pre-commit hooks running against the changes between the current + # and another ref, which is useful for automated CI scripts. + class Diff < Base + def modified_files + @modified_files ||= Overcommit::GitRepo.modified_files(refs: @options[:diff]) + end + + def modified_lines_in_file(file) + @modified_lines ||= {} + @modified_lines[file] ||= Overcommit::GitRepo.extract_modified_lines(file, + refs: @options[:diff]) + end + + def hook_class_name + 'PreCommit' + end + + def hook_type_name + 'pre_commit' + end + + def hook_script_name + 'pre-commit' + end + + def initial_commit? + @initial_commit ||= Overcommit::GitRepo.initial_commit? + end + end +end diff --git a/spec/integration/diff_flag_spec.rb b/spec/integration/diff_flag_spec.rb new file mode 100644 index 00000000..dcaa1bd3 --- /dev/null +++ b/spec/integration/diff_flag_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'overcommit --diff' do + subject { shell(%w[overcommit --diff main]) } + + context 'when using an existing pre-commit hook script' do + let(:script_name) { 'test-script' } + let(:script_contents) { "#!/bin/bash\nexit 0" } + let(:script_path) { ".#{Overcommit::OS::SEPARATOR}#{script_name}" } + + let(:config) do + { + 'PreCommit' => { + 'MyHook' => { + 'enabled' => true, + 'required_executable' => script_path, + } + } + } + end + + around do |example| + repo do + File.open('.overcommit.yml', 'w') { |f| f.puts(config.to_yaml) } + echo(script_contents, script_path) + `git add #{script_path}` + FileUtils.chmod(0o755, script_path) + example.run + end + end + + it 'completes successfully without blocking' do + wait_until(timeout: 10) { subject } # Need to wait long time for JRuby startup + subject.status.should == 0 + end + end +end diff --git a/spec/overcommit/cli_spec.rb b/spec/overcommit/cli_spec.rb index 40923ad3..3d95b09d 100644 --- a/spec/overcommit/cli_spec.rb +++ b/spec/overcommit/cli_spec.rb @@ -2,6 +2,7 @@ require 'spec_helper' require 'overcommit/cli' +require 'overcommit/hook_context/diff' require 'overcommit/hook_context/run_all' describe Overcommit::CLI do @@ -125,5 +126,30 @@ subject end end + + context 'with the diff switch specified' do + let(:arguments) { ['--diff=some-ref'] } + let(:config) { Overcommit::ConfigurationLoader.default_configuration } + + before do + cli.stub(:halt) + Overcommit::HookRunner.any_instance.stub(:run) + end + + it 'creates a HookRunner with the diff context' do + Overcommit::HookRunner.should_receive(:new). + with(config, + logger, + instance_of(Overcommit::HookContext::Diff), + instance_of(Overcommit::Printer)). + and_call_original + subject + end + + it 'runs the HookRunner' do + Overcommit::HookRunner.any_instance.should_receive(:run) + subject + end + end end end diff --git a/spec/overcommit/hook_context/diff_spec.rb b/spec/overcommit/hook_context/diff_spec.rb new file mode 100644 index 00000000..0c712fbc --- /dev/null +++ b/spec/overcommit/hook_context/diff_spec.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'overcommit/hook_context/diff' + +describe Overcommit::HookContext::Diff do + let(:config) { double('config') } + let(:args) { [] } + let(:input) { double('input') } + let(:context) { described_class.new(config, args, input, diff: 'master') } + + describe '#modified_files' do + subject { context.modified_files } + + context 'when repo contains no files' do + around do |example| + repo do + `git commit --allow-empty -m "Initial commit"` + `git checkout -b other-branch 2>&1` + example.run + end + end + + it { should be_empty } + end + + context 'when the repo contains files that are unchanged from the ref' do + around do |example| + repo do + touch('some-file') + `git add some-file` + touch('some-other-file') + `git add some-other-file` + `git commit -m "Add files"` + `git checkout -b other-branch 2>&1` + example.run + end + end + + it { should be_empty } + end + + context 'when repo contains files that have been changed from the ref' do + around do |example| + repo do + touch('some-file') + `git add some-file` + touch('some-other-file') + `git add some-other-file` + `git commit -m "Add files"` + `git checkout -b other-branch 2>&1` + File.open('some-file', 'w') { |f| f.write("hello\n") } + `git add some-file` + `git commit -m "Edit file"` + example.run + end + end + + it { should == %w[some-file].map { |file| File.expand_path(file) } } + end + + context 'when repo contains submodules' do + around do |example| + submodule = repo do + touch 'foo' + `git add foo` + `git commit -m "Initial commit"` + end + + repo do + `git submodule add #{submodule} test-sub 2>&1 > #{File::NULL}` + `git commit --allow-empty -m "Initial commit"` + `git checkout -b other-branch 2>&1` + example.run + end + end + + it { should_not include File.expand_path('test-sub') } + end + end + + describe '#modified_lines_in_file' do + let(:modified_file) { 'some-file' } + subject { context.modified_lines_in_file(modified_file) } + + context 'when file contains a trailing newline' do + around do |example| + repo do + touch(modified_file) + `git add #{modified_file}` + `git commit -m "Add file"` + `git checkout -b other-branch 2>&1` + File.open(modified_file, 'w') { |f| (1..3).each { |i| f.write("#{i}\n") } } + `git add #{modified_file}` + `git commit -m "Edit file"` + example.run + end + end + + it { should == Set.new(1..3) } + end + + context 'when file does not contain a trailing newline' do + around do |example| + repo do + touch(modified_file) + `git add #{modified_file}` + `git commit -m "Add file"` + `git checkout -b other-branch 2>&1` + File.open(modified_file, 'w') do |f| + (1..2).each { |i| f.write("#{i}\n") } + f.write(3) + end + `git add #{modified_file}` + `git commit -m "Edit file"` + example.run + end + end + + it { should == Set.new(1..3) } + end + end + + describe '#hook_type_name' do + subject { context.hook_type_name } + + it { should == 'pre_commit' } + end + + describe '#hook_script_name' do + subject { context.hook_script_name } + + it { should == 'pre-commit' } + end + + describe '#initial_commit?' do + subject { context.initial_commit? } + + before { Overcommit::GitRepo.stub(:initial_commit?).and_return(true) } + + it { should == true } + end +end From 43e17fb384e51101cd45449e03d39188f18959f9 Mon Sep 17 00:00:00 2001 From: Shane da Silva Date: Wed, 29 Jan 2025 16:32:23 -0800 Subject: [PATCH 2/2] Cut version 0.66.0 (#861) --- CHANGELOG.md | 4 ++++ README.md | 1 + lib/overcommit/version.rb | 2 +- 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bbe08fb9..d82b5b50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Overcommit Changelog +## 0.66.0 + +* Add `--diff` CLI option for running pre-commit hooks against only changed files + ## 0.65.0 * Load bundled gems on expected version diff --git a/README.md b/README.md index 877e9396..a0d46726 100644 --- a/README.md +++ b/README.md @@ -131,6 +131,7 @@ Command Line Flag | Description `-f`/`--force` | Don't bail on install if other hooks already exist--overwrite them `-l`/`--list-hooks` | Display all available hooks in the current repository `-r`/`--run` | Run pre-commit hook against all tracked files in repository +`--diff ` | Run pre-commit hook against all changed files relative to `` `-t`/`--template-dir` | Print location of template directory `-h`/`--help` | Show command-line flag documentation `-v`/`--version` | Show version diff --git a/lib/overcommit/version.rb b/lib/overcommit/version.rb index 0b718a39..c1091e52 100644 --- a/lib/overcommit/version.rb +++ b/lib/overcommit/version.rb @@ -2,5 +2,5 @@ # Defines the gem version. module Overcommit - VERSION = '0.65.0' + VERSION = '0.66.0' end