abort on symlink loop + refactor Record#build

lib/listen/record.rb

@@ -1,7 +1,9 @@
+require 'listen/record/entry'
+require 'listen/record/symlink_detector'
+
 module Listen
   class Record
     include Celluloid
-
     # TODO: one Record object per watched directory?
 
     # TODO: deprecate
@@ -100,34 +102,32 @@ def _fast_unset_path(dir, dirname, basename)
       end
     end
 
+    # TODO: test with a file name given
+    # TODO: test other permissions
+    # TODO: test with mixed encoding
     def _fast_build(root)
+      symlink_detector = SymlinkDetector.new
       @paths[root] = _auto_hash
-      left = Queue.new
-      left << '.'
-
-      until left.empty?
-        dirname = left.pop
-        add_dir(root, dirname)
-
-        path = ::File.join(root, dirname)
-        current = Dir.entries(path.to_s) - %w(. ..)
-
-        current.each do |entry|
-          full_path = ::File.join(path, entry)
-
-          if Dir.exist?(full_path)
-            left << (dirname == '.' ? entry : ::File.join(dirname, entry))
-          else
-            begin
-              lstat = ::File.lstat(full_path)
-              data = { mtime: lstat.mtime.to_f, mode: lstat.mode }
-              _fast_update_file(root, dirname, entry, data)
-            rescue SystemCallError
-              _fast_unset_path(root, dirname, entry)
-            end
-          end
-        end
-      end
+      remaining = Queue.new
+      remaining << Entry.new(root, nil, nil)
+      _fast_build_dir(remaining, symlink_detector) until remaining.empty?
+    end
+
+    def _fast_build_dir(remaining, symlink_detector)
+      entry = remaining.pop
+      entry.children.each { |child| remaining << child }
+      symlink_detector.verify_unwatched!(entry)
+      add_dir(entry.root, entry.record_dir_key)
+    rescue Errno::ENOTDIR
+      _fast_try_file(entry)
+    rescue SystemCallError
+      _fast_unset_path(entry.root, entry.relative, entry.name)
+    end
+
+    def _fast_try_file(entry)
+      _fast_update_file(entry.root, entry.relative, entry.name, entry.meta)
+    rescue SystemCallError
+      _fast_unset_path(entry.root, entry.relative, entry.name)
     end
   end
 end

lib/listen/record/entry.rb

@@ -0,0 +1,51 @@
+module Listen
+  # @private api
+  class Record
+    # Represents a directory entry (dir or file)
+    class Entry
+      # file: "/home/me/watched_dir", "app/models", "foo.rb"
+      # dir, "/home/me/watched_dir", "."
+      def initialize(root, relative, name = nil)
+        @root, @relative, @name = root, relative, name
+      end
+
+      attr_reader :root, :relative, :name
+
+      def children
+        child_relative = _join
+        (Dir.entries(sys_path) - %w(. ..)).map do |name|
+          Entry.new(@root, child_relative, name)
+        end
+      end
+
+      def meta
+        lstat = ::File.lstat(sys_path)
+        { mtime: lstat.mtime.to_f, mode: lstat.mode }
+      end
+
+      # record hash is e.g.
+      # if @record["/home/me/watched_dir"]["project/app/models"]["foo.rb"]
+      # if @record["/home/me/watched_dir"]["project/app"]["models"]
+      # record_dir_key is "project/app/models"
+      def record_dir_key
+        ::File.join(*[@relative, @name].compact)
+      end
+
+      def sys_path
+        # Use full path in case someone uses chdir
+        ::File.join(*[@root, @relative, @name].compact)
+      end
+
+      def real_path
+        @real_path ||= ::File.realpath(sys_path)
+      end
+
+      private
+
+      def _join
+        args = [@relative, @name].compact
+        args.empty? ? nil : ::File.join(*args)
+      end
+    end
+  end
+end

lib/listen/record/symlink_detector.rb

