cleaner: clean up broken and orphaned symlinks

2 files changed, 92 insertions, 26 deletions

diff --git a/Library/Homebrew/cleaner.rb b/Library/Homebrew/cleaner.rb
index 76c6ceb89..bf0639835 100644
--- a/Library/Homebrew/cleaner.rb
+++ b/Library/Homebrew/cleaner.rb
@@ -22,28 +22,36 @@ class Cleaner
       f.info.rmtree if f.info.directory? and not f.skip_clean? f.info
     end
 
-    # Remove empty folders.
-    # We want post-order traversal, so use a stack.
-    paths = []
-    f.prefix.find do |path|
-      if path.directory?
-        if f.skip_clean? path
-          Find.prune
-        else
-          paths << path
-        end
+    prune
+  end
+
+  private
+
+  def prune
+    dirs = []
+    symlinks = []
+
+    @f.prefix.find do |path|
+      if @f.skip_clean? path
+        Find.prune
+      elsif path.symlink?
+        symlinks << path
+      elsif path.directory?
+        dirs << path
       end
     end
 
-    paths.reverse_each do |d|
-      if d.children.empty? and not f.skip_clean? d
+    dirs.reverse_each do |d|
+      if d.children.empty?
         puts "rmdir: #{d} (empty)" if ARGV.verbose?
         d.rmdir
       end
     end
-  end
 
-  private
+    symlinks.reverse_each do |s|
+      s.unlink unless s.resolved_path_exists?
+    end
+  end
 
   # Set permissions for executables and non-executables
   def clean_file_permissions path
diff --git a/Library/Homebrew/test/test_cleaner.rb b/Library/Homebrew/test/test_cleaner.rb
index 33205a8d2..9a4f75a86 100644
--- a/Library/Homebrew/test/test_cleaner.rb
+++ b/Library/Homebrew/test/test_cleaner.rb
@@ -63,22 +63,80 @@ class CleanerTests < Test::Unit::TestCase
     assert subdir.directory?
   end
 
-  def test_fails_to_remove_symlink_when_target_was_pruned_first
-    mkpath @f.prefix/'b'
-    ln_s 'b', @f.prefix/'a'
-    assert_raises(Errno::ENOENT) { Cleaner.new @f }
+  def test_removes_symlink_when_target_was_pruned_first
+    dir = @f.prefix/'b'
+    symlink = @f.prefix/'a'
+
+    dir.mkpath
+    ln_s dir.basename, symlink
+
+    Cleaner.new @f
+
+    assert !dir.exist?
+    assert !symlink.symlink?
+    assert !symlink.exist?
+  end
+
+  def test_removes_symlink_pointing_to_empty_directory
+    dir = @f.prefix/'b'
+    symlink = @f.prefix/'c'
+
+    dir.mkpath
+    ln_s dir.basename, symlink
+
+    Cleaner.new @f
+
+    assert !dir.exist?
+    assert !symlink.symlink?
+    assert !symlink.exist?
   end
 
-  def test_fails_to_remove_symlink_pointing_to_empty_directory
-    mkpath @f.prefix/'b'
-    ln_s 'b', @f.prefix/'c'
-    assert_raises(Errno::ENOTDIR) { Cleaner.new @f }
+  def test_removes_broken_symlinks
+    symlink = @f.prefix/'symlink'
+    ln_s 'target', symlink
+
+    Cleaner.new @f
+
+    assert !symlink.symlink?
   end
 
-  def test_fails_to_remove_broken_symlink
-    ln_s 'target', @f.prefix/'symlink'
+  def test_skip_clean_broken_symlink
+    @f.class.skip_clean 'symlink'
+    symlink = @f.prefix/'symlink'
+    ln_s 'target', symlink
+
     Cleaner.new @f
-    assert @f.prefix.join('symlink').symlink?, "not a symlink"
-    assert !@f.prefix.join('symlink').exist?, "target exists"
+
+    assert symlink.symlink?
+  end
+
+  def test_skip_clean_symlink_pointing_to_empty_directory
+    @f.class.skip_clean 'c'
+    dir = @f.prefix/'b'
+    symlink = @f.prefix/'c'
+
+    dir.mkpath
+    ln_s dir.basename, symlink
+
+    Cleaner.new @f
+
+    assert !dir.exist?
+    assert symlink.symlink?
+    assert !symlink.exist?
+  end
+
+  def test_skip_clean_symlink_when_target_pruned
+    @f.class.skip_clean 'a'
+    dir = @f.prefix/'b'
+    symlink = @f.prefix/'a'
+
+    dir.mkpath
+    ln_s dir.basename, symlink
+
+    Cleaner.new @f
+
+    assert !dir.exist?
+    assert symlink.symlink?
+    assert !symlink.exist?
   end
 end