Merge pull request #215 from chronomantic/linux_editing_fixes

fix + workarounds for editor support on Linux

README.md

@@ -148,6 +148,9 @@ debug: true                                     # Enable Celluloid logger
                                                 # default: false
 ```
 
+Also, setting the environment variable `LISTEN_GEM_DEBUGGING=1` does the same as `debug: true` above.
+
+
 ## Listen adapters
 
 The Listen gem has a set of adapters to notify it when there are changes.
@@ -200,6 +203,8 @@ Here are some things you could try to avoid forcing polling.
 
 If your application keeps using the polling-adapter and you can't figure out why, feel free to [open an issue](https://github.com/guard/listen/issues/new) (and be sure to [give all the details](https://github.com/guard/listen/blob/master/CONTRIBUTING.md)).
 
+Also, if you have problems related to receiving the wrong events, too many events or none at all, be sure set the environment variable `LISTEN_GEM_DEBUGGING=1` and include the output when reporting a new issue.
+
 ## Forwarding file events over TCP
 
 Listen is capable of forwarding file events over the network using a messaging protocol. This can be useful for virtualized development environments when file events are unavailable, as is the case with [Vagrant](https://github.com/mitchellh/vagrant).

lib/listen/adapter/linux.rb

@@ -55,10 +55,15 @@ def _worker_callback
         lambda do |event|
           next if _skip_event?(event)
 
+          path = _event_path(event)
+          cookie_opts = event.cookie.zero? ? {} : { cookie: event.cookie }
+
+          Celluloid.logger.info "listen: inotify event: #{event.flags.inspect}: #{event.name}"
+
           if _dir_event?(event)
-            _notify_change(_event_path(event), type: 'Dir')
+            _notify_change(path, { type: 'Dir'}.merge(cookie_opts))
           else
-            _notify_change(_event_path(event), type: 'File', change: _change(event.flags))
+            _notify_change(path, { type: 'File', change: _change(event.flags)}.merge(cookie_opts))
           end
         end
       end
@@ -74,9 +79,11 @@ def _skip_event?(event)
       end
 
       def _change(event_flags)
-        { modified: [:attrib],
-          added:    [:moved_to, :create],
-          removed:  [:moved_from, :delete] }.each do |change, flags|
+        { modified:   [:attrib, :close_write],
+          moved_to:   [:moved_to],
+          moved_from: [:moved_from],
+          added:      [:create],
+          removed:    [:delete] }.each do |change, flags|
           return change unless (flags & event_flags).empty?
         end
         nil

lib/listen/change.rb

@@ -12,10 +12,16 @@ def initialize(listener)
     end
 
     def change(path, options)
-      return if _silencer.silenced?(path, options[:type])
+      change = options[:change]
+      cookie = options[:cookie]
 
-      if change = options[:change]
-        _notify_listener(change, path)
+      unless cookie
+        #TODO: remove silencing here (it's done later)
+        return if _silencer.silenced?(path, options[:type])
+      end
+
+      if change
+        _notify_listener(change, path, cookie ? { cookie: cookie } : {})
       else
         send("_#{options[:type].downcase}_change", path, options)
       end
@@ -34,8 +40,8 @@ def _dir_change(path, options)
       Directory.new(listener, path, options).scan
     end
 
-    def _notify_listener(change, path)
-      listener.changes << { change => path }
+    def _notify_listener(change, path, options = {})
+      listener.changes << { change => path }.merge(options)
     end
 
     def _silencer

lib/listen/listener.rb

@@ -117,10 +117,10 @@ def _init_options(options = {})
     end
 
     def _init_debug
-      if options[:debug]
+      if options[:debug] || ENV['LISTEN_GEM_DEBUGGING'] =~ /true|1/i
         Celluloid.logger.level = Logger::INFO
       else
-        Celluloid.logger = nil
+        Celluloid.logger.level = Logger::FATAL
       end
     end
 
@@ -156,15 +156,97 @@ def _wait_for_changes
 
     def _pop_changes
       popped = []
-      popped << @changes.pop until @changes.empty?
+      popped << @changes.shift until @changes.empty?
       popped
     end
 
     def _smoosh_changes(changes)
-      smooshed = { modified: [], added: [], removed: [] }
-      changes.each { |h| type = h.keys.first; smooshed[type] << h[type].to_s }
-      smooshed.each { |_, v| v.uniq! }
-      smooshed
+      if _local_fs?
+        cookies = changes.group_by { |x| x[:cookie] }
+        _squash_changes(_reinterpret_related_changes(cookies))
+      else
+        smooshed = { modified: [], added: [], removed: [] }
+        changes.each { |h| type = h.keys.first; smooshed[type] << h[type].to_s }
+        smooshed.each { |_, v| v.uniq! }
+        smooshed
+      end
+    end
+
+    def _local_fs?
+      !registry[:adapter].is_a?(Adapter::TCP)
+    end
+
+    def _squash_changes(changes)
+      actions = changes.group_by(&:last).map do |path, action_list|
+        [_logical_action_for(path, action_list.map(&:first)), path.to_s]
+      end
+      Celluloid.logger.info "listen: raw changes: #{actions.inspect}"
+
+      { modified: [], added: [], removed: [] }.tap do |squashed|
+        actions.each do |type, path|
+          squashed[type] << path unless type.nil?
+        end
+        Celluloid.logger.info "listen: final changes: #{squashed.inspect}"
+      end
+    end
+
+    def _logical_action_for(path, actions)
+      actions << :added if actions.delete(:moved_to)
+      actions << :removed if actions.delete(:moved_from)
+
+      modified = actions.find { |x| x == :modified }
+      _calculate_add_remove_difference(actions, path, modified)
+    end
+
+    def _calculate_add_remove_difference(actions, path, default_if_exists)
+      added = actions.count { |x| x == :added }
+      removed = actions.count { |x| x == :removed }
+      diff = added - removed
+
+      if path.exist?
+        if diff > 0
+          :added
+        elsif diff.zero? && added > 0
+          :modified
+        else
+          default_if_exists
+        end
+      else
+        diff < 0 ? :removed : nil
+      end
+    end
+
+    # remove extraneous rb-inotify events, keeping them only if it's a possible
+    # editor rename() call (e.g. Kate and Sublime)
+    def _reinterpret_related_changes(cookies)
+      table = { moved_to: :added, moved_from: :removed }
+      cookies.map do |cookie, changes|
+        file = _detect_possible_editor_save(changes)
+        if file
+          [[:modified, file]]
+        else
+          changes.map(&:first).reject do |type, path|
+            _silenced?(path)
+          end.map { |type, path| [table.fetch(type, type), path] }
+        end
+      end.flatten(1)
+    end
+
+    def _detect_possible_editor_save(changes)
+      return unless changes.size == 2
+
+      from, to = changes.sort { |x,y| x.keys.first <=> y.keys.first }
+      from, to = from[:moved_from], to[:moved_to]
+      return unless from and to
+
+      # Expect an ignored moved_from and non-ignored moved_to
+      # to qualify as an "editor modify"
+      _silenced?(from) && !_silenced?(to) ? to : nil
+    end
+
+    def _silenced?(path)
+      type = path.directory? ? 'Dir' : 'File'
+      registry[:silencer].silenced?(path, type)
     end
   end
 end

spec/lib/listen/adapter/linux_spec.rb

@@ -37,6 +37,40 @@
         expect{adapter.start}.to raise_error RuntimeError, expected_message
       end
     end
+
+    describe '_worker_callback' do
+
+      let(:expect_change) {
+        ->(change) {
+          allow_any_instance_of(Listen::Adapter::Base).to receive(:_notify_change).with(Pathname.new('path/foo.txt'), type: 'File', change: change, cookie: 123)
+        }
+      }
+
+      let(:event_callback) {
+        ->(flags) {
+          callback = adapter.send(:_worker_callback)
+          callback.call double(:event, name: 'foo.txt', flags: flags, absolute_name: 'path/foo.txt', cookie: 123)
+        }
+      }
+
+      # use case: close_write is the only way to detect changes
+      # on ecryptfs
+      it 'recognizes close_write as modify' do
+        expect_change.(:modified)
+        event_callback.([:close_write])
+      end
+
+      it 'recognizes moved_to as moved_to' do
+        expect_change.(:moved_to)
+        event_callback.([:moved_to])
+      end
+
+      it 'recognizes moved_from as moved_from' do
+        expect_change.(:moved_from)
+        event_callback.([:moved_from])
+      end
+    end
+
   end
 
   if darwin?

spec/lib/listen/change_spec.rb

@@ -16,16 +16,16 @@
     context "file path" do
       context "with known change" do
         it "notifies change directly to listener" do
-          expect(listener_changes).to receive(:<<).with(modified: 'file_path')
-          change.change('file_path', type: 'File', change: :modified)
+          expect(listener_changes).to receive(:<<).with(modified: Pathname.new('file_path'))
+          change.change(Pathname.new('file_path'), type: 'File', change: :modified)
         end
 
         it "doesn't notify to listener if path is silenced" do
           # expect(silencer).to receive(:silenced?).with('file_path', 'File').and_return(true)
           expect(silencer).to receive(:silenced?).and_return(true)
           expect(listener_changes).to_not receive(:<<)
 
-          change.change('file_path', type: 'File', change: :modified)
+          change.change(Pathname.new('file_path'), type: 'File', change: :modified)
         end
       end
 
@@ -34,16 +34,16 @@
         before { Listen::File.stub(:new) { file } }
 
         it "calls Listen::File#change" do
-          expect(Listen::File).to receive(:new).with(listener, 'file_path') { file }
+          expect(Listen::File).to receive(:new).with(listener, Pathname.new('file_path')) { file }
           expect(file).to receive(:change)
-          change.change('file_path', type: 'File')
+          change.change(Pathname.new('file_path'), type: 'File')
         end
 
         it "doesn't call Listen::File#change if path is silenced" do
-          expect(silencer).to receive(:silenced?).with('file_path', 'File').and_return(true)
+          expect(silencer).to receive(:silenced?).with(Pathname.new('file_path'), 'File').and_return(true)
           expect(Listen::File).to_not receive(:new)
 
-          change.change('file_path', type: 'File')
+          change.change(Pathname.new('file_path'), type: 'File')
         end
 
         context "that returns a change" do
@@ -53,14 +53,15 @@
             before { listener.stub(:listen?) { true } }
 
             it "notifies change to listener" do
-              expect(listener_changes).to receive(:<<).with(modified: 'file_path')
-              change.change('file_path', type: 'File')
+              file_path = double(Pathname, to_s: 'file_path', exist?: true)
+              expect(listener_changes).to receive(:<<).with(modified: file_path)
+              change.change(file_path, type: 'File')
             end
 
             context "silence option" do
               it "notifies change to listener" do
                 expect(listener_changes).to_not receive(:<<)
-                change.change('file_path', type: 'File', silence: true)
+                change.change(Pathname.new('file_path'), type: 'File', silence: true)
               end
             end
           end
@@ -70,7 +71,7 @@
 
             it "notifies change to listener" do
               expect(listener_changes).to_not receive(:<<)
-              change.change('file_path', type: 'File')
+              change.change(Pathname.new('file_path'), type: 'File')
             end
           end
         end
@@ -80,7 +81,7 @@
 
           it "doesn't notifies no change" do
             expect(listener_changes).to_not receive(:<<)
-            change.change('file_path', type: 'File')
+            change.change(Pathname.new('file_path'), type: 'File')
           end
         end
       end
@@ -92,9 +93,9 @@
       before { Listen::Directory.stub(:new) { dir } }
 
       it "calls Listen::Directory#scan" do
-        expect(Listen::Directory).to receive(:new).with(listener, 'dir_path', dir_options) { dir }
+        expect(Listen::Directory).to receive(:new).with(listener, Pathname.new('dir_path'), dir_options) { dir }
         expect(dir).to receive(:scan)
-        change.change('dir_path', dir_options)
+        change.change(Pathname.new('dir_path'), dir_options)
       end
     end
   end