Rename ignore_branch_deletions to include_branch_deletions

This emphasizes that the default is not to include remote branch
deletions, and uses language like "include" which is already used in
other option names.

Author: sds

README.md

@@ -228,6 +228,7 @@ Option                                  | Description
 `exclude`                               | File paths or glob patterns of files that do not apply to this hook. This is used to exclude any files that would have been matched by `include`.
 `exclude_branches`                      | List of branch names or glob patterns of branches that this hook should not run against.
 `exclude_remotes`                       | *`PrePush` hooks only.* List of remote names that the hook should not run against.
+`include_branch_deletions`              | *`PrePush` hooks only.* By default, `PrePush` hooks will **not** run for pushes that delete a remote branch. Set to `true` to have the hook run even for deleted remote branches.
 `problem_on_unmodified_line`            | How to treat errors reported on lines that weren't modified during the action captured by this hook (e.g. for pre-commit hooks, warnings/errors reported on lines that were not staged with `git add` may not be warnings/errors you care about). Valid values are `report`: report errors/warnings as-is regardless of line location (default); `warn`: report errors as warnings if they are on lines you didn't modify; and `ignore`: don't display errors/warnings at all if they are on lines you didn't modify (`ignore` is _not_ recommended).
 `on_fail`                               | Change the status of a failed hook to `warn` or `pass`. This allows you to treat failures as warnings or potentially ignore them entirely, but you should use caution when doing so as you might be hiding important information.
 `on_warn`                               | Similar to `on_fail`, change the status of a hook that returns a warning status to either `pass` (you wish to silence warnings entirely) or `fail` (you wish to treat all warnings as errors).

lib/overcommit/hook/pre_push/base.rb

@@ -9,24 +9,20 @@ class Base < Overcommit::Hook::Base
 
     def_delegators :@context, :remote_name, :remote_url, :pushed_refs
 
-    def skip?
-      super ||
-        exclude_remotes.include?(remote_name) ||
-        skip_for_remote_branch_deletion?
+    def run?
+      super &&
+        !exclude_remotes.include?(remote_name) &&
+        (include_branch_deletions? || !@context.remote_branch_deletion?)
     end
 
     private
 
-    def skip_for_remote_branch_deletion?
-      ignore_branch_deletions? && @context.remote_branch_deletion?
-    end
-
     def exclude_remotes
       @config['exclude_remotes'] || []
     end
 
-    def ignore_branch_deletions?
-      @config['ignore_branch_deletions'] != false
+    def include_branch_deletions?
+      @config['include_branch_deletions']
     end
   end
 end

spec/overcommit/hook/pre_push/base_spec.rb

@@ -8,153 +8,91 @@
   let(:config) { double('config') }
   let(:context) { double('context') }
   let(:hook) { described_class.new(config, context) }
+
   describe '#run?' do
-    let(:hook_config) do
-      { 'skip' => skip }
-    end
+    let(:hook_config) { {} }
 
     before do
       allow(context).to receive(:remote_name).and_return(remote_name)
       allow(context).to receive(:remote_branch_deletion?).and_return(remote_branch_deletion?)
       allow(config).to receive(:for_hook).and_return(hook_config)
     end
 
-    subject { hook.skip? }
-
-    context 'skip is true' do
-      let(:skip) { true }
-
-      it { subject.should == true }
-    end
-
-    context 'skip is false' do
-      let(:skip) { false }
-
-      it { subject.should == false }
-    end
+    subject { hook.run? }
 
     context 'with exclude_remotes specified' do
       let(:hook_config) do
-        { 'skip' => skip, 'exclude_remotes' => exclude_remotes }
+        { 'exclude_remotes' => exclude_remotes }
       end
-      let(:exclude_remotes) { nil }
 
-      context 'skip is true and exclude_remotes is nil' do
-        let(:skip) { true }
+      context 'exclude_remotes is nil' do
         let(:exclude_remotes) { nil }
 
         it { subject.should == true }
       end
 
-      context 'skip is false and exclude_remotes is nil' do
-        let(:skip) { false }
-        let(:exclude_remotes) { nil }
-      end
+      context 'exclude_remotes includes the remote' do
+        let(:exclude_remotes) { [remote_name] }
 