@@ -0,0 +1,59 @@
+require 'set'
+
+module Listen
+  # @private api
+  class Record
+    class SymlinkDetector
+      SYMLINK_LOOP_ERROR = <<-EOS
+        ** ERROR: Listen detected a duplicate directory being watched! **
+
+        (This may be due to symlinks pointing to parent directories).
+
+        Duplicate: %s
+
+        which already is added as: %s
+
+        Listen is refusing to continue, because this may likely result in
+        an infinite loop.
+
+        Suggestions:
+
+          1) (best option) watch only directories you care about, e.g.
+          either symlinked folders or folders with the real directories,
+          but not both.
+
+          2) reorganize your project so that watched directories do not
+          contain symlinked directories
+
+          3) submit patches so that Listen can reliably and quickly (!)
+          detect symlinks to already watched read directories, skip
+          them, and then reasonably choose which symlinked paths to
+          report as changed (if any)
+
+          4) (not worth it) help implement a "reverse symlink lookup"
+          function in Listen, which - given a real directory - would
+          return all the symlinks pointing to that directory
+
+        Issue: https://github.com/guard/listen/issues/259
+      EOS
+
+      def initialize
+        @real_dirs = Set.new
+      end
+
+      def verify_unwatched!(entry)
+        real_path = entry.real_path
+        @real_dirs.add?(real_path) || _fail(entry.sys_path, real_path)
+      end
+
+      private
+
+      def _fail(symlinked, real_path)
+        STDERR.puts format(SYMLINK_LOOP_ERROR, symlinked, real_path)
+
+        # Note Celluloid eats up abort message anyway
+        fail 'Failed due to looped symlinks'
+      end
+    end
+  end
+end

spec/lib/listen/record_spec.rb

@@ -193,21 +193,24 @@
     before do
       allow(listener).to receive(:directories) { directories }
 
-      allow(::File).to receive(:lstat) do |path|
-        fail "::File.lstat stub called with: #{path.inspect}"
-      end
-
-      allow(::Dir).to receive(:entries) do |path|
-        fail "::Dir.entries stub called with: #{path.inspect}"
-      end
-
-      allow(::Dir).to receive(:exist?) do |path|
-        fail "::Dir.exist? stub called with: #{path.inspect}"
+      stubs = {
+        ::File => %w(lstat realpath),
+        ::Dir => %w(entries exist?)
+      }
+
+      stubs.each do |klass, meths|
+        meths.each do |meth|
+          allow(klass).to receive(meth.to_sym) do |*args|
+            fail "stub called: #{klass}.#{meth}(#{args.map(&:inspect) * ', '})"
+          end
+        end
       end
     end
 
     it 're-inits paths' do
-      allow(::Dir).to receive(:entries) { [] }
+      allow(::Dir).to receive(:entries).and_return([])
+      allow(::File).to receive(:realpath).with('/dir1').and_return('/dir1')
+      allow(::File).to receive(:realpath).with('/dir2').and_return('/dir2')
 
       record.update_file(dir, 'path/file.rb', mtime: 1.1)
       record.build
@@ -219,18 +222,19 @@
     let(:bar_stat) { instance_double(::File::Stat, mtime: 2.3, mode: 0755) }
 
     context 'with no subdirs' do
-
       before do
-        expect(::Dir).to receive(:entries).with('/dir1/.') { %w(foo bar) }
-        expect(::Dir).to receive(:exist?).with('/dir1/./foo') { false }
-        expect(::Dir).to receive(:exist?).with('/dir1/./bar') { false }
-        expect(::File).to receive(:lstat).with('/dir1/./foo') { foo_stat }
-        expect(::File).to receive(:lstat).with('/dir1/./bar') { bar_stat }
-
-        expect(::Dir).to receive(:entries).with('/dir2/.') { [] }
+        allow(::Dir).to receive(:entries).with('/dir1') { %w(foo bar) }
+        allow(::Dir).to receive(:entries).with('/dir1/foo').and_raise(Errno::ENOTDIR)
+        allow(::Dir).to receive(:entries).with('/dir1/bar').and_raise(Errno::ENOTDIR)
+        allow(::File).to receive(:lstat).with('/dir1/foo') { foo_stat }
+        allow(::File).to receive(:lstat).with('/dir1/bar') { bar_stat }
+        allow(::Dir).to receive(:entries).with('/dir2') { [] }
+
+        allow(::File).to receive(:realpath).with('/dir1').and_return('/dir1')
+        allow(::File).to receive(:realpath).with('/dir2').and_return('/dir2')
       end
 
