Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix vim editing (Linux) + group related fs events #212

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions lib/listen/adapter/linux.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,12 @@ def _worker_callback
lambda do |event|
next if _skip_event?(event)

path = _event_path(event)

if _dir_event?(event)
_notify_change(_event_path(event), type: 'Dir')
_notify_change(path, type: 'Dir')
else
_notify_change(_event_path(event), type: 'File', change: _change(event.flags))
_notify_change(path, type: 'File', change: _change(event.flags))
end
end
end
Expand All @@ -72,7 +74,7 @@ def _skip_event?(event)
end

def _change(event_flags)
{ modified: [:attrib],
{ modified: [:attrib, :close_write],
added: [:moved_to, :create],
removed: [:moved_from, :delete] }.each do |change, flags|
return change unless (flags & event_flags).empty?
Expand Down
55 changes: 52 additions & 3 deletions lib/listen/listener.rb
Original file line number Diff line number Diff line change
Expand Up @@ -156,14 +156,63 @@ 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)
if _local_fs?
_squash_changes(changes)
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?
not registry[:adapter].is_a? Adapter::TCP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to follow bbatsov Ruby style guideline, please could you change it to: !registry[:adapter].is_a?(Adapter::TCP)

end

def _squash_changes(changes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This _squash_changes method is quite complex, would it be possible to refactorize it a little bit with some more detailed sub method? Thanks!

smooshed = { modified: [], added: [], removed: [] }
changes.each { |h| type = h.keys.first; smooshed[type] << h[type].to_s }
smooshed.each { |_, v| v.uniq! }

actions = {}

# group by file for more readable code below
changes.each do |change|
type = change.keys.first
path = change[type]

actions[path] ||= []
actions[path] << type
end

actions.each do |path, action_list|
action = nil

# This is to distinquish e.g. 'touch' (a+m) from 'vim patchmode' ('m+r+a+m')
# (touch "adds" a file, while edit "modifies" a file)
added_count = action_list.count {|x| x == :added }
removed_count = action_list.count {|x| x == :removed }
diff = added_count - removed_count

if path.exist?

if diff > 0
action = action_list.detect {|x| x == :added }
elsif diff.zero? and added_count > 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here with elsif diff.zero? && added_count > 0. Thanks.

The and and or keywords are banned. It's just not worth it. Always use && and || instead.

action = :modified
end

action ||= action_list.detect {|x| x == :modified }
elsif diff < 0
action = action_list.detect {|x| x == :removed }
end

smooshed[action] << path.to_s if action
end
smooshed
end
end
Expand Down
24 changes: 24 additions & 0 deletions spec/lib/listen/adapter/linux_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,30 @@
expect(defined?(INotify)).to be_true
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)
}
}

let(:event_callback) {
->(flags) {
callback = adapter.send(:_worker_callback)
callback.call double(Pathname, name: 'foo.txt', flags: flags, absolute_name: 'path/foo.txt')
}
}

# 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
end

end

if darwin?
Expand Down
29 changes: 15 additions & 14 deletions spec/lib/listen/change_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
46 changes: 43 additions & 3 deletions spec/lib/listen/listener_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@
end

it "calls block on changes" do
listener.changes = [{ modified: 'foo' }]
foo = double(Pathname, to_s: 'foo', exist?: true)
listener.changes = [{ modified: foo }]
block_stub = double('block')
listener.block = block_stub
expect(block_stub).to receive(:call).with(['foo'], [], [])
Expand Down Expand Up @@ -279,16 +280,19 @@
expect(added).to eql(['bar.txt'])
}

foo = double(Pathname, to_s: 'foo.txt', exist?: true)
bar = double(Pathname, to_s: 'bar.txt', exist?: true)

i = 0
listener.stub(:_pop_changes) do
i+=1
case i
when 1
[]
when 2
[{modified: 'foo.txt'}]
[{modified: foo}]
when 3
[{added: 'bar.txt'}]
[{added: bar}]
else
[]
end
Expand All @@ -298,4 +302,40 @@
end
end

describe '_smoosh_changes' do
it 'recognizes rename from temp file' do
path = double(Pathname, to_s: 'foo', exist?: true)
changes = [
{ modified: path },
{ removed: path },
{ added: path },
{ modified: path },
]
smooshed = listener.send :_smoosh_changes, changes
expect(smooshed).to eq({modified: ['foo'], added: [], removed: []})
end

it 'recognizes deleted temp file' do
path = double(Pathname, to_s: 'foo', exist?: false)
changes = [
{ added: path },
{ modified: path },
{ removed: path },
{ modified: path },
]
smooshed = listener.send :_smoosh_changes, changes
expect(smooshed).to eq({modified: [], added: [], removed: []})
end

it 'recognizes double move as modification' do
# e.g. "mv foo x && mv x foo" is like "touch foo"
path = double(Pathname, to_s: 'foo', exist?: true)
changes = [
{ removed: path },
{ added: path },
]
smooshed = listener.send :_smoosh_changes, changes
expect(smooshed).to eq({modified: ['foo'], added: [], removed: []})
end
end
end