-      context 'skip is true and matching exclude_remotes is nil' do
-        let(:skip) { true }
-        let(:exclude_remotes) { ['origin'] }
-
-        it { subject.should == true }
-      end
-
-      context 'skip is false and matching exclude_remotes is nil' do
-        let(:skip) { false }
-        let(:exclude_remotes) { ['origin'] }
+        it { subject.should == false }
       end
 
-      context 'skip is true and non-matching exclude_remotes is nil' do
-        let(:skip) { true }
+      context 'exclude_remotes does not include the remote' do
         let(:exclude_remotes) { ['heroku'] }
 
         it { subject.should == true }
       end
-
-      context 'skip is false and non-matching exclude_remotes is nil' do
-        let(:skip) { false }
-        let(:exclude_remotes) { ['heroku'] }
-
-        it { subject.should == false }
-      end
     end
 
-    context 'with ignore_branch_deletions specified' do
+    context 'with include_branch_deletions specified' do
       let(:hook_config) do
-        { 'skip' => skip, 'ignore_branch_deletions' => ignore_branch_deletions }
+        { 'include_branch_deletions' => include_branch_deletions }
       end
       let(:remote_branch_deletion?) { false }
-      let(:ignore_branch_deletions) { false }
-
-      context(<<~DESC) do
-        skip is true and
-        remote_branch_deletion? is false and
-        ignore_branch_deletions false' do
-      DESC
-        let(:skip) { true }
+      let(:include_branch_deletions) { false }
+
+      context 'when remote branch is not being deleted' do
         let(:remote_branch_deletion?) { false }
-        let(:ignore_branch_deletions) { nil }
 
-        it { subject.should == true }
-      end
+        context 'when include_branch_deletions is not specified' do
+          let(:include_branch_deletions) { nil }
 
-      context(<<~DESC) do
-        skip is false and
-        remote_branch_deletion? is false and
-        ignore_branch_deletions false' do
-      DESC
-        let(:skip) { false }
-        let(:remote_branch_deletion?) { false }
-        let(:ignore_branch_deletions) { false }
+          it { subject.should == true }
+        end
 
-        it { subject.should == false }
-      end
+        context 'when include_branch_deletions is false' do
+          let(:include_branch_deletions) { false }
 
-      context(<<~DESC) do
-        skip is false and
-        remote_branch_deletion? is true and
-        ignore_branch_deletions false' do
-      DESC
-        let(:skip) { false }
-        let(:remote_branch_deletion?) { true }
-        let(:ignore_branch_deletions) { false }
+          it { subject.should == true }
+        end
 
-        it { subject.should == false }
+        context 'when include_branch_deletions is true' do
+          let(:include_branch_deletions) { true }
+
+          it { subject.should == true }
+        end
       end
 
-      context(<<~DESC) do
-        skip is false and
-        remote_branch_deletion? is true and
-        ignore_branch_deletions true' do
-      DESC
-        let(:skip) { false }
+      context 'when remote branch is being deleted' do
         let(:remote_branch_deletion?) { true }
-        let(:ignore_branch_deletions) { true }
 
-        it { subject.should == true }
-      end
+        context 'when include_branch_deletions is not specified' do
+          let(:include_branch_deletions) { nil }
 
-      context(<<~DESC) do
-        skip is false and
-        remote_branch_deletion? is false and
-        ignore_branch_deletions true' do
-      DESC
-        let(:skip) { false }
-        let(:remote_branch_deletion?) { false }
-        let(:ignore_branch_deletions) { true }
+          it { subject.should == false }
+        end
 
-        it { subject.should == false }
-      end
+        context 'when include_branch_deletions is false' do
+          let(:include_branch_deletions) { false }
 
-      context(<<-DESC) do
-        skip is true and
-        remote_branch_deletion? is true and
-        ignore_branch_deletions true' do
-      DESC
-        let(:skip) { true }
-        let(:remote_branch_deletion?) { true }
-        let(:ignore_branch_deletions) { true }
+          it { subject.should == false }
+        end
 
-        it { subject.should == true }
+        context 'when include_branch_deletions is true' do
+          let(:include_branch_deletions) { true }
+
+          it { subject.should == true }
+        end
       end
     end
   end