-      it 'builds record'  do
+      it 'builds record' do
         record.build
         expect(record.paths.keys).to eq %w( /dir1 /dir2 )
         expect(record.paths['/dir1']).
@@ -242,15 +246,16 @@
 
     context 'with subdir containing files' do
       before do
-        expect(::Dir).to receive(:entries).with('/dir1/.') { %w(foo) }
-        expect(::Dir).to receive(:exist?).with('/dir1/./foo') { true }
-
-        expect(::Dir).to receive(:entries).with('/dir1/foo') { %w(bar) }
-
-        expect(::Dir).to receive(:exist?).with('/dir1/foo/bar') { false }
-        expect(::File).to receive(:lstat).with('/dir1/foo/bar') { bar_stat }
-
-        expect(::Dir).to receive(:entries).with('/dir2/.') { [] }
+        allow(::Dir).to receive(:entries).with('/dir1') { %w(foo) }
+        allow(::Dir).to receive(:entries).with('/dir1/foo') { %w(bar) }
+        allow(::Dir).to receive(:entries).with('/dir1/foo/bar').and_raise(Errno::ENOTDIR)
+        allow(::File).to receive(:lstat).with('/dir1/foo/bar') { bar_stat }
+        allow(::Dir).to receive(:entries).with('/dir2') { [] }
+
+        allow(::File).to receive(:realpath).with('/dir1').and_return('/dir1')
+        allow(::File).to receive(:realpath).with('/dir2').and_return('/dir2')
+        allow(::File).to receive(:realpath).with('/dir1/foo').
+          and_return('/dir1/foo')
       end
 
       it 'builds record'  do
@@ -264,18 +269,12 @@
 
     context 'with subdir containing dirs' do
       before do
-        expect(::Dir).to receive(:entries).with('/dir1/.') { %w(foo) }
-        expect(::Dir).to receive(:exist?).with('/dir1/./foo') { true }
-
-        expect(::Dir).to receive(:entries).with('/dir1/foo') { %w(bar baz) }
-
-        expect(::Dir).to receive(:exist?).with('/dir1/foo/bar') { true }
-        expect(::Dir).to receive(:entries).with('/dir1/foo/bar') { [] }
-
-        expect(::Dir).to receive(:exist?).with('/dir1/foo/baz') { true }
-        expect(::Dir).to receive(:entries).with('/dir1/foo/baz') { [] }
-
-        expect(::Dir).to receive(:entries).with('/dir2/.') { [] }
+        allow(::File).to receive(:realpath) { |path| path }
+        allow(::Dir).to receive(:entries).with('/dir1') { %w(foo) }
+        allow(::Dir).to receive(:entries).with('/dir1/foo') { %w(bar baz) }
+        allow(::Dir).to receive(:entries).with('/dir1/foo/bar') { [] }
+        allow(::Dir).to receive(:entries).with('/dir1/foo/baz') { [] }
+        allow(::Dir).to receive(:entries).with('/dir2') { [] }
       end
 
       it 'builds record'  do
@@ -290,5 +289,22 @@
         expect(record.paths['/dir2']).to eq({})
       end
     end
+
+    context 'with subdir containing symlink to parent' do
+      subject { record.paths }
+      before do
+        allow(::Dir).to receive(:entries).with('/dir1') { %w(foo) }
+        allow(::Dir).to receive(:entries).with('/dir1/foo') { %w(foo) }
+        allow(::File).to receive(:realpath).with('/dir1').and_return('/bar')
+        allow(::File).to receive(:realpath).with('/dir1/foo').and_return('/bar')
+      end
+
+      it 'shows message and aborts with error' do
+        expect(STDERR).to receive(:puts).with(/detected a duplicate directory/)
+
+        expect { record.build }.to raise_error(RuntimeError,
+                                               /Failed due to looped symlinks/)
+      end
+    end
   end
 end