tap: run readall when tapping. (#396)

* readall: move readall logic to new class.

* tap: run readall when tapping.

This will prevent tapping an tap with syntax errors from causing issues
for users.

Fixes #58.

4 files changed, 101 insertions, 68 deletions

diff --git a/Library/Homebrew/cmd/readall.rb b/Library/Homebrew/cmd/readall.rb
index a2447c0c8..5442e4206 100644
--- a/Library/Homebrew/cmd/readall.rb
+++ b/Library/Homebrew/cmd/readall.rb
@@ -3,14 +3,12 @@
 # when making significant changes to formula.rb,
 # or to determine if any current formulae have Ruby issues
 
-require "formula"
-require "tap"
-require "thread"
+require "readall"
 
 module Homebrew
   def readall
-    if ARGV.delete("--syntax")
-      ruby_files = Queue.new
+    if ARGV.include?("--syntax")
+      ruby_files = []
       scan_files = %W[
         #{HOMEBREW_LIBRARY}/*.rb
         #{HOMEBREW_LIBRARY}/Homebrew/**/*.rb
@@ -20,69 +18,17 @@ module Homebrew
         ruby_files << rb
       end
 
-      failed = false
-      workers = (0...Hardware::CPU.cores).map do
-        Thread.new do
-          begin
-            while rb = ruby_files.pop(true)
-              # As a side effect, print syntax errors/warnings to `$stderr`.
-              failed = true if syntax_errors_or_warnings?(rb)
-            end
-          rescue ThreadError # ignore empty queue error
-          end
-        end
-      end
-      workers.each(&:join)
-      Homebrew.failed = failed
+      Homebrew.failed = true unless Readall.valid_ruby_syntax?(ruby_files)
     end
 
-    formulae = []
-    alias_dirs = []
-    if ARGV.named.empty?
-      formulae = Formula.files
-      alias_dirs = Tap.map(&:alias_dir)
+    options = { :aliases => ARGV.include?("--aliases") }
+    taps = if ARGV.named.any?
+      [Tap.fetch(ARGV.named.first)]
     else
-      tap = Tap.fetch(ARGV.named.first)
-      raise TapUnavailableError, tap.name unless tap.installed?
-      formulae = tap.formula_files
-      alias_dirs = [tap.alias_dir]
+      Tap
     end
-
-    if ARGV.delete("--aliases")
-      alias_dirs.each do |alias_dir|
-        next unless alias_dir.directory?
-        Pathname.glob("#{alias_dir}/*").each do |f|
-          next unless f.symlink?
-          next if f.file?
-          onoe "Broken alias: #{f}"
-          Homebrew.failed = true
-        end
-      end
-    end
-
-    formulae.each do |file|
-      begin
-        Formulary.factory(file)
-      rescue Interrupt
-        raise
-      rescue Exception => e
-        onoe "problem in #{file}"
-        puts e
-        Homebrew.failed = true
-      end
+    taps.each do |tap|
+      Homebrew.failed = true unless Readall.valid_tap?(tap, options)
     end
   end
-
-  private
-
-  def syntax_errors_or_warnings?(rb)
-    # Retrieve messages about syntax errors/warnings printed to `$stderr`, but
-    # discard a `Syntax OK` printed to `$stdout` (in absence of syntax errors).
-    messages = Utils.popen_read("#{RUBY_PATH} -c -w #{rb} 2>&1 >/dev/null")
-    $stderr.print messages
-
-    # Only syntax errors result in a non-zero status code. To detect syntax
-    # warnings we also need to inspect the output to `$stderr`.
-    !$?.success? || !messages.chomp.empty?
-  end
 end
diff --git a/Library/Homebrew/readall.rb b/Library/Homebrew/readall.rb
new file mode 100644
index 000000000..4d09226f5
--- /dev/null
+++ b/Library/Homebrew/readall.rb
@@ -0,0 +1,81 @@
+require "formula"
+require "tap"
+require "thread"
+require "readall"
+
+module Readall
+  class << self
+    def valid_ruby_syntax?(ruby_files)
+      ruby_files_queue = Queue.new
+      ruby_files.each { |f| ruby_files_queue << f }
+      failed = false
+      workers = (0...Hardware::CPU.cores).map do
+        Thread.new do
+          begin
+            while rb = ruby_files_queue.pop(true)
+              # As a side effect, print syntax errors/warnings to `$stderr`.
+              failed = true if syntax_errors_or_warnings?(rb)
+            end
+          rescue ThreadError # ignore empty queue error
+          end
+        end
+      end
+      workers.each(&:join)
+      !failed
+    end
+
+    def valid_aliases?(alias_dirs)
+      failed = false
+      alias_dirs.each do |alias_dir|
+        next unless alias_dir.directory?
+        alias_dir.children.each do |f|
+          next unless f.symlink?
+          next if f.file?
+          onoe "Broken alias: #{f}"
+          failed = true
+        end
+      end
+      !failed
+    end
+
+    def valid_formulae?(formulae)
+      failed = false
+      formulae.each do |file|
+        begin
+          Formulary.factory(file)
+        rescue Interrupt
+          raise
+        rescue Exception => e
+          onoe "Invalid formula: #{file}"
+          puts e
+          failed = true
+        end
+      end
+      !failed
+    end
+
+    def valid_tap?(tap, options = {})
+      failed = false
+      if options[:aliases]
+        valid_aliases = valid_aliases?([tap.alias_dir])
+        failed = true unless valid_aliases
+      end
+      valid_formulae = valid_formulae?(tap.formula_files)
+      failed = true unless valid_formulae
+      !failed
+    end
+
+    private
+
+    def syntax_errors_or_warnings?(rb)
+      # Retrieve messages about syntax errors/warnings printed to `$stderr`, but
+      # discard a `Syntax OK` printed to `$stdout` (in absence of syntax errors).
+      messages = Utils.popen_read("#{RUBY_PATH} -c -w #{rb} 2>&1 >/dev/null")
+      $stderr.print messages
+
+      # Only syntax errors result in a non-zero status code. To detect syntax
+      # warnings we also need to inspect the output to `$stderr`.
+      !$?.success? || !messages.chomp.empty?
+    end
+  end
+end
diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb
index 0b10f20c0..3dda1cab8 100644
--- a/Library/Homebrew/tap.rb
+++ b/Library/Homebrew/tap.rb
@@ -1,4 +1,5 @@
 require "extend/string"
+require "readall"
 
 # a {Tap} is used to extend the formulae provided by Homebrew core.
 # Usually, it's synced with a remote git repository. And it's likely
@@ -211,9 +212,14 @@ class Tap
 
     begin
       safe_system "git", *args
-    rescue Interrupt, ErrorDuringExecution
+      unless Readall.valid_tap?(self, :aliases => true)
+        raise "Cannot tap #{name}: invalid syntax in tap!"
+      end
+    rescue Interrupt, ErrorDuringExecution, RuntimeError
       ignore_interrupts do
-        sleep 0.1 # wait for git to cleanup the top directory when interrupt happens.
+        # wait for git to possibly cleanup the top directory when interrupt happens.
+        sleep 0.1
+        FileUtils.rm_rf path
         path.parent.rmdir_if_possible
       end
       raise
diff --git a/Library/Homebrew/test/test_tap.rb b/Library/Homebrew/test/test_tap.rb
index 94500babb..8a03a305d 100644
--- a/Library/Homebrew/test/test_tap.rb
+++ b/Library/Homebrew/test/test_tap.rb
@@ -135,7 +135,7 @@ class TapTest < Homebrew::TestCase
     end
     refute_predicate version_tap, :private?
   ensure
-    version_tap.path.rmtree
+    version_tap.path.rmtree if version_tap
   end
 
   def test_remote_not_git_repo
@@ -220,7 +220,7 @@ class TapTest < Homebrew::TestCase
     refute_predicate HOMEBREW_PREFIX/"share/man/man1/brew-tap-cmd.1", :exist?
     refute_predicate HOMEBREW_PREFIX/"share/man/man1", :exist?
   ensure
-    (HOMEBREW_PREFIX/"share").rmtree
+    (HOMEBREW_PREFIX/"share").rmtree if (HOMEBREW_PREFIX/"share").exist?
   end
 
   def test_pin_and_unpin