From 31b0b593fec503c78e669270b46ae8bd088782ff Mon Sep 17 00:00:00 2001
From: Ivan Kuchin <tadump+git@gmail.com>
Date: Fri, 7 Nov 2014 00:13:49 +0100
Subject: [PATCH] extract check_fail! from check! in Bin to show warnings only
 once, resolves #69

---
 CHANGELOG.markdown                    |  1 +
 lib/image_optim/bin_resolver.rb       |  4 +++-
 lib/image_optim/bin_resolver/bin.rb   | 20 ++++++++++++++++----
 spec/image_optim/bin_resolver_spec.rb | 13 ++++++++-----
 4 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/CHANGELOG.markdown b/CHANGELOG.markdown
index bcdae8cd..5cb3b084 100644
--- a/CHANGELOG.markdown
+++ b/CHANGELOG.markdown
@@ -2,6 +2,7 @@
 
 ## unreleased
 
+* Don't warn multiple times about problematic binary [#69](https://github.com/toy/image_optim/issues/69) [@toy](https://github.com/toy)
 * Run gisicle two times (with interlace off then with on) if interlace is not set explicitly [#70](https://github.com/toy/image_optim/issues/70) [@toy](https://github.com/toy)
 * Remove app and other extensions from gif images [@toy](https://github.com/toy)
 * Change behaviour of gifsicle interlace option to deinterlace for `false`, pass `nil` to leave as is [@toy](https://github.com/toy)
diff --git a/lib/image_optim/bin_resolver.rb b/lib/image_optim/bin_resolver.rb
index 5a86987a..12e1c880 100644
--- a/lib/image_optim/bin_resolver.rb
+++ b/lib/image_optim/bin_resolver.rb
@@ -40,10 +40,12 @@ def resolve!(name)
         end
 
         @bins[name] = bin
+
+        bin.check! if bin
       end
 
       if @bins[name]
-        @bins[name].check!
+        @bins[name].check_fail!
       else
         fail BinNotFound, "`#{name}` not found"
       end
diff --git a/lib/image_optim/bin_resolver/bin.rb b/lib/image_optim/bin_resolver/bin.rb
index 20e16b6c..428fc446 100644
--- a/lib/image_optim/bin_resolver/bin.rb
+++ b/lib/image_optim/bin_resolver/bin.rb
@@ -21,8 +21,8 @@ def to_s
         "#{name} #{version || '?'} at #{path}"
       end
 
-      # Fail or warn if version is known to misbehave depending on severity
-      def check!
+      # Fail if version will not work properly
+      def check_fail!
         fail BadVersion, "didn't get version of #{self}" unless version
 
         is = ComparableCondition.is
@@ -32,6 +32,20 @@ def check!
           when c = is.between?('1.7.60', '1.7.65')
             fail BadVersion, "#{self} (#{c}) is known to produce broken pngs"
           end
+        when :pngquant
+          case version
+          when c = is < '2.0'
+            fail BadVersion, "#{self} (#{c}) is not supported"
+          end
+        end
+      end
+
+      # Run check_fail!, otherwise warn if version is known to misbehave
+      def check!
+        check_fail!
+
+        is = ComparableCondition.is
+        case name
         when :advpng
           case version
           when c = is < '1.17'
@@ -39,8 +53,6 @@ def check!
           end
         when :pngquant
           case version
-          when c = is < '2.0'
-            fail BadVersion, "#{self} (#{c}) is not supported"
           when c = is < '2.1'
             warn "WARN: #{self} (#{c}) may be lossy even with quality `100-`"
           end
diff --git a/spec/image_optim/bin_resolver_spec.rb b/spec/image_optim/bin_resolver_spec.rb
index 35b7a49b..a3a7926b 100644
--- a/spec/image_optim/bin_resolver_spec.rb
+++ b/spec/image_optim/bin_resolver_spec.rb
@@ -102,7 +102,8 @@ def self.path
       expect(resolver).to receive(:full_path).with(:ls).and_return('/bin/ls')
       bin = double
       expect(Bin).to receive(:new).with(:ls, '/bin/ls').and_return(bin)
-      expect(bin).to receive(:check!).exactly(5).times
+      expect(bin).to receive(:check!).once
+      expect(bin).to receive(:check_fail!).exactly(5).times
 
       5.times do
         expect(resolver.resolve!(:ls)).to eq(bin)
@@ -149,7 +150,8 @@ def self.path
       bin = double
       expect(Bin).to receive(:new).
         with(:image_optim, File.expand_path(path)).and_return(bin)
-      expect(bin).to receive(:check!).exactly(5).times
+      expect(bin).to receive(:check!).once
+      expect(bin).to receive(:check_fail!).exactly(5).times
 
       at_exit_blocks = []
       expect(resolver).to receive(:at_exit).once do |&block|
@@ -202,9 +204,10 @@ def self.path
       bin = double
       expect(Bin).to receive(:new).once.with(:ls, '/bin/ls').and_return(bin)
 
-      check_count = 0
+      count = 0
       mutex = Mutex.new
-      allow(bin).to receive(:check!){ mutex.synchronize{ check_count += 1 } }
+      allow(bin).to receive(:check!).once
+      allow(bin).to receive(:check_fail!){ mutex.synchronize{ count += 1 } }
 
       10.times.map do
         Thread.new do
@@ -212,7 +215,7 @@ def self.path
         end
       end.each(&:join)
 
-      expect(check_count).to eq(10)
+      expect(count).to eq(10)
     end
   end